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

nrsim · 2020-02-03T16:35:35Z

No description provided.

rsgowman · 2020-02-05T20:47:27Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecordArgs.cs

+                }
+
+                if (args.providerToLink != null)
+                {


We also need to allow other non-federated providers to work here. For instance, if providerToLink.ProviderId == "phone", then we should set this.PhoneNumber = CheckPhoneNumber(providerToLink.ProviderUid) and not set this.ProviderToLink at all. (See the node port for how I did it... firebase/firebase-admin-node#770. But that's not fully reviewed yet, so take it with a grain of salt.)

I'm not sure what this means for the email provider. Under node, the email parameter is optional. But under C#, it looks like it might be required? If so, I think it would be sufficient to immediately throw should providerToLink.ProviderId == "email".

rsgowman · 2020-02-05T20:56:12Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecordArgs.cs

        private Optional<string> phoneNumber;
        private Optional<IReadOnlyDictionary<string, object>> customClaims;
+        private Optional<ProviderUserInfo> providerToLink;
+        private Optional<IList<string>> providersToDelete;


Rather than IList, you could probably use IEnumerable.

rsgowman · 2020-02-05T21:06:08Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecordArgs.cs

+                var iteratedIds = new HashSet<string>();
+                foreach (string providerId in providerIds)
+                {
+                    if (iteratedIds.Contains(providerId))


This block could become:

if (!iteratedIds.Add(providerId)) { throw ...; }

(ISet.Add returns false if the item was already present.)

rsgowman · 2020-02-05T21:13:39Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecordArgs.cs


                if (args.phoneNumber != null)
                {
+                    providerIdsToUpdate.Add("phone");


This is an interesting pattern. As an alternative, you could:

void AddOrThrow(ISet<T> s, T t) { if (!s.add(t)) { throw ...; } }

And then use it here like this:

AddOrThrow(providerIdsToUpdate, "phone");

(And switch providerIdsToUpdate to a hashset and remove AssertDistinctProviderIds())

Another alternative is to just check everything up front, since there aren't actually many cases where we can run into this.

Both optional.

+1 to this. Let's call it AddIfAbsent() or something similar.

rsgowman · 2020-02-05T21:18:13Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecord.cs

                for (int i = 0; i < count; i++)
                {
-                    this.ProviderData[i] = new ProviderUserInfo(user.Providers[i]);
+                    this.ProviderData[i] = ProviderUserInfo.Create(user.Providers[i]);


OOC, why did you convert the ctor to a Create() method?

rsgowman · 2020-02-05T21:18:45Z

FirebaseAdmin/FirebaseAdmin/Auth/ProviderUserInfo.cs

    /// Instances of this class are immutable and thread safe.
    /// </summary>
-    internal sealed class ProviderUserInfo : IUserInfo
+    public sealed class ProviderUserInfo : IUserInfo


docstring claims that these are immutable and thread safe. I don't think that's the case anymore (for either).

Options:

Remove that sentence.

Remove the setters. Do we need them after construction? (They were present, though private previously. I don't know if they actually needed to be present at all though.)

Keep the setters, but mark them private.

...

Oh, I see! You've made the properties publicly settable so that uses can construct them more naturally, i.e.:

var x = new ProviderUserInfo() { Email: "user@example.com" };

But that means that the object looses immutability/threadsafety. Elsewhere in the code base, this pattern is used:

public ProviderUserInfo(ProviderUserInfoArgs x) { ... }

which would allow it to be used almost as nicely, but with thread safety, etc. i.e.:

var x = new ProviderUserInfo(new ProviderUserInfoArgs() { Email: "user@example.com" });

(or something like that.)

Check out UserRecord and UserRecordArgs for a concrete example.

The API proposal has the following:

public class UserProviderArgs : IUserInfo { public string ProviderId { get; set; } public string Uid { get; set; } public string DisplayName { get; set; } public string Email { get; set; } public string PhoneNumber { get; set; } public string PhotoUrl ( get; set; } }

So just implement that?

rsgowman · 2020-02-05T21:51:23Z

FirebaseAdmin/FirebaseAdmin.Tests/Auth/FirebaseUserManagerTest.cs

+            Assert.Empty(handler.Requests);
+
+            // Empty provider UID
+            args.ProviderToLink.Uid = string.Empty;


Could we parameterize these tests instead? (grep for [Theory] to see an example or two in our code base. I can't find much documentation on xunit, but there's also a bunch of blog posts that go into more detail.)

rsgowman · 2020-02-05T22:02:11Z

FirebaseAdmin/FirebaseAdmin.IntegrationTests/FirebaseAuthTest.cs

                Assert.Null(user.UserMetaData.LastSignInTimestamp);
-                Assert.Equal(2, user.ProviderData.Length);
+                Assert.Equal(3, user.ProviderData.Length);
+                var providerIds = new HashSet<string>();


HashSet has a ctor that takes an enumerable. Combining that with Select, I think you could replace lines 224-228 with:

var providerIds = new HashSet<string>(user.ProviderData.Select((providerData) => providerData.ProviderId));

Optional.

hiranya911

Just a few pointers in addition to what @rsgowman has already commented on.

hiranya911 · 2020-02-06T00:36:53Z

FirebaseAdmin/FirebaseAdmin/Auth/ProviderUserInfo.cs

    /// Instances of this class are immutable and thread safe.
    /// </summary>
-    internal sealed class ProviderUserInfo : IUserInfo
+    public sealed class ProviderUserInfo : IUserInfo


The API proposal has the following:

public class UserProviderArgs : IUserInfo { public string ProviderId { get; set; } public string Uid { get; set; } public string DisplayName { get; set; } public string Email { get; set; } public string PhoneNumber { get; set; } public string PhotoUrl ( get; set; } }

So just implement that?

hiranya911 · 2020-02-06T00:42:51Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecordArgs.cs

+        public ProviderUserInfo ProviderToLink
+        {
+            get => this.providerToLink?.Value;
+            set => this.providerToLink = this.Wrap(value);


I think you can use a plain old {get; set;} here. Wrap() is only used when setting a null value has special meanin. For example PhotoUrl = null has special meaning in the Auth API -- it's a request to unset the currently set photo URL. I don't think that applies here.

hiranya911 · 2020-02-06T00:43:03Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecordArgs.cs

+        /// </summary>
+        public IList<string> ProvidersToDelete
+        {
+            get => this.providersToDelete?.Value;


hiranya911 · 2020-02-06T00:51:32Z

FirebaseAdmin/FirebaseAdmin/Auth/UserRecordArgs.cs


                if (args.phoneNumber != null)
                {
+                    providerIdsToUpdate.Add("phone");


+1 to this. Let's call it AddIfAbsent() or something similar.

hiranya911 · 2020-02-06T00:57:11Z

FirebaseAdmin/FirebaseAdmin.IntegrationTests/FirebaseAuthTest.cs

+                    providerIds.Add(providerData.ProviderId);
+                }
+
+                Assert.Equal(providerIds, new HashSet<string>() { "phone", "password", "google.com" });


Expected value goes first.

hiranya911 · 2020-02-06T01:00:39Z

FirebaseAdmin/FirebaseAdmin.IntegrationTests/FirebaseAuthTest.cs

                Assert.Equal(uid, user.Uid);

+                // Delete the linked provider and phone number
+                var unlinkArgs = new UserRecordArgs()


Shall we merge this into the existing logic that disables the user account and removes other properties?

var disableArgs = new UserRecordArgs() { Uid = uid, Disabled = true, DisplayName = null, PhoneNumber = null, PhotoUrl = null, ProvidersToDelete = .... };

Thaina · 2020-12-30T07:27:06Z

Why this pull request got stuck since february?

Add the ability to link/unlink a provider user id to an account.

5a4f886

nrsim requested review from hiranya911 and rsgowman February 3, 2020 16:35

nrsim assigned rsgowman Feb 5, 2020

rsgowman reviewed Feb 5, 2020

View reviewed changes

rsgowman assigned nrsim and unassigned rsgowman Feb 5, 2020

hiranya911 suggested changes Feb 6, 2020

View reviewed changes

samtstern mentioned this pull request Apr 1, 2020

Snippets for new Admin auth features #163

Open

Alhetus mentioned this pull request Aug 22, 2023

Add the ability to link/unlink a provider user id to an account. #359

Open

Conversation

nrsim commented Feb 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Thaina commented Dec 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants