User Management API with Phone Auth Support #49
Conversation
tests/test_auth.py
Outdated
|
|
||
| class TestUserRecord(object): | ||
|
|
||
| @pytest.mark.parametrize('data', INVALID_DICTS + [{}, {'foo':'bar'}]) |
There was a problem hiding this comment.
it might be useful to mark why the last two elements are invalid in this case.
tests/test_auth.py
Outdated
|
|
||
| def test_get_user(self, user_mgt_app): | ||
| _instrument_user_manager(user_mgt_app, 200, testutils.resource('get_user.json')) | ||
| _check_user_record(auth.get_user('testuser', user_mgt_app)) |
There was a problem hiding this comment.
It might be more readable if the values you are using here are constants. Then it becomes clearer that these should actually return results
tests/test_auth.py
Outdated
| def test_get_user_non_existing(self, user_mgt_app): | ||
| _instrument_user_manager(user_mgt_app, 200, '{"users":[]}') | ||
| with pytest.raises(auth.AuthError) as excinfo: | ||
| auth.get_user('testuser', user_mgt_app) |
There was a problem hiding this comment.
Might be better to use 'nonexistentuser'
tests/test_auth.py
Outdated
| def test_get_user_by_email_http_error(self, user_mgt_app): | ||
| _instrument_user_manager(user_mgt_app, 500, '{"error":"test"}') | ||
| with pytest.raises(auth.AuthError) as excinfo: | ||
| auth.get_user_by_email('testuser@example.com', user_mgt_app) |
|
|
||
| Args: | ||
| uid: A user ID string. | ||
| app: An App instance (optional). |
There was a problem hiding this comment.
What is the behavior when an app isn't provided? where do the default values come from?
There was a problem hiding this comment.
If an app is not explicitly provided (which is the more common use case), we use the default app returned by firebase_admin.get_app(). This is a feature common to all APIs in this library. Almost all public functions accept an optional app argument.
The equivalent logic in Java/Android SDKs is:
FirebaseAuth.getInstance() // Uses default app
FirebaseAuth.getInstance(app) // Uses the provided app
firebase_admin/auth.py
Outdated
| signal success. | ||
| """ | ||
|
|
||
| EMAIL_PATTERN = re.compile('^[^@]+@[^@]+$') |
There was a problem hiding this comment.
This seems too simplistic to be the regex pattern for emails. This is going to let a fair bit of invalid emails through. If this is all you are looking for, wouldn't it be simpler to just do:
parts = email.split('@')
if len(parts) == 2 and len(parts[0]) > 1 and len(parts[1] > 1:
# Valid email address
There was a problem hiding this comment.
Changed the validation as suggested. We only need minimal checking here. Backend does more comprehensive checking.
firebase_admin/auth.py
Outdated
| self._session = session | ||
|
|
||
| def get_user(self, uid): | ||
| """Gets the user data corresponding to the specified user ID.""" |
There was a problem hiding this comment.
all these get users are basically the same except for the error message. Does it make sense to have a shared implementation that takes in a parameter for the error messages?
firebase_admin/auth.py
Outdated
| def create_user(self, **kwargs): | ||
| """Creates a new user account with the specified properties.""" | ||
| payload = {} | ||
| for key, value in _UserManager._CREATE_USER_FIELDS.items(): |
There was a problem hiding this comment.
I wonder if the kwargs validation/copying logic can be shared between create_user and update_user.
| @@ -14,48 +14,50 @@ | |||
|
|
|||
There was a problem hiding this comment.
Can we split up the contents of this file? Maybe the validator and the user manager can go in a separate file?
There was a problem hiding this comment.
Done. Extracted to a new _user_mgt.py module. I had to change things around a little bit to break a few cyclic dependencies.
|
Made the suggested changes. |
|
Hi Firebase team, I would like to know when Firebase team will release this new version include user management APIs ? |
|
Will there be a way to lookup multiple users at once? If I have a bunch of user IDs do I need to get their emails one at a time? |
|
@grennis Not in the initial release of this API. Can you create an issue for multi-user look up? We can consider it for a future iteration (or you can help us implement it ;) ) |
|
Sure thing. Thanks and looking forward to the release of this PR. |
Adds the basic user management operations to the
authmodule:get_user()get_user_by_email()get_user_by_phone_number()create_user()update_user()delete_user()Also introduces the following new types for handling user account information:
auth.UserRecordauth.UserInfoauth.UserMetadatago/firebase-python-user-mgt