feat(auth): Add ability to link a federated ID with the `updateUser()` method. #770

rsgowman · 2020-01-20T21:35:31Z

No description provided.

nrsim

LGTM overall.

nrsim · 2020-01-22T23:07:40Z

src/auth/auth-api-request.ts

@@ -265,6 +265,8 @@ function validateCreateEditRequest(request: any, uploadAccountRequest: boolean =
    phoneNumber: true,
    customAttributes: true,
    validSince: true,
+    // Pass linkProviderUserInfo only for updates (i.e. not for uploads.)
+    linkProviderUserInfo: !uploadAccountRequest,


Add deleteProvider as well?

It's already present (line 263) since it was used previously to remove phone auth entries.

nrsim · 2020-01-22T23:07:46Z

src/auth/auth-api-request.ts

@@ -410,6 +412,11 @@ function validateCreateEditRequest(request: any, uploadAccountRequest: boolean =
      validateProviderUserInfo(providerUserInfoEntry);
    });
  }
+
+  // linkProviderUserInfo must be a (single) UserInfo value.


UserProvider?

nrsim · 2020-01-22T23:07:55Z

src/auth/auth-api-request.ts

+    }
+
+    if (typeof(request.providersToDelete) !== 'undefined') {
+      if (!validator.isArray(request.deleteProvider)) {


Isn't this check redundant, considering the condition gets checked at validation?

deleteProvider might not yet exist. (Or technically, the end user could set it to anything they like if they're using javascript rather than typescript.)

This will achieve the same result without the if condition I think:

request.deleteProvider = (request.deleteProvider || []).concat(request.providersToUnlink);

The user could set this value to anything they like which could interact badly:

request.deleteProvider = "foo" request.providersToUnlink = [ "phone", "email" ] request.deleteProvider = (request.deleteProvider || []).concat(request.providersToUnlink); console.log(request.deleteProvider) > ["foophone", "email"]

Such use would be pretty questionable since deleteProvider doesn't show up in the UpdateRequest interface... though that doesn't stop the user from setting it anyways for their own purposes. (Although we modify it here, we also take a copy first, so this won't impact the user's copy.) Given that, it seems easiest to just check to see if it's an array first.

Thinking about it a bit more though, another alternative would be to just set deleteProvider to undefined (or []) after we take the copy, thus ensuring any value that the user sets can't interfere with this value.

I've left it as is for now, though could switch to the alternative if you'd prefer.

nrsim · 2020-01-22T23:08:01Z

src/auth/user-record.ts

+  /**
+   * The user identifier for the linked provider.
+   */
+  uid?: string;


providerUid?

It's just uid in the other ports. Combined with the interface name (which contains 'provider'), I think that's probably ok.

nrsim · 2020-01-22T23:08:06Z

src/index.d.ts

+    providerToLink?: UserProvider;
+
+    /**
+     * Unlinks this user from the specified federated providers.


s/federated providers/providers?

Done. (Also above)

nrsim · 2020-01-22T23:08:10Z

test/integration/auth.spec.ts

+    });
+
+    it('can link/unlink with a federated provider', async () => {
+      const federatedUid = 'google_uid_' + generateRandomString(10);


Could this be changed to googleUid/googleFederatedUid, to avoid an unnecessary confusion between federatedUid and providerUid?

nrsim · 2020-01-22T23:08:19Z

test/unit/auth/auth.spec.ts

+      it('should be rejected given an empty providersToDelete list', () => {
+        expect(() => {
+          auth.updateUser(uid, {
+            providersToDelete: [],


Could this be a no-op instead?

You're right; there's not really any reason to reject this. I've moved the test to an integration test (to ensure the backend is happy with the noop. If that ever changes, we can add code to special case this.)

nrsim · 2020-01-22T23:08:26Z

test/unit/auth/auth.spec.ts

+            });
+        });
+
+        it('specifying both phoneNumber=null and providersToDelete=phone should be rejected', () => {


nit: does this need to be reject?

I don't know. It does indicate a slight misuse of our API as there's not really any situation where the user should be deleting both by setting phoneNumber=null and also by adding phone to providersToDelete. OTOH, the user's intent is clear in this case. See also the comment in the implementation (src/auth/auth.ts:updateUser:~313)

I'm inclined to be a bit conservative and reject this situation as an error and then relax it later if necessary. But am also willing to be convinced otherwise. We'll get a few more reviews before this lands, so we can probably just go with the consensus that results from that.

I've left this alone for now.

nrsim · 2020-02-03T17:01:23Z

src/auth/auth-api-request.ts

@@ -961,6 +968,31 @@ export abstract class AbstractAuthRequestHandler {
          'Properties argument must be a non-null object.',
        ),
      );
+    } else if (validator.isNonNullObject(properties.providerToLink)) {


Aren't the validations at validateCreateEditRequest sufficient? Seems like the validation is added in two places.

(Missed this comment, sorry!)

IIUC, the parameters are slightly different, so it doesn't quite work. (eg rawId). Some refactoring could be done though... I've added a TODO.

…derated_id

google-cla · 2020-12-22T15:00:26Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

hiranya911 · 2021-01-07T20:36:55Z

Hi @rsgowman. Is this PR ready for a round of review? Looks like the build failed due to an out of date API report. You might wan to run npm run api-extractor:local and commit the changes.

…derated_id

google-cla · 2021-02-04T18:48:32Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

Add ability to link a federated id with the updateUser() method.

59d93aa

rsgowman assigned rsgowman and nrsim and unassigned rsgowman Jan 20, 2020

rsgowman requested a review from nrsim January 20, 2020 21:37

nrsim suggested changes Jan 22, 2020

View reviewed changes

nrsim assigned rsgowman and unassigned nrsim Jan 24, 2020

review feedback

c79f42d

rsgowman force-pushed the rsgowman/link_by_federated_id branch from caac5bc to c79f42d Compare January 28, 2020 15:53

rsgowman assigned nrsim and unassigned rsgowman Jan 28, 2020

nrsim approved these changes Feb 3, 2020

View reviewed changes

rsgowman mentioned this pull request Feb 5, 2020

Add the ability to link/unlink a provider user id to an account. firebase/firebase-admin-dotnet#143

Open

rsgowman and others added 4 commits March 19, 2020 15:22

more details on the docs

dfa1fce

Merge remote-tracking branch 'origin/master' into rsgowman/link_by_fe…

fe88272

…derated_id

providersToDelete -> providersToUnlink

a04e175

add todo re refactoring

3393af3

rsgowman assigned hiranya911 and unassigned nrsim Dec 22, 2020

hiranya911 assigned rsgowman and unassigned hiranya911 Jan 7, 2021

Merge remote-tracking branch 'origin/master' into rsgowman/link_by_fe…

ba5361f

…derated_id

api-extractor

a3f8f69

rsgowman force-pushed the rsgowman/link_by_federated_id branch from 7f077e8 to a3f8f69 Compare February 4, 2021 18:49

davidhin mentioned this pull request Jul 6, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#682

Open

This was referenced Jul 17, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#685

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#687

Open

This was referenced Aug 10, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#697

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#700

Open

This was referenced Aug 28, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#707

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#709

Open

This was referenced Sep 5, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#711

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#712

Open

This was referenced Oct 26, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#729

Open

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#731

Open

davidhin mentioned this pull request Nov 9, 2024

[Snyk] Upgrade firebase-admin from 9.4.2 to 9.12.0 davidhin/vuldoctor#732

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): Add ability to link a federated ID with the `updateUser()` method. #770

feat(auth): Add ability to link a federated ID with the `updateUser()` method. #770

rsgowman commented Jan 20, 2020

nrsim left a comment

nrsim Jan 22, 2020

rsgowman Jan 27, 2020

nrsim Jan 22, 2020

rsgowman Jan 28, 2020

nrsim Jan 22, 2020

rsgowman Jan 27, 2020

hiranya911 Feb 5, 2021

rsgowman Feb 8, 2021

nrsim Jan 22, 2020

rsgowman Jan 27, 2020

nrsim Jan 22, 2020

rsgowman Jan 27, 2020

nrsim Jan 22, 2020

rsgowman Jan 27, 2020

nrsim Jan 22, 2020

rsgowman Jan 28, 2020

nrsim Jan 22, 2020

rsgowman Jan 28, 2020

nrsim Feb 3, 2020

rsgowman Dec 22, 2020

google-cla bot commented Dec 22, 2020

hiranya911 commented Jan 7, 2021

google-cla bot commented Feb 4, 2021

feat(auth): Add ability to link a federated ID with the updateUser() method. #770

feat(auth): Add ability to link a federated ID with the updateUser() method. #770

Conversation

rsgowman commented Jan 20, 2020

nrsim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Dec 22, 2020

hiranya911 commented Jan 7, 2021

google-cla bot commented Feb 4, 2021

feat(auth): Add ability to link a federated ID with the `updateUser()` method. #770

feat(auth): Add ability to link a federated ID with the `updateUser()` method. #770