Consolidate Service Account classes.

[dhermes]

Attempt at #211 (and integrating changes from #1 in as well).

@nathanielmanistaatgoogle This doesn't implement the scope-less credentials yet but I wanted you to get a chance to look at it before then. I am going to remove the SignedJwtAssertionCredentials class and will probably update this PR when that is done as well.

[nathanielmanistaatgoogle]

Looks mostly really good.

[dhermes]

@nathanielmanistaatgoogle Addressed review comments (with some remarks) and also added a commit to remove the old service account implementation (as well as to use json_str instead of s in all of the from_json methods)

[dhermes]

@nathanielmanistaatgoogle FYI https://bitbucket.org/ned/coveragepy/issues/473/context-manager-confuses-branch-miss is causing test_service_account.py to report less than 100% coverage but it'll resolve itself once the update mentioned in the Bitbucket issue is released

[dhermes]

@nathanielmanistaatgoogle It'd be nice to use ServiceAccountCredentials.from_json_keyfile in GoogleCredentials.from_json and oauth2client.client._get_application_default_credential_from_file, but as of right now the factory only takes a filename. What do you think about making from_json_keyfile accept one of three objects as its first argument:

• filename
• file / stream object
• parsed dictionary (that used to be JSON)

[nathanielmanistaatgoogle]

As for what I think about making from_json_keyfile accept one of three objects as its first argument: ServiceAccountCredentials.from_json_keyfile is a wholly original method that neither implements nor overrides anything else, right? Does it need to be able to take the other two kinds of input or is it simply enough to provide some way to accept the other two kinds of input? Why not define ServiceAccountCredentials.from_json_keydict and ServiceAccountCredentials.from_json_keyfilename and then have all three internally and privately delegate to common code? Would that work?

[dhermes]

I also wanted to discuss the fact that this implementation double-stores scopes and shadows lots of fields from the parent that are unused

Here is a quick rundown of everything in __dict__ for a ServiceAccountCredentials object:

_kwargs -> {}
_private_key_id -> 9ad967...
_private_key_password -> None
_private_key_pkcs12 -> None
_private_key_pkcs8_pem -> -----BEGIN PRIVATE KEY-----...
_scopes -> https://www.googleapis.com/auth/userinfo.email
_service_account_email -> [email protected]
_service_account_id -> 10639...
_signer -> <oauth2client._openssl_crypt.OpenSSLSigner object at 0x7f6d2801d090>
_user_agent -> None
access_token -> ya29.fQ...
assertion_type -> None
client_id -> None
client_secret -> None
id_token -> None
invalid -> False
refresh_token -> None
revoke_uri -> https://accounts.google.com/o/oauth2/revoke
scopes -> set([])
store -> None
token_expiry -> 2016-02-03 22:34:22.926351
token_info_uri -> None
token_response -> {u'access_token': u'ya29.fQ...', u'token_type': u'Bearer', u'expires_in': 3600}
token_uri -> https://accounts.google.com/o/oauth2/token
user_agent -> None

[dhermes]

@nathanielmanistaatgoogle I'm fine with have three factories (from_json_keyfile, from_json_keydict and from_json_keyfile_name) for varying stages of parsed-ness. Is the reason for this just an aversion to allowing multiple types in a function signature?

[dhermes]

@nathanielmanistaatgoogle We should also discuss how to break this up. I think the first two PRs could be

• pure-Python module as a stand-alone
• make service_account.py use oauth2client.crypt instead of the module's own hand-rolled auth in current use
• remove client.SignedJWTCredentials (will require temporarily disabling / breaking the system tests for .p12 keys)
• everything else?

[nathanielmanistaatgoogle]

Responding to "I also wanted to discuss the fact that this implementation double-stores scopes and shadows lots of fields from the parent that are unused": my first reaction is "grumble grumble public classes grumble grumble implementation inheritance grumble grumble cuss words cuss words", but that's not productive. So my second reaction is: that's just kind of a thing that happens with deep inheritance hierarchies, but does any of it negatively effect either the implementation complexity or the correctness of the particular leaf class currently under review?

[nathanielmanistaatgoogle]

Responding to "Is the reason for this just an aversion to allowing multiple types in a function signature?": yes. In my experience ad-hoc polymorphism always harms the long-term maintenance of a codebase and should be avoided wherever it can be avoided.

[nathanielmanistaatgoogle]

Responding to "We should also discuss how to break this up": that does sound like a good plan. Certainly "introduce new code in its own commit" is a winning first step. Why break the system tests? Would it just be a matter of changing the out-of-repository artifacts of those tests at the same time as changing the in-repository artifacts of those tests?

[dhermes]

@nathanielmanistaatgoogle Pushed a new commit to address the docstring issues and to remove _token_uri and _revoke_uri from the svc. acct. creds constructor.

RE: Breaking system tests, I just meant that if we have a commit where client.SignedJWTCredentials is deleted and ServiceAccountCredentials doesn't support .p12 keys, then the system test corresponding to a .p12 key can't be run.

[nathanielmanistaatgoogle]

Responding to: "if we have a commit where client.SignedJWTCredentials is deleted and ServiceAccountCredentials doesn't support .p12 keys, then the system test corresponding to a .p12 key can't be run" - that would be an intermediate commit, right? ... and we don't even care if tests pass at intermediate commit points, only at head-of-pull-request and head-of-repository, right?

[dhermes]

Well my plan was for 4 PRs, not 4 commits. You can break it up as you like, not worth discussing with 4 posts (which we've done already).

[nathanielmanistaatgoogle]

Ah, okay. I have no objection to four pull requests if that's your preference. Thank you for the clarity.

[dhermes]

Closing in lieu of #395, #396, #397, #398, #399