API: Use POST to (un)block a user

changelogs/unreleased/api-post-block.yml

+---
+title: 'API: Use POST to (un)block a user'
+merge_request: 9371
+author: Robert Schilling

doc/api/users.md

@@ -659,14 +659,14 @@ Will return `200 OK` on success, or `404 Not found` if either user or email cann
 Blocks the specified user.  Available only for admin.
 
 ```
-PUT /users/:id/block
+POST /users/:id/block
 ```
 
 Parameters:
 
 - `id` (required) - id of specified user
 
-Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
+Will return `201 OK` on success, `404 User Not Found` is user cannot be found or
 `403 Forbidden` when trying to block an already blocked user by LDAP synchronization.
 
 ## Unblock user
@@ -674,14 +674,14 @@ Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
 Unblocks the specified user.  Available only for admin.
 
 ```
-PUT /users/:id/unblock
+POST /users/:id/unblock
 ```
 
 Parameters:
 
 - `id` (required) - id of specified user
 
-Will return `200 OK` on success, `404 User Not Found` is user cannot be found or
+Will return `201 OK` on success, `404 User Not Found` is user cannot be found or
 `403 Forbidden` when trying to unblock a user blocked by LDAP synchronization.
 
 ### Get user contribution events

doc/api/v3_to_v4.md

@@ -26,3 +26,4 @@ changes are in V4:
 - Endpoints `/projects/owned`, `/projects/visible`, `/projects/starred` & `/projects/all` are consolidated into `/projects` using query parameters
 - Return pagination headers for all endpoints that return an array
 - Removed `DELETE projects/:id/deploy_keys/:key_id/disable`. Use `DELETE projects/:id/deploy_keys/:key_id` instead
+- Moved `PUT /users/:id/(block|unblock)` to `POST /users/:id/(block|unblock)`

lib/api/users.rb

@@ -314,7 +314,7 @@ class Users < Grape::API
       params do
         requires :id, type: Integer, desc: 'The ID of the user'
       end
-      put ':id/block' do
+      post ':id/block' do
         authenticated_as_admin!
         user = User.find_by(id: params[:id])
         not_found!('User') unless user
@@ -330,7 +330,7 @@ class Users < Grape::API
       params do
         requires :id, type: Integer, desc: 'The ID of the user'
       end
-      put ':id/unblock' do
+      post ':id/unblock' do
         authenticated_as_admin!
         user = User.find_by(id: params[:id])
         not_found!('User') unless user

lib/api/v3/users.rb

@@ -39,6 +39,38 @@ class Users < Grape::API
 
           present user.emails, with: ::API::Entities::Email
         end
+
+        desc 'Block a user. Available only for admins.'
+        params do
+          requires :id, type: Integer, desc: 'The ID of the user'
+        end
+        put ':id/block' do
+          authenticated_as_admin!
+          user = User.find_by(id: params[:id])
+          not_found!('User') unless user
+
+          if !user.ldap_blocked?
+            user.block
+          else
+            forbidden!('LDAP blocked users cannot be modified by the API')
+          end
+        end
+
+        desc 'Unblock a user. Available only for admins.'
+        params do
+          requires :id, type: Integer, desc: 'The ID of the user'
+        end
+        put ':id/unblock' do
+          authenticated_as_admin!
+          user = User.find_by(id: params[:id])
+          not_found!('User') unless user
+
+          if user.ldap_blocked?
+            forbidden!('LDAP blocked users cannot be unblocked by the API')
+          else
+            user.activate
+          end
+        end
       end
 
       resource :user do

spec/requests/api/users_spec.rb

@@ -1003,69 +1003,69 @@
     end
   end
 
-  describe 'PUT /users/:id/block' do
+  describe 'POST /users/:id/block' do
     before { admin }
     it 'blocks existing user' do
-      put api("/users/#{user.id}/block", admin)
-      expect(response).to have_http_status(200)
+      post api("/users/#{user.id}/block", admin)
+      expect(response).to have_http_status(201)
       expect(user.reload.state).to eq('blocked')
     end
 
     it 'does not re-block ldap blocked users' do
-      put api("/users/#{ldap_blocked_user.id}/block", admin)
+      post api("/users/#{ldap_blocked_user.id}/block", admin)
       expect(response).to have_http_status(403)
       expect(ldap_blocked_user.reload.state).to eq('ldap_blocked')
     end
 
     it 'does not be available for non admin users' do
-      put api("/users/#{user.id}/block", user)
+      post api("/users/#{user.id}/block", user)
       expect(response).to have_http_status(403)
       expect(user.reload.state).to eq('active')
     end
 
     it 'returns a 404 error if user id not found' do
-      put api('/users/9999/block', admin)
+      post api('/users/9999/block', admin)
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 User Not Found')
     end
   end
 
-  describe 'PUT /users/:id/unblock' do
+  describe 'POST /users/:id/unblock' do
     let(:blocked_user)  { create(:user, state: 'blocked') }
     before { admin }
 
     it 'unblocks existing user' do
-      put api("/users/#{user.id}/unblock", admin)
-      expect(response).to have_http_status(200)
+      post api("/users/#{user.id}/unblock", admin)
+      expect(response).to have_http_status(201)
       expect(user.reload.state).to eq('active')
     end
 
     it 'unblocks a blocked user' do
-      put api("/users/#{blocked_user.id}/unblock", admin)
-      expect(response).to have_http_status(200)
+      post api("/users/#{blocked_user.id}/unblock", admin)
+      expect(response).to have_http_status(201)
       expect(blocked_user.reload.state).to eq('active')
     end
 
     it 'does not unblock ldap blocked users' do
-      put api("/users/#{ldap_blocked_user.id}/unblock", admin)
+      post api("/users/#{ldap_blocked_user.id}/unblock", admin)
       expect(response).to have_http_status(403)
       expect(ldap_blocked_user.reload.state).to eq('ldap_blocked')
     end
 
     it 'does not be available for non admin users' do
-      put api("/users/#{user.id}/unblock", user)
+      post api("/users/#{user.id}/unblock", user)
       expect(response).to have_http_status(403)
       expect(user.reload.state).to eq('active')
     end
 
     it 'returns a 404 error if user id not found' do
-      put api('/users/9999/block', admin)
+      post api('/users/9999/block', admin)
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 User Not Found')
     end
 
     it "returns a 404 for invalid ID" do
-      put api("/users/ASDF/block", admin)
+      post api("/users/ASDF/block", admin)
 
       expect(response).to have_http_status(404)
     end

spec/requests/api/v3/users_spec.rb

@@ -7,6 +7,7 @@
   let(:admin) { create(:admin) }
   let(:key)   { create(:key, user: user) }
   let(:email)   { create(:email, user: user) }
+  let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') }
 
   describe 'GET /user/:id/keys' do
     before { admin }
@@ -117,4 +118,72 @@
       end
     end
   end
+
+  describe 'PUT /users/:id/block' do
+    before { admin }
+    it 'blocks existing user' do
+      put v3_api("/users/#{user.id}/block", admin)
+      expect(response).to have_http_status(200)
+      expect(user.reload.state).to eq('blocked')
+    end
+
+    it 'does not re-block ldap blocked users' do
+      put v3_api("/users/#{ldap_blocked_user.id}/block", admin)
+      expect(response).to have_http_status(403)
+      expect(ldap_blocked_user.reload.state).to eq('ldap_blocked')
+    end
+
+    it 'does not be available for non admin users' do
+      put v3_api("/users/#{user.id}/block", user)
+      expect(response).to have_http_status(403)
+      expect(user.reload.state).to eq('active')
+    end
+
+    it 'returns a 404 error if user id not found' do
+      put v3_api('/users/9999/block', admin)
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 User Not Found')
+    end
+  end
+
+  describe 'PUT /users/:id/unblock' do
+    let(:blocked_user)  { create(:user, state: 'blocked') }
+    before { admin }
+
+    it 'unblocks existing user' do
+      put v3_api("/users/#{user.id}/unblock", admin)
+      expect(response).to have_http_status(200)
+      expect(user.reload.state).to eq('active')
+    end
+
+    it 'unblocks a blocked user' do
+      put v3_api("/users/#{blocked_user.id}/unblock", admin)
+      expect(response).to have_http_status(200)
+      expect(blocked_user.reload.state).to eq('active')
+    end
+
+    it 'does not unblock ldap blocked users' do
+      put v3_api("/users/#{ldap_blocked_user.id}/unblock", admin)
+      expect(response).to have_http_status(403)
+      expect(ldap_blocked_user.reload.state).to eq('ldap_blocked')
+    end
+
+    it 'does not be available for non admin users' do
+      put v3_api("/users/#{user.id}/unblock", user)
+      expect(response).to have_http_status(403)
+      expect(user.reload.state).to eq('active')
+    end
+
+    it 'returns a 404 error if user id not found' do
+      put v3_api('/users/9999/block', admin)
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 User Not Found')
+    end
+
+    it "returns a 404 for invalid ID" do
+      put v3_api("/users/ASDF/block", admin)
+
+      expect(response).to have_http_status(404)
+    end
+  end
 end