Skip to content

Commit a780fbb

Browse files
committed
Adapt internal user v2 token handling to hashed tokens
With `hash_token_secrets` enabled, `token.plaintext_token` is only available at creation or rotation time and cannot be retrieved later. This change ensures secure handling of API tokens in line with best practices for hashed token storage. - Update `InternalUserAccessTokenService#rotate!` to return `token.plaintext_token` at creation time - Pass the plaintext token from InternalUserAccessTokensController#create to `app/views/devise/registrations/_v2_api_token.html.erb` - In `_v2_api_token.html.erb`, render the plaintext token when available and display a warning to users to copy and store the token securely, as it will not be shown again after leaving or refreshing the page. - Updated all affected Spec files as well. - The `context 'when user is not authenticated' do` test has been updated. It now enables CSRF protection, enabling the test to accurately capture the behaviour that will be captured in production.
1 parent fdae153 commit a780fbb

File tree

9 files changed

+65
-85
lines changed

9 files changed

+65
-85
lines changed

app/controllers/api/v2/internal_user_access_tokens_controller.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ class InternalUserAccessTokensController < ApplicationController
99
# POST "/api/v2/internal_user_access_token"
1010
def create
1111
authorize current_user, :internal_user_v2_access_token?
12-
@token = Api::V2::InternalUserAccessTokenService.rotate!(current_user)
12+
@v2_token = Api::V2::InternalUserAccessTokenService.rotate!(current_user)
1313
@success = true
1414
respond_to do |format|
1515
format.js { render 'users/refresh_token' }

app/services/api/v2/internal_user_access_token_service.rb

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,24 +24,16 @@ class InternalUserAccessTokenService
2424
INTERNAL_OAUTH_APP_NAME = Rails.application.config.x.application.internal_oauth_app_name
2525

2626
class << self
27-
def for_user(user)
28-
Doorkeeper::AccessToken.find_by(
29-
application_id: application!.id,
30-
resource_owner_id: user.id,
31-
scopes: READ_SCOPE,
32-
revoked_at: nil
33-
)
34-
end
35-
3627
def rotate!(user)
3728
revoke_existing!(user)
3829

39-
Doorkeeper::AccessToken.create!(
30+
token = Doorkeeper::AccessToken.create!(
4031
application_id: application!.id,
4132
resource_owner_id: user.id,
4233
scopes: READ_SCOPE,
4334
expires_in: nil # Overrides Doorkeeper's `access_token_expires_in`
4435
)
36+
token.plaintext_token
4537
end
4638

4739
# Used by views (e.g. devise/registrations/_v2_api_token.html.erb) to safely

app/views/devise/registrations/_api_token.html.erb

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
<%# locals: user %>
2+
<% v2_token ||= nil %>
23

34
<div id="api-tokens">
45
<%# v2 API token %>
5-
<%= render partial: "devise/registrations/v2_api_token", locals: { user: user } %>
6+
<%= render partial: "devise/registrations/v2_api_token", locals: { user: user, token: v2_token } %>
67

78
<% if user.can_use_api? %>
89
<%# v0/v1 API token %>

app/views/devise/registrations/_v2_api_token.html.erb

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,19 @@
66
</div>
77
<div class="card-body">
88
<% if Api::V2::InternalUserAccessTokenService.application_present? %>
9-
<% token = Api::V2::InternalUserAccessTokenService.for_user(user) %>
109
<div class="form-control mb-3 col-xs-8">
1110
<%= label_tag(:api_token, _('Access token'), class: 'form-label') %>
1211
<% if token.present? %>
13-
<code><%= token.token %></code>
12+
<code><%= token %></code>
13+
<div class="alert alert-warning">
14+
<%= _( "Please copy this token now and store it somewhere safely." ) %><br>
15+
<%= _( "It will disappear after you leave or refresh this page." ) %>
16+
</div>
1417
<% else %>
15-
<%= _("Click the button below to generate an API token") %>
18+
<%= _( "Click the button below to generate an API token" ) %><br>
19+
<div class="alert alert-warning">
20+
<%= _("If you previously generated and saved a token, please continue using that token.") %>
21+
</div>
1622
<% end %>
1723
</div>
1824

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
1+
// This view is called by both InternalUserAccessTokensController#create (provides @v2_token)
2+
// and UsersController#refresh_token (does not provide @v2_token).
13
var msg = '<%= @success ? _("Successfully regenerated your API token.") : _("Unable to regenerate your API token.") %>';
24

35
var context = $('#api-tokens');
4-
context.html('<%= escape_javascript(render partial: "/devise/registrations/api_token", locals: { user: current_user }) %>');
6+
context.html('<%= escape_javascript(render partial: "/devise/registrations/api_token", locals: { user: current_user, v2_token: @v2_token }) %>');
57
renderNotice(msg);
68
toggleSpinner(false);

spec/requests/api/v2/internal_user_access_tokens_controller_spec.rb

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,11 @@ def post_create_token
4141
end.to change { Doorkeeper::AccessToken.count }.by(1)
4242
end
4343

44-
it 'assigns the token' do
44+
it 'assigns the plaintext token' do
4545
post_create_token
4646

47-
expect(assigns(:token)).to be_a(Doorkeeper::AccessToken)
48-
expect(assigns(:token).resource_owner_id).to eq(user.id)
47+
expect(assigns(:v2_token)).to be_a(String)
48+
expect(assigns(:v2_token)).not_to be_blank
4949
end
5050

5151
it 'renders the refresh_token template' do

spec/services/api/v2/internal_user_access_token_service_spec.rb

Lines changed: 29 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -11,56 +11,46 @@ def create_internal_user_access_token
1111
create(:oauth_access_token, application: oauth_app, resource_owner_id: user.id, scopes: 'read')
1212
end
1313

14-
describe '#for_user' do
15-
context 'when a token exists for the user' do
16-
let!(:access_token) do
17-
create_internal_user_access_token
18-
end
19-
20-
it 'returns the access token' do
21-
token = described_class.for_user(user)
22-
expect(token).to be_present
23-
expect(token.resource_owner_id).to eq(user.id)
24-
end
25-
end
26-
27-
context 'when no token exists for the user' do
28-
it 'returns nil' do
29-
token = described_class.for_user(user)
30-
expect(token).to be_nil
31-
end
32-
end
33-
end
34-
3514
describe '#rotate!' do
36-
def rotate_token_expectations(new_token, old_token = nil) # rubocop:disable Metrics/AbcSize
37-
expect(new_token).to be_persisted
15+
def rotate_token_expectations(plaintext_token, old_token = nil) # rubocop:disable Metrics/AbcSize
16+
# Doorkeeper hashes token via Digest::SHA256
17+
hashed = Digest::SHA256.hexdigest(plaintext_token)
18+
new_token = Doorkeeper::AccessToken.find_by!(token: hashed)
19+
expect(new_token).to be_present
3820
expect(new_token.resource_owner_id).to eq(user.id)
3921
expect(new_token.revoked_at).to be_nil
4022
expect(new_token.scopes.to_s).to include('read')
41-
return unless old_token
42-
43-
expect(new_token).not_to eq(old_token)
44-
expect(old_token.revoked_at).not_to be_nil
23+
expect(old_token.revoked_at).not_to be_nil if old_token
4524
end
4625

47-
context 'when a token already exists' do
48-
let!(:old_token) do
49-
create_internal_user_access_token
26+
shared_examples 'token rotation' do |has_old_token|
27+
it "#{if has_old_token
28+
'revokes the old token and creates a new one'
29+
else
30+
'creates a new token'
31+
end}
32+
(returns plaintext)" do
33+
plaintext_token = nil
34+
# Ensure .rotate!(user) creates a new AccessToken db entry for user
35+
expect { plaintext_token = described_class.rotate!(user) }
36+
.to change { Doorkeeper::AccessToken.where(resource_owner_id: user.id).count }
37+
.by(1)
38+
if has_old_token
39+
old_token.reload
40+
rotate_token_expectations(plaintext_token, old_token)
41+
else
42+
rotate_token_expectations(plaintext_token)
43+
end
5044
end
45+
end
5146

52-
it 'revokes the old token and creates a new one' do
53-
new_token = described_class.rotate!(user)
54-
old_token.reload
55-
rotate_token_expectations(new_token, old_token)
56-
end
47+
context 'when a token already exists' do
48+
let!(:old_token) { create_internal_user_access_token }
49+
include_examples 'token rotation', true
5750
end
5851

5952
context 'when no token exists' do
60-
it 'creates a new token' do
61-
token = described_class.rotate!(user)
62-
rotate_token_expectations(token)
63-
end
53+
include_examples 'token rotation', false
6454
end
6555
end
6656

spec/views/devise/registrations/_api_token.html.erb_spec.rb

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,6 @@
66
let(:app_name) { Rails.application.config.x.application.internal_oauth_app_name }
77
let!(:oauth_app) { create(:oauth_application, name: app_name) }
88

9-
before do
10-
# Clear memoization between tests
11-
Api::V2::InternalUserAccessTokenService.instance_variable_set(:@application, nil)
12-
end
13-
149
context 'When a user has the `use_api` permission' do
1510
it 'renders both the v2 and legacy API token sections' do
1611
user = create(:user, :org_admin)

spec/views/devise/registrations/_v2_api_token.html.erb_spec.rb

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,36 +6,32 @@
66
let(:user) { create(:user) }
77
let(:app_name) { Rails.application.config.x.application.internal_oauth_app_name }
88

9+
def render_token_partial(token: nil)
10+
render partial: 'devise/registrations/v2_api_token', locals: { user: user, token: token }
11+
end
12+
913
context 'when the OAuth application exists' do
1014
let!(:oauth_app) { create(:oauth_application, name: app_name) }
1115

12-
it 'displays the regenerate button' do
13-
render partial: 'devise/registrations/v2_api_token', locals: { user: user }
14-
15-
expect(rendered).to have_link('Regenerate token',
16-
href: api_v2_internal_user_access_token_path)
16+
it 'displays the regenerate button when no token is present' do
17+
render_token_partial(token: nil)
18+
expect(rendered).to have_selector('button', text: 'Regenerate token')
1719
end
1820

1921
context 'when user has a token' do
20-
let!(:token) do
21-
create(:oauth_access_token,
22-
application: oauth_app,
23-
resource_owner_id: user.id,
24-
scopes: 'read')
25-
end
22+
let(:plaintext_token) { 'plaintext-token-value' }
2623

27-
it 'displays the token' do
28-
render partial: 'devise/registrations/v2_api_token', locals: { user: user }
29-
30-
expect(rendered).to have_selector('code', text: token.token)
24+
it 'displays the token and disables the regenerate button' do
25+
render_token_partial(token: plaintext_token)
26+
expect(rendered).to have_selector('code', text: plaintext_token)
3127
expect(rendered).not_to have_content('Click the button below to generate an API token')
28+
expect(rendered).to have_selector('button[disabled]', text: 'Regenerate token')
3229
end
3330
end
3431

3532
context 'when user does not have a token' do
3633
it 'displays the generate message' do
37-
render partial: 'devise/registrations/v2_api_token', locals: { user: user }
38-
34+
render_token_partial(token: nil)
3935
expect(rendered).to have_content('Click the button below to generate an API token')
4036
expect(rendered).not_to have_selector('code')
4137
end
@@ -44,17 +40,15 @@
4440

4541
context 'when the OAuth application does not exist' do
4642
it 'displays the warning message and helpdesk email link' do
47-
render partial: 'devise/registrations/v2_api_token', locals: { user: user }
48-
43+
render_token_partial(token: nil)
4944
expect(rendered).to have_selector('.alert-warning')
5045
expect(rendered).to have_content('V2 API token service is currently unavailable')
5146
expect(rendered).to have_link(href: "mailto:#{Rails.application.config.x.organisation.helpdesk_email}")
5247
end
5348

5449
it 'does not display the token or regenerate button' do
55-
render partial: 'devise/registrations/v2_api_token', locals: { user: user }
56-
57-
expect(rendered).not_to have_link('Regenerate token')
50+
render_token_partial(token: nil)
51+
expect(rendered).not_to have_selector('button', text: 'Regenerate token')
5852
expect(rendered).not_to have_selector('code')
5953
end
6054
end

0 commit comments

Comments
 (0)