Skip to content
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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

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
52 changes: 50 additions & 2 deletions FirebaseAdmin/FirebaseAdmin.IntegrationTests/FirebaseAuthTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,9 @@ public async Task UserLifecycle()
Assert.Empty(user.ProviderData);
Assert.Empty(user.CustomClaims);

// Update user
// Update user with new properties as well as a provider to link to the user
var randomUser = RandomUser.Create();

var updateArgs = new UserRecordArgs()
{
Uid = uid,
Expand All @@ -207,6 +208,7 @@ public async Task UserLifecycle()
PhotoUrl = "https://example.com/photo.png",
EmailVerified = true,
Password = "secret",
ProviderToLink = randomUser.ProviderUser,
};
user = await FirebaseAuth.DefaultInstance.UpdateUserAsync(updateArgs);
Assert.Equal(uid, user.Uid);
Expand All @@ -218,13 +220,49 @@ public async Task UserLifecycle()
Assert.False(user.Disabled);
Assert.NotNull(user.UserMetaData.CreationTimestamp);
Assert.Null(user.UserMetaData.LastSignInTimestamp);
Assert.Equal(2, user.ProviderData.Length);
Assert.Equal(3, user.ProviderData.Length);
var providerIds = new HashSet<string>();
Copy link
Member

Choose a reason for hiding this comment

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

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.

foreach (ProviderUserInfo providerData in user.ProviderData)
{
providerIds.Add(providerData.ProviderId);
}

Assert.Equal(providerIds, new HashSet<string>() { "phone", "password", "google.com" });
Copy link
Contributor

Choose a reason for hiding this comment

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

Expected value goes first.

Assert.Empty(user.CustomClaims);

// Get user by email
user = await FirebaseAuth.DefaultInstance.GetUserByEmailAsync(randomUser.Email);
Assert.Equal(uid, user.Uid);

// Delete the linked provider and phone number
var unlinkArgs = new UserRecordArgs()
Copy link
Contributor

Choose a reason for hiding this comment

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

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 = ....
                };

{
Uid = uid,
DisplayName = "Updated Name",
Email = randomUser.Email,
PhoneNumber = null,
PhotoUrl = "https://example.com/photo.png",
EmailVerified = true,
Password = "secret",
ProvidersToDelete = new List<string>()
{
randomUser.ProviderUser.ProviderId,
},
};
user = await FirebaseAuth.DefaultInstance.UpdateUserAsync(unlinkArgs);
Assert.Equal(uid, user.Uid);
Assert.Equal(randomUser.Email, user.Email);
Assert.Null(user.PhoneNumber);
Assert.Equal("Updated Name", user.DisplayName);
Assert.Equal("https://example.com/photo.png", user.PhotoUrl);
Assert.True(user.EmailVerified);
Assert.False(user.Disabled);
Assert.NotNull(user.UserMetaData.CreationTimestamp);
Assert.Null(user.UserMetaData.LastSignInTimestamp);
Assert.Single(user.ProviderData);
Assert.Equal("password", user.ProviderData.First().ProviderId);
Assert.Empty(user.CustomClaims);

// Disable user and remove properties
var disableArgs = new UserRecordArgs()
{
Expand All @@ -245,6 +283,7 @@ public async Task UserLifecycle()
Assert.NotNull(user.UserMetaData.CreationTimestamp);
Assert.Null(user.UserMetaData.LastSignInTimestamp);
Assert.Single(user.ProviderData);
Assert.Equal("password", user.ProviderData.First().ProviderId);
Assert.Empty(user.CustomClaims);
}
finally
Expand Down Expand Up @@ -409,6 +448,8 @@ internal class RandomUser

internal string PhoneNumber { get; private set; }

internal ProviderUserInfo ProviderUser { get; private set; }

internal static RandomUser Create()
{
var uid = Guid.NewGuid().ToString().Replace("-", string.Empty);
Expand All @@ -421,11 +462,18 @@ internal static RandomUser Create()
phone += rand.Next(10);
}

var providerUser = new ProviderUserInfo()
{
Uid = "google_" + uid,
ProviderId = "google.com",
};

return new RandomUser()
{
Uid = uid,
Email = email,
PhoneNumber = phone,
ProviderUser = providerUser,
};
}
}
Expand Down
114 changes: 108 additions & 6 deletions FirebaseAdmin/FirebaseAdmin.Tests/Auth/FirebaseUserManagerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,11 @@ public async Task UpdateUser()
{ "level", 4 },
{ "package", "gold" },
};

var providerToLink = new ProviderUserInfo()
{
Uid = "google_user1",
ProviderId = "google.com",
};
var user = await auth.UpdateUserAsync(new UserRecordArgs()
{
CustomClaims = customClaims,
Expand All @@ -1044,6 +1048,7 @@ public async Task UpdateUser()
PhoneNumber = "+1234567890",
PhotoUrl = "https://example.com/user.png",
Uid = "user1",
ProviderToLink = providerToLink,
});

Assert.Equal("user1", user.Uid);
Expand All @@ -1057,7 +1062,10 @@ public async Task UpdateUser()
Assert.Equal("secret", request["password"]);
Assert.Equal("+1234567890", request["phoneNumber"]);
Assert.Equal("https://example.com/user.png", request["photoUrl"]);

var expectedProviderUserInfo = new JObject();
expectedProviderUserInfo.Add("Uid", "google_user1");
expectedProviderUserInfo.Add("ProviderId", "google.com");
Assert.Equal(expectedProviderUserInfo, request["linkProviderUserInfo"]);
var claims = NewtonsoftJsonSerializer.Instance.Deserialize<JObject>((string)request["customAttributes"]);
Assert.True((bool)claims["admin"]);
Assert.Equal(4L, claims["level"]);
Expand Down Expand Up @@ -1094,7 +1102,40 @@ public async Task UpdateUserPartial()
}

[Fact]
public async Task UpdateUserRemoveAttributes()
public async Task UpdateUserLinkProvider()
{
var handler = new MockMessageHandler()
{
Response = new List<string>() { CreateUserResponse, GetUserResponse },
};
var auth = this.CreateFirebaseAuth(handler);

var user = await auth.UpdateUserAsync(new UserRecordArgs()
{
Uid = "user1",
ProviderToLink = new ProviderUserInfo()
{
Uid = "google_user1",
ProviderId = "google.com",
},
});

Assert.Equal("user1", user.Uid);
Assert.Equal(2, handler.Requests.Count);
var request = NewtonsoftJsonSerializer.Instance.Deserialize<JObject>(handler.Requests[0].Body);
Assert.Equal(2, request.Count);
Assert.Equal("user1", request["localId"]);
var expectedProviderUserInfo = new JObject();
expectedProviderUserInfo.Add("Uid", "google_user1");
expectedProviderUserInfo.Add("ProviderId", "google.com");
Assert.Equal(expectedProviderUserInfo, request["linkProviderUserInfo"]);

this.AssertClientVersion(handler.Requests[0].Headers);
this.AssertClientVersion(handler.Requests[1].Headers);
}

[Fact]
public async Task UpdateUserDeleteAttributes()
{
var handler = new MockMessageHandler()
{
Expand Down Expand Up @@ -1123,7 +1164,7 @@ public async Task UpdateUserRemoveAttributes()
}

[Fact]
public async Task UpdateUserRemoveProviders()
public async Task UpdateUserDeleteProviders()
{
var handler = new MockMessageHandler()
{
Expand All @@ -1135,16 +1176,17 @@ public async Task UpdateUserRemoveProviders()
{
PhoneNumber = null,
Uid = "user1",
ProvidersToDelete = new List<string>() { "google.com" },
});

Assert.Equal("user1", user.Uid);
Assert.Equal(2, handler.Requests.Count);
var request = NewtonsoftJsonSerializer.Instance.Deserialize<JObject>(handler.Requests[0].Body);
Assert.Equal(2, request.Count);
Assert.Equal("user1", request["localId"]);
Assert.Null(request["phone"]);
Assert.Equal(
new JArray() { "phone" },
request["deleteProvider"]);
new JArray() { "phone", "google.com" }, request["deleteProvider"]);

this.AssertClientVersion(handler.Requests[0].Headers);
this.AssertClientVersion(handler.Requests[1].Headers);
Expand Down Expand Up @@ -1393,6 +1435,66 @@ public async Task UpdateUserShortPassword()
Assert.Empty(handler.Requests);
}

[Fact]
public async Task UpdateUserInvalidProviderToLink()
{
var handler = new MockMessageHandler() { Response = CreateUserResponse };
var auth = this.CreateFirebaseAuth(handler);

// Empty provider ID
var args = new UserRecordArgs()
{
ProviderToLink = new ProviderUserInfo()
{
Uid = "google_user1",
ProviderId = string.Empty,
},
Uid = "user1",
};
await Assert.ThrowsAsync<ArgumentException>(
async () => await auth.UpdateUserAsync(args));
Assert.Empty(handler.Requests);

// Empty provider UID
args.ProviderToLink.Uid = string.Empty;
Copy link
Member

Choose a reason for hiding this comment

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

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.)

args.ProviderToLink.ProviderId = "google.com";
await Assert.ThrowsAsync<ArgumentException>(
async () => await auth.UpdateUserAsync(args));
Assert.Empty(handler.Requests);

// Phone provider updates in two places
args.PhoneNumber = "+11234567890";
args.ProviderToLink.ProviderId = "phone";
args.ProviderToLink.Uid = "+11234567891";
await Assert.ThrowsAsync<ArgumentException>(
async () => await auth.UpdateUserAsync(args));
Assert.Empty(handler.Requests);
}

[Fact]
public async Task UpdateUserInvalidProvidersToDelete()
{
var handler = new MockMessageHandler() { Response = CreateUserResponse };
var auth = this.CreateFirebaseAuth(handler);

// Empty provider ID
var args = new UserRecordArgs()
{
ProvidersToDelete = new List<string>() { "google.com", string.Empty },
Uid = "user1",
};
await Assert.ThrowsAsync<ArgumentException>(
async () => await auth.UpdateUserAsync(args));
Assert.Empty(handler.Requests);

// Phone provider updates in two places
args.PhoneNumber = null;
args.ProvidersToDelete = new List<string>() { "google.com", "phone" };
await Assert.ThrowsAsync<ArgumentException>(
async () => await auth.UpdateUserAsync(args));
Assert.Empty(handler.Requests);
}

[Fact]
public void EmptyNameClaims()
{
Expand Down
67 changes: 39 additions & 28 deletions FirebaseAdmin/FirebaseAdmin/Auth/ProviderUserInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,64 +1,75 @@
using System;
using System.Collections.Generic;
using System.Text;
using Google.Apis.Json;
using Newtonsoft.Json;

namespace FirebaseAdmin.Auth
{
/// <summary>
/// Contains metadata regarding how a user is known by a particular identity provider (IdP).
/// Instances of this class are immutable and thread safe.
/// </summary>
internal sealed class ProviderUserInfo : IUserInfo
public sealed class ProviderUserInfo : IUserInfo
Copy link
Member

Choose a reason for hiding this comment

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

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

Options:

  1. Remove that sentence.
  2. 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.)
  3. 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: "[email protected]"
};

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: "[email protected]"
});

(or something like that.)

Check out UserRecord and UserRecordArgs for a concrete example.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

{
/// <summary>
/// Initializes a new instance of the <see cref="ProviderUserInfo"/> class with data provided by an authentication provider.
/// </summary>
/// <param name="provider">The deserialized JSON user data from the provider.</param>
internal ProviderUserInfo(GetAccountInfoResponse.Provider provider)
{
this.Uid = provider.UserId;
this.DisplayName = provider.DisplayName;
this.Email = provider.Email;
this.PhoneNumber = provider.PhoneNumber;
this.PhotoUrl = provider.PhotoUrl;
this.ProviderId = provider.ProviderID;
}

/// <summary>
/// Gets the user's unique ID assigned by the identity provider.
/// Gets or sets the user's unique ID assigned by the identity provider.
/// </summary>
/// <returns>a user ID string.</returns>
public string Uid { get; private set; }
[JsonProperty("rawId")]
public string Uid { get; set; }

/// <summary>
/// Gets the user's display name, if available.
/// Gets or sets the user's display name, if available.
/// </summary>
/// <returns>a display name string or null.</returns>
public string DisplayName { get; private set; }
[JsonProperty("displayName")]
public string DisplayName { get; set; }

/// <summary>
/// Gets the user's email address, if available.
/// Gets or sets the user's email address, if available.
/// </summary>
/// <returns>an email address string or null.</returns>
public string Email { get; private set; }
[JsonProperty("email")]
public string Email { get; set; }

/// <summary>
/// Gets the user's phone number.
/// Gets or sets the user's phone number.
/// </summary>
/// <returns>a phone number string or null.</returns>
public string PhoneNumber { get; private set; }
[JsonProperty("phoneNumber")]
public string PhoneNumber { get; set; }

/// <summary>
/// Gets the user's photo URL, if available.
/// Gets or sets the user's photo URL, if available.
/// </summary>
/// <returns>a URL string or null.</returns>
public string PhotoUrl { get; private set; }
[JsonProperty("photoUrl")]
public string PhotoUrl { get; set; }

/// <summary>
/// Gets the ID of the identity provider. This can be a short domain name (e.g. google.com) or
/// the identifier of an OpenID identity provider.
/// Gets or sets the ID of the identity provider. This can be a short domain name (e.g.
/// google.com) or the identifier of an OpenID identity provider.
/// </summary>
/// <returns>an ID string that uniquely identifies the identity provider.</returns>
public string ProviderId { get; private set; }
[JsonProperty("providerId")]
public string ProviderId { get; set; }

/// <summary>
/// Initializes a new instance of the <see cref="ProviderUserInfo"/> class with data provided by an authentication provider.
/// </summary>
/// <param name="provider">The deserialized JSON user data from the provider.</param>
internal static ProviderUserInfo Create(GetAccountInfoResponse.Provider provider)
{
return new ProviderUserInfo()
{
Uid = provider.UserId,
DisplayName = provider.DisplayName,
Email = provider.Email,
PhoneNumber = provider.PhoneNumber,
PhotoUrl = provider.PhotoUrl,
ProviderId = provider.ProviderID,
};
}
}
}
2 changes: 1 addition & 1 deletion FirebaseAdmin/FirebaseAdmin/Auth/UserRecord.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ internal UserRecord(GetAccountInfoResponse.User user)
this.ProviderData = new IUserInfo[count];
for (int i = 0; i < count; i++)
{
this.ProviderData[i] = new ProviderUserInfo(user.Providers[i]);
this.ProviderData[i] = ProviderUserInfo.Create(user.Providers[i]);
Copy link
Member

Choose a reason for hiding this comment

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

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

}
}

Expand Down
Loading