Create a new access token for the SSO Push API user *before* expiry not *at* expiry #2418
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Ugh - failure is due to Brakeman SQL Injection check - I'll sort it out after lunch! |
floehopper
force-pushed
the
rotate-sso-push-access-tokens-before-expiry
branch
from
05deb28
to
3515c5a
Compare
|
This all looks good to me. Thanks for taking the time to extract some scopes and remove that unnecessary We spoke on slack about the use of |
chrislo
approved these changes
Oct 10, 2023
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.
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
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]: alphagov/govuk-helm-charts#1303 [2]: alphagov/govuk-infrastructure#953
This is used in a bunch of places across the codebase, so it makes sense to extract it into a scope.
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.
There's no need to do the date addition of `created_at + expires_in` when the `doorkeeper` gem can do it for us.
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]: 43331d0
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.
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
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]: c79c39a
floehopper
force-pushed
the
rotate-sso-push-access-tokens-before-expiry
branch
from
3515c5a
to
633b2c5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Trello: https://trello.com/c/XfBBRL5M
The main change in this PR is that we will now create a new access token for the SSO Push API user on-demand if the current token is due to expire in the next 4 weeks rather than if it has already expired.
Some Prometheus alerting was added in #2354 but these tokens for the SSO Push API user were excluded in #2413, because, unlike tokens for other API users, they shouldn't need to be manually rotated by 2nd Line Tech. However, @theseanything suggested that if we create the tokens before they expire and before the alert fires, then we can include these tokens in the alerting so that if anything goes wrong with the automatic token creation 2nd Line Tech will receive an alert.
I've also made some minor improvements to the API user edit page which should make it a bit easier to work out what's going on with a user's tokens:
I've also extracted a bunch of scopes to try to reduce duplication and make the code a bit easier to read.