feat: adding support for url-based credential files #645

ScruffyProdigy · 2020-11-19T00:26:08Z

No description provided.

bojeil-google

Thanks for making the changes.

Summary of feedback:

headers need to be honored.
urllib not needed. request can be used instead and is provided in the retrieve_subject_token()
More test coverage.

bojeil-google · 2020-11-23T05:47:45Z

google/auth/identity_pool.py

        client_secret=None,
        quota_project_id=None,
        scopes=None,
+        success_codes=(http_client.OK,),


Why do you need this? This can be hardcoded below. Since this is publicly available, we don't want to expose it.

bojeil-google · 2020-11-23T05:50:54Z

google/auth/identity_pool.py

+            return file_obj.read(), filename
+
+    def _get_url_data(self, url):
+        response = urllib.request.urlopen(url)


You need to also honor the headers field when sending the request.

bojeil-google · 2020-11-23T05:54:34Z

google/auth/identity_pool.py

+            return file_obj.read(), filename
+
+    def _get_url_data(self, url):
+        response = urllib.request.urlopen(url)


You don't need to use urllib. You already have an instance of request in retrieve_subject_token(). Pass it through.

def _get_token_url(self, request, url, headers): response = request( url=url, method="GET", headers=headers ) # support both string and bytes type response.data response_body = ( response.data.decode("utf-8") if hasattr(response.data, "decode") else response.data ) if response.status != 200: raise exceptions.RefreshError( "Unable to retrieve Identity Pool subject token", response_body ) return response_body

bojeil-google · 2020-11-23T05:55:14Z

google/auth/identity_pool.py

 from google.auth import exceptions
 from google.auth import external_account
+from six.moves import http_client
+from six.moves import urllib


Check below. You don't need urllib or http_client.

bojeil-google · 2020-11-23T06:07:19Z

tests/test_identity_pool.py

            )
        )
+
+    @mock.patch.object(urllib.request, "urlopen", return_value=FakeResponse(


We need more tests. We will need to cover the following test cases (some you already covered):

test_from_info_full_options but with url sourced credential_source

test_from_file_full_options but with url sourced credential_source

test_retrieve_subject_token but from url with raw text format

test_retrieve_subject_token but from url with json format

test_retrieve_subject_token but from url with invalid field name

test_retrieve_subject_token but from url with invalid json response

test_retrieve_subject_token but with url error (non-200 response)

test_refresh with url-sourced cred without service account impersonation

test_refresh with url-sourced cred with service account impersonation

test_refresh with url-sourced cred error such as non-200 response.

bojeil-google

Forgot to mention, the file description and public function comments need to be updated to cover url-source credentials.

bojeil-google

Thanks for making the changes! This is looking really good. Just a few nits and minor change requests. Should be easy to resolve.

bojeil-google · 2020-12-04T01:30:25Z

google/auth/identity_pool.py

 ID tokens) retrieved from a file location, typical for K8s workloads
-registered with Hub with Hub workload identity enabled.
+registered with Hub with Hub workload identity enabled, or retrieved from an
+url, typical for AWS and Azure based workflows.


Since we have a dedicated credentials class for AWS, let's exclude AWS:

url, typical for Azure based workflows.

bojeil-google · 2020-12-04T02:34:26Z

tests/test_identity_pool.py

+        subject_token = credentials.retrieve_subject_token(
+            self.make_mock_request(token_data=JSON_FILE_CONTENT))
+
+        assert subject_token == JSON_FILE_SUBJECT_TOKEN


Can you also assert the request was sent with the expected parameters? eg. the expected URL, GET method and headers and no body? We don't have any coverage for this at the moment.

bojeil-google · 2020-12-04T02:36:17Z

tests/test_identity_pool.py

+        with pytest.raises(exceptions.RefreshError) as excinfo:
+            credentials.retrieve_subject_token(
+                self.make_mock_request(
+                    token_status=500,


nit: since this is a not found error, maybe we can use 404?

bojeil-google · 2020-12-04T02:36:55Z

tests/test_identity_pool.py

+            "url": "http://fakeurl.com",
+        }
+        credentials = self.make_credentials(credential_source=credential_source)
+        with pytest.raises(exceptions.RefreshError) as excinfo:


Can you assert the error message too?

bojeil-google · 2020-12-04T21:40:41Z

tests/test_identity_pool.py

+    ):
+        assert request_kwargs["url"] == url
+        assert request_kwargs["method"] == "GET"
+        assert request_kwargs["headers"] is None


We still need a test with non-none headers. eg:

"credential_source": { "url": "http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource=...", "headers": { "Metadata": "True" }, "format": { "type": "json", "subject_token_field_name": "access_token" } }

busunkim96

Mostly LGTM, just left a few docs-related asks.

google/auth/identity_pool.py

gcf-merge-on-green · 2020-12-11T06:22:02Z

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

busunkim96 · 2020-12-11T20:20:54Z

@ScruffyProdigy If you join the googleapis org CI will run for you automatically.

ScruffyProdigy requested a review from a team as a code owner November 19, 2020 00:26

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 19, 2020

feat: Adding support for url-based credential files

6d9efce

ScruffyProdigy force-pushed the url-support branch from 4141291 to 6d9efce Compare November 19, 2020 00:29

ScruffyProdigy changed the title ~~Adding support for url-based credential files~~ feat: Adding support for url-based credential files Nov 19, 2020

ScruffyProdigy changed the title ~~feat: Adding support for url-based credential files~~ feat: adding support for url-based credential files Nov 19, 2020

bojeil-google suggested changes Nov 23, 2020

View reviewed changes

Making changes requested by Bassam

96d61db

bojeil-google suggested changes Dec 4, 2020

View reviewed changes

More changes requested by bojeil-google

333899b

bojeil-google reviewed Dec 4, 2020

View reviewed changes

Adding tests that include headers

12feca3

bojeil-google approved these changes Dec 5, 2020

View reviewed changes

busunkim96 reviewed Dec 8, 2020

View reviewed changes

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

google/auth/identity_pool.py Show resolved Hide resolved

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

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

changes requested by @busunkim96

a76c698

busunkim96 approved these changes Dec 11, 2020

View reviewed changes

busunkim96 added automerge Merge the pull request once unit tests and other checks pass. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Dec 11, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2020

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 11, 2020

Making changes suggested by @bojeil-google to pass Kokoro Run

b1eb9ea

busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 11, 2020

busunkim96 merged commit 2bd9e83 into googleapis:byoid Dec 11, 2020

ScruffyProdigy deleted the url-support branch December 11, 2020 20:40

feat: adding support for url-based credential files #645

feat: adding support for url-based credential files #645

Uh oh!

Conversation

ScruffyProdigy commented Nov 19, 2020

Uh oh!

bojeil-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bojeil-google left a comment

Choose a reason for hiding this comment

Uh oh!

bojeil-google left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

busunkim96 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gcf-merge-on-green bot commented Dec 11, 2020

Uh oh!

busunkim96 commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants