Skip to content

feat(auth): Link federatedid #345

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.

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 7 commits into from
Apr 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
84 changes: 83 additions & 1 deletion src/main/java/com/google/firebase/auth/UserRecord.java
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,29 @@ public UpdateRequest setPhoneNumber(@Nullable String phone) {
if (phone != null) {
checkPhoneNumber(phone);
}

if (phone == null && properties.containsKey("deleteProvider")) {
Object deleteProvider = properties.get("deleteProvider");
if (deleteProvider != null) {
// Due to java's type erasure, we can't fully check the type. :(
@SuppressWarnings("unchecked")
Iterable<String> deleteProviderIterable = (Iterable<String>)deleteProvider;

// If we've been told to unlink the phone provider both via setting phoneNumber to null
// *and* by setting providersToUnlink to include 'phone', then we'll reject that. Though
// it might also be reasonable to relax this restriction and just unlink it.
for (String dp : deleteProviderIterable) {
if (dp == "phone") {
throw new IllegalArgumentException(
"Both UpdateRequest.setPhoneNumber(null) and "
+ "UpdateRequest.setProvidersToUnlink(['phone']) were set. To unlink from a "
+ "phone provider, only specify UpdateRequest.setPhoneNumber(null).");

}
}
}
}

properties.put("phoneNumber", phone);
return this;
}
Expand Down Expand Up @@ -548,6 +571,52 @@ public UpdateRequest setCustomClaims(Map<String,Object> customClaims) {
return this;
}

/**
* Links this user to the specified provider.
*
* <p>Linking a provider to an existing user account does not invalidate the
* refresh token of that account. In other words, the existing account
* would continue to be able to access resources, despite not having used
* the newly linked provider to log in. If you wish to force the user to
* authenticate with this new provider, you need to (a) revoke their
* refresh token (see
* https://firebase.google.com/docs/auth/admin/manage-sessions#revoke_refresh_tokens),
* and (b) ensure no other authentication methods are present on this
* account.
*
* @param providerToLink provider info to be linked to this user\'s account.
*/
public UpdateRequest setProviderToLink(@NonNull UserProvider providerToLink) {
properties.put("linkProviderUserInfo", checkNotNull(providerToLink));
return this;
}

/**
* Unlinks this user from the specified providers.
*
* @param providerIds list of identifiers for the identity providers.
*/
public UpdateRequest setProvidersToUnlink(Iterable<String> providerIds) {
checkNotNull(providerIds);
for (String id : providerIds) {
checkArgument(!Strings.isNullOrEmpty(id), "providerIds must not be null or empty");

if (id == "phone" && properties.containsKey("phoneNumber")
&& properties.get("phoneNumber") == null) {
// If we've been told to unlink the phone provider both via setting phoneNumber to null
// *and* by setting providersToUnlink to include 'phone', then we'll reject that. Though
// it might also be reasonable to relax this restriction and just unlink it.
throw new IllegalArgumentException(
"Both UpdateRequest.setPhoneNumber(null) and "
+ "UpdateRequest.setProvidersToUnlink(['phone']) were set. To unlink from a phone "
+ "provider, only specify UpdateRequest.setPhoneNumber(null).");
}
}

properties.put("deleteProvider", providerIds);
return this;
}

UpdateRequest setValidSince(long epochSeconds) {
checkValidSince(epochSeconds);
properties.put("validSince", epochSeconds);
Expand All @@ -569,7 +638,20 @@ Map<String, Object> getProperties(JsonFactory jsonFactory) {
}

if (copy.containsKey("phoneNumber") && copy.get("phoneNumber") == null) {
copy.put("deleteProvider", ImmutableList.of("phone"));
Object deleteProvider = copy.get("deleteProvider");
if (deleteProvider != null) {
// Due to java's type erasure, we can't fully check the type. :(
@SuppressWarnings("unchecked")
Iterable<String> deleteProviderIterable = (Iterable<String>)deleteProvider;

copy.put("deleteProvider", new ImmutableList.Builder<String>()
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 handle the double specification case?

user.updateRequest()
  .setPhoneNumber(null)
  .setProvidersToUnlink(ImmutableList.of("phone"))

I remember handling this case is other languages.

Copy link
Member

Choose a reason for hiding this comment

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

Done. Note that this results in the IllegalArgumentException being wrapped in an ExecutionException, which is a little awkward. (Option 1)

Option 2:
We can resolve that by pulling the property calculation out of the async portion. It's a bit more disruptive, and diverges from how get/posts are handled throughout the rest of the code base. Roughly:

FirebaseUserManager.java:
...
- void updateUser(UserRecord.UpdateRequest request JsonFactory jsonFactory) {
+ void updateUser(Map<String, Object> payload) {
...

(NB: This is not a public interface.)

Option 3:
Alternatively, we could just call request.getProperties() before the async portion and ignore the results. (i.e. call it twice.)

I've left it as option 1 for now. lmk if you'd prefer 2 or 3.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to enforce this constraint in the setters? We'd have to do so in both setPhoneNumber() and setProvidersToUnlink(), but I think we can reduce the duplication via some sort of a helper function. That would make argument validation consistent with the pattern currently used in this class.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's better; done. I've removed the logic here, i.e. if we somehow get into a bad state, we won't notice it. (But now we shouldn't get into that state.)

I didn't create a helper; there wasn't much duplication (though the exception itself is duplicated). If we set the value first and then checked, it would work out better... but that risks leaving the object in a bad state (eg if user catches and ignores the exception and then proceeds to use the object anyways.)

.addAll(deleteProviderIterable)
.add("phone")
.build());
} else {
copy.put("deleteProvider", ImmutableList.of("phone"));
}

copy.remove("phoneNumber");
}

Expand Down
60 changes: 60 additions & 0 deletions src/test/java/com/google/firebase/auth/FirebaseAuthIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -307,6 +308,65 @@ public void testUserLifecycle() throws Exception {
assertEquals(2, userRecord.getProviderData().length);
assertTrue(userRecord.getCustomClaims().isEmpty());

// Link user to IDP providers
request = userRecord.updateRequest()
.setProviderToLink(
UserProvider
.builder()
.setUid("testuid")
.setProviderId("google.com")
.setEmail("[email protected]")
.setDisplayName("Test User")
.setPhotoUrl("https://test.com/user.png")
.build());
userRecord = auth.updateUserAsync(request).get();
assertEquals(uid, userRecord.getUid());
assertEquals("Updated Name", userRecord.getDisplayName());
assertEquals(randomUser.getEmail(), userRecord.getEmail());
assertEquals(randomUser.getPhoneNumber(), userRecord.getPhoneNumber());
assertEquals("https://example.com/photo.png", userRecord.getPhotoUrl());
assertTrue(userRecord.isEmailVerified());
assertFalse(userRecord.isDisabled());
assertEquals(3, userRecord.getProviderData().length);
List<String> providers = new ArrayList<>();
for (UserInfo provider : userRecord.getProviderData()) {
providers.add(provider.getProviderId());
}
assertTrue(providers.contains("google.com"));
assertTrue(userRecord.getCustomClaims().isEmpty());

// Unlink phone provider
request = userRecord.updateRequest().setProvidersToUnlink(ImmutableList.of("phone"));
userRecord = auth.updateUserAsync(request).get();
assertNull(userRecord.getPhoneNumber());
assertEquals(2, userRecord.getProviderData().length);
providers.clear();
for (UserInfo provider : userRecord.getProviderData()) {
providers.add(provider.getProviderId());
}
assertFalse(providers.contains("phone"));
assertEquals(uid, userRecord.getUid());
assertEquals("Updated Name", userRecord.getDisplayName());
assertEquals(randomUser.getEmail(), userRecord.getEmail());
assertEquals("https://example.com/photo.png", userRecord.getPhotoUrl());
assertTrue(userRecord.isEmailVerified());
assertFalse(userRecord.isDisabled());
assertTrue(userRecord.getCustomClaims().isEmpty());

// Unlink IDP provider
request = userRecord.updateRequest().setProvidersToUnlink(ImmutableList.of("google.com"));
userRecord = auth.updateUserAsync(request).get();
assertEquals(1, userRecord.getProviderData().length);
assertNotEquals("google.com", userRecord.getProviderData()[0].getProviderId());
assertEquals(uid, userRecord.getUid());
assertEquals("Updated Name", userRecord.getDisplayName());
assertEquals(randomUser.getEmail(), userRecord.getEmail());
assertNull(userRecord.getPhoneNumber());
assertEquals("https://example.com/photo.png", userRecord.getPhotoUrl());
assertTrue(userRecord.isEmailVerified());
assertFalse(userRecord.isDisabled());
assertTrue(userRecord.getCustomClaims().isEmpty());

// Get user by email
userRecord = auth.getUserByEmailAsync(userRecord.getEmail()).get();
assertEquals(uid, userRecord.getUid());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ public class FirebaseUserManagerTest {

private static final Map<String, Object> ACTION_CODE_SETTINGS_MAP =
ACTION_CODE_SETTINGS.getProperties();
private static final UserProvider USER_PROVIDER = UserProvider.builder()
.setUid("testuid")
.setProviderId("facebook.com")
.setEmail("[email protected]")
.setDisplayName("Test User")
.setPhotoUrl("https://test.com/user.png")
.build();

private static final String PROJECT_BASE_URL =
"https://identitytoolkit.googleapis.com/v2/projects/test-project-id";
Expand Down Expand Up @@ -1091,8 +1098,10 @@ public void testUserUpdater() throws IOException {
.setEmailVerified(true)
.setPassword("secret")
.setCustomClaims(claims)
.setProviderToLink(USER_PROVIDER)
.setProvidersToUnlink(ImmutableList.of("google.com"))
.getProperties(JSON_FACTORY);
assertEquals(8, map.size());
assertEquals(10, map.size());
assertEquals(update.getUid(), map.get("localId"));
assertEquals("Display Name", map.get("displayName"));
assertEquals("http://test.com/example.png", map.get("photoUrl"));
Expand All @@ -1101,6 +1110,8 @@ public void testUserUpdater() throws IOException {
assertTrue((Boolean) map.get("emailVerified"));
assertEquals("secret", map.get("password"));
assertEquals(JSON_FACTORY.toString(claims), map.get("customAttributes"));
assertEquals(USER_PROVIDER, map.get("linkProviderUserInfo"));
assertEquals(ImmutableList.of("google.com"), map.get("deleteProvider"));
}

@Test
Expand Down Expand Up @@ -1138,6 +1149,64 @@ public void testEmptyCustomClaims() {
assertEquals("{}", map.get("customAttributes"));
}

@Test
public void testLinkProvider() {
UserRecord.UpdateRequest update = new UserRecord.UpdateRequest("test");
Map<String, Object> map = update
.setProviderToLink(USER_PROVIDER)
.getProperties(Utils.getDefaultJsonFactory());
assertEquals(2, map.size());
assertEquals(update.getUid(), map.get("localId"));
assertEquals(USER_PROVIDER, map.get("linkProviderUserInfo"));
}

@Test
public void testDeleteProvider() {
UserRecord.UpdateRequest update = new UserRecord.UpdateRequest("test");
Map<String, Object> map = update
.setProvidersToUnlink(ImmutableList.of("google.com"))
.getProperties(Utils.getDefaultJsonFactory());
assertEquals(2, map.size());
assertEquals(update.getUid(), map.get("localId"));
assertEquals(ImmutableList.of("google.com"), map.get("deleteProvider"));
}

@Test
public void testDeleteProviderAndPhone() {
UserRecord.UpdateRequest update = new UserRecord.UpdateRequest("test");
Map<String, Object> map = update
.setProvidersToUnlink(ImmutableList.of("google.com"))
.setPhoneNumber(null)
.getProperties(Utils.getDefaultJsonFactory());
assertEquals(2, map.size());
assertEquals(update.getUid(), map.get("localId"));
assertEquals(ImmutableList.of("google.com", "phone"), map.get("deleteProvider"));
}

@Test
public void testDoubleDeletePhoneProvider() throws Exception {
UserRecord.UpdateRequest update = new UserRecord.UpdateRequest("uid")
.setPhoneNumber(null);

try {
update.setProvidersToUnlink(ImmutableList.of("phone"));
fail("No error thrown for double delete phone provider");
} catch (IllegalArgumentException expected) {
}
}

@Test
public void testDoubleDeletePhoneProviderReverseOrder() throws Exception {
UserRecord.UpdateRequest update = new UserRecord.UpdateRequest("uid")
.setProvidersToUnlink(ImmutableList.of("phone"));

try {
update.setPhoneNumber(null);
fail("No error thrown for double delete phone provider");
} catch (IllegalArgumentException expected) {
}
}

@Test
public void testDeleteDisplayName() {
Map<String, Object> map = new UserRecord.UpdateRequest("test")
Expand Down