Skip to content

Commit

Permalink
Ignore expired & revoked tokens in SSOPushCredential.credentials
Browse files Browse the repository at this point in the history
Trello: https://trello.com/c/piKwrxYP

Previously when a token reached its (10 year) expiry, the code in
SSOPushCredential.credentials was still finding the expired token and
trying to use it in the SSOPushClient, but triggering a SSOPushError
exception due to an invalid bearer token.

While we were debugging this also realised that
SSOPushCredential.credentials was also finding revoked tokens, thus
making it impossible to resolve the situation from the Signon UI.

Now we ignore both expired & revoked tokens when deciding whether we
already have a valid token or that we need to create one.

Note that the `Doorkeeper::AccessToken.not_expired` scope [1] seems to
helpfully exclude both expired *and* revoked tokens.

[1]: https://github.com/doorkeeper-gem/doorkeeper/blob/986115cc228ff30dc1ead0f4101195448994f5d4/lib/doorkeeper/orm/active_record/mixins/access_token.rb#L47-L66
  • Loading branch information
floehopper committed Oct 6, 2023
1 parent b65d415 commit 0a718b9
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 0 deletions.
1 change: 1 addition & 0 deletions lib/sso_push_credential.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ def credentials(application)
user.grant_application_permissions(application, PERMISSIONS)

user.authorisations
.not_expired
.create_with(expires_in: 10.years)
.find_or_create_by!(application_id: application.id).token
end
Expand Down
30 changes: 30 additions & 0 deletions test/lib/sso_push_credential_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,36 @@ class SSOPushCredentialTest < ActiveSupport::TestCase
end
end

context "given an application with a revoked authorisation" do
setup do
@user.authorisations.create!(application_id: @application.id, revoked_at: Time.current)
end

should "create a new authorisation to replace the revoked one" do
bearer_token = SSOPushCredential.credentials(@application)

new_authorisation = @user.authorisations.find_by(token: bearer_token)
assert_nil new_authorisation.revoked_at
assert_equal @application.id, new_authorisation.application_id
end
end

context "given an application with an expired authorisation" do
setup do
travel(-1.day) do
@user.authorisations.create!(application_id: @application.id, expires_in: 0)
end
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 > Time.current
assert_equal @application.id, new_authorisation.application_id
end
end

should "create an authorisation if one does not already exist" do
assert_equal 0, @user.authorisations.count

Expand Down

0 comments on commit 0a718b9

Please sign in to comment.