Exposing group_saml_id in the users API

Updated users API documentation
Adding additional tests for exposed property
Added additional helper methods for the users API
Moved API level changes to the service level

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