-
Notifications
You must be signed in to change notification settings - Fork 431
Add JWT access token support for service accounts #274
Conversation
} | ||
payload.update(self._kwargs) | ||
|
||
first_segment = _urlsafe_b64encode(_json_encode(header)) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Finished review pass. |
signature = base64.urlsafe_b64encode(rsa_bytes).rstrip(b'=') | ||
|
||
return assertion_input + b'.' + signature | ||
if self._scopes: |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
New code paths with no supporting tests. Write the tests please. Also the design questions is any important one. Why did you elect to do this with the existing class rather than a subclass? |
@dhermes You seem to assume that there was a design to begin with; I'm still trying to figure out what it is that is desired here (and frankly I don't have enough familiarity with the oauth2client library structure or its use to confidently figure out what it is that needs to be here for this feature). At this point I've sort of relegated it to a constraint problem that'll need to be figured out through the pains of circumlocution. |
@dhermes Actually, can we talk offline? This warrants an offline discussion. |
Sorry I forgot to peek at the outdated diffs, that was a fail by me. I won't have time for a VC until probably Friday, but am happy to chat over email. |
Got all caught up on the outdated diffs/comments. LMK when / how you want to chat off of GitHub. |
This is similar to #161, but takes into account the intended use case of simply 'promoting' an assertion to using a self-issued JWT access token instead of something completely different.
But, I'm not quite sure of the appropriate way to test this. There's some test code that covers it (apparently the test code generated the assertion even when the old
create_scoped_required
would have returnedTrue
), but nothing particularly functional.It might negatively interact with the spirit of #211.
cc @nathanielmanistaatgoogle @anthmgoogle #252 #253