Merge branch 'sk/250679-fix-deactivate-user-api' into 'master'

Fix incorrect HTTP response in deactivate user API for internal user

See merge request gitlab-org/gitlab!43356

app/controllers/admin/users_controller.rb

@@ -72,6 +72,7 @@ class Admin::UsersController < Admin::ApplicationController
   def deactivate
     return redirect_back_or_admin_user(notice: _("Error occurred. A blocked user cannot be deactivated")) if user.blocked?
     return redirect_back_or_admin_user(notice: _("Successfully deactivated")) if user.deactivated?
+    return redirect_back_or_admin_user(notice: _("Internal users cannot be deactivated")) if user.internal?
     return redirect_back_or_admin_user(notice: _("The user you are trying to deactivate has been active in the past %{minimum_inactive_days} days and cannot be deactivated") % { minimum_inactive_days: ::User::MINIMUM_INACTIVE_DAYS }) unless user.can_be_deactivated?
 
     user.deactivate

app/models/user.rb

@@ -1711,7 +1711,7 @@ class User < ApplicationRecord
   end
 
   def can_be_deactivated?
-    active? && no_recent_activity?
+    active? && no_recent_activity? && !internal?
   end
 
   def last_active_at

app/views/admin/users/show.html.haml

@@ -150,26 +150,27 @@
 
       = render 'admin/users/user_detail_note'
 
-      - if @user.deactivated?
-        .card.border-info
-          .card-header.bg-info.text-white
-            Reactivate this user
-          .card-body
-            = render partial: 'admin/users/user_activation_effects'
-            %br
-            = link_to 'Activate user', activate_admin_user_path(@user), method: :put, class: "btn gl-button btn-info", data: { confirm: 'Are you sure?' }
-      - elsif @user.can_be_deactivated?
-        .card.border-warning
-          .card-header.bg-warning.text-white
-            Deactivate this user
-          .card-body
-            = render partial: 'admin/users/user_deactivation_effects'
-            %br
-            %button.btn.gl-button.btn-warning{ data: { 'gl-modal-action': 'deactivate',
-              content: 'You can always re-activate their account, their data will remain intact.',
-              url: deactivate_admin_user_path(@user),
-              username: sanitize_name(@user.name) } }
-              = s_('AdminUsers|Deactivate user')
+      - unless @user.internal?
+        - if @user.deactivated?
+          .card.border-info
+            .card-header.bg-info.text-white
+              Reactivate this user
+            .card-body
+              = render partial: 'admin/users/user_activation_effects'
+              %br
+              = link_to 'Activate user', activate_admin_user_path(@user), method: :put, class: "btn gl-button btn-info", data: { confirm: 'Are you sure?' }
+        - elsif @user.can_be_deactivated?
+          .card.border-warning
+            .card-header.bg-warning.text-white
+              Deactivate this user
+            .card-body
+              = render partial: 'admin/users/user_deactivation_effects'
+              %br
+              %button.btn.gl-button.btn-warning{ data: { 'gl-modal-action': 'deactivate',
+                content: 'You can always re-activate their account, their data will remain intact.',
+                url: deactivate_admin_user_path(@user),
+                username: sanitize_name(@user.name) } }
+                = s_('AdminUsers|Deactivate user')
 
       - if @user.blocked?
         .card.border-info

changelogs/unreleased/sk-250679-fix-deactivate-user-api.yml

+---
+title: Fix incorrect HTTP response in deactivate user API for internal user
+merge_request: 43356
+author: Sashi Kumar
+type: fixed

doc/api/users.md

@@ -1250,6 +1250,7 @@ Returns:
 - `403 Forbidden` when trying to deactivate a user:
   - Blocked by admin or by LDAP synchronization.
   - That has any activity in past 180 days. These users cannot be deactivated.
+  - That is internal.
 
 ## Activate user
 

lib/api/users.rb

@@ -547,10 +547,15 @@ module API
 
         unless user.can_be_deactivated?
           forbidden!('A blocked user cannot be deactivated by the API') if user.blocked?
+          forbidden!('An internal user cannot be deactivated by the API') if user.internal?
           forbidden!("The user you are trying to deactivate has been active in the past #{::User::MINIMUM_INACTIVE_DAYS} days and cannot be deactivated")
         end
 
-        user.deactivate
+        if user.deactivate
+          true
+        else
+          render_api_error!(user.errors.full_messages, 400)
+        end
       end
       # rubocop: enable CodeReuse/ActiveRecord
 

locale/gitlab.pot

@@ -14096,6 +14096,9 @@ msgstr ""
 msgid "Internal users"
 msgstr ""
 
+msgid "Internal users cannot be deactivated"
+msgstr ""
+
 msgid "Interval Pattern"
 msgstr ""
 

spec/controllers/admin/users_controller_spec.rb

@@ -190,6 +190,17 @@ RSpec.describe Admin::UsersController do
         expect(flash[:notice]).to eq('Error occurred. A blocked user cannot be deactivated')
       end
     end
+
+    context 'for an internal user' do
+      it 'does not deactivate the user' do
+        internal_user = User.alert_bot
+
+        put :deactivate, params: { id: internal_user.username }
+
+        expect(internal_user.reload.deactivated?).to be_falsey
+        expect(flash[:notice]).to eq('Internal users cannot be deactivated')
+      end
+    end
   end
 
   describe 'PUT block/:id' do

spec/models/user_spec.rb

@@ -2781,6 +2781,14 @@ RSpec.describe User do
 
       it_behaves_like 'eligible for deactivation'
     end
+
+    context 'a user who is internal' do
+      it 'returns false' do
+        internal_user = create(:user, :bot)
+
+        expect(internal_user.can_be_deactivated?).to be_falsey
+      end
+    end
   end
 
   describe "#contributed_projects" do

spec/requests/api/users_spec.rb

@@ -2482,6 +2482,17 @@ RSpec.describe API::Users, :do_not_mock_admin_mode do
           end
         end
 
+        context 'for an internal user' do
+          it 'returns 403' do
+            internal_user = User.alert_bot
+
+            post api("/users/#{internal_user.id}/deactivate", admin)
+
+            expect(response).to have_gitlab_http_status(:forbidden)
+            expect(json_response['message']).to eq('403 Forbidden  - An internal user cannot be deactivated by the API')
+          end
+        end
+
         context 'for a user that does not exist' do
           before do
             post api("/users/0/deactivate", admin)