Skip to content

[core-http] Use an external token cache in BearerTokenAuthenticationPolicy#4174

Merged
daviwil merged 3 commits intoAzure:masterfrom
daviwil:cache-auth-policy
Jul 8, 2019
Merged

[core-http] Use an external token cache in BearerTokenAuthenticationPolicy#4174
daviwil merged 3 commits intoAzure:masterfrom
daviwil:cache-auth-policy

Conversation

@daviwil
Copy link
Contributor

@daviwil daviwil commented Jul 3, 2019

This change implements a simple AccessToken cache in the bearerTokenAuthenticationPolicy factory so that tokens will be cached correctly across BearerTokenAuthenticationPolicy instances. This is needed because core-http uses a RequestPolicyFactory array to create a fresh list of request policies for each request sent through the pipeline.

The old caching behavior in BearerTokenAuthenticationPolicy assumed that there would only ever be a single instance of that class per client, meaning that the existing token cache implementation is ineffective across multiple requests.

This new behavior stores a simple object in the factory function's closure which gets used as an AccessTokenCache for every BearerTokenAuthenticationPolicy instance created by that factory, resulting in a per-client token cache.

Questions for Reviewers

  • It seems that this interface needs to be exported through the public interface of core-http so that anyone using the BearerTokenAuthenticationPolicy class directly (not through the factory function) will be able to see the type. Does that sound like something we should be doing?
  • Since this type of token cache will probably be needed for other authentication policies (like @jonathandturner's ChallengeBasedAuthenticationPolicy Challenge-based authentication #4141 ), should I move the AccessTokenCache into the new core-auth once that comes online?

@daviwil daviwil requested review from bterlson, schaabs and sophiajt July 3, 2019 20:02
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what part will manage the cache?

Copy link
Contributor Author

@daviwil daviwil Jul 3, 2019

Choose a reason for hiding this comment

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

The consuming policy, though right after sending this I realized that there's probably value in sharing token expiration logic in a more legitimate class instead of using a simple object that must be managed by the policy implementation. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that there's definitely value in centralizing token expiration management, otherwise we will end up with a bug somewhere, or at least different implementations to a small extent. I wonder what would be the benefits of distributed token expiration logic, if that route is decided.

@daviwil
Copy link
Contributor Author

daviwil commented Jul 3, 2019

An all-around better implementation for this could be the following:

  • Change the AccessTokenCache interface to remove the accessToken? and add some methods:
      interface AccessTokenCache {
        setCachedToken(accessToken: AccessToken);
        getCachedToken(): AccessToken | null;
      }
    
  • Add an implementation of AccessTokenCache called ExpiringAccessTokenCache which implements token expiration logic in getCachedToken so that BearerTokenAuthenticationPolicy and ChallengeBasedAuthenticationPolicy can share it.
  • Create an instance of ExpiringAccessTokenCache in the bearerTokenAuthenticationPolicy factory closure to pass into BearerTokenAuthenticationPolicy instances.
  • Move both AccessTokenCache and ExpiringAccessTokenCache into @azure/core-auth which is merging soon. (See next comment)

I think that gives us a bit more flexibility and some code reuse since we're already going to be using an external cache with the current RequestPolicyFactory design.

@daviwil
Copy link
Contributor Author

daviwil commented Jul 3, 2019

Now that I think about it, the AccessTokenCache, etc should probably stay in core-http (for now) since it's relevant only to policy implementations.

@daviwil daviwil force-pushed the cache-auth-policy branch from b97bd76 to 2c18eea Compare July 3, 2019 21:47
@daviwil daviwil force-pushed the cache-auth-policy branch 2 times, most recently from ea9a438 to 9b57d66 Compare July 5, 2019 19:10
@daviwil daviwil force-pushed the cache-auth-policy branch from 9b57d66 to 3b86ee1 Compare July 5, 2019 19:45
@daviwil
Copy link
Contributor Author

daviwil commented Jul 5, 2019

Redesigned the AccessTokenCache as I described above and added an ExpiringAccessTokenCache implementation with tests. I think this is the right way to go, let me know what you think!

@daviwil daviwil force-pushed the cache-auth-policy branch from 3b86ee1 to 0490c89 Compare July 5, 2019 19:52
@daviwil daviwil force-pushed the cache-auth-policy branch from 0490c89 to 1e01e71 Compare July 8, 2019 17:38
@daviwil
Copy link
Contributor Author

daviwil commented Jul 8, 2019

Confirmed with @jonathandturner offline that this approach looks good for use in the challenge-based auth implementation. Merging this, thanks all!

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