Skip to content

Commit

Permalink
Exclude retired apps from User#authorisations
Browse files Browse the repository at this point in the history
We shouldn't ever need to use access tokens for retired applications and
so `User#authorisations` should not include them. To this end I've added
this `joins(:application)` scope to the `User#authorisations` `has_many`
association so that it picks up the default scope on
`Doorkeeper::Application`, i.e.  `not_retired`. c.f.
`User#application_permissions` & `User#supported_permissions`.

I don't think the `joins` scope itself will change which
`Doorkeeper::AccessToken` records are returned, because
`oauth_access_tokens.application_id` has a `NOT NULL` and a foreign key
constraint. The `joins` scope just serves to include the default scope
on `Doorkeeper::Application` in the query.

I've tried to tests to highlight behaviour changes in the wider
codebase:

* Access tokens for retired apps no longer appear on the API user edit
  page.
* No longer send SSO Push requests to retired apps. Addresses this
  Trello card [1].
* No longer include token expiry times to Prometheus for retired apps.
  This should mean that 2nd Line Tech do not see alerts for such tokens.
  At least partly addresses this Trello card [2].

I've had to change the assertion in the `should "not allow editing
permissions for retired application"` test in `ApiUsersControllerTest`,
because the template no longer displays the table at all (as opposed to
displaying an empty table) and I think the new behaviour is correct.

The `should "not show API user's access tokens for retired
applications"` test in `ApiUsersControllerTest` was actually passing
prior to this commit but only by accident, because the
`app/views/api_users/edit.html.erb` template calls
`Doorkeeper::AccessToken.ordered_by_application_name` which calls
`Doorkeeper::Application.ordered_by_name` which triggers the default
scope in `Doorkeeper::Application` which includes
`Doorkeeper::Application.not_retired`. The change to
`User#authorisations` in this commit makes the behaviour more
intentional.

[1]: https://trello.com/c/pN8KOiQV
[2]: https://trello.com/c/huqPdMv8
  • Loading branch information
floehopper committed Oct 24, 2023
1 parent a823de7 commit 3d4410c
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 6 deletions.
2 changes: 1 addition & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class User < ApplicationRecord
validate :exemption_from_2sv_data_is_complete
validate :organisation_has_mandatory_2sv, on: :create

has_many :authorisations, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id
has_many :authorisations, -> { joins(:application) }, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id
has_many :application_permissions, -> { joins(:application) }, class_name: "UserApplicationPermission", inverse_of: :user, dependent: :destroy
has_many :supported_permissions, -> { joins(:application) }, through: :application_permissions
has_many :batch_invitations
Expand Down
13 changes: 10 additions & 3 deletions test/controllers/api_users_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,7 @@ class ApiUsersControllerTest < ActionController::TestCase

get :edit, params: { id: api_user.id }

assert_select "table#editable-permissions tr" do
assert_select "td", text: "retired-app-name", count: 0
end
assert_select "table#editable-permissions", count: 0
end

should "allow editing permissions for API-only application" do
Expand Down Expand Up @@ -213,6 +211,15 @@ class ApiUsersControllerTest < ActionController::TestCase

assert_select "table#authorisations tbody td", text: application.name, count: 0
end

should "not show API user's access tokens for retired applications" do
application = create(:application, retired: true)
create(:access_token, resource_owner_id: @api_user.id, application:)

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

assert_select "table#authorisations tbody td", text: application.name, count: 0
end
end

context "PUT update" do
Expand Down
11 changes: 11 additions & 0 deletions test/jobs/push_user_updates_job_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,16 @@ class TestJob < PushUserUpdatesJob
TestJob.perform_on(user)
end
end

should "not perform_async updates on user's retired applications" do
user = create(:user)
retired_app = create(:application, retired: true)

create(:access_token, resource_owner_id: user.id, application: retired_app)

assert_no_enqueued_jobs do
TestJob.perform_on(user)
end
end
end
end
5 changes: 3 additions & 2 deletions test/lib/collectors/global_prometheus_collector_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
class GlobalPrometheusCollectorTest < ActiveSupport::TestCase
def setup
@collector = Collectors::GlobalPrometheusCollector.new
@api_user = api_user_with_token("user1", token_count: 3)
@api_user = api_user_with_token("user1", token_count: 4)
end

context "#metrics" do
should "list all non-revoked token expiry timestamps" do
should "list all non-revoked token expiry timestamps for non-retired aps" do
@api_user.authorisations[2].revoke
@api_user.authorisations[3].application.update!(retired: true)

metrics = @collector.metrics

Expand Down
7 changes: 7 additions & 0 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ def setup
assert_includes @user.authorisations, token
assert_not_includes @user.authorisations, token_for_another_user
end

should "not include access tokens for retired applications" do
application = create(:application, retired: true)
token = create(:access_token, resource_owner_id: @user.id, application:)

assert_not_includes @user.authorisations, token
end
end

context "#application_permissions" do
Expand Down

0 comments on commit 3d4410c

Please sign in to comment.