Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Support token_uri and revoke_uri in ServiceAccountCredentials #510

Merged
merged 6 commits into from
Jun 7, 2016

Conversation

thobrla
Copy link
Contributor

@thobrla thobrla commented May 20, 2016

This change adds support for custom authorization token
and revocation URIs in the various ServiceAccountCredentials
factory methods.

It also makes the factory method from_p12_keyfile_contents
public for use by clients who have already loaded the file.

This change adds support for custom authorization token
and revocation URIs in the various ServiceAccountCredentials
factory methods.

It also makes the factory method from_p12_keyfile_contents
public for use by clients who have already loaded the file.
token_uri: string, URI for token endpoint. For convenience defaults
to Google's endpoints but any OAuth 2.0 provider can be
used.
revoke_uri: string, URI for revoke endpoint.

This comment was marked as spam.

This comment was marked as spam.

@nathanielmanistaatgoogle
Copy link
Contributor

Who wants to lead review? @dhermes? @jonparrott? Should we be reticent about promoting ServiceAccountCredentials._from_p12_keyfile_contents to public visibility?

@dhermes
Copy link
Contributor

dhermes commented May 23, 2016

@nathanielmanistaatgoogle You get the prize (as the official official maintainer).

No need to make _from_p12_keyfile_contents public though, maybe @thobrla has a reason I haven't though of?

@thobrla
Copy link
Contributor Author

thobrla commented May 23, 2016

The argument for publicizing from_p12_keyfile_contents is that currently if I have already read the file (say for the purposes of determining whether it's a JSON file or not), there is no need for oauth2client to re-open the file

@thobrla
Copy link
Contributor Author

thobrla commented May 25, 2016

ping @nathanielmanistaatgoogle (this request blocks a few others in my pipeline)

@dhermes
Copy link
Contributor

dhermes commented May 25, 2016

RE:

The argument for publicizing from_p12_keyfile_contents is that currently if I have already read the file (say for the purposes of determining whether it's a JSON file or not), there is no need for oauth2client to re-open the file

A user can just use from_p12_keyfile_buffer and put the already read contents in a StringIO buffer.

@thobrla
Copy link
Contributor Author

thobrla commented May 25, 2016

A user can just use from_p12_keyfile_buffer and put the already read contents in a StringIO buffer.

Done.

@thobrla
Copy link
Contributor Author

thobrla commented May 26, 2016

FYI, I need to amend this to take token/revoke URIs from a JSON key file to fix #513. The new behavior will be:

  1. If the user specifies a token/revoke URI in arguments, use that.
  2. Else if the key file specifies token/revoke URI, use that.
  3. Else use the Google defaults.

@thobrla
Copy link
Contributor Author

thobrla commented May 31, 2016

Amendments complete, this now fixes #513

@thobrla
Copy link
Contributor Author

thobrla commented Jun 2, 2016

Ping - this should be ready to go.

@theacodes
Copy link
Contributor

theacodes commented Jun 3, 2016

@nathanielmanistaatgoogle is out on vacation.

@thobrla I'd still like to see the comments around the revoke_uri parameter addressed. Please completely document or cross-ref that argument.

After that, this change LGTM. However, I'd like to have @dhermes take a look before I merge if he has the time.

@thobrla
Copy link
Contributor Author

thobrla commented Jun 6, 2016

PTAL

Made revoke_uri comments more verbose.

@@ -172,22 +188,41 @@ def _from_parsed_json_keyfile(cls, keyfile_dict, scopes):
private_key_pkcs8_pem = keyfile_dict['private_key']
private_key_id = keyfile_dict['private_key_id']
client_id = keyfile_dict['client_id']
if not token_uri:
if 'token_uri' in keyfile_dict:

This comment was marked as spam.

This comment was marked as spam.

@theacodes
Copy link
Contributor

Two more comments. If @dhermes doesn't object by EOD today, I'll merge once those two comments are addressed.

@dhermes
Copy link
Contributor

dhermes commented Jun 7, 2016

I don't have much bandwidth but this looks mostly OK.

@theacodes
Copy link
Contributor

Thanks, @dhermes

@theacodes theacodes merged commit b557440 into googleapis:master Jun 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants