Commit cfceb01f authored by Rémy Coutable's avatar Rémy Coutable

Merge branch '34141-allow-unauthenticated-access-to-the-users-api-ee' into 'master'

Fix EE conflicts for "Allow unauthenticated access to the `/api/v4/users` API"

See merge request !2247
parents b99e3ca2 013276ea
......@@ -62,13 +62,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
......
require_dependency 'declarative_policy'
class BasePolicy < DeclarativePolicy::Base
include Gitlab::CurrentSettings
desc "User is an instance admin"
with_options scope: :user, score: 0
condition(:admin) { @user&.admin? }
......@@ -11,6 +13,11 @@ class BasePolicy < DeclarativePolicy::Base
with_options scope: :user, score: 0
condition(:can_create_group) { @user&.can_create_group }
desc "The application is restricted from public visibility"
condition(:restricted_public_level, scope: :global) do
current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
end
# EE Extensions
with_scope :user
condition(:auditor, score: 0) { @user&.auditor? }
......
......@@ -11,10 +11,16 @@ class GlobalPolicy < BasePolicy
with_options scope: :user, score: 0
condition(:access_locked) { @user.access_locked? }
rule { anonymous }.prevent_all
rule { anonymous }.policy do
prevent :log_in
prevent :access_api
prevent :access_git
prevent :receive_notifications
prevent :use_quick_actions
prevent :create_group
end
rule { default }.policy do
enable :read_users_list
enable :log_in
enable :access_api
enable :access_git
......@@ -37,4 +43,8 @@ class GlobalPolicy < BasePolicy
rule { access_locked }.policy do
prevent :log_in
end
rule { ~restricted_public_level }.policy do
enable :read_users_list
end
end
class UserPolicy < BasePolicy
include Gitlab::CurrentSettings
desc "The application is restricted from public visibility"
condition(:restricted_public_level, scope: :global) do
current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC)
end
desc "The current user is the user in question"
condition(:user_is_self, score: 0) { @subject == @user }
......
---
title: Allow unauthenticated access to the /api/v4/users API
merge_request: 12445
author:
......@@ -4,10 +4,13 @@ module API
before do
allow_access_with_scope :read_user if request.get?
authenticate!
end
resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
before do
authenticate_non_get!
end
helpers do
def find_user(params)
id = params[:user_id] || params[:id]
......@@ -57,15 +60,22 @@ module API
use :pagination
end
get do
unless can?(current_user, :read_users_list)
render_api_error!("Not authorized.", 403)
end
authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?)
users = UsersFinder.new(current_user, params).execute
entity = current_user.admin? ? Entities::UserWithAdmin : Entities::UserBasic
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?
forbidden!("Not authorized to access /api/v4/users") unless authorized
entity = current_user&.admin? ? Entities::UserWithAdmin : Entities::UserBasic
present paginate(users), with: entity
end
......@@ -404,6 +414,10 @@ module API
end
resource :user do
before do
authenticate!
end
desc 'Get the currently authenticated user' do
success Entities::UserPublic
end
......
require 'spec_helper'
describe GlobalPolicy, models: true do
let(:current_user) { create(:user) }
let(:user) { create(:user) }
subject { GlobalPolicy.new(current_user, [user]) }
describe "reading the list of users" do
context "for a logged in user" do
it { is_expected.to be_allowed(:read_users_list) }
end
context "for an anonymous user" do
let(:current_user) { nil }
context "when the public level is restricted" do
before do
stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
end
it { is_expected.not_to be_allowed(:read_users_list) }
end
context "when the public level is not restricted" do
before do
stub_application_setting(restricted_visibility_levels: [])
end
it { is_expected.to be_allowed(:read_users_list) }
end
end
end
end
......@@ -13,9 +13,40 @@ describe API::Users do
describe 'GET /users' do
context "when unauthenticated" do
it "returns authentication error" do
it "returns authorization error when the `username` parameter is not passed" do
get api("/users")
expect(response).to have_http_status(401)
expect(response).to have_http_status(403)
end
it "returns the user when a valid `username` parameter is passed" do
user = create(:user)
get api("/users"), username: user.username
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.size).to eq(1)
expect(json_response[0]['id']).to eq(user.id)
expect(json_response[0]['username']).to eq(user.username)
end
it "returns authorization error when the `username` parameter refers to an inaccessible user" do
user = create(:user)
stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC])
get api("/users"), username: user.username
expect(response).to have_http_status(403)
end
it "returns an empty response when an invalid `username` parameter is passed" do
get api("/users"), username: 'invalid'
expect(response).to have_http_status(200)
expect(json_response).to be_an Array
expect(json_response.size).to eq(0)
end
end
......@@ -150,6 +181,7 @@ describe API::Users do
describe "GET /users/:id" do
it "returns a user by id" do
get api("/users/#{user.id}", user)
expect(response).to have_http_status(200)
expect(json_response['username']).to eq(user.username)
end
......@@ -160,9 +192,22 @@ describe API::Users do
expect(json_response['is_admin']).to be_nil
end
it "returns a 401 if unauthenticated" do
get api("/users/9998")
expect(response).to have_http_status(401)
context 'for an anonymous user' do
it "returns a user by id" do
get api("/users/#{user.id}")
expect(response).to have_http_status(200)
expect(json_response['username']).to eq(user.username)
end
it "returns a 404 if the target user is present but inaccessible" do
allow(Ability).to receive(:allowed?).and_call_original
allow(Ability).to receive(:allowed?).with(nil, :read_user, user).and_return(false)
get api("/users/#{user.id}")
expect(response).to have_http_status(404)
end
end
it "returns a 404 error if user id not found" do
......
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