Merge branch 'dblessing_scim_provisioned_user' into 'master'

Mark SCIM-created accounts as provisioned by group

See merge request gitlab-org/gitlab!48483

app/models/user_detail.rb

@@ -31,3 +31,5 @@ class UserDetail < ApplicationRecord
     self.bio = '' if bio_changed? && bio.nil?
   end
 end
+
+UserDetail.prepend_if_ee('EE::UserDetail')

changelogs/unreleased/dblessing_scim_provisioned_user.yml

+---
+title: Mark SCIM-created accounts as provisioned by group
+merge_request: 48483
+author:
+type: added

db/migrate/20201123161611_add_provisioned_by_group_to_user_details.rb

+# frozen_string_literal: true
+
+class AddProvisionedByGroupToUserDetails < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_user_details_on_provisioned_by_group_id'
+
+  disable_ddl_transaction!
+
+  def up
+    unless column_exists?(:user_details, :provisioned_by_group_id)
+      with_lock_retries { add_column(:user_details, :provisioned_by_group_id, :integer, limit: 8) }
+    end
+
+    add_concurrent_index :user_details, :provisioned_by_group_id, name: INDEX_NAME
+    add_concurrent_foreign_key :user_details, :namespaces, column: :provisioned_by_group_id, on_delete: :nullify
+  end
+
+  def down
+    with_lock_retries { remove_foreign_key_without_error :user_details, column: :provisioned_by_group_id }
+
+    remove_concurrent_index_by_name :user_details, INDEX_NAME
+
+    if column_exists?(:user_details, :provisioned_by_group_id)
+      with_lock_retries { remove_column(:user_details, :provisioned_by_group_id) }
+    end
+  end
+end

db/schema_migrations/20201123161611

+9d69938cda6db1510ed17d087cc1a582af1e5482d65e4fb457e34011e09c3469
\ No newline at end of file

db/structure.sql

@@ -16998,6 +16998,7 @@ CREATE TABLE user_details (
     cached_markdown_version integer,
     webauthn_xid text,
     other_role text,
+    provisioned_by_group_id bigint,
     CONSTRAINT check_245664af82 CHECK ((char_length(webauthn_xid) <= 100)),
     CONSTRAINT check_b132136b01 CHECK ((char_length(other_role) <= 100))
 );
@@ -22472,6 +22473,8 @@ CREATE INDEX index_user_custom_attributes_on_key_and_value ON user_custom_attrib
 
 CREATE UNIQUE INDEX index_user_custom_attributes_on_user_id_and_key ON user_custom_attributes USING btree (user_id, key);
 
+CREATE INDEX index_user_details_on_provisioned_by_group_id ON user_details USING btree (provisioned_by_group_id);
+
 CREATE UNIQUE INDEX index_user_details_on_user_id ON user_details USING btree (user_id);
 
 CREATE INDEX index_user_highest_roles_on_user_id_and_highest_access_level ON user_highest_roles USING btree (user_id, highest_access_level);
@@ -23067,6 +23070,9 @@ ALTER TABLE ONLY project_features
 ALTER TABLE ONLY ci_pipelines
     ADD CONSTRAINT fk_190998ef09 FOREIGN KEY (external_pull_request_id) REFERENCES external_pull_requests(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY user_details
+    ADD CONSTRAINT fk_190e4fcc88 FOREIGN KEY (provisioned_by_group_id) REFERENCES namespaces(id) ON DELETE SET NULL;
+
 ALTER TABLE ONLY vulnerabilities
     ADD CONSTRAINT fk_1d37cddf91 FOREIGN KEY (epic_id) REFERENCES epics(id) ON DELETE SET NULL;
 

ee/app/models/ee/group.rb

@@ -40,6 +40,8 @@ module EE
       has_many :project_templates, through: :projects, foreign_key: 'custom_project_templates_group_id'
 
       has_many :managed_users, class_name: 'User', foreign_key: 'managing_group_id', inverse_of: :managing_group
+      has_many :provisioned_user_details, class_name: 'UserDetail', foreign_key: 'provisioned_by_group_id', inverse_of: :provisioned_by_group
+      has_many :provisioned_users, through: :provisioned_user_details, source: :user
       has_many :cycle_analytics_stages, class_name: 'Analytics::CycleAnalytics::GroupStage'
       has_many :value_streams, class_name: 'Analytics::CycleAnalytics::GroupValueStream'
 

ee/app/models/ee/user.rb

@@ -33,6 +33,9 @@ module EE
       delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=,
                :extra_shared_runners_minutes_limit, :extra_shared_runners_minutes_limit=,
                to: :namespace
+      delegate :provisioned_by_group, :provisioned_by_group=,
+               :provisioned_by_group_id, :provisioned_by_group_id=,
+               to: :user_detail, allow_nil: true
 
       has_many :epics,                    foreign_key: :author_id
       has_many :requirements,             foreign_key: :author_id, inverse_of: :author, class_name: 'RequirementsManagement::Requirement'

ee/app/models/ee/user_detail.rb

+# frozen_string_literal: true
+
+module EE
+  module UserDetail
+    extend ActiveSupport::Concern
+
+    prepended do
+      belongs_to :provisioned_by_group, class_name: 'Group', optional: true, inverse_of: :provisioned_user_details
+    end
+  end
+end

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

@@ -62,6 +62,8 @@ module EE
         build_scim_identity(user)
         identity_params[:provider] = GROUP_SAML_PROVIDER
 
+        user.provisioned_by_group_id = params[:group_id]
+
         super
       end
 

ee/spec/models/group_spec.rb

@@ -24,6 +24,8 @@ RSpec.describe Group do
     it { is_expected.to have_many(:saml_group_links) }
     it { is_expected.to have_many(:epics) }
     it { is_expected.to have_many(:epic_boards).inverse_of(:group) }
+    it { is_expected.to have_many(:provisioned_user_details).inverse_of(:provisioned_by_group) }
+    it { is_expected.to have_many(:provisioned_users) }
 
     it_behaves_like 'model with wiki' do
       let(:container) { create(:group, :nested, :wiki_repo) }

ee/spec/models/user_detail_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe UserDetail do
+  it { is_expected.to belong_to(:provisioned_by_group) }
+end

ee/spec/requests/api/scim_spec.rb

@@ -208,12 +208,16 @@ RSpec.describe API::Scim do
         it 'created the identity' do
           expect(group.scim_identities.with_extern_uid('test_uid').first).not_to be_nil
         end
+
+        it 'marks the user as provisioned by the group' do
+          expect(new_user.provisioned_by_group).to eq(group)
+        end
       end
 
       context 'existing user' do
-        before do
-          old_user = create(:user, email: 'work@example.com')
+        let(:old_user) { create(:user, email: 'work@example.com') }
 
+        before do
           create(:scim_identity, user: old_user, group: group, extern_uid: 'test_uid')
           group.add_guest(old_user)
 
@@ -227,6 +231,10 @@ RSpec.describe API::Scim do
         it 'has the user external ID' do
           expect(json_response['id']).to eq('test_uid')
         end
+
+        it 'does not mark the user as provisioned' do
+          expect(old_user.reload.provisioned_by_group).to be_nil
+        end
       end
 
       it_behaves_like 'storing arguments in the application context' do

ee/spec/services/ee/users/build_service_spec.rb

@@ -30,19 +30,26 @@ RSpec.describe Users::BuildService do
         end
 
         context 'with scim identity' do
+          let_it_be(:group) { create(:group) }
+          let_it_be(:scim_identity_params) { { extern_uid: 'uid', provider: 'group_scim', group_id: group.id } }
+
           before do
             params.merge!(scim_identity_params)
           end
-          let_it_be(:scim_identity_params) { { extern_uid: 'uid', provider: 'group_scim', group_id: 1 } }
 
           it 'passes allowed attributes to both scim and saml identity' do
-            scim_identity_params.delete(:provider)
+            scim_params = scim_identity_params.dup
+            scim_params.delete(:provider)
 
-            expect(ScimIdentity).to receive(:new).with(hash_including(scim_identity_params)).and_call_original
+            expect(ScimIdentity).to receive(:new).with(hash_including(scim_params)).and_call_original
             expect(Identity).to receive(:new).with(hash_including(identity_params)).and_call_original
 
             service.execute
           end
+
+          it 'marks the user as provisioned by group' do
+            expect(service.execute.provisioned_by_group_id).to eq(group.id)
+          end
         end
       end