Commit 5f4afef6 authored by Saikat Sarkar's avatar Saikat Sarkar

Avoid unnecessary Sidekiq retries for Security::TokenRevocationService

parent 84947413
...@@ -11,7 +11,10 @@ module Security ...@@ -11,7 +11,10 @@ module Security
end end
def execute def execute
raise RevocationFailedError, 'Missing revocation token data' if missing_token_data?
return error('Token revocation is disabled') unless token_revocation_enabled? return error('Token revocation is disabled') unless token_revocation_enabled?
return success if revoke_token_body.blank?
response = revoke_tokens response = revoke_tokens
response.success? ? success : error('Failed to revoke tokens') response.success? ? success : error('Failed to revoke tokens')
...@@ -29,11 +32,9 @@ module Security ...@@ -29,11 +32,9 @@ module Security
end end
def revoke_tokens def revoke_tokens
raise RevocationFailedError, 'Missing revocation tokens data' if missing_token_data?
::Gitlab::HTTP.post( ::Gitlab::HTTP.post(
token_revocation_url, token_revocation_url,
body: message, body: revoke_token_body,
headers: { headers: {
'Content-Type' => 'application/json', 'Content-Type' => 'application/json',
'Authorization' => revocation_api_token 'Authorization' => revocation_api_token
...@@ -54,23 +55,25 @@ module Security ...@@ -54,23 +55,25 @@ module Security
) )
end end
def message def revoke_token_body
response = ::Gitlab::HTTP.get( @revoke_token_body ||= begin
token_types_url, response = ::Gitlab::HTTP.get(
headers: { token_types_url,
'Content-Type' => 'application/json', headers: {
'Authorization' => revocation_api_token 'Content-Type' => 'application/json',
} 'Authorization' => revocation_api_token
) }
raise RevocationFailedError, 'Failed to get revocation token types' unless response.success? )
raise RevocationFailedError, 'Failed to get revocation token types' unless response.success?
token_types = ::Gitlab::Json.parse(response.body)['types']
raise RevocationFailedError, 'No token type is available' if token_types.blank? token_types = ::Gitlab::Json.parse(response.body)['types']
return if token_types.blank?
@revocable_keys.filter! { |key| token_types.include?(key[:type]) }
raise RevocationFailedError, 'No revocable key is present' if @revocable_keys.blank? @revocable_keys.filter! { |key| token_types.include?(key[:type]) }
return if @revocable_keys.blank?
@revocable_keys.to_json
@revocable_keys.to_json
end
end end
def token_types_url def token_types_url
......
---
title: Avoid unnecessary Sidekiq retries for Security::TokenRevocationService
merge_request: 48636
author:
type: performance
...@@ -49,13 +49,13 @@ RSpec.describe Security::TokenRevocationService, '#execute' do ...@@ -49,13 +49,13 @@ RSpec.describe Security::TokenRevocationService, '#execute' do
end end
end end
context 'when revocation token API returns invalid token types' do context 'when revocation token types API returns empty list of types' do
before do before do
stub_application_setting(secret_detection_token_revocation_enabled: true) stub_application_setting(secret_detection_token_revocation_enabled: true)
stub_invalid_token_types_api_with_success stub_invalid_token_types_api_with_success
end end
specify { expect(subject).to eql({ message: 'No token type is available', status: :error }) } specify { expect(subject).to eql({ status: :success }) }
end end
context 'when revocation service is disabled' do context 'when revocation service is disabled' do
...@@ -84,7 +84,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do ...@@ -84,7 +84,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do
end end
end end
specify { expect(subject).to eql({ message: 'Missing revocation tokens data', status: :error }) } specify { expect(subject).to eql({ message: 'Missing revocation token data', status: :error }) }
end end
context 'when token_types_url is missing' do context 'when token_types_url is missing' do
...@@ -94,7 +94,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do ...@@ -94,7 +94,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do
end end
end end
specify { expect(subject).to eql({ message: 'Missing revocation tokens data', status: :error }) } specify { expect(subject).to eql({ message: 'Missing revocation token data', status: :error }) }
end end
context 'when revocation_api_token is missing' do context 'when revocation_api_token is missing' do
...@@ -104,7 +104,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do ...@@ -104,7 +104,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do
end end
end end
specify { expect(subject).to eql({ message: 'Missing revocation tokens data', status: :error }) } specify { expect(subject).to eql({ message: 'Missing revocation token data', status: :error }) }
end end
context 'when there is no token to be revoked' do context 'when there is no token to be revoked' do
...@@ -112,7 +112,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do ...@@ -112,7 +112,7 @@ RSpec.describe Security::TokenRevocationService, '#execute' do
{ 'types': %w() } { 'types': %w() }
end end
specify { expect(subject).to eql({ message: 'No token type is available', status: :error }) } specify { expect(subject).to eql({ status: :success }) }
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment