diff --git a/app/models/ability.rb b/app/models/ability.rb index 7c452c69d14..e327d4eef28 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -91,8 +91,8 @@ class Ability subject.group end - if group - rules << :read_group if group.public? + if group.public? + rules << :read_group rules << :read_group_members unless restricted_public_level? end @@ -483,7 +483,7 @@ class Ability private def restricted_public_level? - @public_restricted ||= current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) + current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) end def named_abilities(name) diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index b9994fcefda..6ce5529abfa 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -79,6 +79,10 @@ module APIGuard @current_user end + def public_access_restricted? + current_application_settings.restricted_visibility_levels.include?(Gitlab::VisibilityLevel::PUBLIC) + end + private def find_access_token @access_token ||= Doorkeeper.authenticate(doorkeeper_request, Doorkeeper.configuration.access_token_methods) diff --git a/lib/api/users.rb b/lib/api/users.rb index 0a14bac07c0..9647a40686e 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -11,6 +11,10 @@ module API # GET /users?search=Admin # GET /users?username=root get do + if !current_user && public_access_restricted? + render_api_error!("Not authorized.", 403) + end + if params[:username].present? @users = User.where(username: params[:username]) else @@ -36,10 +40,12 @@ module API get ":id" do @user = User.find(params[:id]) - if current_user.is_admin? + if current_user.present? && current_user.is_admin? present @user, with: Entities::UserFull - else + elsif can?(current_user, :read_user, @user) present @user, with: Entities::User + else + render_api_error!("User not found.", 404) end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 948935bc10d..8045c8b940d 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -30,8 +30,6 @@ describe UsersController do end describe 'when logged out' do - before { stub_application_setting(restricted_visibility_levels: []) } - it 'renders the show template' do get :show, username: user.username diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 679227bf881..40b24c125b5 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -20,6 +20,24 @@ describe API::API, api: true do end context "when authenticated" do + #These specs are written just in case API authentication is not required anymore + context "when public level is restricted" do + before do + stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::PUBLIC]) + allow_any_instance_of(API::Helpers).to receive(:authenticate!).and_return(true) + end + + it "renders 403" do + get api("/users") + expect(response.status).to eq(403) + end + + it "renders 404" do + get api("/users/#{user.id}") + expect(response.status).to eq(404) + end + end + it "should return an array of users" do get api("/users", user) expect(response.status).to eq(200)