feat: all asynchronous credential types required for all authentication types (service accounts, default... etc) #573

anibadde · 2020-07-28T02:34:39Z

PR # 2 for AsyncIO Auth Library:

Implemented all asynchronous credentials types
- oauth2
  - client
  - credentials (inherited from credentials_async)
  - service account
- JSON Web Token
- default credentials

google-cla · 2020-07-28T02:34:46Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

…ext manager

…n class

…p_requests

google/auth/_default_async.py

…onship

google/auth/_default_async.py

crwilcox · 2020-07-30T05:17:18Z

google/auth/_default_async.py

+from google.auth import exceptions
+
+
+def _warn_about_problematic_credentials(credentials):


extract to a helper to share?

Changed to extract from auth/_default

crwilcox · 2020-07-30T05:23:10Z

google/auth/credentials_async.py

@@ -62,7 +63,10 @@ async def before_request(self, request, method, url, headers):
        # the http request.)

        if not self.valid:
-            self.refresh(request)
+            if inspect.iscoroutinefunction(self.refresh):


what is the scenario where this wouldn't be a coroutine?

There are a couple of async credentials scenarios where self.refresh for those class instances return a None Type object (when it is not an async function). We run into this in an auth/jwt credentials object, and therefore add the check on whether refresh for that specific class instance is a coroutine or not.

crwilcox · 2020-07-30T05:24:24Z

google/auth/jwt_async.py

+from google.auth import jwt
+
+
+def encode(signer, payload, header=None, key_id=None):


The top level methods of this don't appear to be specific to async. If that is so, you can get them here with from google.auth.jwt import encode, _unverified_decode...

Made this change.

Turns out that the jwt_async test file defines our async version of the jwt token, and then calls encode/decode etc with the token object, so the direct import of that form wouldn't account for a token.decode/token.encode statement. Is there another way I could do the import to make that type of logic work? The reasoning is that there could be external locations where these encode/decode etc methods are called on the async tokens, so I'm leaning towards the current approach to account for those cases.

crwilcox · 2020-07-30T05:27:46Z

google/oauth2/_client_async.py

+        )
+
+        """
+        except exceptions.TransportError as caught_exc:


commented out code

crwilcox · 2020-07-30T05:28:02Z

google/oauth2/_client_async.py

+            if hasattr(response_body1, "decode")
+            else response_body1
+        )
+        # CHANGE TO READ TO END OF STREAM


Made this change as the approach to reading in the http response was modified, comment deleted.

crwilcox · 2020-07-30T05:29:49Z

google/oauth2/_client_async.py

+            error_code = response_data.get("error") or ""
+            if (
+                any(e == "internal_failure" for e in (error_code, error_desc))
+                and retry < 1


the condition for controlling the number of loops is pretty buried. I also think you may be able to use an existing retry helper for this?

I thought about changing this logic but am wary of diverging from the sync implementation of the error handling as we would want both to behave the same for consistency (especially if a user does use both async and sync clients). I would like to further understand the reasoning for using an existing retry helper so we could clarify this in our next call.

crwilcox · 2020-07-30T05:37:07Z

tests_async/test__oauth2client.py

+import sys
+
+import mock
+import oauth2client.client


This doesn't appear to be testing an async module, at least according to the naming. Is this a direct copy of what is in tests?

These do test the async _oauth2client internal to the auth library implementation. The other library that is imported is from https://github.com/googleapis/oauth2client, and the tests in this file test conversion between credentials from the oauth2client to the types of async credentials that we define in the auth library and that are added to this PR. Therefore, though these tests parallel those of the sync oauth2client, they use the async version and are as a result not a direct copy.

crwilcox · 2020-07-30T05:38:30Z

tests_async/test_credentials.py

@@ -1,4 +1,4 @@
-# Copyright 2016 Google LLC
+# Copyright 2020 Google LLC


similar, is this a test that is unique to async? If not we shouldn't duplicate it.

These tests are unique to async, I changed the names of the test files so that this is clear. The import of credentials_async is renamed to credentials to make writing the tests shorter but we need to test the new async constructions.

crwilcox · 2020-07-30T16:33:11Z

google/auth/_default_async.py

-
-    if credentials.client_id == _cloud_sdk.CLOUD_SDK_CLIENT_ID:
-        warnings.warn(_default._CLOUD_SDK_CREDENTIALS_WARNING)
+    return _default._warn_about_problematic_credentials(credentials)


99% sure you can just import _warn_about_problematic credentials and not wrap it here, further reducing duplication.

Makes sense, made the modification to directly import and remove this method.

busunkim96 · 2020-07-30T16:46:10Z

google/auth/_oauth2client_async.py

+"""Helpers for transitioning from oauth2client to google-auth.
+
+.. warning::
+    This module is private as it is intended to assist first-party downstream
+    clients with the transition from oauth2client to google-auth.
+"""


Hmm I'd kind of be alright with us not having this in the async version, given that oauth2client has been deprecated for a few years now. @crwilcox @anibadde thoughts?

Interesting, wasn't aware that oauth2client had been depricated - As long as this isn't used in any of the other internal libraries like resumable media it may make sense to remove it if it is not going to be used.

tritone

Generally looks good aside from issues already raised by @crwilcox

tritone · 2020-07-30T18:39:16Z

noxfile.py

@@ -1,4 +1,4 @@
-# Copyright 2019 Google LLC
+# Copyright 2020 Google LLC


This should be reverted to 2019 since it's not a new file.

anibadde added 2 commits July 27, 2020 20:53

feat: asyncio http request logic and asynchronous credentials log c

ef6d872

feat: all asynchronous credentials types implemented and with tests

0a63b3c

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Jul 28, 2020

tritone requested review from busunkim96, crwilcox, andrewsg and frankyn July 28, 2020 15:07

frankyn changed the base branch from master to async July 28, 2020 16:52

anibadde added 6 commits July 28, 2020 12:39

feat: added the private scope for Response class

06606ff

feat: added docstring for Auth Session request method

cd57fad

fix: Changed initialization of client session to within an async cont…

a38e333

…ext manager

changed aiohttp_requests abbreviation for the async authorized sessio…

57d6d10

…n class

Merge branch 'auth1' into auth2

1db84f2

fix: changed abbrevation of the aiohttp_requests file

cc14082

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jul 28, 2020

anibadde added 7 commits July 28, 2020 15:16

fix: comments on PR regarding shared data between requests and aiohtt…

aa6ece2

…p_requests

fix: fixed noxfile test dependency sharing

d88839b

fix: Made fixes based on comments on first PR

dfc6251

fix: fixed the noxfile dependencies between sync and async unit tests

c62dd1a

fix: cover async dependency

7080d14

Merge branch 'auth1' into auth2

8122dbb

Merge branch 'async' into auth2

77ca4c7

crwilcox reviewed Jul 28, 2020

View reviewed changes

google/auth/_default_async.py Outdated Show resolved Hide resolved

anibadde added 4 commits July 28, 2020 18:20

fix: merge conflict issue with credentials

b4c306f

fix: merge conflict googleapis#2

9ddb911

fix: changed duplicated constants for sync-->async inheritance relati…

8f2d0ef

…onship

fix: async docstring

9ec8277

crwilcox reviewed Jul 29, 2020

View reviewed changes

google/auth/_default_async.py Show resolved Hide resolved

anibadde added 2 commits July 29, 2020 16:46

refactoring

8f254de

fix: refactoring

aa04dc9

crwilcox suggested changes Jul 30, 2020

View reviewed changes

fix: first round of comments, refactoring and test duplication changes

0c4c3b6

crwilcox reviewed Jul 30, 2020

View reviewed changes

fix: removed duplication in _default_async

92175f2

busunkim96 reviewed Jul 30, 2020

View reviewed changes

tritone reviewed Jul 30, 2020

View reviewed changes

tseaver mentioned this pull request Jul 30, 2020

feat: asyncio support and aiohttp transport #465

Closed

fix: removed oauth2 client

77d7b6e

crwilcox approved these changes Jul 31, 2020

View reviewed changes

crwilcox merged commit b822e48 into googleapis:async Jul 31, 2020

crwilcox mentioned this pull request Sep 25, 2020

feat: add asyncio based auth flow #612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: all asynchronous credential types required for all authentication types (service accounts, default... etc) #573

feat: all asynchronous credential types required for all authentication types (service accounts, default... etc) #573

anibadde commented Jul 28, 2020 •

edited

google-cla bot commented Jul 28, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

anibadde Jul 30, 2020

busunkim96 Jul 30, 2020

anibadde Jul 30, 2020

crwilcox Jul 30, 2020

tritone left a comment

tritone Jul 30, 2020

anibadde Jul 30, 2020

		from google.auth import exceptions


		def _warn_about_problematic_credentials(credentials):

		from google.auth import jwt


		def encode(signer, payload, header=None, key_id=None):

		@@ -1,4 +1,4 @@
		# Copyright 2016 Google LLC
		# Copyright 2020 Google LLC

		@@ -1,4 +1,4 @@
		# Copyright 2019 Google LLC
		# Copyright 2020 Google LLC

feat: all asynchronous credential types required for all authentication types (service accounts, default... etc) #573

feat: all asynchronous credential types required for all authentication types (service accounts, default... etc) #573

Conversation

anibadde commented Jul 28, 2020 • edited

google-cla bot commented Jul 28, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tritone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anibadde commented Jul 28, 2020 •

edited