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

V2.0 #322

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

V2.0 #322

wants to merge 11 commits into from

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Feb 9, 2021

See UPGRADING.md

TODO

  • Fix SysVCachePool race condition
  • Better token expiration
    • Verify token is not expired before using it (This is now done with the cache but have additional check)
    • Fix bug where token expiration is never checked (b/149049606)
    • Set expiration for ID token / JWT / self-signed
  • Use PSR-7 exception interfaces instead of Guzzle Exceptions
    • verify using these interfaces is sufficient
    • Make wrapping work for Guzzle 6
  • Make fetching IAP certs vs OIDC certs implicit (currently the user has to specify the IAP cert URL)
  • Wrap Firebase/JWT Exceptions
  • Consider adding caching to OAuth2
  • Consider adding method hasValidToken to OAuth2
  • Consider making some classes final
  • Consider removing "scope" and "project" methods from ADC in favor of withScope and withProject methods which clone the immutable objects

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 9, 2021
@bshaffer bshaffer changed the title V2.x V2.0 Feb 19, 2021
@bshaffer bshaffer marked this pull request as ready for review February 19, 2021 00:16
@bshaffer
Copy link
Contributor Author

cc @ericnorris

@bshaffer
Copy link
Contributor Author

bshaffer commented Mar 1, 2021

@dwsupplee @jdpedrie this is ready for review!


private function fetchAuthTokenNoCache(): array
{
throw new \LogicException(

Choose a reason for hiding this comment

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

Traits can have abstract methods, would that make more sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. It might make sense to make them abstract, but I was thinking that since both these functions are optional (credentials don't have to support caching) that it makes more sense to throw the exception at runtime. In the case of AnonymousCredentials, there's no way for those exceptions to be thrown because they'd never be called.

@ericnorris
Copy link

Thanks for tagging me in this! There's a lot to go through, and I'll also have to dig into my memory a bit to see if there's anything else I had thought about since the last time we spoke.

After a quick glance though, I have some high-level comments that you can take or leave, in no particular order:

  • Have you considered having separate fetchAccessToken and fetchIdentityToken methods? I think it is more clear and explicit to separate them, rather than fetchAuthToken, which does something depending on if target_audience is passed into the options argument in the constructor. fetchIdentityToken could then take the audience as a required string parameter instead, which also adds type safety (e.g. fetchIdentityToken(string $targetAudience)). I know that users at my company found the combined behavior unintuitive.
  • It'd be nice if IO was done lazily; it seems that calling makeCredentials still hits the metadata server immediately. That being said, I do see that you have caching there - I wonder if it's necessary to cache with a cache lifetime? I can't imagine that you would somehow suddenly not be on a Compute Engine VM if you were 1500 seconds ago. It feels like caching permanently is safe here.
  • Have you considered making an impersonated credentials class, for when you'd like to have Service Account A impersonate Service Account B? I think I brought this up when we chatted, but it's useful to have this be a first-class citizen of the Google auth library rather than implementing it ourselves.
  • Calls to getProjectId are not cached. In general, it's ideal for my company that anything that could hit the metadata server or make an API call be cached, as we don't want to potentially incur that cost on every request.
  • Two comments about the JWT verification:
    • Have you considered splitting out the JWT verification into a separate library? Seems like the functionality is different enough, or perhaps the credentials stuff should be in a "credentials" library.
    • Google's OAuth2 certs URL responds with Cache-Control and Expires headers, but if I understand the code correctly it's just caching for the cacheLifetime parameter. Any chance it could intelligently cache for as long as Google says is okay?

Semi-relatedly, during some down-time last year I took a stab at writing what I would have done for a Google PHP auth library. You can take a look at my repo here for any ideas, since I chose to do some of the things I suggested above, or not - no pressure on my end.

@wlasnapl
Copy link

wlasnapl commented Mar 2, 2022

Do you know when this version with PSR Cache v3 support will be released?

@bshaffer
Copy link
Contributor Author

bshaffer commented Mar 2, 2022

@wlasnapl we do not have an ETA on that, but we do support psr/cache:v2, and any repository which supports psr/cache:v3 SHOULD support psr/cache:v2 as well, for compatibility. That will prevent you from encountering diamond dependency issues with this library.

If you find a library which supports psr/cache:v3 without supporting psr/cache:v2, please submit a PR to include v2 support or open an issue, OR post it here and I'll write the PR, because there's no reason for it to not be supported.

@wlasnapl
Copy link

wlasnapl commented Mar 6, 2022

@bshaffer I've wrote an issue to Contetful support and I can see that there is PR for that: contentful/contentful.php#308

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add locking to FetchAuthTokenCache
3 participants