-
Notifications
You must be signed in to change notification settings - Fork 404
Update InitiateLongRunningObo method to retrieve tokens from the cache if request assertion matches. #4111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Update InitiateLongRunningObo method to retrieve tokens from the cache if request assertion matches. #4111
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
ae6180c
Add telemetry API ID for long-running OBO. Add tests.
pmaytak 6ac0e09
Add separate Api IDs for both long-running OBO methods.
pmaytak 5ac978b
Merge branch 'main' into pmaytak/lr-obo-apiid
gladjohn 1565951
Update InitiateLongRunningObo method to retrieve tokens from the cach…
pmaytak f444814
Merge remote-tracking branch 'origin/pmaytak/lr-obo-apiid' into pmayt…
pmaytak d0a1bff
Apply suggestions from code review
pmaytak c929d66
Merge main.
pmaytak 0633f64
Fix comments.
pmaytak 353efba
Merge pmaytak/lr-obo-apiid
pmaytak 1bd518c
Update comment, variable naming.
pmaytak 8cbd62b
Fix test.
pmaytak a363e4c
Merge main.
pmaytak 3287ed3
Add assertion hash check to refresh tokens. Update tests.
pmaytak f35705c
Merge remote-tracking branch 'origin/main' into pmaytak/lr-obo-assertion
pmaytak e78feeb
Fix log text.
pmaytak 3f3f2aa
Update if check.
pmaytak f148f28
Add test when cached tokens don't have an assertion hash.
pmaytak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently cached/serialized token items (aka legacy tokens) don't have this new assertion hash property. So there's a scenario if app is updated to this new MSAL version that checks the hash now.
An option here is to add additional check -
&& !string.IsNullOrEmpty(cachedAccessToken.OboAssertionHash).InitiateObowill never match these cached legacy tokens, and will always use the passed-in assertion to get tokens from AAD, even if the cached tokens are unexpired. This actually mimics current behavior (4.51.0+).InitiateObowill return the valid matched legacy access tokens and, if expired, will use legacy refresh token to refresh. This mimics the 4.50.0- behavior but only for legacy tokens. But once the legacy RT is used to refresh, then new tokens will be saved with the assertion hash.In the scenario when AAD revokes AT and RT, method 2 reintroduces the previous issue of returning revoked tokens, so devs would have to use the
StopLongObomethod.AcquireLongObois not affected since it doesn't have the assertion in the request.A more general question, do we even want to have an assertion hash match for refresh tokens?
Thoughts @bgavrilMS, @trwalke? Maybe I missed something?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason we discussed adding this was to prevent the scenario where someone is using initiate long running OBO over and over again from breaking after 4.51.0+. The initial implementation for this PR lead us to the realization that in order to actually do that while maintaining the logic that fixes the revocation issue is to add another check to the refresh OBO logic to check for a matching token hash. So, if we want to prevent breaking customers, we should keep the hash match for refresh tokens.
I think this will only mimic the behavior of 4.51.0+ for the first iteration. So, once the user is required to sign in again, the legacy tokens should be overwritten and assertion will be cached with the new tokens. I think this is fine. The other option (#2) will require the developer to add the
StopLongRunningApimethod to remove the revoked tokens. That is a much more difficult state to recover from than simply asking the user to sign in again.So, I dont think we need to add the null check. The end user experience is affected, but all they need to do is sign in again to recover and then everything will work fine.
Adding this null check will put the user in a state they cant recover from if the token are revoked without the app developer changing the app or deleting tokens manually.
Lets sync on this tomorrow @pmaytak incase I am missing something.