Skip to content

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
wants to merge 1 commit into
base: master
Choose a base branch
from
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions firebase_admin/_auth_utils.py
View file
Open in desktop
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
View file
Open in desktop
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,
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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
View file
Open in desktop
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.
Copy link
Member

Choose a reason for hiding this comment

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

(names)

Copy link
Member

Choose a reason for hiding this comment

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

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
View file
Open in desktop
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='[email protected]',
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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
View file
Open in desktop
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='[email protected]',
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': '[email protected]',
'displayName': 'Test Name',
'photoUrl': 'https://test.com/user.png'
}
}

Copy link
Member

Choose a reason for hiding this comment

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

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