From b94fca824b63921185978378d1afe8d3c7b2a255 Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Sun, 30 Mar 2025 10:33:49 +1000 Subject: [PATCH 1/8] feat: Add admin API to revoke user support PIN Allow administrators to revoke a Support PIN before its natural expiration by setting it to expire immediately. Related to #528035 Changelog: added --- .../users/support_pin/revoke_service.rb | 26 ++++++++++++++++++ lib/api/users.rb | 27 +++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 app/services/users/support_pin/revoke_service.rb diff --git a/app/services/users/support_pin/revoke_service.rb b/app/services/users/support_pin/revoke_service.rb new file mode 100644 index 00000000000000..a6c099b1e84501 --- /dev/null +++ b/app/services/users/support_pin/revoke_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module Users + module SupportPin + class RevokeService < SupportPin::BaseService + def execute + revoked = revoke_pin + + if revoked + { status: :success } + else + { status: :error, message: 'Failed to revoke support PIN' } + end + end + + private + + def revoke_pin + Gitlab::Redis::Cache.with do |redis| + key = pin_key + redis.expire(key, 1) # Set to expire in 1 second + end + end + end + end +end diff --git a/lib/api/users.rb b/lib/api/users.rb index b48fe9c28ab99f..fce1495dfa1008 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -129,6 +129,33 @@ def reorder_users(users) end end + desc 'Revoke support PIN for a user. Available only for admins.' do + detail 'This feature allows administrators to revoke the support PIN for a specified user before its natural expiration' + success code: 204 + is_array false + end + params do + requires :id, type: Integer, desc: 'The ID of the user' + end + post ":id/support_pin/revoke", feature_category: :user_management do + authenticated_as_admin! + + user = User.find_by_id(params[:id]) + not_found!('User') unless user + + begin + result = ::Users::SupportPin::RevokeService.new(user).execute + rescue StandardError + error!("Error revoking Support PIN for user.", :unprocessable_entity) + end + + if result&.fetch(:status) == :success + status :accepted + else + error!(result&.fetch(:message, "Failed to revoke Support PIN"), :bad_request) + end + end + desc 'Get the list of users' do success Entities::UserBasic end -- GitLab From bd288cb871d3b1cdf0b198973c3a0f7bf23c37d7 Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Mon, 31 Mar 2025 11:56:16 +1000 Subject: [PATCH 2/8] Improve Support PIN revocation behavior Check PIN existence before attempting revocation and return appropriate error message when PIN not found. --- app/services/users/support_pin/base_service.rb | 6 ++++++ app/services/users/support_pin/revoke_service.rb | 2 ++ lib/api/users.rb | 7 +++++-- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/app/services/users/support_pin/base_service.rb b/app/services/users/support_pin/base_service.rb index cc363c38403a19..6fa55f33e2ec38 100644 --- a/app/services/users/support_pin/base_service.rb +++ b/app/services/users/support_pin/base_service.rb @@ -13,6 +13,12 @@ def initialize(user) def pin_key "#{SUPPORT_PIN_PREFIX}:#{@user.id}" end + + def pin_exists? + Gitlab::Redis::Cache.with do |redis| + redis.exists(pin_key).to_i > 0 + end + end end end end diff --git a/app/services/users/support_pin/revoke_service.rb b/app/services/users/support_pin/revoke_service.rb index a6c099b1e84501..6a911185aa9ece 100644 --- a/app/services/users/support_pin/revoke_service.rb +++ b/app/services/users/support_pin/revoke_service.rb @@ -4,6 +4,8 @@ module Users module SupportPin class RevokeService < SupportPin::BaseService def execute + return { status: :not_found, message: 'Support PIN not found or already expired' } unless pin_exists? + revoked = revoke_pin if revoked diff --git a/lib/api/users.rb b/lib/api/users.rb index fce1495dfa1008..f20093f7cafb2e 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -149,10 +149,13 @@ def reorder_users(users) error!("Error revoking Support PIN for user.", :unprocessable_entity) end - if result&.fetch(:status) == :success + case result[:status] + when :success status :accepted + when :not_found + not_found!(result[:message]) else - error!(result&.fetch(:message, "Failed to revoke Support PIN"), :bad_request) + error!(result[:message] || "Failed to revoke Support PIN", :bad_request) end end -- GitLab From 65c65f681183f2c554c60593147f89fff0f638c3 Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Thu, 10 Apr 2025 15:52:05 +1000 Subject: [PATCH 3/8] Add tests for Support PIN revocation Add tests for the RevokeService and the admin API endpoint that revokes user Support PINs. Verify PIN removal through API calls to ensure proper functionality. --- spec/requests/api/users_spec.rb | 37 ++++++++++++ .../users/support_pin/revoke_service_spec.rb | 57 +++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 spec/services/users/support_pin/revoke_service_spec.rb diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 51bcbd4e3de067..f41f6ca477ba04 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -5557,4 +5557,41 @@ def request end end end + + describe 'POST /users/:id/support_pin/revoke' do + let(:path) { "/users/#{user.id}/support_pin/revoke" } + + context 'when current user is an admin' do + context 'when a PIN exists' do + it 'returns accepted status' do + post api(path, admin, admin_mode: true) + + expect(response).to have_gitlab_http_status(:accepted) + end + + it 'revokes the pin' do + post api(path, admin, admin_mode: true) + + # Short sleep to allow Redis to expire key + sleep(1) + + # Verify PIN is no longer accessible after revocation + get api("/users/#{user.id}/support_pin", admin, admin_mode: true) + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when no PIN exists' do + it 'returns not found' do + post api(path, admin, admin_mode: true) + + # Short sleep to allow Redis to expire key + sleep(1) + + expect(response).to have_gitlab_http_status(:not_found) + expect(json_response['message']).to include('Support PIN not found or already expired') + end + end + end + end end diff --git a/spec/services/users/support_pin/revoke_service_spec.rb b/spec/services/users/support_pin/revoke_service_spec.rb new file mode 100644 index 00000000000000..d85aecdf0594ef --- /dev/null +++ b/spec/services/users/support_pin/revoke_service_spec.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Users::SupportPin::RevokeService, feature_category: :user_management do + let(:user) { create(:user) } + let(:service) { described_class.new(user) } + + describe '#execute' do + context 'when a PIN exists' do + before do + # Create a PIN using the UpdateService + Users::SupportPin::UpdateService.new(user).execute + end + + it 'revokes the PIN successfully' do + # Verify PIN exists before revocation + expect(Users::SupportPin::RetrieveService.new(user).execute).not_to be_nil + + result = service.execute + expect(result[:status]).to eq(:success) + + # Give Redis time to expire the key + sleep(1.1) + + # Verify PIN is no longer accessible after revocation + expect(Users::SupportPin::RetrieveService.new(user).execute).to be_nil + end + end + + context 'when no PIN exists' do + it 'returns not_found status' do + result = service.execute + + expect(result[:status]).to eq(:not_found) + expect(result[:message]).to eq('Support PIN not found or already expired') + end + end + + context 'when Redis operation fails' do + before do + # Create a PIN first + Users::SupportPin::UpdateService.new(user).execute + + allow_next_instance_of(described_class) do |instance| + allow(instance).to receive(:revoke_pin).and_return(false) + end + end + + it 'returns an error' do + result = service.execute + + expect(result).to eq({ status: :error, message: 'Failed to revoke support PIN' }) + end + end + end +end -- GitLab From bcbff3596ccbc8975e3c31ccfbc22d6aab9fac86 Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Thu, 10 Apr 2025 16:09:06 +1000 Subject: [PATCH 4/8] Make Support PIN revocation immediate Set Redis key expiry to 0 seconds for immediate expiration rather than 1 second. This improves reliability and removes need for delays in tests. --- app/services/users/support_pin/revoke_service.rb | 2 +- spec/requests/api/users_spec.rb | 6 ------ spec/services/users/support_pin/revoke_service_spec.rb | 3 --- 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/app/services/users/support_pin/revoke_service.rb b/app/services/users/support_pin/revoke_service.rb index 6a911185aa9ece..aecdddaf030938 100644 --- a/app/services/users/support_pin/revoke_service.rb +++ b/app/services/users/support_pin/revoke_service.rb @@ -20,7 +20,7 @@ def execute def revoke_pin Gitlab::Redis::Cache.with do |redis| key = pin_key - redis.expire(key, 1) # Set to expire in 1 second + redis.expire(key, 0) # Set to expire immediately end end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index f41f6ca477ba04..f1de0211e7d018 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -5572,9 +5572,6 @@ def request it 'revokes the pin' do post api(path, admin, admin_mode: true) - # Short sleep to allow Redis to expire key - sleep(1) - # Verify PIN is no longer accessible after revocation get api("/users/#{user.id}/support_pin", admin, admin_mode: true) expect(response).to have_gitlab_http_status(:not_found) @@ -5585,9 +5582,6 @@ def request it 'returns not found' do post api(path, admin, admin_mode: true) - # Short sleep to allow Redis to expire key - sleep(1) - expect(response).to have_gitlab_http_status(:not_found) expect(json_response['message']).to include('Support PIN not found or already expired') end diff --git a/spec/services/users/support_pin/revoke_service_spec.rb b/spec/services/users/support_pin/revoke_service_spec.rb index d85aecdf0594ef..3951eb8bd06d7b 100644 --- a/spec/services/users/support_pin/revoke_service_spec.rb +++ b/spec/services/users/support_pin/revoke_service_spec.rb @@ -20,9 +20,6 @@ result = service.execute expect(result[:status]).to eq(:success) - # Give Redis time to expire the key - sleep(1.1) - # Verify PIN is no longer accessible after revocation expect(Users::SupportPin::RetrieveService.new(user).execute).to be_nil end -- GitLab From ad9d1ceadde27e15c54f95dafb993a2df11b732d Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Fri, 11 Apr 2025 13:04:32 +1000 Subject: [PATCH 5/8] docs: Add API for Support PIN revocation Document the new admin API endpoint for revoking user Support PINs before their natural expiration date. --- doc/api/users.md | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/doc/api/users.md b/doc/api/users.md index 9c57b8ea04c7c0..3bc8a7b7099aa7 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -1584,3 +1584,48 @@ Supported attributes: | Attribute | Type | Required | Description | |:-----------------------|:---------|:---------|:------------| | `id` | integer | yes | ID of user account | + +## Revoke a Support PIN for a user + +{{< details >}} + +- Tier: Free, Premium, Ultimate +- Offering: GitLab.com, GitLab Self-Managed, GitLab Dedicated + +{{< /details >}} + +{{< history >}} + +- [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/187657) +in GitLab 17.11. + +{{< /history >}} + +Revokes a Support PIN for the specified user before its natural expiration. +This immediately expires the PIN, which removes it. + +Prerequisites: + +- You must be an administrator. + +```plaintext +POST /users/:id/support_pin/revoke +``` + +Example request: + +```shell +curl --request POST \ + --header "PRIVATE-TOKEN: " \ + "https://gitlab.example.com/api/v4/users/1234/support_pin/revoke" +``` + +Example response: + +If successful, returns an HTTP `202` Accepted status code. + +Supported attributes: + +| Attribute | Type | Required | Description | +|:-------------|:----------|:---------|:--------------------| +| `id` | integer | yes | ID of user account | -- GitLab From 10e565b662e225dd8088dd998d68e7f35de61488 Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Mon, 14 Apr 2025 16:19:00 +1000 Subject: [PATCH 6/8] Add non-admin user tests for Support PIN revocation API Verify that non-admin users cannot revoke Support PINs. --- spec/requests/api/users_spec.rb | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index f1de0211e7d018..e2b15b9ccc58f7 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -5587,5 +5587,37 @@ def request end end end + + context 'when current user is not an admin' do + before do + # First authenticate as the user to create their own PIN + post api("/user/support_pin", user) + end + + it 'returns forbidden' do + # Attempt to revoke as non-admin + post api(path, user) + + expect(response).to have_gitlab_http_status(:forbidden) + end + + it 'does not revoke the PIN' do + # Attempt to revoke as non-admin + post api(path, user) + + # Verify PIN still exists via API + get api('/user/support_pin', user) + + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'when user is not authenticated' do + it 'returns unauthorized' do + post api(path) + + expect(response).to have_gitlab_http_status(:unauthorized) + end + end end end -- GitLab From 752edadee8d2d317f2348a800a9ad03e2470e9f8 Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Tue, 15 Apr 2025 10:27:22 +1000 Subject: [PATCH 7/8] test: Cover error handling in Support PIN revocation API Add tests for service exceptions and error status responses to resolve test undercoverage. --- spec/requests/api/users_spec.rb | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index e2b15b9ccc58f7..25c7720e6b84bf 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -5586,6 +5586,36 @@ def request expect(json_response['message']).to include('Support PIN not found or already expired') end end + + context 'when an error occurs during revocation' do + before do + allow_next_instance_of(Users::SupportPin::RevokeService) do |instance| + allow(instance).to receive(:execute).and_raise(StandardError, 'Something went wrong') + end + end + + it 'returns unprocessable_entity' do + post api(path, admin, admin_mode: true) + + expect(response).to have_gitlab_http_status(:unprocessable_entity) + expect(json_response['error']).to eq('Error revoking Support PIN for user.') + end + end + + context 'when the service returns an error status' do + before do + allow_next_instance_of(Users::SupportPin::RevokeService) do |instance| + allow(instance).to receive(:execute).and_return({ status: :error, message: 'Service error' }) + end + end + + it 'returns bad_request' do + post api(path, admin, admin_mode: true) + + expect(response).to have_gitlab_http_status(:bad_request) + expect(json_response['error']).to eq('Service error') + end + end end context 'when current user is not an admin' do -- GitLab From b194f2fd2c142fcf0dbe7b573c12a9023252473d Mon Sep 17 00:00:00 2001 From: Michael Trainor Date: Tue, 15 Apr 2025 12:15:19 +1000 Subject: [PATCH 8/8] Apply 3 suggestion(s) to 1 file(s) Co-authored-by: Isaac Durham --- doc/api/users.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/api/users.md b/doc/api/users.md index 3bc8a7b7099aa7..d83cce3b3b61ea 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -1602,7 +1602,7 @@ in GitLab 17.11. {{< /history >}} Revokes a Support PIN for the specified user before its natural expiration. -This immediately expires the PIN, which removes it. +This immediately expires and removes the PIN. Prerequisites: @@ -1617,15 +1617,15 @@ Example request: ```shell curl --request POST \ --header "PRIVATE-TOKEN: " \ - "https://gitlab.example.com/api/v4/users/1234/support_pin/revoke" + --url "https://gitlab.example.com/api/v4/users/1234/support_pin/revoke" ``` Example response: -If successful, returns an HTTP `202` Accepted status code. +If successful, returns `202 Accepted`. Supported attributes: | Attribute | Type | Required | Description | |:-------------|:----------|:---------|:--------------------| -| `id` | integer | yes | ID of user account | +| `id` | integer | yes | ID of a user | -- GitLab