Skip to content

Commit

Permalink
Remove button for re-generating access token for API user
Browse files Browse the repository at this point in the history
The "Re-generate" button revokes the access token and creates a new one
for the same API user and application. However, in the meantime
application(s) will continue to use the old (now revoked) access token
and thus API requests will fail. A separate step is needed to sync the
new valid access token to the relevant application container(s).

I suspect this is why the documentation [1] used by #govuk-2ndline-tech
recommends first creating a new access token and only revoking the old
one once the new one has been synced to the relevant application
container(s) and is confirmed to be working OK.

I've also looked at the count of `EventLog` records in production which
seems to confirm that the "Re-generate" button has rarely been used:
* `EventLog::ACCESS_TOKEN_GENERATED`: 88
* `EventLog::ACCESS_TOKEN_REVOKED`: 74
* `EventLog::ACCESS_TOKEN_REGENERATED`: 4

I asked about this in Slack and @theseanything confirmed that the button
would not be missed, because it's still possible to achieve the same
effect via the UI albeit with more clicks.

I'm about to move the "Manage tokens for API user" page to use the
GOV.UK Design System [2]. Doing this first will make that easier.

I've left the `EventLog::ACCESS_TOKEN_REGENERATED` constant defined in
order to support the historical records. However, I've changed an
unrelated test to use a different constant and added a comment to make
it clear that this constant is deprecated.

[1]: https://docs.publishing.service.gov.uk/manual/alerts/signon-api-user-token-expires-soon.html
[2]: https://trello.com/c/75Jyg8zR
  • Loading branch information
floehopper committed Jan 3, 2024
1 parent 0eb9d53 commit 32ac507
Show file tree
Hide file tree
Showing 6 changed files with 4 additions and 48 deletions.
15 changes: 2 additions & 13 deletions app/controllers/authorisations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,8 @@ def create
def revoke
authorisation = @api_user.authorisations.find(params[:id])
if authorisation.revoke
if params[:regenerate]
regenerated_authorisation = @api_user.authorisations.create!(
expires_in: ApiUser::DEFAULT_TOKEN_LIFE,
application_id: authorisation.application_id,
)

EventLog.record_event(@api_user, EventLog::ACCESS_TOKEN_REGENERATED, initiator: current_user, application: authorisation.application, ip_address: user_ip_address)
flash[:authorisation] = { application_name: regenerated_authorisation.application.name,
token: regenerated_authorisation.token }
else
EventLog.record_event(@api_user, EventLog::ACCESS_TOKEN_REVOKED, initiator: current_user, application: authorisation.application, ip_address: user_ip_address)
flash[:notice] = "Access for #{authorisation.application.name} was revoked"
end
EventLog.record_event(@api_user, EventLog::ACCESS_TOKEN_REVOKED, initiator: current_user, application: authorisation.application, ip_address: user_ip_address)
flash[:notice] = "Access for #{authorisation.application.name} was revoked"
else
flash[:error] = "There was an error while revoking access for #{authorisation.application.name}"
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/event_log.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class EventLog < ApplicationRecord
TWO_STEP_CHANGE_FAILED = LogEntry.new(id: 25, description: "2-step verification phone change failed", require_uid: true),
TWO_STEP_PROMPT_DEFERRED = LogEntry.new(id: 26, description: "2-step prompt deferred", require_uid: true),
API_USER_CREATED = LogEntry.new(id: 27, description: "Account created", require_uid: true, require_initiator: true),
ACCESS_TOKEN_REGENERATED = LogEntry.new(id: 28, description: "Access token re-generated", require_uid: true, require_application: true),
ACCESS_TOKEN_REGENERATED = LogEntry.new(id: 28, description: "Access token re-generated", require_uid: true, require_application: true), # deprecated
ACCESS_TOKEN_GENERATED = LogEntry.new(id: 29, description: "Access token generated", require_uid: true, require_application: true, require_initiator: true),
ACCESS_TOKEN_REVOKED = LogEntry.new(id: 30, description: "Access token revoked", require_uid: true, require_application: true, require_initiator: true),
PASSWORD_RESET_LOADED_BUT_TOKEN_EXPIRED = LogEntry.new(id: 31, description: "Password reset page loaded but the token has expired", require_uid: true),
Expand Down
3 changes: 0 additions & 3 deletions app/views/api_users/manage_tokens.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@
</td>
<td>
<div class="btn-group">
<%= form_tag(revoke_api_user_authorisation_path(@api_user.id, authorisation.id, regenerate: true), method: "post") do %>
<%= submit_tag("Re-generate", class: "btn btn-default") %>
<% end %>
<%= form_tag(revoke_api_user_authorisation_path(@api_user.id, authorisation.id), method: "post") do %>
<%= submit_tag("Revoke", class: "btn btn-default") %>
<% end %>
Expand Down
15 changes: 0 additions & 15 deletions test/controllers/api_users_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -190,21 +190,6 @@ class ApiUsersControllerTest < ActionController::TestCase
end
end

should "show button for regenerating API user's access token for an application" do
application = create(:application)
token = create(:access_token, resource_owner_id: @api_user.id, application:)

get :manage_tokens, params: { id: @api_user }

regenerate_token_path = revoke_api_user_authorisation_path(@api_user, token, regenerate: true)

assert_select "table#authorisations tbody td", text: application.name do |td|
assert_select td.first.parent, "form[action='#{regenerate_token_path}']" do
assert_select "input[type='submit']", value: "Re-generate"
end
end
end

should "show button for revoking API user's access token for an application" do
application = create(:application)
token = create(:access_token, resource_owner_id: @api_user.id, application:)
Expand Down
15 changes: 0 additions & 15 deletions test/integration/manage_api_users_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -101,21 +101,6 @@ class ManageApiUsersTest < ActionDispatch::IntegrationTest
assert page.has_text?("Access token revoked for #{@application.name} by #{@superadmin.name}")
end

should "be able to regenerate application access token for an API user which should get recorded in event log" do
click_link @api_user.name
click_link "Manage tokens"

assert page.has_selector?("td:first-child", text: @application.name)
click_button "Re-generate"

assert page.has_selector?("div.alert-danger", text: "Make sure to copy the access token for #{@application.name} now. You won't be able to see it again!")
assert page.has_selector?("div.alert-info", text: "Access token for #{@application.name}: #{@api_user.authorisations.last.token}")

click_link @api_user.name
click_link "View account access log"
assert page.has_text?("Access token re-generated for #{@application.name} by #{@superadmin.name}")
end

should "be able to suspend and unsuspend API user" do
click_link @api_user.name
click_link "Suspend user"
Expand Down
2 changes: 1 addition & 1 deletion test/models/event_log_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class EventLogTest < ActiveSupport::TestCase

test "records the application associated with the event passed as an option" do
application = create(:application)
EventLog.record_event(create(:user), EventLog::ACCESS_TOKEN_REGENERATED, application:)
EventLog.record_event(create(:user), EventLog::SUCCESSFUL_PASSWORD_RESET, application:)

assert_equal application, EventLog.last.application
end
Expand Down

0 comments on commit 32ac507

Please sign in to comment.