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

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

Allow unauthenticated access to the `/api/v4/users` API

Closes #34141

See merge request !12445
parents a69236cd d1488268
...@@ -60,13 +60,13 @@ class UsersFinder ...@@ -60,13 +60,13 @@ class UsersFinder
end end
def by_external_identity(users) 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])) users.joins(:identities).merge(Identity.with_extern_uid(params[:provider], params[:extern_uid]))
end end
def by_external(users) 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] return users unless params[:external]
users.external users.external
......
require_dependency 'declarative_policy' require_dependency 'declarative_policy'
class BasePolicy < DeclarativePolicy::Base class BasePolicy < DeclarativePolicy::Base
include Gitlab::CurrentSettings
desc "User is an instance admin" desc "User is an instance admin"
with_options scope: :user, score: 0 with_options scope: :user, score: 0
condition(:admin) { @user&.admin? } condition(:admin) { @user&.admin? }
...@@ -10,4 +12,9 @@ class BasePolicy < DeclarativePolicy::Base ...@@ -10,4 +12,9 @@ class BasePolicy < DeclarativePolicy::Base
with_options scope: :user, score: 0 with_options scope: :user, score: 0
condition(:can_create_group) { @user&.can_create_group } 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
end end
...@@ -11,10 +11,16 @@ class GlobalPolicy < BasePolicy ...@@ -11,10 +11,16 @@ class GlobalPolicy < BasePolicy
with_options scope: :user, score: 0 with_options scope: :user, score: 0
condition(:access_locked) { @user.access_locked? } 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 rule { default }.policy do
enable :read_users_list
enable :log_in enable :log_in
enable :access_api enable :access_api
enable :access_git enable :access_git
...@@ -37,4 +43,8 @@ class GlobalPolicy < BasePolicy ...@@ -37,4 +43,8 @@ class GlobalPolicy < BasePolicy
rule { access_locked }.policy do rule { access_locked }.policy do
prevent :log_in prevent :log_in
end end
rule { ~restricted_public_level }.policy do
enable :read_users_list
end
end end
class UserPolicy < BasePolicy 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" desc "The current user is the user in question"
condition(:user_is_self, score: 0) { @subject == @user } 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 ...@@ -4,10 +4,13 @@ module API
before do before do
allow_access_with_scope :read_user if request.get? allow_access_with_scope :read_user if request.get?
authenticate!
end end
resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
before do
authenticate_non_get!
end
helpers do helpers do
def find_user(params) def find_user(params)
id = params[:user_id] || params[:id] id = params[:user_id] || params[:id]
...@@ -51,15 +54,22 @@ module API ...@@ -51,15 +54,22 @@ module API
use :pagination use :pagination
end end
get do 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?) authenticated_as_admin! if params[:external].present? || (params[:extern_uid].present? && params[:provider].present?)
users = UsersFinder.new(current_user, params).execute 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 present paginate(users), with: entity
end end
...@@ -398,6 +408,10 @@ module API ...@@ -398,6 +408,10 @@ module API
end end
resource :user do resource :user do
before do
authenticate!
end
desc 'Get the currently authenticated user' do desc 'Get the currently authenticated user' do
success Entities::UserPublic success Entities::UserPublic
end 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 ...@@ -13,9 +13,40 @@ describe API::Users do
describe 'GET /users' do describe 'GET /users' do
context "when unauthenticated" 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") 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
end end
...@@ -138,6 +169,7 @@ describe API::Users do ...@@ -138,6 +169,7 @@ describe API::Users do
describe "GET /users/:id" do describe "GET /users/:id" do
it "returns a user by id" do it "returns a user by id" do
get api("/users/#{user.id}", user) get api("/users/#{user.id}", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['username']).to eq(user.username) expect(json_response['username']).to eq(user.username)
end end
...@@ -148,9 +180,22 @@ describe API::Users do ...@@ -148,9 +180,22 @@ describe API::Users do
expect(json_response['is_admin']).to be_nil expect(json_response['is_admin']).to be_nil
end end
it "returns a 401 if unauthenticated" do context 'for an anonymous user' do
get api("/users/9998") it "returns a user by id" do
expect(response).to have_http_status(401) 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 end
it "returns a 404 error if user id not found" do 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