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

feat: add expires_in and token_type to ServiceAccountJwtAccessCredentials #513

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

vishwarajanand
Copy link
Contributor

@vishwarajanand vishwarajanand commented Dec 29, 2023

Fixing two issues here:

  1. Returning type and expires_in with the token in case of JWT keys. ref: feat: add universe domain support to core, bigquery, storage, and pubsub google-cloud-php#6850 (comment) Screenshot 2023-12-28 at 00 06 36

  2. Fixing lint issues with AccessToken.php:
    The lint check is complaining about retuning string|array in loadPhpsecPublicKey method (ref) and expects a string instead. I am not sure whether this is the best fix, but looks like __toString method (ref) is marked as public also returns a key. Throwing a TypeError if the key is array and not a string since thats consistent with an existing error in that case from here.

We confirmed that the access_token cannot have any other type and expires_in is usually ~3600 always. So, this change might be redundant, but IMHO its good to have.

BEGIN_COMMIT_OVERRIDE
feat: add expires_in and token_type to tokens from ServiceAccountJwtAccessCredentials (#513)
END_COMMIT_OVERRIDE

@vishwarajanand vishwarajanand marked this pull request as ready for review December 29, 2023 18:02
@vishwarajanand vishwarajanand requested a review from a team as a code owner December 29, 2023 18:02
@bshaffer
Copy link
Contributor

I think the best thing here would be to allow the method loadPhpsecPublicKey to return string or array, rather than throw a type error, which could be a breaking change.

@vishwarajanand
Copy link
Contributor Author

@bshaffer this is not a breaking change because it's limited to a private function return type and the TypeError in the affected case would anyways be thrown from Firebase\JWT\Key class, here.

If we allow string|array, it will fail lint check because Firebase\JWT\Key::__construct(..) doesn't accept array and will throw a lint failure.

@bshaffer
Copy link
Contributor

bshaffer commented Jan 2, 2024

this is not a breaking change because it's limited to a private function

When throwing an exception, the visibility of the function isn't necessarily important since the exception can bubble up through to public functions

the TypeError in the affected case would anyways be thrown from Firebase\JWT\Key class

That's a good point

If we allow string|array, it will fail lint check

I see. I'd be curious to know 1. What caused the type to change (and suddenly trigger the lint error when it wasn't before) and 2. In what scenarios does phpseclib return a string here? This will help us make sure that throwing an exception is indeed the right option here (as opposed to say, excluding a version of phpseclib in our dependencies, or casting the string to array).

src/AccessToken.php Outdated Show resolved Hide resolved
@vishwarajanand
Copy link
Contributor Author

vishwarajanand commented Jan 2, 2024

@bshaffer here are the answers:

  1. What caused the type to change (and suddenly trigger the lint error when it wasn't before)
    It's because of a recent phpseclin release v3.0.35, relevant commit. toString() seems to have had a conscious (maybe I am not sure?) change which is breaking our lint. There is only one place where the AsymmetricKey::toString() is implemented and doesn't return string, and that's PublicKey::toString(), ref which returns a mixed.
  2. throwing an exception is indeed the right option here (as opposed to say, excluding a version of phpseclib in our dependencies, or casting the string to array)
    Excluding versions likely wont help coz this will be visible in all future 3.0.35+ versions.
    Wrt Casting, I am not sure because its mixed so not sure of the array key, etc and this class is used in a lot of places.

I will raise this with phpseclib as well, in hope of a reply that maybe they also think this was unintended.

src/AccessToken.php Outdated Show resolved Hide resolved
src/AccessToken.php Outdated Show resolved Hide resolved
@vishwarajanand vishwarajanand added the release blocking Required feature/issue must be fixed prior to next release. label Jan 3, 2024
@vishwarajanand
Copy link
Contributor Author

@bshaffer I have split this PR into 2 more PRs, listed in order of how they could be merged:

  1. For fixing lint: fix: lint error from new phpseclib #517
  2. For adding expires_in and token_type in Jwt: this PR
  3. For deprecating phpseclibV2 classes: deprecate: remove support for phpseclib V2 #518

@bshaffer bshaffer removed the release blocking Required feature/issue must be fixed prior to next release. label Jan 3, 2024
@bshaffer bshaffer changed the title fix: ServiceAccountJwtAccessCredentials omits expires_in and token_type feat: ServiceAccountJwtAccessCredentials omits expires_in and token_type Jan 4, 2024
Copy link
Contributor

@bshaffer bshaffer left a comment

Choose a reason for hiding this comment

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

I'm calling this a feature since it adds functionality that previously wasn't there

@bshaffer bshaffer merged commit ee2436d into main Jan 4, 2024
13 checks passed
@bshaffer bshaffer deleted the fix_tpc branch January 4, 2024 15:52
@bshaffer bshaffer changed the title feat: ServiceAccountJwtAccessCredentials omits expires_in and token_type feat: add expires_in and token_type to ServiceAccountJwtAccessCredentials Jan 4, 2024
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.

2 participants