Insert users check into api

5 files changed, 33 insertions, 7 deletions

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)