Implement review comments for !12445 from @godfat and @rymai.

- Use `GlobalPolicy` to authorize the users that a non-authenticated user can
  fetch from `/api/v4/users`. We allow access if the `Gitlab::VisibilityLevel::PUBLIC`
  visibility level is not restricted.

- Further, as before, `/api/v4/users` is only accessible to unauthenticated users if
  the `username` parameter is passed.

- Turn off `authenticate!` for the `/api/v4/users` endpoint by matching on the actual
  route + method, rather than the description.

- Change the type of `current_user` check in `UsersFinder` to be more
  compatible with EE.

7 files changed, 26 insertions, 32 deletions

diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb
index 0534317df8f..07deceb827b 100644
--- a/app/finders/users_finder.rb
+++ b/app/finders/users_finder.rb
@@ -27,11 +27,8 @@ class UsersFinder
     users = by_search(users)
     users = by_blocked(users)
     users = by_active(users)
-
-    if current_user
-      users = by_external_identity(users)
-      users = by_external(users)
-    end
+    users = by_external_identity(users)
+    users = by_external(users)
 
     users
   end
@@ -63,13 +60,13 @@ class UsersFinder
   end
 
   def by_external_identity(users)
-    return users unless current_user.admin? && params[:extern_uid] && params[:provider]
+    return users unless current_user&.admin? && params[:extern_uid] && params[:provider]
 
     users.joins(:identities).merge(Identity.with_extern_uid(params[:provider], params[:extern_uid]))
   end
 
   def by_external(users)
-    return users = users.where.not(external: true) unless current_user.admin?
+    return users = users.where.not(external: true) unless current_user&.admin?
     return users unless params[:external]
 
     users.external
diff --git a/app/policies/base_policy.rb b/app/policies/base_policy.rb
index 623424c63e0..261a2e780c5 100644
--- a/app/policies/base_policy.rb
+++ b/app/policies/base_policy.rb
@@ -1,4 +1,6 @@
 class BasePolicy
+  include Gitlab::CurrentSettings
+
   class RuleSet
     attr_reader :can_set, :cannot_set
     def initialize(can_set, cannot_set)
@@ -124,4 +126,8 @@ class BasePolicy
     yield
     @rule_set
   end
+
+  def restricted_public_level?
+    current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
+  end
 end
diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index 2683aaad981..e9be43a5037 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -1,9 +1,10 @@
 class GlobalPolicy < BasePolicy
   def rules
+    can! :read_users_list unless restricted_public_level?
+
     return unless @user
 
     can! :create_group if @user.can_create_group
-    can! :read_users_list
 
     unless @user.blocked? || @user.internal?
       can! :log_in unless @user.access_locked?
diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
index 229846e368c..265c56aba53 100644
--- a/app/policies/user_policy.rb
+++ b/app/policies/user_policy.rb
@@ -1,6 +1,4 @@
 class UserPolicy < BasePolicy
-  include Gitlab::CurrentSettings
-
   def rules
     can! :read_user if @user || !restricted_public_level?
 
@@ -12,8 +10,4 @@ class UserPolicy < BasePolicy
       cannot! :destroy_user if @subject.ghost?
     end
   end
-
-  def restricted_public_level?
-    current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
-  end
 end
diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb
index 1322afaa64f..a3aec8889d7 100644
--- a/lib/api/helpers.rb
+++ b/lib/api/helpers.rb
@@ -410,8 +410,8 @@ module API
 
     # Does the current route match the route identified by
     # `description`?
-    def route_matches_description?(description)
-      options.dig(:route_options, :description) == description
+    def request_matches_route?(method, route)
+      request.request_method == method && request.path == route
     end
   end
 end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 34619c90d8b..18ce58299e7 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -4,7 +4,7 @@ module API
 
     before do
       allow_access_with_scope :read_user if request.get?
-      authenticate! unless route_matches_description?("Get the list of users")
+      authenticate! unless request_matches_route?('GET', '/api/v4/users')
     end
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
@@ -55,22 +55,18 @@ module API
 
         users = UsersFinder.new(current_user, params).execute
 
-        authorized =
-          if current_user
-            can?(current_user, :read_users_list)
-          else
-            # When `current_user` is not present, require that the `username`
-            # parameter is passed, to prevent an unauthenticated user from accessing
-            # a list of all the users on the GitLab instance. `UsersFinder` performs
-            # an exact match on the `username` parameter, so we are guaranteed to
-            # get either 0 or 1 `users` here.
-            params[:username].present? &&
-              users.all? { |user| can?(current_user, :read_user, user) }
-          end
+        authorized = can?(current_user, :read_users_list)
+
+        # When `current_user` is not present, require that the `username`
+        # parameter is passed, to prevent an unauthenticated user from accessing
+        # a list of all the users on the GitLab instance. `UsersFinder` performs
+        # an exact match on the `username` parameter, so we are guaranteed to
+        # get either 0 or 1 `users` here.
+        authorized &&= params[:username].present? if current_user.blank?
 
-        render_api_error!("Not authorized.", 403) unless authorized
+        forbidden!("Not authorized to access /api/v4/users") unless authorized
 
-        entity = current_user.try(:admin?) ? Entities::UserWithAdmin : Entities::UserBasic
+        entity = current_user&.admin? ? Entities::UserWithAdmin : Entities::UserBasic
         present paginate(users), with: entity
       end
 
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 01541901330..bf7ed2d3ad6 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -34,7 +34,7 @@ describe API::Users do
       it "returns authorization error when the `username` parameter refers to an inaccessible user" do
         user = create(:user)
 
-        expect(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false)
+        stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
 
         get api("/users"), username: user.username