Skip to content
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

[Proposal] TokenCredentialRefresher and authenticationOptions #16924

Closed

Conversation

sadasant
Copy link
Contributor

@sadasant sadasant commented Aug 13, 2021

This PR is a proposal to solve the concerns related to the OBO credential, and a refactoring of the TokenCycler.

Identity Context:

  • Issue Feature: Add On-Behalf-Of (OBO) Auth Flow for the Microsoft Graph Team #15804 asks us to implement a credential to support On-Behalf-Flow. This credential would need us to switch how we do caching, in memory and persistent caching, arbitrarily on specific requests.
  • We’ve been thinking on approaches for the OBO credential, here are the last two that Scott Schaab and I found the most reasonable: OBO Design for JS.
  • Out of those two designs, the one that seems to give us a greater benefit over time, according to @xirzec and I, is the “Swapping TokenCredentials” approach.
  • This approach focuses on replacing the pipeline credential through the options of any client method that supports TokenCredentials. The idea is that, by replacing the credential, any caching, including persistence caching and the underlying MSAL client, will remain scoped to the credential, thus allowing clients to swap authenticating a client from one set of credentials, cached anywhere or not cached, to another (implying from one OnBehalfOfCredential to another, but also any other credential just as well).
  • (this will be relevant once you see the proposal) Identity is in v2-beta, which allows us to do changes that we would not otherwise consider.

core-rest-pipeline context:

  • Not too long ago, we changed how we worked with token cache by implementing the tokenCycler.
  • The tokenCycler was implemented to solve bugs we had regarding the caching of tokens.
  • The tokenCycler represents a significative increase of complexity on core-rest-pipeline. When core-rest-pipeline is generally as simple as posible (considering avoiding as many breaking changes as possible with the previous core-http client), and as shallow as possible in the complexity per file/folder, the tokenCycler adds a significant pit of complexity in a single place, which concerned me in particular. I’ve been trying to clean this up for some months, see PR: [core-rest-pipeline] Idea for improving the tokenCycler #15120

Proposals in this PR:

  • This PR adds the concept of authenticationOptions to the pipelines, in order to swap the TokenCredential used to authenticate on any client, so that users can call to client.method({ authenticationOptions: { credential: new Credential() } }) at any time in their code, and swap the credential used to authenticate only for that request.
  • This PR also moves the tokenCycler logic to Identity. The idea is that all of the Identity credentials will now extend a RefreshTokenCredential abstract class which will encapsulate all of the tokenCycler behavior.
  • core-rest-pipeline then will now not have any of the complexity of the tokenCycler.
    • All future improvements to the cycler logic would then happen only in Identity.
  • Identity credentials will now have a new method called refreshToken which will return a previously obtained token or retrieve a new token if either it’s the first time, or if the cached token is about to expire.
  • When users swap credentials, they will also be swapping the in-memory cache, which means that if they re-use the credentials, they will benefit from immediate responses if there are previously obtained tokens.

TODOs:

  • ⚠️ We could improve this even further by removing the getAccessToken method in core-rest-pipeline’s AuthorizeRequestOptions and replace it with just a credential: TokenCredential | TokenCredentialRefresher property. This would be a breaking change on core-rest-pipeline though.
  • This PR doesn’t include the OBO credential itself. I’ll be doing that in another PR.
  • This PR only migrates the DefaultAzureCredential to extend the RefreshTokenCredential class. If this approach is good enough, I will move all the other Identity credentials to extend the RefreshTokenCredential class.

This PR also:
Closes #15120

@@ -4,6 +4,8 @@

### Features Added

- New interface added: `TokenCredentialRefresher`. Represents a credential that can refresh a token over time.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the description of this PR ✨

@sadasant
Copy link
Contributor Author

sadasant commented Aug 13, 2021

Just added this to the description:
This approach focuses on replacing the pipeline credential through the options of any client method that supports TokenCredentials. The idea is that, by replacing the credential, any caching, including persistence caching and the underlying MSAL client, will remain scoped to the credential, thus allowing clients to swap authenticating a client from one set of credentials, cached anywhere or not cached, to another (implying from one OnBehalfOfCredential to another, but also any other credential just as well).

@sadasant
Copy link
Contributor Author

This approach has a considerable problem: we would need to make the refresher way more complicated, to handle caching per scope, and to handle cleaning up previously cached tokens. I’m exploring alternative designs.

Here’s another idea: #16940

@sadasant
Copy link
Contributor Author

Closed on favor of: #16995

@sadasant sadasant closed this Aug 20, 2021
@sadasant sadasant deleted the identity/obo-swapping-credentials branch August 20, 2021 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant