-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add cred info to auth related errors #2115
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ from collections import OrderedDict | |
{% if service.any_extended_operations_methods %} | ||
import functools | ||
{% endif %} | ||
from http import HTTPStatus | ||
import json | ||
import os | ||
import re | ||
from typing import Dict, Callable, Mapping, MutableMapping, MutableSequence, Optional, {% if service.any_server_streaming %}Iterable, {% endif %}{% if service.any_client_streaming %}Iterator, {% endif %}Sequence, Tuple, Type, Union, cast | ||
|
@@ -417,6 +419,26 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): | |
{{ service.client_name }}._compare_universes(self.universe_domain, self.transport._credentials)) | ||
return self._is_universe_domain_valid | ||
|
||
def _add_cred_info_for_auth_errors( | ||
self, | ||
error: core_exceptions.GoogleAPICallError | ||
) -> None: | ||
"""Adds credential info string to error details for 401/403/404 errors. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this part of a client library AIP? https://google.aip.dev/client-libraries. If not, should we consider creating one? The AIPs are meant to help define requirements for client libraries which will also be used when we create client libraries for a new language like rust. |
||
|
||
Args: | ||
error (google.api_core.exceptions.GoogleAPICallError): The error to add the cred info. | ||
""" | ||
if error.code not in [HTTPStatus.UNAUTHORIZED, HTTPStatus.FORBIDDEN, HTTPStatus.NOT_FOUND]: | ||
return | ||
|
||
cred = self._transport._credentials | ||
if not hasattr(cred, "get_cred_info"): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we emit a warning/ update the error details to let users know that they can get more helpful error messages if they upgrade to a certain version of google-auth? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that makes sense There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually on second thought, we cannot do that since its not necessary that all credentials in all scenarios have cred info. |
||
return | ||
|
||
cred_info = cred.get_cred_info() # type: ignore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please could you add a comment to clarify the reason that we have |
||
if cred_info and hasattr(error._details, "append"): | ||
error._details.append(json.dumps(cred_info)) | ||
|
||
@property | ||
def api_endpoint(self): | ||
"""Return the API endpoint used by the client instance. | ||
|
@@ -706,12 +728,16 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): | |
# Validate the universe domain. | ||
self._validate_universe_domain() | ||
|
||
# Send the request. | ||
response = rpc( | ||
request, retry=retry, timeout=timeout, metadata=metadata,) | ||
try: | ||
# Send the request. | ||
response = rpc( | ||
request, retry=retry, timeout=timeout, metadata=metadata,) | ||
|
||
# Done; return the response. | ||
return response | ||
# Done; return the response. | ||
return response | ||
except core_exceptions.GoogleAPICallError as e: | ||
self._add_cred_info_for_auth_errors(e) | ||
raise e | ||
|
||
def get_iam_policy( | ||
self, | ||
|
@@ -827,12 +853,16 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): | |
# Validate the universe domain. | ||
self._validate_universe_domain() | ||
|
||
# Send the request. | ||
response = rpc( | ||
request, retry=retry, timeout=timeout, metadata=metadata,) | ||
try: | ||
# Send the request. | ||
response = rpc( | ||
request, retry=retry, timeout=timeout, metadata=metadata,) | ||
|
||
# Done; return the response. | ||
return response | ||
# Done; return the response. | ||
return response | ||
except core_exceptions.GoogleAPICallError as e: | ||
self._add_cred_info_for_auth_errors(e) | ||
raise e | ||
|
||
def test_iam_permissions( | ||
self, | ||
|
@@ -886,12 +916,16 @@ class {{ service.client_name }}(metaclass={{ service.client_name }}Meta): | |
# Validate the universe domain. | ||
self._validate_universe_domain() | ||
|
||
# Send the request. | ||
response = rpc( | ||
request, retry=retry, timeout=timeout, metadata=metadata,) | ||
try: | ||
# Send the request. | ||
response = rpc( | ||
request, retry=retry, timeout=timeout, metadata=metadata,) | ||
|
||
# Done; return the response. | ||
return response | ||
# Done; return the response. | ||
return response | ||
except core_exceptions.GoogleAPICallError as e: | ||
self._add_cred_info_for_auth_errors(e) | ||
raise e | ||
{% endif %} | ||
|
||
DEFAULT_CLIENT_INFO = gapic_v1.client_info.ClientInfo(gapic_version=package_version.__version__) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,8 +20,8 @@ from grpc.experimental import aio | |
{% if "rest" in opts.transport %} | ||
from collections.abc import Iterable | ||
from google.protobuf import json_format | ||
import json | ||
{% endif %} | ||
import json | ||
import math | ||
import pytest | ||
from google.api_core import api_core_version | ||
|
@@ -89,6 +89,13 @@ from google.iam.v1 import policy_pb2 # type: ignore | |
{% endfilter %} | ||
{{ shared_macros.add_google_api_core_version_header_import(service.version) }} | ||
|
||
CRED_INFO_JSON = { | ||
"credential_source": "/path/to/file", | ||
"credential_type": "service account credentials", | ||
"principal": "[email protected]", | ||
} | ||
CRED_INFO_STRING = json.dumps(CRED_INFO_JSON) | ||
|
||
def client_cert_source_callback(): | ||
return b"cert bytes", b"key bytes" | ||
|
||
|
@@ -250,6 +257,44 @@ def test__get_universe_domain(): | |
{{ service.client_name }}._get_universe_domain("", None) | ||
assert str(excinfo.value) == "Universe Domain cannot be an empty string." | ||
|
||
@pytest.mark.parametrize("error_code,cred_info_json,show_cred_info", [ | ||
(401, CRED_INFO_JSON, True), | ||
(403, CRED_INFO_JSON, True), | ||
(404, CRED_INFO_JSON, True), | ||
(500, CRED_INFO_JSON, False), | ||
(401, None, False), | ||
(403, None, False), | ||
(404, None, False), | ||
(500, None, False) | ||
]) | ||
def test__add_cred_info_for_auth_errors(error_code, cred_info_json, show_cred_info): | ||
cred = mock.Mock(["get_cred_info"]) | ||
cred.get_cred_info = mock.Mock(return_value=cred_info_json) | ||
client = {{ service.client_name }}(credentials=cred) | ||
client._transport._credentials = cred | ||
|
||
error = core_exceptions.GoogleAPICallError("message", details=[]) | ||
error.code = error_code | ||
|
||
client._add_cred_info_for_auth_errors(error) | ||
if show_cred_info: | ||
assert error.details == [CRED_INFO_STRING] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also add a test to ensure that error details that we receive from the API are not clobbered? As an example, we could update the |
||
else: | ||
assert error.details == [] | ||
|
||
@pytest.mark.parametrize("error_code", [401,403,404,500]) | ||
def test__add_cred_info_for_auth_errors_no_get_cred_info(error_code): | ||
cred = mock.Mock([]) | ||
assert not hasattr(cred, "get_cred_info") | ||
client = {{ service.client_name }}(credentials=cred) | ||
client._transport._credentials = cred | ||
|
||
error = core_exceptions.GoogleAPICallError("message", details=[]) | ||
error.code = error_code | ||
|
||
client._add_cred_info_for_auth_errors(error) | ||
assert error.details == [] | ||
|
||
@pytest.mark.parametrize("client_class,transport_class,transport_name", [ | ||
{% if 'grpc' in opts.transport %} | ||
({{ service.client_name }}, transports.{{ service.grpc_transport_name }}, "grpc"), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking comment. This PR touches a lot of duplicate code. Can we refactor this into a macro to avoid making the change in so many places? Feel free to file a bug to follow up on it later. If we address it now, then subsequent updates to the same code will be less toilsome.