V2.0

[bshaffer]

See UPGRADING.md

TODO

• [x] ~~Fix SysVCachePool race condition~~
• [x] Better token expiration
  • [x] ~~Verify token is not expired before using it (This is now done with the cache but have additional check)~~
  • [x] Fix bug where token expiration is never checked (b/149049606)
  • [x] Set expiration for ID token / JWT / self-signed
• [x] Use PSR-7 exception interfaces instead of Guzzle Exceptions
  • [x] verify using these interfaces is sufficient
  • [ ] Make wrapping work for Guzzle 6
• [x] ~~Make fetching IAP certs vs OIDC certs implicit (currently the user has to specify the IAP cert URL)~~
• [x] Wrap Firebase/JWT Exceptions
• [x] ~~Consider adding caching to OAuth2~~
• [x] ~~Consider adding method hasValidToken to OAuth2~~
• [x] 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

[bshaffer]

cc @ericnorris

[bshaffer]

@dwsupplee @jdpedrie this is ready for review!

[ericnorris]

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]

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

[bshaffer]

@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]

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