Commit 3c88a786 authored by Timothy Andrew's avatar Timothy Andrew

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.
parent c39e4ccf
......@@ -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
......
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
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?
......
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
......@@ -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
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment