From 3c88a7869b87693ba8c3fb9814d39437dd569a31 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 29 Jun 2017 07:43:41 +0000 Subject: [PATCH] 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. --- app/finders/users_finder.rb | 11 ++++------- app/policies/base_policy.rb | 6 ++++++ app/policies/global_policy.rb | 3 ++- app/policies/user_policy.rb | 6 ------ lib/api/helpers.rb | 4 ++-- lib/api/users.rb | 26 +++++++++++--------------- spec/requests/api/users_spec.rb | 2 +- 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) - render_api_error!("Not authorized.", 403) unless authorized + # 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? - entity = current_user.try(:admin?) ? Entities::UserWithAdmin : Entities::UserBasic + forbidden!("Not authorized to access /api/v4/users") unless authorized + + 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