diff --git a/app/models/member.rb b/app/models/member.rb
index 845a711e986cd762e1075cee5bebbffe008da90b..d31016567390f32bf498aa51aecd6c96fdaedb71 100644
--- a/app/models/member.rb
+++ b/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?
diff --git a/app/policies/organizations/organization_policy.rb b/app/policies/organizations/organization_policy.rb
index afd8c6e144f3470d0cc08bf7ce7855da75f92a90..a203a58b16435716a038d25f7f52dc7a03b83bb8 100644
--- a/app/policies/organizations/organization_policy.rb
+++ b/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
diff --git a/config/feature_flags/gitlab_com_derisk/update_organization_users.yml b/config/feature_flags/gitlab_com_derisk/update_organization_users.yml
new file mode 100644
index 0000000000000000000000000000000000000000..090748501505c5c6df13535e27d7d0b28fad33a0
--- /dev/null
+++ b/config/feature_flags/gitlab_com_derisk/update_organization_users.yml
@@ -0,0 +1,9 @@
+---
+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
diff --git a/spec/factories/groups.rb b/spec/factories/groups.rb
index 807df94e1150b7f70f46226976770646938b9e62..2f55c3ab567a24b7cb177b74655fed57ca70b898 100644
--- a/spec/factories/groups.rb
+++ b/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
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
index 9419bfa76a76cdfe83672b675a200ac0fb575822..bd74af9b7e83846e942cc881a3cfda167387aee8 100644
--- a/spec/models/member_spec.rb
+++ b/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) }
diff --git a/spec/policies/organizations/organization_policy_spec.rb b/spec/policies/organizations/organization_policy_spec.rb
index a1a2f1db30590ed5b81bb25356aaedf615be7d47..9660ed578f7147bfd9d433d4cd4273cf9015056c 100644
--- a/spec/policies/organizations/organization_policy_spec.rb
+++ b/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) }
diff --git a/spec/requests/api/graphql/mutations/organizations/update_spec.rb b/spec/requests/api/graphql/mutations/organizations/update_spec.rb
index 4e819c280d02dcc12e57c8799b735c46c1e393bc..33890ae45921d198449cb197db64e6b4d99ab383 100644
--- a/spec/requests/api/graphql/mutations/organizations/update_spec.rb
+++ b/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) }
diff --git a/spec/requests/api/graphql/organizations/organization_query_spec.rb b/spec/requests/api/graphql/organizations/organization_query_spec.rb
index 73bcfe81d76a53a285bd1bbf41e13740c8a5630c..14becd52e93fbb384e6dddcee08028931dca4ec0 100644
--- a/spec/requests/api/graphql/organizations/organization_query_spec.rb
+++ b/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
diff --git a/spec/requests/organizations/settings_controller_spec.rb b/spec/requests/organizations/settings_controller_spec.rb
index 1d98e59815902c30e65696143baed925ec845558..0177187e3a3f9fa7bd578f38c017b6642bc2343a 100644
--- a/spec/requests/organizations/settings_controller_spec.rb
+++ b/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'
diff --git a/spec/services/organizations/update_service_spec.rb b/spec/services/organizations/update_service_spec.rb
index 630bfdfe1d775bbb47fabaff99743850da169b68..30c07ae1d139fa65a63a0997d6821bfd7a88720b 100644
--- a/spec/services/organizations/update_service_spec.rb
+++ b/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