auth refactor

[martindurant]

Fixes #54
Fixes #56

Trying to turn this around quickly, reviews welcome. Some of the auth modes will not be testable in typical CI or locally.

[martindurant]

So far, I have confirmed manually that using token=None, you get the default token or anon if there are no credentials available, and you can select the various connection methods either by setting token= as required or by directly calling the various methods. Is it useful to have token=False be a proxy to not calling connect() at all?

The attribute method is stored so that in testing (or introspection) we can tell which auth method finally succeeded.

Note that for now, token='cloud' will fail silently (since it would fall back to anon when token=None); perhaps we want to warn and/or raise for some combinations.

[martindurant]

(ps: tests pass locally in direct mode, since in the common case, the connection is not affected at all by these changes)

[mrocklin]

Is gtoken a standard term?

[martindurant]

No, but I wanted something different from the previously provided "cloud" term. "gcloud" might otherwise have been more reasonable.

[mrocklin]

spellcheck previoudly

[mrocklin]

Might we also provide the token explicitly as a dictionary?

[martindurant]

Yes, you can provide a dictionary - it does say that, but could provide an example.

[mrocklin]

Perhaps this should return success or failure? Perhaps it should raise and we should try-except around it?

[martindurant]

Basically, if self.token remains unset (None), auth failed for that method. I had tried return True/False, but it felt pretty ugly. The exception is 'anon', so that could instead give self.token=False or other special value.

[mrocklin]

cc @asford

[mrocklin]

I'm trying this out locally and running into a couple of small problems:

1. Tests fail

gcsfs/tests/test_core.py::test_simple FAILED

==================================================== FAILURES =====================================================
___________________________________________________ test_simple ___________________________________________________

token_restore = None

    @my_vcr.use_cassette(match=['all'])
    def test_simple(token_restore):
        assert not GCSFileSystem.tokens
>       gcs = GCSFileSystem(TEST_PROJECT, token=GOOGLE_TOKEN)

gcsfs/tests/test_core.py:18: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
gcsfs/core.py:188: in __init__
    self.connect(method=token)
gcsfs/core.py:316: in connect
    self.__getattribute__('_connect_' + method)()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <gcsfs.core.GCSFileSystem object at 0x7fc407596978>

    def _connect_token(self):
>       if 'type' in self.token or isinstance(self.token, str):
E       TypeError: argument of type 'NoneType' is not iterable

2.  There are merge conflicts with the fuse branch, though I can guess at how to merge short-term

[asford]

I'm testing a bit now on a GCE instance I use as a dev box. On this machine I've the compute sdk installed, but I'm using the service account for credentials:

fordas@salish:~/distributed_dev/gcsfs$ gcloud auth list
                  Credentialed Accounts
ACTIVE  ACCOUNT
*       353972334481-compute@developer.gserviceaccount.com

In this context auth2client.client.GoogleCredentials.get_application_default returns an token component that manages access to the service credentials and does not include a refresh_token, client_secret. It instead provides access tokens via the get_access_token interface, which handles token expiration and refresh.

This currently breaks the current master implementation, which expects a refresh token to be available. This will also break this implementation.

My recommendation, though it's a bit of a refactor, would be to store the credential object returned from get_application_default and then access the access token via credential.get_access_token().access_token. I can follow up later today to determine if this will also work for situations in which the user has obtained credentials via gcloud auth login.

[asford]

I've tested again after setting the application default credentials to my user account via gcloud auth application-default login. In this context auth2client.client.GoogleCredentials.get_application_default returns a GoogleCredentials object that can also handle access token generation and refresh.

Given that behavior, my current recommendation would be to hold the credential object returned by the get_application_default and use the interface credential.get_access_token().access_token to manage access. gcsfs would then not be responsible for managing auth token management and refresh.

In a broader sense, I would suggest emphasizing use of the default app credentials instead of obtaining and directly caching a credential object for gcsfs. In "off cloud" contexts, the user can then manage authorization via the gcloud auth application-default command, which provides login, revoke. In "on cloud" this then provides easy access to the default service account, with the option for a user to override this setting by gcloud auth login.

[asford]

@martindurant How quickly are you trying to turn this around? If you can wait until tomorrow I can open a PR-onto-this-PR that implements the 'application-default' behavior I've described above. As it stands now the strategy of unpacking the credential object returned by GoogleCredentials will fail in contexts where a service account is available.

[mrocklin]

(for context @asford I've asked @martindurant to accelerate some work here for an upcoming conference talk this coming Monday)

[martindurant]

@asford , @mrocklin , would it be a good idea to merge this, to allow for the service account PR to follow and fix merge conflicts in the fuse branch (I can handle that).
Again, vcr tests can wait, but should be re-recorded and working before release.

[mrocklin]

This is fine with me. @asford might have other thoughts.

…

On Thu, Jan 4, 2018 at 2:55 PM, Martin Durant ***@***.***> wrote: @asford <https://github.com/asford> , @mrocklin <https://github.com/mrocklin> , would it be a good idea to merge this, to allow for the service account PR to follow and fix merge conflicts in the fuse branch (I can handle that). Again, vcr tests can wait, but should be re-recorded and working before release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AASszOyihI0vQXGkpJsow_6zcOx6D4D2ks5tHTrZgaJpZM4RTXfZ> .

[asford]

No problem with that plan on my end, we've all been there. Let's plan you merging what's needed for your talk on Monday. I can setup a follow-up PR that handles application default credentials afterward.

In light of that, can we agree that the internal API implemented here may need to change to reflect the final authorization story> I think we'll be able to continue supporting all the named token modes (cloud, anon, browser, etc...) but I'd like to have a chance to think through the client library integration and make sure it's being used properly.

@martindurant As a side note, it may be better if gtoken was named application-default, as that's the name of the related gcloud auth subcommand.

[martindurant]

@asford , I specifically decided against 'application_default', because it sounds like we think of it as the default, which it isn't. I've decided that 'google_default' is sufficiently unambiguous.

Note as another point for follow up, _call() should maybe check for the continued validity of the token as given by its expiry and call connect again self.connect(method=self.method) if necessary.