Skip to content

LG-13139: Fix race condition with token verifier expired validity#10481

Merged
aduth merged 4 commits intomainfrom
aduth-lg-13139-memoize-token-verifier-validity
Apr 23, 2024
Merged

LG-13139: Fix race condition with token verifier expired validity#10481
aduth merged 4 commits intomainfrom
aduth-lg-13139-memoize-token-verifier-validity

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Apr 23, 2024

🎫 Ticket

LG-13139

🛠 Summary of changes

Fixes an issue where a token may become invalid between the time it's initially validated and when the identity is referenced, since each independently triggers validation to run.

The proposed solution is to memoize the validity result, since the submission is not parameterized, so the result on an instance would not be expected to change after the initial validation call. Edit: After 93edf8f, updated to use the second of the two "Alternative solution"s presented below (see discussion).

Alternative solution could include: (I'm open to any of these)

  • Removing the call to valid? within the identity method, since UserInfoController is the only place this class is used, and it's guaranteed by the controller code that identity would only be referenced on a valid result.
  • Returning identity as part of the submission "result", i.e. FormResponse#extra, or as a tuple result [response, identity]

📜 Testing Plan

Validate that specs pass:

spec/controllers/openid_connect/user_info_controller_spec.rb

For good measure, sign-in end-to-end using the OIDC sample application.

changelog: Bug Fixes, OpenID Connect, Fix occasional errors resulting from race condition on expiring token
@aduth aduth requested a review from a team April 23, 2024 14:01
@aduth
Copy link
Contributor Author

aduth commented Apr 23, 2024

A side benefit of this is it appears that previously the validations in AccessTokenValidator would have been run 4 times based on references to identity ([1], [2], [3], [4]). Now, validation is only run once.

@aduth aduth requested a review from matthinz April 23, 2024 14:44
@aduth aduth changed the title LG-13139: Fix race condition with verifier expired validity LG-13139: Fix race condition with token verifier expired validity Apr 23, 2024
aduth and others added 2 commits April 23, 2024 12:18
Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth merged commit bc0ade9 into main Apr 23, 2024
@aduth aduth deleted the aduth-lg-13139-memoize-token-verifier-validity branch April 23, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants