diff --git a/app/finders/concerns/finder_with_group_hierarchy.rb b/app/finders/concerns/finder_with_group_hierarchy.rb index 99b58fa6954890370fb29e36d97eee319eaa56a7..96d3eca935eb7c3347d7d6630fdc5b7f886dc25c 100644 --- a/app/finders/concerns/finder_with_group_hierarchy.rb +++ b/app/finders/concerns/finder_with_group_hierarchy.rb @@ -75,6 +75,11 @@ def groups_user_can_read_items(groups) end def preload_associations(groups) + ActiveRecord::Associations::Preloader.new( + records: groups, + associations: [:organization] + ).call + Preloaders::UserMaxAccessLevelInGroupsPreloader.new(groups, current_user).execute end end diff --git a/app/models/group.rb b/app/models/group.rb index b3c408d3bea359f6c0afade80d72c462a2196afa..43f2198561bcf28ad6a9f496aa980b9faf4bc4f8 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -729,7 +729,7 @@ def max_member_access_for_user(user, only_concrete_membership: false) unless only_concrete_membership return GroupMember::OWNER if user.can_admin_all_resources? - return GroupMember::OWNER if user.can_admin_organization?(organization_id) + return GroupMember::OWNER if user.can_admin_organization?(organization) end max_member_access(user) diff --git a/app/models/preloaders/group_policy_preloader.rb b/app/models/preloaders/group_policy_preloader.rb index 23632a9b6c271849f2b2160a91885ca43b4dcf78..c52e49408c581df97da151fdadf99a626ae76f86 100644 --- a/app/models/preloaders/group_policy_preloader.rb +++ b/app/models/preloaders/group_policy_preloader.rb @@ -8,6 +8,11 @@ def initialize(groups, current_user) end def execute + ActiveRecord::Associations::Preloader.new( + records: groups, + associations: [:organization] + ).call + Preloaders::UserMaxAccessLevelInGroupsPreloader.new(groups, current_user).execute end diff --git a/app/models/user.rb b/app/models/user.rb index edb7021544304f3fb5557da1bb862aabc4ea4496..aef0a3af7c4b96bb0c3a3e63a4ea5310c8c720ff 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2145,7 +2145,7 @@ def owns_organization?(organization) end def can_admin_organization?(organization) - owns_organization?(organization) + can?(:admin_organization, organization) end def update_two_factor_requirement diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index c9a7af3052a462ef55476f3cf72bdc8ffab4ff04..5d626aaba3533db5dc3709298887fd93da0d5cee 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -34,13 +34,11 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy desc "User owns the group's organization" condition(:organization_owner) do - if @user.is_a?(User) - @user.owns_organization?(@subject.organization_id) - else - false - end + owns_group_organization? end + rule { admin | organization_owner }.enable :admin_organization + with_options scope: :subject, score: 0 condition(:request_access_enabled) { @subject.request_access_enabled } @@ -456,6 +454,21 @@ def resource_access_token_creation_allowed? def valid_dependency_proxy_deploy_token @user.is_a?(DeployToken) && @user&.valid_for_dependency_proxy? && @user&.has_access_to_group?(@subject) end + + # rubocop:disable Cop/UserAdmin -- specifically check the admin attribute + def owns_group_organization? + return false unless @user + return false unless user_is_user? + return false unless @subject.organization + # Ensure admins can't bypass admin mode. + return false if @user.admin? && !can?(:admin) + + # Load the owners with a single query. + @subject.organization + .owner_user_ids + .include?(@user.id) + end + # rubocop:enable Cop/UserAdmin end GroupPolicy.prepend_mod_with('GroupPolicy') diff --git a/ee/spec/lib/gitlab/llm/chain/utils/chat_authorizer_spec.rb b/ee/spec/lib/gitlab/llm/chain/utils/chat_authorizer_spec.rb index b00e20ac19a373e67655a62f65755f84e9ee0c89..f4b27bfbb86a568928de3b2ec7529da699992bcf 100644 --- a/ee/spec/lib/gitlab/llm/chain/utils/chat_authorizer_spec.rb +++ b/ee/spec/lib/gitlab/llm/chain/utils/chat_authorizer_spec.rb @@ -203,6 +203,7 @@ let(:container) { create(:group) } before do + allow(user).to receive(:can?).with(:admin_organization, nil).and_call_original allow(user).to receive(:can?).with(:admin_all_resources).and_call_original end diff --git a/ee/spec/models/ee/event_spec.rb b/ee/spec/models/ee/event_spec.rb index 6c2d82e59f95082a5d55350237b14d8b2646c05f..6e27c5188e2f7a6656afa6b231b323c78f79d99f 100644 --- a/ee/spec/models/ee/event_spec.rb +++ b/ee/spec/models/ee/event_spec.rb @@ -9,7 +9,7 @@ let_it_be(:guest) { create(:user) } let_it_be(:reporter) { create(:user) } let_it_be(:author) { create(:author) } - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } let(:users) { [non_member, member, reporter, guest, author, admin] } diff --git a/ee/spec/policies/gitlab_subscriptions/add_on_purchase_policy_spec.rb b/ee/spec/policies/gitlab_subscriptions/add_on_purchase_policy_spec.rb index 8320e6b804e8319b0abe7b4ab2635ee2e599bbf0..56378a1fe9027abdad6eae7b16612bbd537de987 100644 --- a/ee/spec/policies/gitlab_subscriptions/add_on_purchase_policy_spec.rb +++ b/ee/spec/policies/gitlab_subscriptions/add_on_purchase_policy_spec.rb @@ -7,7 +7,7 @@ using RSpec::Parameterized::TableSyntax let_it_be(:group) { create(:group) } - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } let_it_be(:owner) { create(:user, owner_of: group) } let_it_be(:maintainer) { create(:user, maintainer_of: group) } let_it_be(:developer) { create(:user, developer_of: group) } diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 4e83b348743af17bec5519aa53762715c9fadc78..377c352b77b12fd56cf02ba3b03f479ccc5ba2a5 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -11,7 +11,7 @@ let_it_be_with_refind(:project) { create(:project, namespace: group) } let_it_be(:user) { create(:user) } let_it_be(:admin_with_admin_mode) { create(:admin) } - let_it_be(:admin_without_admin_mode) { create(:admin, :without_default_org) } + let_it_be(:admin_without_admin_mode) { create(:admin) } let_it_be(:group_member) { create(:group_member, group: group, user: user) } let_it_be(:owner) { group.add_owner(create(:user)).user } let_it_be(:maintainer) { group.add_maintainer(create(:user)).user } diff --git a/spec/features/groups_spec.rb b/spec/features/groups_spec.rb index a3fa05242ef65d3d89ed4fef058bbece1e4dc70b..ad187f0456519c5020df185b6060a37141b237d5 100644 --- a/spec/features/groups_spec.rb +++ b/spec/features/groups_spec.rb @@ -248,7 +248,7 @@ let_it_be(:group) { create(:group, path: 'foo') } context 'as admin' do - let(:user) { create(:admin, :without_default_org) } + let(:user) { create(:admin) } before do visit new_group_path(parent_id: group.id, anchor: 'create-group-pane') diff --git a/spec/features/projects/new_project_spec.rb b/spec/features/projects/new_project_spec.rb index cf9c0012e29c0c15e677297bb27ed16fd80e6e72..d6b27d8c618fafd80c7ce9b162266df6c2c30ee5 100644 --- a/spec/features/projects/new_project_spec.rb +++ b/spec/features/projects/new_project_spec.rb @@ -92,7 +92,7 @@ end context 'as an admin' do - let(:user) { create(:admin, :without_default_org) } + let(:user) { create(:admin) } it_behaves_like 'shows correct navigation' diff --git a/spec/lib/gitlab/import_export/project/relation_factory_spec.rb b/spec/lib/gitlab/import_export/project/relation_factory_spec.rb index 3e731a75fe356bad0b5aa9ac31e666733a64ed87..5421b0966b7b5e627e483dc2345fb3210761727f 100644 --- a/spec/lib/gitlab/import_export/project/relation_factory_spec.rb +++ b/spec/lib/gitlab/import_export/project/relation_factory_spec.rb @@ -6,7 +6,7 @@ let(:group) { create(:group, maintainers: importer_user) } let(:project) { create(:project, :repository, group: group) } let(:members_mapper) { double('members_mapper').as_null_object } - let(:admin) { create(:admin, :without_default_org) } + let(:admin) { create(:admin) } let(:importer_user) { admin } let(:excluded_keys) { [] } let(:additional_relation_attributes) { {} } diff --git a/spec/lib/gitlab/import_export/project/tree_saver_spec.rb b/spec/lib/gitlab/import_export/project/tree_saver_spec.rb index 6e5b9b7027eaea1477dc607513a674103dc888e3..5747194a7d0517434457ecdcd595eefa92ee2f00 100644 --- a/spec/lib/gitlab/import_export/project/tree_saver_spec.rb +++ b/spec/lib/gitlab/import_export/project/tree_saver_spec.rb @@ -393,7 +393,7 @@ end context 'as admin' do - let(:user) { create(:admin, :without_default_org) } + let(:user) { create(:admin) } before do project_tree_saver.save # rubocop:disable Rails/SaveBang diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 788201727bfcaaefa3bf60a0c37b574ec8e321fd..51be2051a368d507ed9b0733dde70e51c6a9868e 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1983,7 +1983,7 @@ def setup_group_members(group) end context 'evaluating admin access level' do - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } context 'when admin mode is enabled', :enable_admin_mode do it 'returns OWNER by default' do @@ -2006,8 +2006,16 @@ def setup_group_members(group) context 'when organization owner' do let_it_be(:admin) { create(:admin) } - it 'returns OWNER by default' do - expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER) + context 'when admin mode is enabled', :enable_admin_mode do + it 'returns OWNER by default' do + expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::OWNER) + end + end + + context 'when admin mode is disabled' do + it 'returns NO_ACCESS by default' do + expect(group.max_member_access_for_user(admin)).to eq(Gitlab::Access::NO_ACCESS) + end end context 'when only concrete members' do diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 0a546c44f6ccde3a2af3445671e884dac1fb78af..634e4c9c6111ead26a565eaf859f912e9b64f2d7 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -253,6 +253,18 @@ expect_allowed(*owner_permissions) expect_allowed(*admin_permissions) end + + context 'when user is also an admin' do + before do + organization_owner.update!(admin: true) + end + + it { expect_disallowed(:admin_organization) } + + context 'with admin mode', :enable_admin_mode do + it { expect_allowed(:admin_organization) } + end + end end context 'migration bot' do diff --git a/spec/policies/upload_policy_spec.rb b/spec/policies/upload_policy_spec.rb index 1dfc988502795ee342aee059cd998fc156932073..b08cc8a276b6d3ad30707d4dcecb66ce155016ba 100644 --- a/spec/policies/upload_policy_spec.rb +++ b/spec/policies/upload_policy_spec.rb @@ -9,7 +9,7 @@ let_it_be(:developer) { create(:user, developer_of: group) } let_it_be(:maintainer) { create(:user, maintainer_of: group) } let_it_be(:owner) { create(:user, owner_of: group) } - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } let_it_be(:non_member_user) { create(:user) } let(:upload_permissions) { [:read_upload, :destroy_upload] } diff --git a/spec/presenters/projects/import_export/project_export_presenter_spec.rb b/spec/presenters/projects/import_export/project_export_presenter_spec.rb index 277df5d4277ad355de6121400f8d64fc1820a818..d5776ba2323e913b87aed5689c7e79131bdaad12 100644 --- a/spec/presenters/projects/import_export/project_export_presenter_spec.rb +++ b/spec/presenters/projects/import_export/project_export_presenter_spec.rb @@ -84,7 +84,7 @@ end context 'as admin' do - let(:user) { create(:admin, :without_default_org) } + let(:user) { create(:admin) } context 'when admin mode is enabled', :enable_admin_mode do it 'exports group members as admin' do diff --git a/spec/requests/api/user_runners_spec.rb b/spec/requests/api/user_runners_spec.rb index 5a79f6c4e34dee811908196d23ae40edb8516b9b..d889dc48fb98a33141c303af2e7b489245817bbb 100644 --- a/spec/requests/api/user_runners_spec.rb +++ b/spec/requests/api/user_runners_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.describe API::UserRunners, :aggregate_failures, feature_category: :fleet_visibility do - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } let_it_be(:user, reload: true) { create(:user, username: 'user.withdot') } describe 'POST /user/runners' do diff --git a/spec/services/ci/runners/create_runner_service_spec.rb b/spec/services/ci/runners/create_runner_service_spec.rb index cab6266cc482beeb03ea3c4b81a4b8ff90496102..efab371482b0789a6d8cc6d4fae91c5e18a2231c 100644 --- a/spec/services/ci/runners/create_runner_service_spec.rb +++ b/spec/services/ci/runners/create_runner_service_spec.rb @@ -7,7 +7,7 @@ let(:runner) { execute.payload[:runner] } - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } let_it_be(:non_admin_user) { create(:user) } let_it_be(:anonymous) { nil } let_it_be(:group_owner) { create(:user) } diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 6810f2f0209d3669d8c537a80101f3f8216dd7c7..e0f3f9e6109bf677774e1d4be2a782482d3f1a3d 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -62,7 +62,7 @@ end context 'when created by an admin' do - let(:user) { create(:admin, :without_default_org) } + let(:user) { create(:admin) } context 'when admin mode is enabled', :enable_admin_mode do it_behaves_like 'creates a user that has their email confirmed' diff --git a/spec/support/matchers/access_matchers.rb b/spec/support/matchers/access_matchers.rb index 741fed412c6bc1d37a7d27860d471ed335c5a25f..1b460fbdbf7ff5ecfb015a9ac94020918c422866 100644 --- a/spec/support/matchers/access_matchers.rb +++ b/spec/support/matchers/access_matchers.rb @@ -11,9 +11,7 @@ module AccessMatchers def emulate_user(user_type_or_trait, membership = nil) case user_type_or_trait - when :admin - login_as(create(user_type_or_trait, :without_default_org)) - when :user + when :user, :admin login_as(create(user_type_or_trait)) when :external, :auditor login_as(create(:user, user_type_or_trait)) diff --git a/spec/support/matchers/access_matchers_for_controller.rb b/spec/support/matchers/access_matchers_for_controller.rb index 2188128d60fe7227ced35ef35bfd456638d0c372..401bf6c196e842c9830bbfc014946452403f1b24 100644 --- a/spec/support/matchers/access_matchers_for_controller.rb +++ b/spec/support/matchers/access_matchers_for_controller.rb @@ -13,7 +13,7 @@ module AccessMatchersForController def emulate_user(role, membership = nil) case role when :admin - user = create(:admin, :without_default_org) + user = create(:admin) sign_in(user) when :user user = create(:user) diff --git a/spec/support/shared_contexts/graphql/resolvers/runners_resolver_shared_context.rb b/spec/support/shared_contexts/graphql/resolvers/runners_resolver_shared_context.rb index b24ad8a535d430bdf9dcaa49ff4cdd1586ce3736..2dbb903a2724c1b857b0839b63418410714b770d 100644 --- a/spec/support/shared_contexts/graphql/resolvers/runners_resolver_shared_context.rb +++ b/spec/support/shared_contexts/graphql/resolvers/runners_resolver_shared_context.rb @@ -3,7 +3,7 @@ require 'spec_helper' RSpec.shared_context 'runners resolver setup' do - let_it_be(:user) { create_default(:user, :admin, :without_default_org) } + let_it_be(:user) { create_default(:user, :admin) } let_it_be(:group) { create(:group, :public) } let_it_be(:subgroup) { create(:group, :public, parent: group) } let_it_be(:project) { create(:project, :public, group: group) } diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb index 18fa8a89e298e262e5a68526434a737f293162a3..bb8c36812b5edc55f9870ec823462ddda972ce95 100644 --- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb +++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb @@ -12,7 +12,7 @@ let_it_be(:developer) { create(:user, developer_of: group) } let_it_be(:maintainer) { create(:user, maintainer_of: group) } let_it_be(:owner) { create(:user, owner_of: group) } - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } let_it_be(:non_group_member) { create(:user) } let_it_be(:organization_owner) { create(:organization_user, :owner, organization: organization).user } @@ -94,7 +94,7 @@ ] end - let(:admin_permissions) { %i[read_confidential_issues read_internal_note] } + let(:admin_permissions) { %i[admin_organization read_confidential_issues read_internal_note] } subject { described_class.new(current_user, group) } end diff --git a/spec/support/shared_examples/models/member_shared_examples.rb b/spec/support/shared_examples/models/member_shared_examples.rb index 4b5d991e5cac4288d544041ba454a0db80e59f8b..250791b1e60ad29c46099ae93ad75f72feba6472 100644 --- a/spec/support/shared_examples/models/member_shared_examples.rb +++ b/spec/support/shared_examples/models/member_shared_examples.rb @@ -112,7 +112,7 @@ end RSpec.shared_examples_for "member creation" do - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } it 'returns a Member object', :aggregate_failures do member = described_class.add_member(source, user, :maintainer) @@ -385,7 +385,7 @@ end RSpec.shared_examples_for "bulk member creation" do - let_it_be(:admin) { create(:admin, :without_default_org) } + let_it_be(:admin) { create(:admin) } let_it_be(:user1) { create(:user, email: 'bob@example.com') } let_it_be(:user2) { create(:user) } diff --git a/spec/support/shared_examples/requests/admin_mode_shared_examples.rb b/spec/support/shared_examples/requests/admin_mode_shared_examples.rb index 475e9bf6fe265c8b15646857353333a2596559a5..4f198dfb740dede8e28c1341e9360648ceda456d 100644 --- a/spec/support/shared_examples/requests/admin_mode_shared_examples.rb +++ b/spec/support/shared_examples/requests/admin_mode_shared_examples.rb @@ -55,7 +55,7 @@ end RSpec.shared_examples 'when admin' do - let_it_be(:current_user) { create(:admin, :without_default_org) } + let_it_be(:current_user) { create(:admin) } it_behaves_like 'makes request' do let(:status) { success_status_code }