Skip to content
This repository was archived by the owner on Mar 6, 2026. It is now read-only.

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

Merged
busunkim96 merged 6 commits into from
Dec 11, 2020
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6d9efce
feat: Adding support for url-based credential files
Nov 18, 2020
96d61db
Making changes requested by Bassam
Dec 2, 2020
333899b
More changes requested by bojeil-google
Dec 4, 2020
12feca3
Adding tests that include headers
Dec 5, 2020
a76c698
changes requested by @busunkim96
Dec 11, 2020
b1eb9ea
Making changes suggested by @bojeil-google to pass Kokoro Run
Dec 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 37 additions & 11 deletions google/auth/identity_pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
from google.auth import _helpers
from google.auth import exceptions
from google.auth import external_account
from six.moves import http_client
from six.moves import urllib

@bojeil-google bojeil-google Nov 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



class Credentials(external_account.Credentials):
Expand All @@ -52,6 +54,7 @@ def __init__(
client_secret=None,
quota_project_id=None,
scopes=None,
success_codes=(http_client.OK,),

@bojeil-google bojeil-google Nov 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

):
"""Instantiates a file-sourced external account credentials object.

Expand Down Expand Up @@ -91,9 +94,14 @@ def __init__(
quota_project_id=quota_project_id,
scopes=scopes,
)
if isinstance(credential_source, dict):
if not isinstance(credential_source, dict):
Comment thread
ScruffyProdigy marked this conversation as resolved.
Outdated
self._credential_source_file = None
self._credential_source_url = None
else:
self._credential_source_file = credential_source.get("file")
self._credential_source_url = credential_source.get("url")
self._credential_source_headers = credential_source.get("headers")
self._success_codes = success_codes
credential_source_format = credential_source.get("format") or {}
Comment thread
ScruffyProdigy marked this conversation as resolved.
Outdated
# Get credential_source format type. When not provided, this
# defaults to text.
Expand All @@ -117,28 +125,46 @@ def __init__(
)
else:
self._credential_source_field_name = None
else:
self._credential_source_file = None
if not self._credential_source_file:
raise ValueError("Missing credential_source file")

Comment thread
ScruffyProdigy marked this conversation as resolved.
if self._credential_source_file and self._credential_source_url:
raise ValueError("Ambiguous credential_source")
if not self._credential_source_file and not self._credential_source_url:
raise ValueError("Missing credential_source")
Comment thread
ScruffyProdigy marked this conversation as resolved.
Outdated

@_helpers.copy_docstring(external_account.Credentials)
def retrieve_subject_token(self, request):
return self._get_token_file(
self._credential_source_file,
return self._parse_token_data(
self._get_token_data(),
self._credential_source_format_type,
self._credential_source_field_name,
)

def _get_token_file(
self, filename, format_type="text", subject_token_field_name=None
):
def _get_token_data(self):
if self._credential_source_file:
return self._get_file_data(self._credential_source_file)
if self._credential_source_url:
return self._get_url_data(self._credential_source_url)

def _get_file_data(self, filename):
if not os.path.exists(filename):
raise exceptions.RefreshError("File '{}' was not found.".format(filename))

with io.open(filename, "r", encoding="utf-8") as file_obj:
content = file_obj.read()
return file_obj.read(), filename

def _get_url_data(self, url):
response = urllib.request.urlopen(url)

@bojeil-google bojeil-google Nov 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bojeil-google bojeil-google Nov 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if response.status not in self._success_codes:
raise exceptions.RefreshError("Url '{}' was not found.".format(url))
return response.read(), url

def _parse_token_data(
self,
token_content,
format_type="text",
subject_token_field_name=None
):
content, filename = token_content
if format_type == "text":
token = content
else:
Expand Down
51 changes: 47 additions & 4 deletions tests/test_identity_pool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
TEXT_FILE_SUBJECT_TOKEN = fh.read()

with open(SUBJECT_TOKEN_JSON_FILE) as fh:
content = json.load(fh)
JSON_FILE_SUBJECT_TOKEN = content.get(SUBJECT_TOKEN_FIELD_NAME)
JSON_FILE_CONTENT = json.load(fh)
JSON_FILE_SUBJECT_TOKEN = JSON_FILE_CONTENT.get(SUBJECT_TOKEN_FIELD_NAME)

TOKEN_URL = "https://sts.googleapis.com/v1/token"
SUBJECT_TOKEN_TYPE = "urn:ietf:params:oauth:token-type:jwt"
Expand All @@ -69,6 +69,16 @@ class TestCredentials(object):
"scope": " ".join(SCOPES),
}

class FakeResponse:
def __init__(self, data, status=http_client.OK):
self.status = status
self.data = data
if isinstance(data, dict):
self.data = json.dumps(data)

def read(self):
return self.data

@classmethod
def make_mock_request(
cls,
Expand Down Expand Up @@ -352,13 +362,13 @@ def test_constructor_invalid_options(self):
with pytest.raises(ValueError) as excinfo:
self.make_credentials(credential_source=credential_source)

assert excinfo.match(r"Missing credential_source file")
assert excinfo.match(r"Missing credential_source")

def test_constructor_invalid_credential_source(self):
with pytest.raises(ValueError) as excinfo:
self.make_credentials(credential_source="non-dict")

assert excinfo.match(r"Missing credential_source file")
assert excinfo.match(r"Missing credential_source")

def test_constructor_invalid_credential_source_format_type(self):
credential_source = {"format": {"type": "xml"}}
Expand Down Expand Up @@ -551,3 +561,36 @@ def test_refresh_with_retrieve_subject_token_error(self):
SUBJECT_TOKEN_JSON_FILE, "not_found"
)
)

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

@bojeil-google bojeil-google Nov 23, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

TEXT_FILE_SUBJECT_TOKEN))
def test_retrieve_subject_token_from_url(self, mock_urlopen):
credential_source = {
"url": "http://fakeurl.com",
}
credentials = self.make_credentials(credential_source=credential_source)
subject_token = credentials.retrieve_subject_token(None)

assert subject_token == TEXT_FILE_SUBJECT_TOKEN

@mock.patch.object(urllib.request, "urlopen", return_value=FakeResponse(
JSON_FILE_CONTENT))
def test_retrieve_subject_token_from_url_json(self, mock_urlopen):
credential_source = {
"url": "http://fakeurl.com",
"format": {"type": "json", "subject_token_field_name": "access_token"},
}
credentials = self.make_credentials(credential_source=credential_source)
subject_token = credentials.retrieve_subject_token(None)

assert subject_token == JSON_FILE_SUBJECT_TOKEN

@bojeil-google bojeil-google Dec 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@mock.patch.object(urllib.request, "urlopen", return_value=FakeResponse(
TEXT_FILE_SUBJECT_TOKEN, status=http_client.NOT_FOUND))
def test_retrieve_subject_token_from_url_not_found(self, mock_urlopen):
credential_source = {
"url": "http://fakeurl.com",
}
credentials = self.make_credentials(credential_source=credential_source)
with pytest.raises(exceptions.RefreshError) as excinfo:

@bojeil-google bojeil-google Dec 4, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you assert the error message too?

credentials.retrieve_subject_token(None)