Skip to content

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
merged 1 commit into from
Mar 15, 2021
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion firebase_admin/_auth_client.py
View file
Open in desktop
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
View file
Open in desktop
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?).

Copy link
Contributor

Choose a reason for hiding this comment

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

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