Merge branch 'expose-saml-provider-id-to-users-api' into 'master'

Expose saml_provider_id in the users API

Closes #12225

See merge request gitlab-org/gitlab-ee!14045

app/services/users/build_service.rb

@@ -5,10 +5,12 @@ module Users
     delegate :user_default_internal_regex_enabled?,
              :user_default_internal_regex_instance,
              to: :'Gitlab::CurrentSettings.current_application_settings'
+    attr_reader :identity_params
 
     def initialize(current_user, params = {})
       @current_user = current_user
       @params = params.dup
+      @identity_params = params.slice(*identity_attributes)
     end
 
     def execute(skip_authorization: false)
@@ -26,10 +28,8 @@ module Users
         end
       end
 
-      identity_attrs = params.slice(*identity_params)
-
-      unless identity_attrs.empty?
-        user.identities.build(identity_attrs)
+      unless identity_params.empty?
+        user.identities.build(identity_params)
       end
 
       user
@@ -37,7 +37,7 @@ module Users
 
     private
 
-    def identity_params
+    def identity_attributes
       [:extern_uid, :provider]
     end
 

app/services/users/update_service.rb

@@ -3,11 +3,13 @@
 module Users
   class UpdateService < BaseService
     include NewUserNotifier
+    attr_reader :user, :identity_params
 
     def initialize(current_user, params = {})
       @current_user = current_user
       @user = params.delete(:user)
       @status_params = params.delete(:status)
+      @identity_params = params.slice(*identity_attributes)
       @params = params.dup
     end
 
@@ -15,8 +17,8 @@ module Users
       yield(@user) if block_given?
 
       user_exists = @user.persisted?
-
       assign_attributes
+      assign_identity
 
       if @user.save(validate: validate) && update_status
         notify_success(user_exists)
@@ -55,7 +57,18 @@ module Users
         params.reject! { |key, _| read_only.include?(key.to_sym) }
       end
 
-      @user.assign_attributes(params) unless params.empty?
+      @user.assign_attributes(params.except(*identity_attributes)) unless params.empty? # rubocop: disable CodeReuse/ActiveRecord
+    end
+
+    def assign_identity
+      return unless identity_params.present?
+
+      identity = user.identities.find_or_create_by(provider: identity_params[:provider]) # rubocop: disable CodeReuse/ActiveRecord
+      identity.update(identity_params)
+    end
+
+    def identity_attributes
+      [:provider, :extern_uid]
     end
   end
 end

doc/api/users.md

@@ -105,7 +105,8 @@ GET /users
     "identities": [
       {"provider": "github", "extern_uid": "2435223452345"},
       {"provider": "bitbucket", "extern_uid": "john.smith"},
-      {"provider": "google_oauth2", "extern_uid": "8776128412476123468721346"}
+      {"provider": "google_oauth2", "extern_uid": "8776128412476123468721346"},
+      {"provider": "group_saml", "extern_uid": "123789", "saml_provider_id": 10}
     ],
     "can_create_group": true,
     "can_create_project": true,
@@ -252,14 +253,15 @@ Parameters:
   "identities": [
     {"provider": "github", "extern_uid": "2435223452345"},
     {"provider": "bitbucket", "extern_uid": "john.smith"},
-    {"provider": "google_oauth2", "extern_uid": "8776128412476123468721346"}
+    {"provider": "google_oauth2", "extern_uid": "8776128412476123468721346"},
+    {"provider": "group_saml", "extern_uid": "123789", "saml_provider_id": 10}
   ],
   "can_create_group": true,
   "can_create_project": true,
   "two_factor_enabled": true,
   "external": false,
   "private_profile": false,
-  "shared_runners_minutes_limit": 133
+  "shared_runners_minutes_limit": 133,
   "extra_shared_runners_minutes_limit": 133
 }
 ```
@@ -293,6 +295,7 @@ Parameters:
 - `projects_limit` (optional)    - Number of projects user can create
 - `extern_uid` (optional)        - External UID
 - `provider` (optional)          - External provider name
+- `group_id_for_saml` (optional) - ID of group where SAML has been configured
 - `bio` (optional)               - User's biography
 - `location` (optional)          - User's location
 - `public_email` (optional)      - The public email of the user
@@ -327,6 +330,7 @@ Parameters:
 - `projects_limit`                 - Limit projects each user can create
 - `extern_uid`                     - External UID
 - `provider`                       - External provider name
+- `group_id_for_saml` (optional)   - ID of group where SAML has been configured
 - `bio`                            - User's biography
 - `location` (optional)            - User's location
 - `public_email` (optional)        - The public email of the user

ee/app/models/ee/identity.rb

@@ -9,6 +9,8 @@ module EE
 
       validates :name_id, presence: true, if: :saml_provider
 
+      validates :saml_provider_id, presence: true, if: :group_saml?
+
       validates :secondary_extern_uid,
         allow_blank: true,
         uniqueness: {
@@ -25,6 +27,10 @@ module EE
       def name_id
         extern_uid
       end
+
+      def group_saml?
+        provider.to_s == "group_saml"
+      end
     end
 
     class_methods do

ee/app/services/ee/users/build_service.rb

@@ -4,6 +4,15 @@ module EE
   module Users
     module BuildService
       extend ::Gitlab::Utils::Override
+      include ::Gitlab::Utils::StrongMemoize
+
+      attr_reader :group_id_for_saml
+
+      override :initialize
+      def initialize(current_user, params = {})
+        super
+        @group_id_for_saml = params.delete(:group_id_for_saml)
+      end
 
       override :execute
       def execute(skip_authorization: false)
@@ -30,9 +39,25 @@ module EE
         ]
       end
 
+      override :identity_attributes
+      def identity_attributes
+        super.push(:saml_provider_id)
+      end
+
       override :identity_params
       def identity_params
-        super.push(:saml_provider_id)
+        if group_id_for_saml.present?
+          super.merge(saml_provider_id: saml_provider_id)
+        else
+          super
+        end
+      end
+
+      def saml_provider_id
+        strong_memoize(:saml_provider_id) do
+          group = GroupFinder.new(current_user).execute(id: group_id_for_saml)
+          group&.saml_provider&.id
+        end
       end
 
       def build_smartcard_identity(user, params)

ee/app/services/ee/users/update_service.rb

@@ -5,6 +5,16 @@ module EE
     module UpdateService
       extend ::Gitlab::Utils::Override
       include EE::Audit::Changes # rubocop: disable Cop/InjectEnterpriseEditionModule
+      include ::Gitlab::Utils::StrongMemoize
+
+      attr_reader :group_id_for_saml
+
+      override :initialize
+      def initialize(current_user, params = {})
+        super
+        @group_id_for_saml = params.delete(:group_id_for_saml)
+        @params = params.dup
+      end
 
       private
 
@@ -21,11 +31,36 @@ module EE
         @user
       end
 
+      override :identity_params
+      def identity_params
+        if group_id_for_saml.present?
+          super.merge(saml_provider_id: saml_provider_id)
+        else
+          super
+        end
+      end
+
+      override :identity_attributes
+      def identity_attributes
+        super.push(:saml_provider_id)
+      end
+
       override :assign_attributes
       def assign_attributes
         params.reject! { |key, _| SamlProvider::USER_ATTRIBUTES_LOCKED_FOR_MANAGED_ACCOUNTS.include?(key.to_sym) } if model.group_managed_account?
         super
       end
+
+      def saml_provider_id
+        strong_memoize(:saml_provider_id) do
+          if group_id_for_saml.present?
+            group = GroupFinder.new(current_user).execute(id: group_id_for_saml)
+            group&.saml_provider&.id
+          else
+            nil
+          end
+        end
+      end
     end
   end
 end

ee/changelogs/unreleased/expose-saml-provider-id-to-users-api.yml

+---
+title: Expose saml_provider_id in the users API
+merge_request: 14045
+author:
+type: added

ee/lib/ee/api/entities.rb

@@ -69,6 +69,14 @@ module EE
         end
       end
 
+      module Identity
+        extend ActiveSupport::Concern
+
+        prepended do
+          expose :saml_provider_id
+        end
+      end
+
       module ProtectedRefAccess
         extend ActiveSupport::Concern
 

ee/lib/ee/api/helpers/users_helpers.rb

@@ -5,11 +5,13 @@ module EE
     module Helpers
       module UsersHelpers
         extend ActiveSupport::Concern
+        extend ::Gitlab::Utils::Override
 
         prepended do
           params :optional_params_ee do
             optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this user'
             optional :extra_shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Extra pipeline minutes quota for this user'
+            optional :group_id_for_saml, type: Integer, desc: 'ID for group where SAML has been configured'
           end
 
           params :optional_index_params_ee do

ee/spec/requests/api/users_spec.rb

@@ -43,4 +43,47 @@ describe API::Users do
       end
     end
   end
+
+  context 'with group SAML' do
+    let(:saml_provider) { create(:saml_provider) }
+
+    it 'creates user with new identity' do
+      post api("/users", admin), params: attributes_for(:user, provider: 'group_saml', extern_uid: '67890', group_id_for_saml: saml_provider.group.id)
+
+      expect(response).to have_gitlab_http_status(201)
+      expect(json_response['identities'].first['extern_uid']).to eq('67890')
+      expect(json_response['identities'].first['provider']).to eq('group_saml')
+      expect(json_response['identities'].first['saml_provider_id']).to eq(saml_provider.id)
+    end
+
+    it 'creates user with new identity without sending reset password email' do
+      post api("/users", admin), params: attributes_for(:user, reset_password: false, provider: 'group_saml', extern_uid: '67890', group_id_for_saml: saml_provider.group.id)
+
+      expect(response).to have_gitlab_http_status(201)
+
+      new_user = User.find(json_response['id'])
+      expect(new_user.recently_sent_password_reset?).to eq(false)
+    end
+
+    it 'updates user with new identity' do
+      put api("/users/#{user.id}", admin), params: { provider: 'group_saml', extern_uid: '67890', group_id_for_saml: saml_provider.group.id }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['identities'].first['extern_uid']).to eq('67890')
+      expect(json_response['identities'].first['provider']).to eq('group_saml')
+      expect(json_response['identities'].first['saml_provider_id']).to eq(saml_provider.id)
+    end
+
+    it 'fails to update user with nonexistent identity' do
+      put api("/users/#{user.id}", admin), params: { provider: 'group_saml', extern_uid: '67890', group_id_for_saml: 15 }
+      expect(response).to have_gitlab_http_status(400)
+      expect(json_response['message']).to eq({ "identities.saml_provider_id" => ["can't be blank"] })
+    end
+
+    it 'fails to update user with nonexistent provider' do
+      put api("/users/#{user.id}", admin), params: { provider: nil, extern_uid: '67890', group_id_for_saml: saml_provider.group.id }
+      expect(response).to have_gitlab_http_status(400)
+      expect(json_response['message']).to eq({ "identities.provider" => ["can't be blank"] })
+    end
+  end
 end

ee/spec/services/users/update_service_spec.rb

@@ -37,6 +37,34 @@ describe Users::UpdateService do
       end.not_to change { user.reload.notification_email }
     end
 
+    context 'with an admin user' do
+      let!(:admin_user) { create(:admin) }
+      let(:service) { described_class.new(admin_user, ActionController::Parameters.new(params).permit!) }
+      let(:params) do
+        { name: 'John Doe', username: 'jduser', email: 'jd@example.com', password: 'mydummypass' }
+      end
+
+      context 'allowed params' do
+        context 'with identity' do
+          let(:provider) { create(:saml_provider) }
+          let(:identity_params) { { extern_uid: 'uid', provider: 'group_saml', saml_group_id: provider.group.id } }
+
+          before do
+            params.merge!(identity_params)
+          end
+
+          it 'successfully adds identity to user' do
+            result = update_user(user, { extern_uid: 'uid', provider: 'group_saml', saml_provider_id: provider.id })
+
+            expect(result).to be true
+            expect(user.identities.last.saml_provider_id).to eq(provider.id)
+            expect(user.identities.last.extern_uid).to eq('uid')
+            expect(user.identities.last.provider).to eq('group_saml')
+          end
+        end
+      end
+    end
+
     def update_user(user, opts)
       described_class.new(user, opts.merge(user: user)).execute!
     end

lib/api/entities.rb

@@ -1680,3 +1680,4 @@ API::Entities.prepend_entity(::API::Entities::UserPublic, with: EE::API::Entitie
 API::Entities.prepend_entity(::API::Entities::Variable, with: EE::API::Entities::Variable)
 API::Entities.prepend_entity(::API::Entities::Todo, with: EE::API::Entities::Todo)
 API::Entities.prepend_entity(::API::Entities::ProtectedBranch, with: EE::API::Entities::ProtectedBranch)
+API::Entities.prepend_entity(::API::Entities::Identity, with: EE::API::Entities::Identity)

lib/api/users.rb

@@ -209,22 +209,9 @@ module API
                 .where.not(id: user.id).count > 0
 
         user_params = declared_params(include_missing: false)
-        identity_attrs = user_params.slice(:provider, :extern_uid)
-
-        if identity_attrs.any?
-          identity = user.identities.find_by(provider: identity_attrs[:provider])
-
-          if identity
-            identity.update(identity_attrs)
-          else
-            identity = user.identities.build(identity_attrs)
-            identity.save
-          end
-        end
 
         user_params[:password_expires_at] = Time.now if user_params[:password].present?
-
-        result = ::Users::UpdateService.new(current_user, user_params.except(:extern_uid, :provider).merge(user: user)).execute
+        result = ::Users::UpdateService.new(current_user, user_params.merge(user: user)).execute
 
         if result[:status] == :success
           present user, with: Entities::UserPublic, current_user: current_user