prevent access to user api for locked admins

app/services/delete_user_service.rb

@@ -64,7 +64,7 @@ class DeleteUserService
     if actor == user
       Setting.users_deletable_by_self?
     else
-      actor.admin && Setting.users_deletable_by_admins?
+      actor.admin? && actor.active? && Setting.users_deletable_by_admins?
     end
   end
 

lib/api/root.rb

@@ -177,6 +177,14 @@ module API
         authorize_by_with_raise(authorized, &block)
       end
 
+      def authorize_admin
+        authorize_by_with_raise(current_user.admin? && (current_user.active? || current_user.is_a?(SystemUser)))
+      end
+
+      def authorize_logged_in
+        authorize_by_with_raise(current_user.logged? && current_user.active? || current_user.is_a?(SystemUser))
+      end
+
       def raise_invalid_query_on_service_failure
         service = yield
 

lib/api/v3/users/users_api.rb

@@ -47,32 +47,18 @@ module API
               fail ::API::Errors::InvalidUserStatusTransition
             end
           end
-
-          def current_user_if_logged
-            if User.current.logged?
-              User.current
-            else
-              fail ::API::Errors::Unauthorized
-            end
-          end
-
-          def allow_only_admin
-            unless current_user.admin?
-              fail ::API::Errors::Unauthorized
-            end
-          end
         end
 
         resources :users do
           helpers ::API::V3::Users::CreateUser
 
           post do
-            allow_only_admin
+            authorize_admin
             create_user(request_body, current_user)
           end
 
           get do
-            allow_only_admin
+            authorize_admin
 
             query = ParamsToQueryService.new(User, current_user).call(params)
 
@@ -95,9 +81,11 @@ module API
             helpers ::API::V3::Users::UpdateUser
 
             before do
+              authorize_logged_in
+
               @user =
                 if params[:id] == 'me'
-                  current_user_if_logged
+                  User.current
                 else
                   User.find_by_unique!(params[:id])
                 end
@@ -108,7 +96,7 @@ module API
             end
 
             patch do
-              allow_only_admin
+              authorize_admin
               update_user(request_body, current_user)
             end
 
@@ -123,7 +111,7 @@ module API
             namespace :lock do
               # Authenticate lock transitions
               before do
-                allow_only_admin
+                authorize_admin
               end
 
               desc 'Set lock on user account'

spec/lib/api/v3/queries/query_representer_generation_spec.rb

@@ -33,7 +33,7 @@ describe ::API::V3::Queries::QueryRepresenter do
 
   let(:query) { FactoryBot.build_stubbed(:query, project: project) }
   let(:project) { FactoryBot.build_stubbed(:project) }
-  let(:user) { double('current_user', allowed_to?: true, admin: true, admin?: true) }
+  let(:user) { double('current_user', allowed_to?: true, admin: true, admin?: true, active?: true) }
   let(:embed_links) { true }
   let(:representer) do
     described_class.new(query, current_user: user, embed_links: embed_links)

spec/requests/api/v3/authentication_spec.rb

@@ -31,7 +31,7 @@ require 'spec_helper'
 describe API::V3, type: :request do
   describe 'basic auth' do
     let(:user)     { FactoryBot.create :user }
-    let(:resource) { "/api/v3/users/#{user.id}" }
+    let(:resource) { "/api/v3/projects" }
 
     let(:response_401) do
       {

spec/requests/api/v3/user/user_resource_spec.rb

@@ -35,20 +35,25 @@ describe 'API v3 User resource', type: :request, content_type: :json do
 
   let(:current_user) { FactoryBot.create(:user) }
   let(:user) { FactoryBot.create(:user) }
+  let(:admin) { FactoryBot.create(:admin) }
+  let(:locked_admin) { FactoryBot.create :admin, status: Principal::STATUSES[:locked] }
 
   subject(:response) { last_response }
 
+  before do
+    allow(User).to receive(:current).and_return current_user
+  end
+
   describe '#index' do
     let(:get_path) { api_v3_paths.users }
 
     before do
       user
-      allow(User).to receive(:current).and_return current_user
       get get_path
     end
 
     context 'admin user' do
-      let(:current_user) { FactoryBot.create(:admin) }
+      let(:current_user) { admin }
 
       it 'should respond with 200' do
         expect(subject.status).to eq(200)
@@ -58,13 +63,13 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       # meaning the order in which they where saved
       it 'contains the user in the response' do
         expect(subject.body)
-          .to be_json_eql(user.name.to_json)
+          .to be_json_eql(current_user.name.to_json)
           .at_path('_embedded/elements/0/name')
       end
 
       it 'contains the current user in the response' do
         expect(subject.body)
-          .to be_json_eql(current_user.name.to_json)
+          .to be_json_eql(user.name.to_json)
           .at_path('_embedded/elements/1/name')
       end
 
@@ -79,7 +84,7 @@ describe 'API v3 User resource', type: :request, content_type: :json do
 
         it 'contains the current user in the response' do
           expect(subject.body)
-            .to be_json_eql(current_user.name.to_json)
+            .to be_json_eql(user.name.to_json)
             .at_path('_embedded/elements/0/name')
         end
       end
@@ -147,21 +152,25 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       end
     end
 
+    context 'locked admin' do
+      let(:current_user) { locked_admin }
+
+      it_behaves_like 'unauthorized access'
+    end
+
     context 'other user' do
-      it 'should respond with 403' do
-        expect(subject.status).to eq(403)
-      end
+      it_behaves_like 'unauthorized access'
     end
   end
 
   describe '#get' do
-    context 'logged in user' do
-      let(:get_path) { api_v3_paths.user user.id }
-      before do
-        allow(User).to receive(:current).and_return current_user
-        get get_path
-      end
+    let(:get_path) { api_v3_paths.user user.id }
+
+    before do
+      get get_path
+    end
 
+    context 'logged in user' do
       it 'should respond with 200' do
         expect(subject.status).to eq(200)
       end
@@ -191,10 +200,6 @@ describe 'API v3 User resource', type: :request, content_type: :json do
 
     context 'get with login' do
       let(:get_path) { api_v3_paths.user user.login }
-      before do
-        allow(User).to receive(:current).and_return current_user
-        get get_path
-      end
 
       it 'should respond with 200' do
         expect(subject.status).to eq(200)
@@ -208,6 +213,12 @@ describe 'API v3 User resource', type: :request, content_type: :json do
     it_behaves_like 'handling anonymous user' do
       let(:path) { api_v3_paths.user user.id }
     end
+
+    context 'locked admin' do
+      let(:current_user) { locked_admin }
+
+      it_behaves_like 'unauthorized access'
+    end
   end
 
   describe '#delete' do
@@ -216,15 +227,13 @@ describe 'API v3 User resource', type: :request, content_type: :json do
     let(:self_delete) { true }
 
     before do
-      allow(User).to receive(:current).and_return current_user
-
       allow(Setting).to receive(:users_deletable_by_admins?).and_return(admin_delete)
       allow(Setting).to receive(:users_deletable_by_self?).and_return(self_delete)
 
       delete path
     end
 
-    shared_examples 'deletion through allowed user' do
+    shared_examples 'deletion allowed' do
       it 'should respond with 202' do
         expect(subject.status).to eq 202
       end
@@ -241,14 +250,6 @@ describe 'API v3 User resource', type: :request, content_type: :json do
           let(:type) { 'User' }
         end
       end
-
-      context 'with non-admin user' do
-        let(:current_user) { FactoryBot.create :user, admin: false }
-
-        it 'responds with 403' do
-          expect(subject.status).to eq 403
-        end
-      end
     end
 
     shared_examples 'deletion is not allowed' do
@@ -262,12 +263,12 @@ describe 'API v3 User resource', type: :request, content_type: :json do
     end
 
     context 'as admin' do
-      let(:current_user) { FactoryBot.create :admin }
+      let(:current_user) { admin }
 
       context 'with users deletable by admins' do
         let(:admin_delete) { true }
 
-        it_behaves_like 'deletion through allowed user'
+        it_behaves_like 'deletion allowed'
       end
 
       context 'with users not deletable by admins' do
@@ -277,6 +278,12 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       end
     end
 
+    context 'as locked admin' do
+      let(:current_user) { locked_admin }
+
+      it_behaves_like 'deletion is not allowed'
+    end
+
     context 'as non-admin' do
       let(:current_user) { FactoryBot.create :user, admin: false }
 
@@ -289,7 +296,7 @@ describe 'API v3 User resource', type: :request, content_type: :json do
       context 'with self-deletion allowed' do
         let(:self_delete) { true }
 
-        it_behaves_like 'deletion through allowed user'
+        it_behaves_like 'deletion allowed'
       end
 
       context 'with self-deletion not allowed' do