Merge branch 'fix/use-policies-framework-on-full-private-access-user' into 'master'

fix: use policies framework for full_private_access method on user

Closes #33456

See merge request gitlab-org/gitlab!18530

app/models/user.rb

@@ -1454,7 +1454,7 @@ class User < ApplicationRecord
   # Does the user have access to all private groups & projects?
   # Overridden in EE to also check auditor?
   def full_private_access?
-    admin?
+    can?(:read_all_resources)
   end
 
   def update_two_factor_requirement

app/policies/base_policy.rb

@@ -21,10 +21,6 @@ class BasePolicy < DeclarativePolicy::Base
   with_options scope: :user, score: 0
   condition(:deactivated) { @user&.deactivated? }
 
-  desc "User has access to all private groups & projects"
-  with_options scope: :user, score: 0
-  condition(:full_private_access) { @user&.full_private_access? }
-
   with_options scope: :user, score: 0
   condition(:external_user) { @user.nil? || @user.external? }
 
@@ -40,10 +36,12 @@ class BasePolicy < DeclarativePolicy::Base
     ::Gitlab::ExternalAuthorization.perform_check?
   end
 
-  rule { external_authorization_enabled & ~full_private_access }.policy do
+  rule { external_authorization_enabled & ~can?(:read_all_resources) }.policy do
     prevent :read_cross_project
   end
 
+  rule { admin }.enable :read_all_resources
+
   rule { default }.enable :read_cross_project
 end
 

app/policies/personal_snippet_policy.rb

@@ -30,5 +30,5 @@ class PersonalSnippetPolicy < BasePolicy
 
   rule { can?(:create_note) }.enable :award_emoji
 
-  rule { full_private_access }.enable :read_personal_snippet
+  rule { can?(:read_all_resources) }.enable :read_personal_snippet
 end

app/policies/project_snippet_policy.rb

@@ -28,7 +28,7 @@ class ProjectSnippetPolicy < BasePolicy
     all?(private_snippet | (internal_snippet & external_user),
          ~project.guest,
          ~is_author,
-         ~full_private_access)
+         ~can?(:read_all_resources))
   end.prevent :read_project_snippet
 
   rule { internal_snippet & ~is_author & ~admin }.policy do

changelogs/unreleased/hide-projects-when-admin-mode-is-disabled.yml

+---
+title: Hide projects without access to admin user when admin mode is disabled
+merge_request: 18530
+author: Diego Louzán
+type: changed

doc/development/ee_features.md

@@ -245,47 +245,29 @@ end
 
 #### Use self-descriptive wrapper methods
 
-When it's not possible/logical to modify the implementation of a
-method. Wrap it in a self-descriptive method and use that method.
+When it's not possible/logical to modify the implementation of a method, then
+wrap it in a self-descriptive method and use that method.
 
-For example, in CE only an `admin` is allowed to access all private
-projects/groups, but in EE also an `auditor` has full private
-access. It would be incorrect to override the implementation of
-`User#admin?`, so instead add a method `full_private_access?` to
-`app/models/users.rb`. The implementation in CE will be:
+For example, in GitLab-FOSS, the only user created by the system is `User.ghost`
+but in EE there are several types of bot-users that aren't really users. It would
+be incorrect to override the implementation of `User#ghost?`, so instead we add
+a method `#internal?` to `app/models/user.rb`. The implementation will be:
 
 ```ruby
-def full_private_access?
-  admin?
+def internal?
+  ghost?
 end
 ```
 
 In EE, the implementation `ee/app/models/ee/users.rb` would be:
 
 ```ruby
-override :full_private_access?
-def full_private_access?
-  super || auditor?
+override :internal?
+def internal?
+  super || bot?
 end
 ```
 
-In `lib/gitlab/visibility_level.rb` this method is used to return the
-allowed visibility levels:
-
-```ruby
-def levels_for_user(user = nil)
-  if user.full_private_access?
-    [PRIVATE, INTERNAL, PUBLIC]
-  elsif # ...
-end
-```
-
-See [CE MR][ce-mr-full-private] and [EE MR][ee-mr-full-private] for
-full implementation details.
-
-[ce-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-foss/merge_requests/12373
-[ee-mr-full-private]: https://gitlab.com/gitlab-org/gitlab/merge_requests/2199
-
 ### Code in `config/routes`
 
 When we add `draw :admin` in `config/routes.rb`, the application will try to

ee/app/models/ee/user.rb

@@ -183,11 +183,6 @@ module EE
       self.auditor = (new_level == 'auditor')
     end
 
-    # Does the user have access to all private groups & projects?
-    def full_private_access?
-      super || auditor?
-    end
-
     def email_opted_in_source
       email_opted_in_source_id == EMAIL_OPT_IN_SOURCE_ID_GITLAB_COM ? 'GitLab.com' : ''
     end
@@ -302,6 +297,7 @@ module EE
       super
     end
 
+    override :internal?
     def internal?
       super || bot?
     end

ee/app/policies/ee/base_policy.rb

@@ -19,6 +19,8 @@ module EE
 
       with_scope :global
       condition(:license_block) { License.block_changes? }
+
+      rule { auditor }.enable :read_all_resources
     end
   end
 end

ee/spec/lib/gitlab/elastic/snippet_search_results_spec.rb

@@ -52,15 +52,30 @@ describe Gitlab::Elastic::SnippetSearchResults, :elastic, :sidekiq_might_not_nee
     end
   end
 
-  context 'when user has full_private_access' do
+  context 'when user has full_private_access', :do_not_mock_admin_mode do
+    include_context 'custom session'
+
     let(:user) { create(:admin) }
     let(:results) { described_class.new(user, 'foo', :any) }
 
+    context 'admin mode disabled' do
+      it 'returns nothing' do
+        expect(results.snippet_titles_count).to eq(0)
+        expect(results.snippet_blobs_count).to eq(0)
+      end
+    end
+
+    context 'admin mode enabled' do
+      before do
+        Gitlab::Auth::CurrentUserMode.new(user).enable_admin_mode!(password: user.password)
+      end
+
       it 'returns matched snippets' do
         expect(results.snippet_titles_count).to eq(1)
         expect(results.snippet_blobs_count).to eq(1)
       end
     end
+  end
 
   context 'when content is too long' do
     let(:content) { "abc" + (" " * Elastic::Latest::SnippetInstanceProxy::MAX_INDEX_SIZE) + "xyz" }

ee/spec/models/issue_spec.rb

@@ -144,8 +144,8 @@ describe Issue do
 
     describe 'when a user cannot read cross project' do
       it 'only returns issues within the same project' do
-        expect(Ability).to receive(:allowed?).with(user, :read_cross_project)
-                               .and_return(false)
+        expect(Ability).to receive(:allowed?).with(user, :read_all_resources, :global).and_call_original
+        expect(Ability).to receive(:allowed?).with(user, :read_cross_project).and_return(false)
 
         expect(authorized_issue_a.related_issues(user))
             .to contain_exactly(authorized_issue_b)

ee/spec/policies/base_policy_spec.rb

@@ -2,9 +2,13 @@
 
 require 'spec_helper'
 
-describe BasePolicy do
+describe BasePolicy, :do_not_mock_admin_mode do
   include ExternalAuthorizationServiceHelpers
 
+  let(:auditor) { build(:auditor) }
+
+  subject { described_class.new(auditor, nil) }
+
   describe 'read cross project' do
     context 'when an external authorization service is enabled' do
       before do
@@ -12,8 +16,14 @@ describe BasePolicy do
       end
 
       it 'allows auditors' do
-        expect(described_class.new(build(:auditor), nil)).to be_allowed(:read_cross_project)
+        is_expected.to be_allowed(:read_cross_project)
+      end
     end
   end
+
+  describe 'read all resources' do
+    it 'allows auditors' do
+      is_expected.to be_allowed(:read_all_resources)
+    end
   end
 end

spec/finders/projects_finder_spec.rb

@@ -2,7 +2,9 @@
 
 require 'spec_helper'
 
-describe ProjectsFinder do
+describe ProjectsFinder, :do_not_mock_admin_mode do
+  include AdminModeHelper
+
   describe '#execute' do
     let(:user) { create(:user) }
     let(:group) { create(:group, :public) }
@@ -188,5 +190,21 @@ describe ProjectsFinder do
 
       it { is_expected.to eq([internal_project, public_project]) }
     end
+
+    describe 'with admin user' do
+      let(:user) { create(:admin) }
+
+      context 'admin mode enabled' do
+        before do
+          enable_admin_mode!(current_user)
+        end
+
+        it { is_expected.to match_array([public_project, internal_project, private_project, shared_project]) }
+      end
+
+      context 'admin mode disabled' do
+        it { is_expected.to match_array([public_project, internal_project]) }
+      end
+    end
   end
 end

spec/models/user_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-describe User do
+describe User, :do_not_mock_admin_mode do
   include ProjectForksHelper
   include TermsHelper
 
@@ -2797,12 +2797,28 @@ describe User do
       expect(user.full_private_access?).to be_falsy
     end
 
-    it 'returns true for admin user' do
-      user = build(:user, :admin)
+    context 'for admin user' do
+      include_context 'custom session'
 
+      let(:user) { build(:user, :admin) }
+
+      context 'when admin mode is disabled' do
+        it 'returns false' do
+          expect(user.full_private_access?).to be_falsy
+        end
+      end
+
+      context 'when admin mode is enabled' do
+        before do
+          Gitlab::Auth::CurrentUserMode.new(user).enable_admin_mode!(password: user.password)
+        end
+
+        it 'returns true' do
           expect(user.full_private_access?).to be_truthy
         end
       end
+    end
+  end
 
   describe '.ghost' do
     it "creates a ghost user if one isn't already present" do

spec/policies/base_policy_spec.rb

@@ -2,8 +2,9 @@
 
 require 'spec_helper'
 
-describe BasePolicy do
+describe BasePolicy, :do_not_mock_admin_mode do
   include ExternalAuthorizationServiceHelpers
+  include AdminModeHelper
 
   describe '.class_for' do
     it 'detects policy class based on the subject ancestors' do
@@ -36,8 +37,42 @@ describe BasePolicy do
 
       it { is_expected.not_to be_allowed(:read_cross_project) }
 
-      it 'allows admins' do
-        expect(described_class.new(build(:admin), nil)).to be_allowed(:read_cross_project)
+      context 'for admins' do
+        let(:current_user) { build(:admin) }
+
+        subject { described_class.new(current_user, nil) }
+
+        it 'allowed when in admin mode' do
+          enable_admin_mode!(current_user)
+
+          is_expected.to be_allowed(:read_cross_project)
+        end
+
+        it 'prevented when not in admin mode' do
+          is_expected.not_to be_allowed(:read_cross_project)
+        end
+      end
+    end
+  end
+
+  describe 'full private access' do
+    let(:current_user) { create(:user) }
+
+    subject { described_class.new(current_user, nil) }
+
+    it { is_expected.not_to be_allowed(:read_all_resources) }
+
+    context 'for admins' do
+      let(:current_user) { build(:admin) }
+
+      it 'allowed when in admin mode' do
+        enable_admin_mode!(current_user)
+
+        is_expected.to be_allowed(:read_all_resources)
+      end
+
+      it 'prevented when not in admin mode' do
+        is_expected.not_to be_allowed(:read_all_resources)
       end
     end
   end