Merge branch '419366-upsert-organization-users' into 'master'

Update organization_users on member creation

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139188



Merged-by: Aboobacker MK <akarakath@gitlab.com>
Approved-by: mo khan <mo@mokhan.ca>
Approved-by: Aboobacker MK <akarakath@gitlab.com>
Approved-by: Matt Kasa <mkasa@gitlab.com>
Reviewed-by: Doug Stull <dstull@gitlab.com>
Reviewed-by: Abdul Wadood <awadood@gitlab.com>
Co-authored-by: Doug Stull <dstull@gitlab.com>

app/models/member.rb

@@ -288,6 +288,14 @@ class Member < ApplicationRecord
     refresh_member_authorized_projects
   end
 
+  after_create if: :update_organization_user? do
+    Organizations::OrganizationUser.upsert(
+      { organization_id: source.organization_id, user_id: user_id, access_level: :default },
+      unique_by: [:organization_id, :user_id],
+      on_duplicate: :skip # Do not change access_level, could make :owner :default
+    )
+  end
+
   attribute :notification_level, default: -> { NotificationSetting.levels[:global] }
 
   class << self
@@ -657,6 +665,12 @@ def project_bot?
     user&.project_bot?
   end
 
+  def update_organization_user?
+    return false unless Feature.enabled?(:update_organization_users, source.root_ancestor, type: :gitlab_com_derisk)
+
+    !invite? && source.organization.present?
+  end
+
   def log_invitation_token_cleanup
     return true unless Gitlab.com? && invite? && invite_accepted_at?
 

app/policies/organizations/organization_policy.rb

@@ -3,6 +3,7 @@
 module Organizations
   class OrganizationPolicy < BasePolicy
     condition(:organization_user) { @subject.user?(@user) }
+    condition(:organization_owner) { @subject.owner?(@user) }
 
     desc 'Organization is public'
     condition(:public_organization, scope: :subject, score: 0) { true }
@@ -18,11 +19,14 @@ class OrganizationPolicy < BasePolicy
       enable :read_organization_user
     end
 
-    rule { organization_user }.policy do
+    rule { organization_owner }.policy do
       enable :admin_organization
-      enable :create_group
+    end
+
+    rule { organization_user }.policy do
       enable :read_organization
       enable :read_organization_user
+      enable :create_group
     end
   end
 end

config/feature_flags/gitlab_com_derisk/update_organization_users.yml

+---
+name: update_organization_users
+feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/419366
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/139188
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/435868
+milestone: '16.8'
+group: group::tenant scale
+type: gitlab_com_derisk
+default_enabled: false

spec/factories/groups.rb

@@ -18,6 +18,10 @@
       create(:namespace_settings, namespace: group) unless group.namespace_settings
     end
 
+    trait :with_organization do
+      association :organization
+    end
+
     trait :public do
       visibility_level { Gitlab::VisibilityLevel::PUBLIC }
     end

spec/models/member_spec.rb

@@ -1084,6 +1084,110 @@
     end
   end
 
+  context 'for updating organization_users' do
+    let_it_be(:group) { create(:group, :with_organization) }
+    let(:member) { create(:group_member, source: group) }
+    let(:update_organization_users_enabled) { true }
+
+    before do
+      stub_feature_flags(update_organization_users: update_organization_users_enabled)
+    end
+
+    context 'when update_organization_users is enabled' do
+      it 'inserts new record on member creation' do
+        expect { member }.to change { Organizations::OrganizationUser.count }.by(1)
+        record_attrs = { organization: group.organization, user: member.user, access_level: :default }
+        expect(Organizations::OrganizationUser.exists?(record_attrs)).to be(true)
+      end
+
+      context 'when user already exists in the organization_users' do
+        context 'for an already existing default organization_user' do
+          let_it_be(:project) { create(:project, group: group, organization: group.organization) }
+
+          before do
+            member
+          end
+
+          it 'does not insert a new record in organization_users' do
+            expect do
+              create(:project_member, :owner, source: project, user: member.user)
+            end.not_to change { Organizations::OrganizationUser.count }
+
+            expect(
+              Organizations::OrganizationUser.exists?(
+                organization: project.organization, user: member.user, access_level: :default
+              )
+            ).to be(true)
+          end
+
+          it 'does not update timestamps' do
+            travel_to(1.day.from_now) do
+              expect do
+                create(:project_member, :owner, source: project, user: member.user)
+              end.not_to change { Organizations::OrganizationUser.last.updated_at }
+            end
+          end
+        end
+
+        context 'for an already existing owner organization_user' do
+          let_it_be(:user) { create(:user) }
+          let_it_be(:common_attrs) { { organization: group.organization, user: user } }
+
+          before_all do
+            create(:organization_user, :owner, common_attrs)
+          end
+
+          it 'does not insert a new record in organization_users nor update the access_level' do
+            expect do
+              create(:group_member, :owner, source: group, user: user)
+            end.not_to change { Organizations::OrganizationUser.count }
+
+            expect(
+              Organizations::OrganizationUser.exists?(common_attrs.merge(access_level: :default))
+            ).to be(false)
+            expect(
+              Organizations::OrganizationUser.exists?(common_attrs.merge(access_level: :owner))
+            ).to be(true)
+          end
+        end
+      end
+
+      context 'when updating the organization_users is not successful' do
+        it 'rolls back the member creation' do
+          allow(Organizations::OrganizationUser).to receive(:upsert).once.and_raise(ActiveRecord::StatementTimeout)
+
+          expect { member }.to raise_error(ActiveRecord::StatementTimeout)
+          expect(Organizations::OrganizationUser.exists?(organization: group.organization)).to be(false)
+          expect(group.group_members).to be_empty
+        end
+      end
+    end
+
+    shared_examples_for 'does not create an organization_user entry' do
+      specify do
+        expect { member }.not_to change { Organizations::OrganizationUser.count }
+      end
+    end
+
+    context 'when update_organization_users is disabled' do
+      let(:update_organization_users_enabled) { false }
+
+      it_behaves_like 'does not create an organization_user entry'
+    end
+
+    context 'when member is an invite' do
+      let(:member) { create(:group_member, :invited, source: group) }
+
+      it_behaves_like 'does not create an organization_user entry'
+    end
+
+    context 'when organization does not exist' do
+      let(:member) { create(:group_member) }
+
+      it_behaves_like 'does not create an organization_user entry'
+    end
+  end
+
   context 'when after_commit :update_highest_role' do
     let_it_be(:user) { create(:user) }
 

spec/policies/organizations/organization_policy_spec.rb

@@ -32,8 +32,19 @@
   end
 
   context 'when the user is part of the organization' do
-    before do
-      create :organization_user, organization: organization, user: current_user
+    before_all do
+      create(:organization_user, organization: organization, user: current_user)
+    end
+
+    it { is_expected.to be_disallowed(:admin_organization) }
+    it { is_expected.to be_allowed(:create_group) }
+    it { is_expected.to be_allowed(:read_organization) }
+    it { is_expected.to be_allowed(:read_organization_user) }
+  end
+
+  context 'when the user is an owner of the organization' do
+    before_all do
+      create(:organization_user, :owner, organization: organization, user: current_user)
     end
 
     it { is_expected.to be_allowed(:admin_organization) }

spec/requests/api/graphql/mutations/organizations/update_spec.rb

@@ -8,7 +8,7 @@
 
   let_it_be(:user) { create(:user) }
   let_it_be_with_reload(:organization) do
-    create(:organization) { |org| create(:organization_user, organization: org, user: user) }
+    create(:organization) { |org| create(:organization_user, :owner, organization: org, user: user) }
   end
 
   let(:mutation) { graphql_mutation(:organization_update, params) }

spec/requests/api/graphql/organizations/organization_query_spec.rb

@@ -67,13 +67,13 @@
       it 'returns correct organization user fields' do
         request_organization
 
-        organization_user_node = graphql_data_at(:organization, :organizationUsers, :nodes).first
+        organization_user_nodes = graphql_data_at(:organization, :organizationUsers, :nodes)
         expected_attributes = {
           "badges" => [{ "text" => "It's you!", "variant" => 'muted' }],
           "id" => organization_user.to_global_id.to_s,
           "user" => { "id" => user.to_global_id.to_s }
         }
-        expect(organization_user_node).to match(expected_attributes)
+        expect(organization_user_nodes).to include(expected_attributes)
       end
 
       it 'avoids N+1 queries for all the fields' do

spec/requests/organizations/settings_controller_spec.rb

@@ -21,13 +21,13 @@
     end
 
     context 'when the user is signed in' do
+      let_it_be(:user) { create(:user) }
+
       before do
         sign_in(user)
       end
 
       context 'with no association to an organization' do
-        let_it_be(:user) { create(:user) }
-
         it_behaves_like 'organization - not found response'
         it_behaves_like 'organization - action disabled by `ui_for_organizations` feature flag'
       end
@@ -39,11 +39,18 @@
         it_behaves_like 'organization - action disabled by `ui_for_organizations` feature flag'
       end
 
-      context 'as an organization user' do
-        let_it_be(:user) { create :user }
+      context 'as a default organization user' do
+        before_all do
+          create(:organization_user, organization: organization, user: user)
+        end
 
-        before do
-          create :organization_user, organization: organization, user: user
+        it_behaves_like 'organization - not found response'
+        it_behaves_like 'organization - action disabled by `ui_for_organizations` feature flag'
+      end
+
+      context 'as an owner of an organization' do
+        before_all do
+          create(:organization_user, :owner, organization: organization, user: user)
         end
 
         it_behaves_like 'organization - successful response'

spec/services/organizations/update_service_spec.rb

@@ -32,7 +32,7 @@
 
     context 'when user has permission' do
       before_all do
-        create(:organization_user, organization: organization, user: current_user)
+        create(:organization_user, :owner, organization: organization, user: current_user)
       end
 
       shared_examples 'updating an organization' do