FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(auth): Make auth client respect app options httpTimeout by daniellehanks · Pull Request #536 · firebase/firebase-admin-python · GitHub

/ firebase-admin-python Public

fix(auth): Make auth client respect app options httpTimeout #536

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.

Sign up for GitHub

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

Merged
hiranya911 merged 1 commit into from
Mar 15, 2021
Conversation 8 Commits 1 Checks 0 Files changed
Merged

fix(auth): Make auth client respect app options httpTimeout #536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
3 changes: 2 additions & 1 deletion firebase_admin/_auth_client.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,9 @@ def __init__(self, app, tenant_id=None):

credential = app.credential.get_credential()
version_header = 'Python/Admin/{0}'.format(firebase_admin.__version__)
timeout = app.options.get('httpTimeout', _http_client.DEFAULT_TIMEOUT_SECONDS)
http_client = _http_client.JsonHttpClient(
credential=credential, headers={'X-Client-Version': version_header})
credential=credential, headers={'X-Client-Version': version_header}, timeout=timeout)

self._tenant_id = tenant_id
self._token_generator = _token_gen.TokenGenerator(app, http_client)
Expand Down
27 changes: 26 additions & 1 deletion tests/test_user_mgt.py
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@

USER_MGT_URL_PREFIX = 'https://identitytoolkit.googleapis.com/v1/projects/mock-project-id'

TEST_TIMEOUT = 42


@pytest.fixture(scope='module')
def user_mgt_app():
Expand All @@ -60,6 +62,16 @@ def user_mgt_app():
yield app
firebase_admin.delete_app(app)

@pytest.fixture(scope='module')
def user_mgt_app_with_timeout():
app = firebase_admin.initialize_app(
testutils.MockCredential(),
name='userMgtTimeout',
options={'projectId': 'mock-project-id', 'httpTimeout': TEST_TIMEOUT}
)
yield app
firebase_admin.delete_app(app)

def _instrument_user_manager(app, status, payload):
client = auth._get_client(app)
user_manager = client._user_manager
Expand Down Expand Up @@ -105,14 +117,16 @@ def _check_user_record(user, expected_uid='testuser'):
assert provider.provider_id == 'phone'


def _check_request(recorder, want_url, want_body=None):
def _check_request(recorder, want_url, want_body=None, want_timeout=None):
assert len(recorder) == 1
req = recorder[0]
assert req.method == 'POST'
assert req.url == '{0}{1}'.format(USER_MGT_URL_PREFIX, want_url)
if want_body:
body = json.loads(req.body.decode())
assert body == want_body
if want_timeout:
assert recorder[0]._extra_kwargs['timeout'] == pytest.approx(want_timeout, 0.001)

hiranya911 Mar 15, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should we add the following assertion?

timeout_arg = recorder[0]._extra_kwargs['timeout']
if want_timeout:
    assert timeout_arg == pytest.approx(want_timeout, 0.01)
else:
    assert timeout_arg == pytest.approx(_http_client.DEFAULT_TIMEOUT, 0.01)

daniellehanks Mar 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The problem I see with that is it would force any test not using the default to pass that argument. I aligned with the other want args, if it's specified, check it, otherwise don't. That said, it's your code base so I defer to you. Lmk if you want me to change it.

hiranya911 Mar 15, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The problem I see with that is it would force any test not using the default to pass that argument.

It doesn't. It can default to None, in which case we assert against _http_client.DEFAULT_TIMEOUT.

daniellehanks Mar 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

That's what I'm saying, if someone added a test using user_mgt_app_with_timeout, but didn't pass a timeout to this function (presumably because they didn't care about asserting the value), it would fail.

hiranya911 Mar 15, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In the implementation we already default the client to _http_client.DEFAULT_TIMEOUT. In what scenario do we expect that assertion to not hold?

daniellehanks Mar 15, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If an app is used with a non-default timeout, but someone is either unaware of the want_timeout parameter or explicitly does not wish to make an assertion around it. Just biasing towards explicit > implicit per BDFL. Implementing the suggested behavior would implicitly check timeout. It also adds assertions to existing tests, whereas the original implementation results in no change to existing tests/assertions.

For a concrete example, if I removed the TEST_TIMEOUT from test_get_user_with_timeout, the test would fail, but, IMO, it would not be readily obvious just inspecting the test function why (nowhere did I make an assertion or ask an assertion to be made around timeout, so why is that happening?).

hiranya911 Mar 15, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It also adds assertions to existing tests, whereas the original implementation results in no change to existing tests/assertions.

That was my intended goal. Ideally, the _check_request() helper function should make assertions about the traits that are common to all requests made by the auth client. We probably should have had the timeout assertion there to begin with. The want_body shouldn't be an optional argument. It's always specified.

Anyway, all these changes can probably be lumped into a separate PR. For now I'm happy with where this stands.


class TestAuthServiceInitialization:
Expand All @@ -122,6 +136,11 @@ def test_default_timeout(self, user_mgt_app):
user_manager = client._user_manager
assert user_manager.http_client.timeout == _http_client.DEFAULT_TIMEOUT_SECONDS

def test_app_options_timeout(self, user_mgt_app_with_timeout):
client = auth._get_client(user_mgt_app_with_timeout)
user_manager = client._user_manager
assert user_manager.http_client.timeout == TEST_TIMEOUT

def test_fail_on_no_project_id(self):
app = firebase_admin.initialize_app(testutils.MockCredential(), name='userMgt2')
with pytest.raises(ValueError):
Expand Down Expand Up @@ -225,6 +244,12 @@ def test_get_user(self, user_mgt_app):
_check_user_record(auth.get_user('testuser', user_mgt_app))
_check_request(recorder, '/accounts:lookup', {'localId': ['testuser']})

def test_get_user_with_timeout(self, user_mgt_app_with_timeout):
_, recorder = _instrument_user_manager(
user_mgt_app_with_timeout, 200, MOCK_GET_USER_RESPONSE)
_check_user_record(auth.get_user('testuser', user_mgt_app_with_timeout))
_check_request(recorder, '/accounts:lookup', {'localId': ['testuser']}, TEST_TIMEOUT)

@pytest.mark.parametrize('arg', INVALID_STRINGS + ['not-an-email'])
def test_invalid_get_user_by_email(self, arg, user_mgt_app):
with pytest.raises(ValueError):
Expand Down
Back | FazBrowse Home | New Git URL