From f8f8d7a0195e5a1d9c27d14d1187978918678f01 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 9 Oct 2023 12:33:30 +0100 Subject: [PATCH 01/11] Move test for namespaced class into sub-directory This test is testing `Doorkeeper::Application` and the test class is already namespaced with the `Doorkeeper` module, so I think it's more idiomatic to move it into a sub-directory. --- .../application_test.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/models/{doorkeeper_application_test.rb => doorkeeper/application_test.rb} (100%) diff --git a/test/models/doorkeeper_application_test.rb b/test/models/doorkeeper/application_test.rb similarity index 100% rename from test/models/doorkeeper_application_test.rb rename to test/models/doorkeeper/application_test.rb From ac09c3837ca9c47c96107461d7bca58f10b6a328 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 9 Oct 2023 12:38:55 +0100 Subject: [PATCH 02/11] Add Doorkeeper::AccessToken.expires_after scope Trello: https://trello.com/c/XfBBRL5M My motivation for adding this is that I want to use it in `SSOPushCredential.credentials`. I've had to re-open the class from the doorkeeper gem, but we already have precendent for doing that for `Doorkeeper::Application`. I've based the implementation on `Doorkeeper::Orm::ActiveRecord::Mixins::AccessToken::ClassMethods#not_expired` - see this code [1]. Re-using the `#expiration_time_sql` method should make the implementation robust against a change of database (e.g. MySQL -> PostgreSQL). Note that I've had to wrap the call to `#expiration_time_sql` in a call to `ActiveRecord::Sanitization::ClassMethods#sanitize_sql` [2] in order to avoid Brakeman failing with a SQL Injection warning. [1]: https://github.com/doorkeeper-gem/doorkeeper/blob/986115cc228ff30dc1ead0f4101195448994f5d4/lib/doorkeeper/orm/active_record/mixins/access_token.rb#L47-L66 [2]: https://api.rubyonrails.org/v7.0.8/classes/ActiveRecord/Sanitization/ClassMethods.html#method-i-sanitize_sql --- app/models/doorkeeper/access_token.rb | 5 +++++ test/models/doorkeeper/access_token_test.rb | 15 +++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 app/models/doorkeeper/access_token.rb create mode 100644 test/models/doorkeeper/access_token_test.rb diff --git a/app/models/doorkeeper/access_token.rb b/app/models/doorkeeper/access_token.rb new file mode 100644 index 000000000..b7905cf9f --- /dev/null +++ b/app/models/doorkeeper/access_token.rb @@ -0,0 +1,5 @@ +module Doorkeeper + class AccessToken < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord + scope :expires_after, ->(time) { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} > ?", time) } + end +end diff --git a/test/models/doorkeeper/access_token_test.rb b/test/models/doorkeeper/access_token_test.rb new file mode 100644 index 000000000..e49fae893 --- /dev/null +++ b/test/models/doorkeeper/access_token_test.rb @@ -0,0 +1,15 @@ +require "test_helper" + +class Doorkeeper::AccessTokenTest < ActiveSupport::TestCase + context ".expires_after" do + should "return tokens expiring after specified time" do + token_expiring_in_1_week = create(:access_token, expires_in: 1.week) + token_expiring_in_3_weeks = create(:access_token, expires_in: 3.weeks) + + tokens = Doorkeeper::AccessToken.expires_after(2.weeks.from_now) + + assert_not_includes tokens, token_expiring_in_1_week + assert_includes tokens, token_expiring_in_3_weeks + end + end +end From 87f8f36ee61afd882d465f51bac7c94c91ccebd8 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 9 Oct 2023 12:44:25 +0100 Subject: [PATCH 03/11] Create new SSO Push token on demand if close to expiry Trello: https://trello.com/c/XfBBRL5M Previously a token for the SSO Push API user was created on demand if no non-expired and no non-revoked token existed. Recently some changes were introduced [1,2] to alert 2nd Line Tech of tokens expiring in less than 2 weeks. Note that these alerts cover *all* API user tokens; not just those for the SSO Push API user. If we left things unchanged, 2nd Line Tech would be alerted unnecessarily to SSO Push API user tokens within 2 weeks of expiry, even though the code would automatically create new ones once they expired. While it shouldn't do any harm if 2nd Line Tech revoke/regenerate these tokens via the Signon UI, it's unnecessary work. We now create a new token if the current token is due to expire in the next 4 weeks. I've chosen 4 weeks rather than matching the 2 weeks used in the alerting, because the tokens are only ever created *on demand*; not on a schedule. So for applications where Signon doesn't need to make SSO Push requests very often, it might take a while for a token to be created. I'm hoping that giving an extra 2 weeks leeway should cover most scenarios, but it doesn't matter if it doesn't - it just means that 2nd Line Tech might see some alerts they didn't need to. [1]: https://github.com/alphagov/govuk-helm-charts/pull/1303 [2]: https://github.com/alphagov/govuk-infrastructure/pull/953 --- lib/sso_push_credential.rb | 1 + test/lib/sso_push_credential_test.rb | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/sso_push_credential.rb b/lib/sso_push_credential.rb index 38093a4fa..7e0019116 100644 --- a/lib/sso_push_credential.rb +++ b/lib/sso_push_credential.rb @@ -10,6 +10,7 @@ def credentials(application) user.authorisations .not_expired + .expires_after(4.weeks.from_now) .create_with(expires_in: 10.years) .find_or_create_by!(application_id: application.id).token end diff --git a/test/lib/sso_push_credential_test.rb b/test/lib/sso_push_credential_test.rb index 217d9b2f3..2c1de1453 100644 --- a/test/lib/sso_push_credential_test.rb +++ b/test/lib/sso_push_credential_test.rb @@ -8,7 +8,7 @@ class SSOPushCredentialTest < ActiveSupport::TestCase context "given an already authorised application" do setup do - @authorisation = @user.authorisations.create!(application_id: @application.id) + @authorisation = @user.authorisations.create!(application_id: @application.id, expires_in: 5.weeks) end should "return the bearer token for an already-authorized application" do @@ -37,6 +37,20 @@ class SSOPushCredentialTest < ActiveSupport::TestCase end end + context "given an application with an authorisation close to expiry" do + setup do + @user.authorisations.create!(application_id: @application.id, expires_in: 4.weeks) + end + + should "create a new authorisation to replace the expired one" do + bearer_token = SSOPushCredential.credentials(@application) + + new_authorisation = @user.authorisations.find_by(token: bearer_token) + assert new_authorisation.expires_at > 4.weeks.from_now + assert_equal @application.id, new_authorisation.application_id + end + end + context "given an application with a revoked authorisation" do setup do @user.authorisations.create!(application_id: @application.id, revoked_at: Time.current) From 62821e453ccc67070c92b5edb090e26448919408 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 09:39:31 +0100 Subject: [PATCH 04/11] Extract AccessToken.not_revoked scope This is used in a bunch of places across the codebase, so it makes sense to extract it into a scope. --- app/models/doorkeeper/access_token.rb | 1 + app/models/user.rb | 2 +- app/views/api_users/edit.html.erb | 2 +- lib/collectors/global_prometheus_collector.rb | 2 +- lib/tasks/sync_kubernetes_secrets.rake | 2 +- lib/user_permissions_controller_methods.rb | 2 +- test/models/doorkeeper/access_token_test.rb | 12 ++++++++++++ 7 files changed, 18 insertions(+), 5 deletions(-) diff --git a/app/models/doorkeeper/access_token.rb b/app/models/doorkeeper/access_token.rb index b7905cf9f..67aed4a46 100644 --- a/app/models/doorkeeper/access_token.rb +++ b/app/models/doorkeeper/access_token.rb @@ -1,5 +1,6 @@ module Doorkeeper class AccessToken < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord + scope :not_revoked, -> { where(revoked_at: nil) } scope :expires_after, ->(time) { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} > ?", time) } end end diff --git a/app/models/user.rb b/app/models/user.rb index cfab1787d..90a98340c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -192,7 +192,7 @@ def authorised_applications alias_method :applications_used, :authorised_applications def revoke_all_authorisations - authorisations.where(revoked_at: nil).find_each(&:revoke) + authorisations.not_revoked.find_each(&:revoke) end def grant_application_signin_permission(application) diff --git a/app/views/api_users/edit.html.erb b/app/views/api_users/edit.html.erb index a8e7e45ae..4b1eb1538 100644 --- a/app/views/api_users/edit.html.erb +++ b/app/views/api_users/edit.html.erb @@ -51,7 +51,7 @@ - <% @api_user.authorisations.where(revoked_at: nil).each do |authorisation| %> + <% @api_user.authorisations.not_revoked.each do |authorisation| %> <%= authorisation.application.name %> <%= truncate_access_token(authorisation.token) %> diff --git a/lib/collectors/global_prometheus_collector.rb b/lib/collectors/global_prometheus_collector.rb index 0a65a6e26..3784e9aab 100644 --- a/lib/collectors/global_prometheus_collector.rb +++ b/lib/collectors/global_prometheus_collector.rb @@ -26,7 +26,7 @@ def token_expiry_info # Cache metric to prevent needless expensive calls to the database Rails.cache.fetch("token_expiry_info", expires_in: 1.hour) do ApiUser.where.not(email: SSOPushCredential::USER_EMAIL).flat_map do |user| - user.authorisations.where(revoked_at: nil).map do |token| + user.authorisations.not_revoked.map do |token| { expires_at: token.expires_at.to_i, api_user: user.email, diff --git a/lib/tasks/sync_kubernetes_secrets.rake b/lib/tasks/sync_kubernetes_secrets.rake index 2d084017b..ebdd0dacd 100644 --- a/lib/tasks/sync_kubernetes_secrets.rake +++ b/lib/tasks/sync_kubernetes_secrets.rake @@ -10,7 +10,7 @@ namespace :kubernetes do missing_users = emails - api_users.map(&:email) api_users.each do |api_user| - api_user.authorisations.where(revoked_at: nil).each do |token| + api_user.authorisations.not_revoked.each do |token| name = "signon-token-#{api_user.name}-#{token.application.name}".parameterize data = { bearer_token: token.token } diff --git a/lib/user_permissions_controller_methods.rb b/lib/user_permissions_controller_methods.rb index c8103dd04..c8a17ab54 100644 --- a/lib/user_permissions_controller_methods.rb +++ b/lib/user_permissions_controller_methods.rb @@ -5,7 +5,7 @@ def visible_applications(user) if user.api_user? applications = ::Doorkeeper::Application.includes(:supported_permissions) if current_user.superadmin? - api_user_authorised_apps = user.authorisations.where(revoked_at: nil).pluck(:application_id) + api_user_authorised_apps = user.authorisations.not_revoked.pluck(:application_id) applications.where(id: api_user_authorised_apps) else applications.none diff --git a/test/models/doorkeeper/access_token_test.rb b/test/models/doorkeeper/access_token_test.rb index e49fae893..9fc52387b 100644 --- a/test/models/doorkeeper/access_token_test.rb +++ b/test/models/doorkeeper/access_token_test.rb @@ -1,6 +1,18 @@ require "test_helper" class Doorkeeper::AccessTokenTest < ActiveSupport::TestCase + context ".not_revoked" do + should "return tokens that have not been revoked" do + revoked_token = create(:access_token, revoked_at: Time.current) + non_revoked_token = create(:access_token, revoked_at: nil) + + tokens = Doorkeeper::AccessToken.not_revoked + + assert_not_includes tokens, revoked_token + assert_includes tokens, non_revoked_token + end + end + context ".expires_after" do should "return tokens expiring after specified time" do token_expiring_in_1_week = create(:access_token, expires_in: 1.week) From 9cdb7d0e4170fab5ec5ba85fdee3f19587359ce6 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 10:39:14 +0100 Subject: [PATCH 05/11] Order API user tokens by app name & expiry time Previously it was hard to work out whether an API user had a valid token for a given app. This should make it a bit easier. --- app/models/doorkeeper/access_token.rb | 2 ++ app/models/doorkeeper/application.rb | 3 ++- app/views/api_users/edit.html.erb | 2 +- test/models/doorkeeper/access_token_test.rb | 25 +++++++++++++++++++++ test/models/doorkeeper/application_test.rb | 11 +++++++++ 5 files changed, 41 insertions(+), 2 deletions(-) diff --git a/app/models/doorkeeper/access_token.rb b/app/models/doorkeeper/access_token.rb index 67aed4a46..2bb5e8d40 100644 --- a/app/models/doorkeeper/access_token.rb +++ b/app/models/doorkeeper/access_token.rb @@ -2,5 +2,7 @@ module Doorkeeper class AccessToken < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord scope :not_revoked, -> { where(revoked_at: nil) } scope :expires_after, ->(time) { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} > ?", time) } + scope :ordered_by_expires_at, -> { order(expiration_time_sql) } + scope :ordered_by_application_name, -> { includes(:application).merge(Doorkeeper::Application.ordered_by_name) } end end diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index cb8754594..f5a1a8f3b 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -5,7 +5,8 @@ class ::Doorkeeper::Application < ActiveRecord::Base # rubocop:enable Rails/ApplicationRecord has_many :supported_permissions, dependent: :destroy - default_scope { order("oauth_applications.name") } + default_scope { ordered_by_name } + scope :ordered_by_name, -> { order("oauth_applications.name") } scope :support_push_updates, -> { where(supports_push_updates: true) } scope :not_retired, -> { where(retired: false) } scope :can_signin, diff --git a/app/views/api_users/edit.html.erb b/app/views/api_users/edit.html.erb index 4b1eb1538..97847f86a 100644 --- a/app/views/api_users/edit.html.erb +++ b/app/views/api_users/edit.html.erb @@ -51,7 +51,7 @@ - <% @api_user.authorisations.not_revoked.each do |authorisation| %> + <% @api_user.authorisations.not_revoked.ordered_by_application_name.ordered_by_expires_at.each do |authorisation| %> <%= authorisation.application.name %> <%= truncate_access_token(authorisation.token) %> diff --git a/test/models/doorkeeper/access_token_test.rb b/test/models/doorkeeper/access_token_test.rb index 9fc52387b..827f018f9 100644 --- a/test/models/doorkeeper/access_token_test.rb +++ b/test/models/doorkeeper/access_token_test.rb @@ -24,4 +24,29 @@ class Doorkeeper::AccessTokenTest < ActiveSupport::TestCase assert_includes tokens, token_expiring_in_3_weeks end end + + context ".ordered_by_expires_at" do + should "return tokens ordered by expiry time" do + token_expiring_in_2_weeks = create(:access_token, expires_in: 2.weeks) + token_expiring_in_1_week = create(:access_token, expires_in: 1.week) + + tokens = Doorkeeper::AccessToken.ordered_by_expires_at + + assert_equal [token_expiring_in_1_week, token_expiring_in_2_weeks], tokens + end + end + + context ".ordered_by_application_name" do + should "return tokens ordered by application name" do + application_named_foo = create(:application, name: "Foo") + application_named_bar = create(:application, name: "Bar") + + token_for_foo = create(:access_token, application: application_named_foo) + token_for_bar = create(:access_token, application: application_named_bar) + + tokens = Doorkeeper::AccessToken.ordered_by_application_name + + assert_equal [token_for_bar, token_for_foo], tokens + end + end end diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index 8e8d3cb0c..8025957c8 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -242,4 +242,15 @@ class ::Doorkeeper::ApplicationTest < ActiveSupport::TestCase end end end + + context ".ordered_by_name" do + should "return applications ordered by name" do + application_named_foo = create(:application, name: "Foo") + application_named_bar = create(:application, name: "Bar") + + applications = Doorkeeper::Application.ordered_by_name + + assert_equal [application_named_bar, application_named_foo], applications + end + end end From 58dfc3bffff6424e49b69c2a7911bb846c7ce40f Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 10:48:46 +0100 Subject: [PATCH 06/11] Use built-in AccessToken#expires_at There's no need to do the date addition of `created_at + expires_in` when the `doorkeeper` gem can do it for us. --- app/views/api_users/edit.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/api_users/edit.html.erb b/app/views/api_users/edit.html.erb index 97847f86a..def4b16a0 100644 --- a/app/views/api_users/edit.html.erb +++ b/app/views/api_users/edit.html.erb @@ -59,7 +59,7 @@ <%= Time.at(authorisation.created_at).to_date.to_fs(:govuk_date) %> - <%= Time.at(authorisation.created_at.to_f + authorisation.expires_in).to_date.to_fs(:govuk_date) %> + <%= Time.at(authorisation.expires_at).to_date.to_fs(:govuk_date) %>
From 24732799764720a25b5fb102df8f5f8a8559a787 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 11:02:53 +0100 Subject: [PATCH 07/11] Remove unnecessary calls to Time.at in API user edit page These calls were introduced in this commit [1], but there is no explanation for why. I suspect it was due to a misunderstanding about how Rails' Time objects work. [1]: 43331d0ee5ac709266ea123460e32a10deba162e --- app/views/api_users/edit.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/api_users/edit.html.erb b/app/views/api_users/edit.html.erb index def4b16a0..0ac30c28f 100644 --- a/app/views/api_users/edit.html.erb +++ b/app/views/api_users/edit.html.erb @@ -56,10 +56,10 @@ <%= authorisation.application.name %> <%= truncate_access_token(authorisation.token) %> - <%= Time.at(authorisation.created_at).to_date.to_fs(:govuk_date) %> + <%= authorisation.created_at.to_date.to_fs(:govuk_date) %> - <%= Time.at(authorisation.expires_at).to_date.to_fs(:govuk_date) %> + <%= authorisation.expires_at.to_date.to_fs(:govuk_date) %>
From 1d985b2d164d4b51478757d78ba0de9aad795711 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 11:12:43 +0100 Subject: [PATCH 08/11] Make it easier to see expired API user access tokens This adds a "state" column with an indication of whether a token has expired or not. Note that the table doesn't currently include revoked tokens so there's no need to add a state value for them. --- app/views/api_users/edit.html.erb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/views/api_users/edit.html.erb b/app/views/api_users/edit.html.erb index 0ac30c28f..82098ee9a 100644 --- a/app/views/api_users/edit.html.erb +++ b/app/views/api_users/edit.html.erb @@ -47,6 +47,7 @@ Token (hidden) Generated Expires + State Action @@ -61,6 +62,13 @@ <%= authorisation.expires_at.to_date.to_fs(:govuk_date) %> + + <% if authorisation.expired? %> + Expired + <% else %> + Valid + <% end %> +
<%= form_tag(revoke_api_user_authorisation_path(@api_user.id, authorisation.id, regenerate: true), method: "post") do %> From b6c4279a1618875128ed453d0d14dd4c68a0f7d2 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 11:32:11 +0100 Subject: [PATCH 09/11] Extract expired scope on AccessToken & AccessGrant Like I did for AccessToken.expires_after, I've reused the `#expiration_time_sql` method built-in to the `doorkeeper` gem to make the code robust against a change of database e.g. from MySQL to PostgreSQL. I've re-opened the `Doorkeeper:AccessGrant` class like I did for `Doorkeeper::AccessToken`. Unfortunately I've had to explicitly include the `Models::ExpirationTimeSqlMath` concern, because while `Doorkeeper::AccessToken` included it, `Doorkeeper:AccessGrant` does not. Extracting this scope has allowed me to simplify `ExpiredOauthAccessRecordsDeleter`. Note that as before I've had to wrap the call to `#expiration_time_sql` in a call to `ActiveRecord::Sanitization::ClassMethods#sanitize_sql` [1] in order to avoid Brakeman failing with a SQL Injection warning. [1]: https://api.rubyonrails.org/v7.0.8/classes/ActiveRecord/Sanitization/ClassMethods.html#method-i-sanitize_sql --- app/models/doorkeeper/access_grant.rb | 7 +++++++ app/models/doorkeeper/access_token.rb | 1 + lib/expired_oauth_access_records_deleter.rb | 4 +--- test/models/doorkeeper/access_grant_test.rb | 15 +++++++++++++++ test/models/doorkeeper/access_token_test.rb | 12 ++++++++++++ 5 files changed, 36 insertions(+), 3 deletions(-) create mode 100644 app/models/doorkeeper/access_grant.rb create mode 100644 test/models/doorkeeper/access_grant_test.rb diff --git a/app/models/doorkeeper/access_grant.rb b/app/models/doorkeeper/access_grant.rb new file mode 100644 index 000000000..03620d78e --- /dev/null +++ b/app/models/doorkeeper/access_grant.rb @@ -0,0 +1,7 @@ +module Doorkeeper + class AccessGrant < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord + include Models::ExpirationTimeSqlMath + + scope :expired, -> { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} < ?", Time.current) } + end +end diff --git a/app/models/doorkeeper/access_token.rb b/app/models/doorkeeper/access_token.rb index 2bb5e8d40..5f0d7319d 100644 --- a/app/models/doorkeeper/access_token.rb +++ b/app/models/doorkeeper/access_token.rb @@ -2,6 +2,7 @@ module Doorkeeper class AccessToken < ::ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord scope :not_revoked, -> { where(revoked_at: nil) } scope :expires_after, ->(time) { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} > ?", time) } + scope :expired, -> { where.not(expires_in: nil).where("#{sanitize_sql(expiration_time_sql)} < ?", Time.current) } scope :ordered_by_expires_at, -> { order(expiration_time_sql) } scope :ordered_by_application_name, -> { includes(:application).merge(Doorkeeper::Application.ordered_by_name) } end diff --git a/lib/expired_oauth_access_records_deleter.rb b/lib/expired_oauth_access_records_deleter.rb index 552d2d39a..b26b60a12 100644 --- a/lib/expired_oauth_access_records_deleter.rb +++ b/lib/expired_oauth_access_records_deleter.rb @@ -8,8 +8,6 @@ class ExpiredOauthAccessRecordsDeleter access_token: EventLog::ACCESS_TOKENS_DELETED, }.freeze - HAS_EXPIRED = "expires_in is not null AND DATE_ADD(created_at, INTERVAL expires_in second) < ?".freeze - def initialize(record_type:) @record_class = CLASSES.fetch(record_type) @event = EVENTS.fetch(record_type) @@ -19,7 +17,7 @@ def initialize(record_type:) attr_reader :record_class, :total_deleted def delete_expired - @record_class.where(HAS_EXPIRED, Time.zone.now).in_batches do |relation| + @record_class.expired.in_batches do |relation| records_by_user_id = relation.includes(:application).group_by(&:resource_owner_id) all_users = User.where(id: records_by_user_id.keys) diff --git a/test/models/doorkeeper/access_grant_test.rb b/test/models/doorkeeper/access_grant_test.rb new file mode 100644 index 000000000..c9fbaafeb --- /dev/null +++ b/test/models/doorkeeper/access_grant_test.rb @@ -0,0 +1,15 @@ +require "test_helper" + +class Doorkeeper::AccessGrantTest < ActiveSupport::TestCase + context ".expired" do + should "return grants that have expired" do + grant_expiring_1_day_ago = create(:access_grant, expires_in: -1.day) + grant_expiring_in_1_day = create(:access_grant, expires_in: 1.day) + + grants = Doorkeeper::AccessGrant.expired + + assert_includes grants, grant_expiring_1_day_ago + assert_not_includes grants, grant_expiring_in_1_day + end + end +end diff --git a/test/models/doorkeeper/access_token_test.rb b/test/models/doorkeeper/access_token_test.rb index 827f018f9..7bed84a85 100644 --- a/test/models/doorkeeper/access_token_test.rb +++ b/test/models/doorkeeper/access_token_test.rb @@ -25,6 +25,18 @@ class Doorkeeper::AccessTokenTest < ActiveSupport::TestCase end end + context ".expired" do + should "return tokens that have expired" do + token_expiring_1_day_ago = create(:access_token, expires_in: -1.day) + token_expiring_in_1_day = create(:access_token, expires_in: 1.day) + + tokens = Doorkeeper::AccessToken.expired + + assert_includes tokens, token_expiring_1_day_ago + assert_not_includes tokens, token_expiring_in_1_day + end + end + context ".ordered_by_expires_at" do should "return tokens ordered by expiry time" do token_expiring_in_2_weeks = create(:access_token, expires_in: 2.weeks) From c548f115125d6fb77e74bb2fc46e6eb25d9e133c Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 11:37:55 +0100 Subject: [PATCH 10/11] Remove redundant :: prefix from Doorkeeper constant --- app/controllers/application_controller.rb | 2 +- app/controllers/root_controller.rb | 4 ++-- app/jobs/permission_updater.rb | 2 +- app/jobs/reauth_enforcer.rb | 2 +- app/models/doorkeeper/application.rb | 2 +- .../user_permission_manageable_application_policy.rb | 2 +- config/initializers/doorkeeper.rb | 6 +++--- lib/user_permissions_controller_methods.rb | 2 +- script/make_oauth_work_in_dev | 2 +- test/integration/superadmin_application_edit_test.rb | 2 +- test/jobs/push_user_updates_job_test.rb | 2 +- test/models/doorkeeper/application_test.rb | 2 +- test/models/user_test.rb | 2 +- 13 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 268de6845..44c47f194 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -25,7 +25,7 @@ def current_resource_owner end def application_making_request - @_application_making_request ||= ::Doorkeeper::Application.find(doorkeeper_token.application_id) if doorkeeper_token + @_application_making_request ||= Doorkeeper::Application.find(doorkeeper_token.application_id) if doorkeeper_token end private diff --git a/app/controllers/root_controller.rb b/app/controllers/root_controller.rb index dccd5f7f1..50a7d6eda 100644 --- a/app/controllers/root_controller.rb +++ b/app/controllers/root_controller.rb @@ -6,13 +6,13 @@ class RootController < ApplicationController skip_after_action :verify_authorized def index - applications = ::Doorkeeper::Application.where(show_on_dashboard: true).can_signin(current_user) + applications = Doorkeeper::Application.where(show_on_dashboard: true).can_signin(current_user) @applications_and_permissions = zip_permissions(applications, current_user) end def signin_required - @application = ::Doorkeeper::Application.find_by(id: session.delete(:signin_missing_for_application)) + @application = Doorkeeper::Application.find_by(id: session.delete(:signin_missing_for_application)) end def privacy_notice; end diff --git a/app/jobs/permission_updater.rb b/app/jobs/permission_updater.rb index 4fe8624ee..f4326de1f 100644 --- a/app/jobs/permission_updater.rb +++ b/app/jobs/permission_updater.rb @@ -3,7 +3,7 @@ class PermissionUpdater < PushUserUpdatesJob def perform(uid, application_id) user = User.find_by(uid:) - application = ::Doorkeeper::Application.find_by(id: application_id) + application = Doorkeeper::Application.find_by(id: application_id) # It's possible they've been deleted between when the job was scheduled and run. return if user.nil? || application.nil? return unless application.supports_push_updates? diff --git a/app/jobs/reauth_enforcer.rb b/app/jobs/reauth_enforcer.rb index e8040b841..d7d60ceda 100644 --- a/app/jobs/reauth_enforcer.rb +++ b/app/jobs/reauth_enforcer.rb @@ -2,7 +2,7 @@ class ReauthEnforcer < PushUserUpdatesJob def perform(uid, application_id) - application = ::Doorkeeper::Application.find_by(id: application_id) + application = Doorkeeper::Application.find_by(id: application_id) # It's possible the application has been deleted between when the job was scheduled and run. return if application.nil? return unless application.supports_push_updates? diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index f5a1a8f3b..d377b8b7b 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -1,7 +1,7 @@ require "doorkeeper/orm/active_record/application" # rubocop:disable Rails/ApplicationRecord -class ::Doorkeeper::Application < ActiveRecord::Base +class Doorkeeper::Application < ActiveRecord::Base # rubocop:enable Rails/ApplicationRecord has_many :supported_permissions, dependent: :destroy diff --git a/app/policies/user_permission_manageable_application_policy.rb b/app/policies/user_permission_manageable_application_policy.rb index 02f33479d..b1919aec8 100644 --- a/app/policies/user_permission_manageable_application_policy.rb +++ b/app/policies/user_permission_manageable_application_policy.rb @@ -31,7 +31,7 @@ def resolve private def applications - ::Doorkeeper::Application.includes(:supported_permissions) + Doorkeeper::Application.includes(:supported_permissions) end end end diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 61ebf0f1f..f757c8f88 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -74,7 +74,7 @@ # Use a custom class for generating the access token. # See https://doorkeeper.gitbook.io/guides/configuration/other-configurations#custom-access-token-generator # - # access_token_generator '::Doorkeeper::JWT' + # access_token_generator 'Doorkeeper::JWT' # The controller +Doorkeeper::ApplicationController+ inherits from. # Defaults to +ActionController::Base+ unless +api_only+ is set, which changes the default to @@ -119,7 +119,7 @@ # If you wish to use another hashing implementation, you can override # this strategy as follows: # - # hash_token_secrets using: '::Doorkeeper::Hashing::MyCustomHashImpl' + # hash_token_secrets using: 'Doorkeeper::Hashing::MyCustomHashImpl' # # Keep in mind that changing the hashing function will invalidate all existing # secrets, if there are any. @@ -134,7 +134,7 @@ # If you wish to use bcrypt for application secret hashing, uncomment # this line instead: # - # hash_application_secrets using: '::Doorkeeper::SecretStoring::BCrypt' + # hash_application_secrets using: 'Doorkeeper::SecretStoring::BCrypt' # When the above option is enabled, and a hashed token or secret is not found, # you can allow to fall back to another strategy. For users upgrading diff --git a/lib/user_permissions_controller_methods.rb b/lib/user_permissions_controller_methods.rb index c8a17ab54..48da189a3 100644 --- a/lib/user_permissions_controller_methods.rb +++ b/lib/user_permissions_controller_methods.rb @@ -3,7 +3,7 @@ module UserPermissionsControllerMethods def visible_applications(user) if user.api_user? - applications = ::Doorkeeper::Application.includes(:supported_permissions) + applications = Doorkeeper::Application.includes(:supported_permissions) if current_user.superadmin? api_user_authorised_apps = user.authorisations.not_revoked.pluck(:application_id) applications.where(id: api_user_authorised_apps) diff --git a/script/make_oauth_work_in_dev b/script/make_oauth_work_in_dev index 9f1363298..b82653873 100755 --- a/script/make_oauth_work_in_dev +++ b/script/make_oauth_work_in_dev @@ -98,7 +98,7 @@ def deverise_uri(uri) end puts "Updating SSO config so that it works in development..." -applications = ::Doorkeeper::Application.where(retired: false) +applications = Doorkeeper::Application.where(retired: false) applications.each do |application| local_config = values_for_app(application) if local_config.present? diff --git a/test/integration/superadmin_application_edit_test.rb b/test/integration/superadmin_application_edit_test.rb index d23ea0629..01807de4b 100644 --- a/test/integration/superadmin_application_edit_test.rb +++ b/test/integration/superadmin_application_edit_test.rb @@ -14,7 +14,7 @@ class SuperAdminApplicationEditTest < ActionDispatch::IntegrationTest # normal user who's authorised to use app @user = create(:user) - ::Doorkeeper::AccessToken.create!(resource_owner_id: @user.id, application_id: @application.id, token: "1234") + Doorkeeper::AccessToken.create!(resource_owner_id: @user.id, application_id: @application.id, token: "1234") end should "be able to enable push updates to applications" do diff --git a/test/jobs/push_user_updates_job_test.rb b/test/jobs/push_user_updates_job_test.rb index 799021670..72054dde8 100644 --- a/test/jobs/push_user_updates_job_test.rb +++ b/test/jobs/push_user_updates_job_test.rb @@ -12,7 +12,7 @@ class TestJob < PushUserUpdatesJob foo_app, _bar_app = *create_list(:application, 2) # authenticate access - ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: foo_app.id, token: "1234") + Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: foo_app.id, token: "1234") assert_enqueued_with(job: TestJob, args: [user.uid, foo_app.id]) do TestJob.perform_on(user) diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index 8025957c8..a2417f8c0 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -1,6 +1,6 @@ require "test_helper" -class ::Doorkeeper::ApplicationTest < ActiveSupport::TestCase +class Doorkeeper::ApplicationTest < ActiveSupport::TestCase should "have a signin supported permission on create" do assert_not_nil create(:application).signin_permission end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 091b006ad..18c733178 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -1110,7 +1110,7 @@ def setup end def authenticate_access(user, app) - ::Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) + Doorkeeper::AccessToken.create!(resource_owner_id: user.id, application_id: app.id) end def assert_user_has_permissions(expected_permissions, application, user) From 633b2c57df3eb11b9175c2c45f6538d549377e37 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 10 Oct 2023 12:07:15 +0100 Subject: [PATCH 11/11] Don't exclude SSO Push tokens from Prometheus alert In this commit [1], we excluded these tokens, because a new one was created automatically on-demand if the old one had expired. However, now that we're doing this 4 weeks before the token is due to expire, it seems sensible to reinstate the alerting for tokens associated with the SSO Push API user so that if the automatic creation fails for some reason, we still get alerted. It also has the benefit of making the code a bit simpler and easier to understand. [1]: c79c39a8c3dd20c63e6f0db9a046507c2fd0f810 --- lib/collectors/global_prometheus_collector.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/collectors/global_prometheus_collector.rb b/lib/collectors/global_prometheus_collector.rb index 3784e9aab..355379e44 100644 --- a/lib/collectors/global_prometheus_collector.rb +++ b/lib/collectors/global_prometheus_collector.rb @@ -25,7 +25,7 @@ def metrics def token_expiry_info # Cache metric to prevent needless expensive calls to the database Rails.cache.fetch("token_expiry_info", expires_in: 1.hour) do - ApiUser.where.not(email: SSOPushCredential::USER_EMAIL).flat_map do |user| + ApiUser.all.flat_map do |user| user.authorisations.not_revoked.map do |token| { expires_at: token.expires_at.to_i,