-
Notifications
You must be signed in to change notification settings - Fork 192
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
Conversation
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. |
@bshaffer this is not a breaking change because it's limited to a If we allow |
When throwing an exception, the visibility of the function isn't necessarily important since the exception can bubble up through to public functions
That's a good point
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). |
@bshaffer here are the answers:
I will raise this with phpseclib as well, in hope of a reply that maybe they also think this was unintended. |
877e0f2
to
6f66276
Compare
@bshaffer I have split this PR into 2 more PRs, listed in order of how they could be merged:
|
There was a problem hiding this 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
Fixing two issues here:
Returning
type
andexpires_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)Fixing lint issues with
AccessToken.php
:The lint check is complaining about retuning
string|array
inloadPhpsecPublicKey
method (ref) and expects astring
instead. I am not sure whether this is the best fix, but looks like__toString
method (ref) is marked aspublic
also returns a key. Throwing aTypeError
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 othertype
andexpires_in
is usually ~3600 always. So, this change might be redundant, but IMHO its good to have.BEGIN_COMMIT_OVERRIDE
feat: add
expires_in
andtoken_type
to tokens fromServiceAccountJwtAccessCredentials
(#513)END_COMMIT_OVERRIDE