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

Add API to link/unlink provider info to/from user account. by nrsim · Pull Request #383 · firebase/firebase-admin-python · GitHub

/ firebase-admin-python Public

Add API to link/unlink provider info to/from user account. #383

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

Open
nrsim wants to merge 1 commit into
base: main
Choose a base branch
Could not load branches
Branch not found: {{ refName }}

Are you sure you want to change the base?

Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
from
Conversation 9 Commits 1 Checks 0 Files changed
Open

Add API to link/unlink provider info to/from user account. #383

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  (5) 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
10 changes: 10 additions & 0 deletions firebase_admin/_auth_utils.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 @@ -102,6 +102,16 @@ def validate_provider_id(provider_id, required=True):
'string.'.format(provider_id))
return provider_id

def validate_provider_ids(provider_ids, required=False):
if provider_ids is None:
if required:
raise ValueError('Invalid provider IDs. The list must be non-empty.')
else:
return None
for provider_id in provider_ids:
validate_provider_id(provider_id, True)
return provider_ids

def validate_photo_url(photo_url, required=False):
if photo_url is None and not required:
return None
Expand Down
10 changes: 8 additions & 2 deletions firebase_admin/_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 @@ -532,7 +532,8 @@ def create_user(self, uid=None, display_name=None, email=None, phone_number=None

def update_user(self, uid, display_name=None, email=None, phone_number=None,
photo_url=None, password=None, disabled=None, email_verified=None,
valid_since=None, custom_claims=None):
valid_since=None, custom_claims=None, link_provider=None,

rsgowman Jan 15, 2020

Copy link
Copy Markdown
Member

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 names have changed since starting this (and may again; feel free to defer fixing this until the name's are finalized). But as of now:

  • s/link_provider/provider_to_link/
  • s/delete_provider_ids/providers_to_delete/
delete_provider_ids=None):
"""Updates an existing user account with the specified properties"""
payload = {
'localId': _auth_utils.validate_uid(uid, required=True),
Expand All @@ -541,6 +542,8 @@ def update_user(self, uid, display_name=None, email=None, phone_number=None,
'validSince': _auth_utils.validate_timestamp(valid_since, 'valid_since'),
'emailVerified': bool(email_verified) if email_verified is not None else None,
'disableUser': bool(disabled) if disabled is not None else None,
'linkProviderUserInfo': link_provider.to_dict() if link_provider is not None else None,

rsgowman Jan 15, 2020

Copy link
Copy Markdown
Member

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

May be worth validating this parameter? _auth_utils.validate_user_provider() or similar?
'deleteProvider': _auth_utils.validate_provider_ids(delete_provider_ids, required=False),
}

remove = []
Expand All @@ -559,7 +562,10 @@ def update_user(self, uid, display_name=None, email=None, phone_number=None,

if phone_number is not None:
if phone_number is DELETE_ATTRIBUTE:
payload['deleteProvider'] = ['phone']
if payload['deleteProvider'] is None:
payload['deleteProvider'] = ['phone']
else:
payload['deleteProvider'].append('phone')
else:
payload['phoneNumber'] = _auth_utils.validate_phone(phone_number)

Expand Down
2 changes: 2 additions & 0 deletions firebase_admin/auth.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 @@ -388,6 +388,8 @@ def update_user(uid, **kwargs):
user account (optional). To remove all custom claims, pass ``auth.DELETE_ATTRIBUTE``.
valid_since: An integer signifying the seconds since the epoch. This field is set by
``revoke_refresh_tokens`` and it is discouraged to set this field directly.
link_provider: User's provider info to be linked to the user account.

rsgowman Jan 15, 2020

Copy link
Copy Markdown
Member

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

(names)

rsgowman Jan 15, 2020

Copy link
Copy Markdown
Member

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

We should indicate the type here. It's especially important in python, since (absent type annotations) it can be non-trivial to figure that out for the user. In this case, maybe "A UserProvider instance that contains the user's provider info to be linked to the user account." ?
delete_provider_ids: A list of IDs of providers to be unlinked from the user account.

Returns:
UserRecord: An updated UserRecord instance for the user.
Expand Down
30 changes: 30 additions & 0 deletions integration/test_auth.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 @@ -288,6 +288,36 @@ def test_update_user(new_user):
assert user.custom_claims is None
assert len(user.provider_data) == 2

user = auth.update_user(
new_user.uid,
link_provider=auth.UserProvider(
uid='test', provider_id='google.com', email='test@example.com',

rsgowman Jan 15, 2020

Copy link
Copy Markdown
Member

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

Caution: I think that a single provider_uid can only be linked to a single account. So by linking provideruid='test' to this account, unless the account is deleted, the second (and all subsequent) time you run this test, it'll fail. That's mostly ok here though, since the way the new user is being setup, it'll automatically delete itself, even if the test fails. But you could imagine the CI system running kill -9 (or hard power-off, or ...) right in the middle of this test. Highly unlikely... but possible.

A few options:
a) Don't worry about it; it's pretty improbable, and we can fix by hand if we run into it.
b) Instead of using a hardcoded uid ("test") create a random one. (Which makes it another order of magnitude less likely to bite us, even if it doesn't quite eliminate the possibility altogether.)
c) At the start of the test, find the user with provider_id="google.com" and provider_uid="test". Either delete or unlink that user if it exists. (But still delete the user at the end of the test too.)

I'd prefer either b or c.
display_name='Test Name', photo_url='https://test.com/user.png'))
assert user.uid == new_user.uid
assert len(user.provider_data) == 3

rsgowman Jan 15, 2020

Copy link
Copy Markdown
Member

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

This is no worse than what was there before. But consider validating the contents of this list instead. Validating the entire instance might be overkill, but how about:

assert sorted(map(lambda user_info: user_info.provider_id, user.provider_data)) == sorted(["email", "phone", "google.com"])

(untested, and probably too terse. A helper would likely make it more readable.)

user = auth.update_user(
new_user.uid,
phone_number=auth.DELETE_ATTRIBUTE,
delete_provider_ids=['google.com'])
assert user.uid == new_user.uid
assert user.phone_number is None
assert len(user.provider_data) == 1

rsgowman Jan 15, 2020

Copy link
Copy Markdown
Member

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 you make a change above, do so here (and below) as well.

user = auth.update_user(
new_user.uid,
phone_number=phone)
assert user.uid == new_user.uid
assert user.phone_number == phone
assert len(user.provider_data) == 2

user = auth.update_user(
new_user.uid,
delete_provider_ids=['phone', 'google.com'])
assert user.uid == new_user.uid
assert user.phone_number is None
assert len(user.provider_data) == 1

rsgowman Jan 16, 2020

Copy link
Copy Markdown
Member

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

A new requirement is that this API should also work with non-federated providers. (It's not something we're going to actually recommend, since there's more direct ways to do that.) Unfortunately, the backend for this API doesn't support non-federated providers, so we're going to have to fake that a bit.

Concretely, you've used update_user() to link to the 'google.com' provider. Could you also add calls for phone, email, oauth and saml? (Not necessarily in this exact test.) It'll require you to also change the implementation.

If this looks like it'll cause this PR to balloon in size, then let's talk first and see if we can come up with other options.
def test_set_custom_user_claims(new_user, api_key):
claims = {'admin' : True, 'package' : 'gold'}
auth.set_custom_user_claims(new_user.uid, claims)
Expand Down
46 changes: 46 additions & 0 deletions 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 @@ -488,6 +488,52 @@ def test_update_user_delete_fields(self, user_mgt_app):
'deleteProvider' : ['phone'],
}

def test_update_user_delete_providers(self, user_mgt_app):
user_mgt, recorder = _instrument_user_manager(user_mgt_app, 200, '{"localId":"testuser"}')
user_mgt.update_user(
'testuser',
delete_provider_ids=['google.com', 'facebook.com'])
request = json.loads(recorder[0].body.decode())
assert request == {
'localId' : 'testuser',
'deleteProvider' : ['google.com', 'facebook.com'],
}

def test_update_user_delete_fields_and_providers(self, user_mgt_app):
user_mgt, recorder = _instrument_user_manager(user_mgt_app, 200, '{"localId":"testuser"}')
user_mgt.update_user(
'testuser',
display_name=auth.DELETE_ATTRIBUTE,
photo_url=auth.DELETE_ATTRIBUTE,
phone_number=auth.DELETE_ATTRIBUTE,
delete_provider_ids=['google.com', 'facebook.com'])
request = json.loads(recorder[0].body.decode())
print request
assert request == {
'localId' : 'testuser',
'deleteAttribute' : ['DISPLAY_NAME', 'PHOTO_URL'],
'deleteProvider' : ['google.com', 'facebook.com', 'phone'],
}

def test_update_user_link_provider(self, user_mgt_app):
user_mgt, recorder = _instrument_user_manager(user_mgt_app, 200, '{"localId":"testuser"}')
user_mgt.update_user(
'testuser',
link_provider=auth.UserProvider(
uid='test', provider_id='google.com', email='test@example.com',
display_name='Test Name', photo_url='https://test.com/user.png'))
request = json.loads(recorder[0].body.decode())
assert request == {
'localId' : 'testuser',
'linkProviderUserInfo' : {
'rawId': 'test',
'providerId': 'google.com',
'email': 'test@example.com',
'displayName': 'Test Name',
'photoUrl': 'https://test.com/user.png'
}
}

rsgowman Jan 16, 2020

Copy link
Copy Markdown
Member

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

Consider adding "failure" tests.
a) empty delete_providers_ids list
b) invalid provider_id. (I'm not sure we can tell if a provider_id is valid or not... other than checking for None or empty string.)
c) invalid provider_uid. (Same; just None/empty checks.)

These should all cause failures before hitting the network, so are easy to check from a unit test.
def test_update_user_error(self, user_mgt_app):
_instrument_user_manager(user_mgt_app, 500, '{"error": {"message": "UNEXPECTED_CODE"}}')
with pytest.raises(exceptions.InternalError) as excinfo:
Expand Down
Back | FazBrowse Home | New Git URL