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

If ConfirmEmailAddresses config is false, users should not be required to confirm changing their email #6314

Merged
merged 2 commits into from
Aug 16, 2018
Merged

If ConfirmEmailAddresses config is false, users should not be required to confirm changing their email #6314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94da12b
fix confirm email addresses config
Aug 15, 2018
649c20f
fix tests
Aug 15, 2018
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
2 changes: 1 addition & 1 deletion src/NuGetGallery.Core/Entities/User.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public void CancelChangeEmailAddress()
UnconfirmedEmailAddress = null;
}

public void UpdateEmailAddress(string newEmailAddress, Func<string> generateToken)
public void UpdateUnconfirmedEmailAddress(string newEmailAddress, Func<string> generateToken)
{
if (!string.IsNullOrEmpty(UnconfirmedEmailAddress))
{
Expand Down
2 changes: 1 addition & 1 deletion src/NuGetGallery/Controllers/AccountsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ public virtual async Task<ActionResult> ChangeEmail(TAccountViewModel model)
return AccountView(account, model);
}

if (account.Confirmed)
if (account.Confirmed && !string.IsNullOrEmpty(account.UnconfirmedEmailAddress))
{
SendEmailChangedConfirmationNotice(account);
}
Expand Down
13 changes: 12 additions & 1 deletion src/NuGetGallery/Services/UserService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,13 @@ public async Task ChangeEmailAddress(User user, string newEmailAddress)

await Auditing.SaveAuditRecordAsync(new UserAuditRecord(user, AuditedUserAction.ChangeEmail, newEmailAddress));

user.UpdateEmailAddress(newEmailAddress, Crypto.GenerateToken);
user.UpdateUnconfirmedEmailAddress(newEmailAddress, Crypto.GenerateToken);

if (!Config.ConfirmEmailAddresses)
{
user.ConfirmEmailAddress();
}

await UserRepository.CommitChangesAsync();
}

Expand Down Expand Up @@ -555,6 +561,11 @@ public async Task<Organization> AddOrganizationAsync(string username, string ema
Members = new List<Membership>()
};

if (!Config.ConfirmEmailAddresses)
{
organization.ConfirmEmailAddress();
}

var membership = new Membership { Organization = organization, Member = adminUser, IsAdmin = true };

organization.Members.Add(membership);
Expand Down
35 changes: 31 additions & 4 deletions tests/NuGetGallery.Facts/Controllers/AccountsControllerFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,23 @@ public virtual async Task WhenNewEmailIsSame_RedirectsWithoutChange(Func<Fakes,

[Theory]
[MemberData(AllowedCurrentUsersDataName)]
public virtual async Task WhenNewEmailIsDifferentAndWasConfirmed_SavesChanges(Func<Fakes, User> getCurrentUser)
public virtual Task WhenNewEmailIsUnconfirmedAndDifferentAndWasConfirmed_SavesChanges(Func<Fakes, User> getCurrentUser)
{
return WhenNewEmailIsDifferentAndWasConfirmedHelper(getCurrentUser, newEmailIsConfirmed: false);
}

[Theory]
[MemberData(AllowedCurrentUsersDataName)]
public virtual Task WhenNewEmailIsConfirmedAndDifferentAndWasConfirmed_SavesChanges(Func<Fakes, User> getCurrentUser)
{
return WhenNewEmailIsDifferentAndWasConfirmedHelper(getCurrentUser, newEmailIsConfirmed: true);
}

/// <remarks>
/// Normally, you should use a single <see cref="TheoryAttribute"/> that enumerates through the possible values of <paramref name="getCurrentUser"/> and <paramref name="newEmailIsConfirmed"/>,
/// but because we are using test case "inheritance" (search for properties with the same name as <see cref="AllowedCurrentUsersDataName"/>), this is not possible.
/// </remarks>
private async Task WhenNewEmailIsDifferentAndWasConfirmedHelper(Func<Fakes, User> getCurrentUser, bool newEmailIsConfirmed)
{
// Arrange
var controller = GetController();
Expand All @@ -236,15 +252,15 @@ public virtual async Task WhenNewEmailIsDifferentAndWasConfirmed_SavesChanges(Fu
model.ChangeEmail.NewEmail = "[email protected]";

// Act
var result = await InvokeChangeEmail(controller, account, getCurrentUser, model);
var result = await InvokeChangeEmail(controller, account, getCurrentUser, model, newEmailIsConfirmed);

// Assert
GetMock<IUserService>().Verify(u => u.ChangeEmailAddress(It.IsAny<User>(), It.IsAny<string>()), Times.Once);
ResultAssert.IsRedirectToRoute(result, new { action = controller.AccountAction });

GetMock<IMessageService>()
.Verify(m => m.SendEmailChangeConfirmationNotice(It.IsAny<User>(), It.IsAny<string>()),
Times.Once);
newEmailIsConfirmed ? Times.Never() : Times.Once());
}

[Theory]
Expand Down Expand Up @@ -285,6 +301,7 @@ protected virtual Task<ActionResult> InvokeChangeEmail(
TUser account,
Func<Fakes, User> getCurrentUser,
TAccountViewModel model = null,
bool newEmailIsConfirmed = false,
EntityException exception = null)
{
// Arrange
Expand All @@ -299,7 +316,17 @@ protected virtual Task<ActionResult> InvokeChangeEmail(
.Returns(account as User);

var setup = userService.Setup(u => u.ChangeEmailAddress(It.IsAny<User>(), It.IsAny<string>()))
.Callback<User, string>((acct, newEmail) => { acct.UnconfirmedEmailAddress = newEmail; });
.Callback<User, string>((acct, newEmail) =>
{
if (newEmailIsConfirmed)
{
acct.EmailAddress = newEmail;
}
else
{
acct.UnconfirmedEmailAddress = newEmail;
}
});

if (exception != null)
{
Expand Down
114 changes: 95 additions & 19 deletions tests/NuGetGallery.Facts/Services/UserServiceFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,13 +1156,38 @@ public async Task SetsUnconfirmedEmailWhenEmailIsChanged()
Users = new[] { user }
};

service.MockConfig
.Setup(x => x.ConfirmEmailAddresses)
.Returns(true);

await service.ChangeEmailAddress(user, "[email protected]");

Assert.Equal("[email protected]", user.EmailAddress);
Assert.Equal("[email protected]", user.UnconfirmedEmailAddress);
service.FakeEntitiesContext.VerifyCommitChanges();
}

[Fact]
public async Task AutomaticallyConfirmsWhenConfirmEmailAddressesConfigurationIsFalse()
{
var user = new User { Username = "Bob", EmailAddress = "[email protected]" };
var service = new TestableUserServiceWithDBFaking
{
Users = new[] { user }
};

service.MockConfig
.Setup(x => x.ConfirmEmailAddresses)
.Returns(false);

await service.ChangeEmailAddress(user, "[email protected]");

Assert.Equal("[email protected]", user.EmailAddress);
Assert.Null(user.UnconfirmedEmailAddress);
Assert.Null(user.EmailConfirmationToken);
service.FakeEntitiesContext.VerifyCommitChanges();
}

/// <summary>
/// It has to change the pending confirmation token whenever address changes because otherwise you can do
/// 1. change address, get confirmation email
Expand All @@ -1178,6 +1203,10 @@ public async Task ModifiesConfirmationTokenWhenEmailAddressChanged()
Users = new User[] { user },
};

service.MockConfig
.Setup(x => x.ConfirmEmailAddresses)
.Returns(true);

await service.ChangeEmailAddress(user, "[email protected]");
Assert.NotNull(user.EmailConfirmationToken);
Assert.NotEmpty(user.EmailConfirmationToken);
Expand All @@ -1197,6 +1226,10 @@ public async Task DoesNotModifyAnythingWhenConfirmedEmailAddressNotChanged()
Users = new User[] { user },
};

service.MockConfig
.Setup(x => x.ConfirmEmailAddresses)
.Returns(true);

await service.ChangeEmailAddress(user, "[email protected]");
Assert.True(user.Confirmed);
Assert.Equal("[email protected]", user.EmailAddress);
Expand All @@ -1221,6 +1254,10 @@ public async Task DoesNotModifyConfirmationTokenWhenUnconfirmedEmailAddressNotCh
Users = new User[] { user },
};

service.MockConfig
.Setup(x => x.ConfirmEmailAddresses)
.Returns(true);

await service.ChangeEmailAddress(user, "[email protected]");
Assert.Equal("pending-token", user.EmailConfirmationToken);
}
Expand Down Expand Up @@ -1784,11 +1821,16 @@ public class TheAddOrganizationAccountMethod

private TestableUserService _service = new TestableUserService();

[Fact]
public async Task WithUsernameConflict_ThrowsEntityException()
public static IEnumerable<object[]> ConfirmEmailAddresses_Config => MemberDataHelper.AsDataSet(false, true);

[Theory]
[MemberData(nameof(ConfirmEmailAddresses_Config))]
public async Task WithUsernameConflict_ThrowsEntityException(bool confirmEmailAddresses)
{
var conflictUsername = "ialreadyexist";

SetUpConfirmEmailAddressesConfig(confirmEmailAddresses);

_service.MockEntitiesContext
.Setup(x => x.Users)
.Returns(new[] { new User(conflictUsername) }.MockDbSet().Object);
Expand All @@ -1802,11 +1844,14 @@ public async Task WithUsernameConflict_ThrowsEntityException()
Assert.False(_service.Auditing.WroteRecord<UserAuditRecord>());
}

[Fact]
public async Task WithEmailConflict_ThrowsEntityException()
[Theory]
[MemberData(nameof(ConfirmEmailAddresses_Config))]
public async Task WithEmailConflict_ThrowsEntityException(bool confirmEmailAddresses)
{
var conflictEmail = "[email protected]";

SetUpConfirmEmailAddressesConfig(confirmEmailAddresses);

_service.MockEntitiesContext
.Setup(x => x.Users)
.Returns(new[] { new User("user") { EmailAddress = conflictEmail } }.MockDbSet().Object);
Expand All @@ -1820,25 +1865,31 @@ public async Task WithEmailConflict_ThrowsEntityException()
Assert.False(_service.Auditing.WroteRecord<UserAuditRecord>());
}

[Fact]
public async Task WhenAdminHasNoTenant_ReturnsNewOrgWithoutPolicy()
[Theory]
[MemberData(nameof(ConfirmEmailAddresses_Config))]
public async Task WhenAdminHasNoTenant_ReturnsNewOrgWithoutPolicy(bool confirmEmailAddresses)
{
_service.MockEntitiesContext
.Setup(x => x.Users)
.Returns(Enumerable.Empty<User>().MockDbSet().Object);

SetUpConfirmEmailAddressesConfig(confirmEmailAddresses);

var org = await InvokeAddOrganization(admin: new User(AdminName) { Credentials = new Credential[0] });

AssertNewOrganizationReturned(org, subscribedToPolicy: false);
AssertNewOrganizationReturned(org, subscribedToPolicy: false, confirmEmailAddresses: confirmEmailAddresses);
}

[Fact]
public async Task WhenAdminHasUnsupportedTenant_ReturnsNewOrgWithoutPolicy()
[Theory]
[MemberData(nameof(ConfirmEmailAddresses_Config))]
public async Task WhenAdminHasUnsupportedTenant_ReturnsNewOrgWithoutPolicy(bool confirmEmailAddresses)
{
_service.MockEntitiesContext
.Setup(x => x.Users)
.Returns(Enumerable.Empty<User>().MockDbSet().Object);

SetUpConfirmEmailAddressesConfig(confirmEmailAddresses);

var mockLoginDiscontinuationConfiguration = new Mock<ILoginDiscontinuationConfiguration>();
mockLoginDiscontinuationConfiguration
.Setup(x => x.IsTenantIdPolicySupportedForOrganization(It.IsAny<string>(), It.IsAny<string>()))
Expand All @@ -1850,16 +1901,19 @@ public async Task WhenAdminHasUnsupportedTenant_ReturnsNewOrgWithoutPolicy()

var org = await InvokeAddOrganization();

AssertNewOrganizationReturned(org, subscribedToPolicy: false);
AssertNewOrganizationReturned(org, subscribedToPolicy: false, confirmEmailAddresses: confirmEmailAddresses);
}

[Fact]
public async Task WhenSubscribingToPolicyFails_ReturnsNewOrgWithoutPolicy()
[Theory]
[MemberData(nameof(ConfirmEmailAddresses_Config))]
public async Task WhenSubscribingToPolicyFails_ReturnsNewOrgWithoutPolicy(bool confirmEmailAddresses)
{
_service.MockEntitiesContext
.Setup(x => x.Users)
.Returns(Enumerable.Empty<User>().MockDbSet().Object);

SetUpConfirmEmailAddressesConfig(confirmEmailAddresses);

var mockLoginDiscontinuationConfiguration = new Mock<ILoginDiscontinuationConfiguration>();
mockLoginDiscontinuationConfiguration
.Setup(x => x.IsTenantIdPolicySupportedForOrganization(It.IsAny<string>(), It.IsAny<string>()))
Expand All @@ -1875,16 +1929,19 @@ public async Task WhenSubscribingToPolicyFails_ReturnsNewOrgWithoutPolicy()

var org = await InvokeAddOrganization();

AssertNewOrganizationReturned(org, subscribedToPolicy: true);
AssertNewOrganizationReturned(org, subscribedToPolicy: true, confirmEmailAddresses: confirmEmailAddresses);
}

[Fact]
public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg()
[Theory]
[MemberData(nameof(ConfirmEmailAddresses_Config))]
public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg(bool confirmEmailAddresses)
{
_service.MockEntitiesContext
.Setup(x => x.Users)
.Returns(Enumerable.Empty<User>().MockDbSet().Object);

SetUpConfirmEmailAddressesConfig(confirmEmailAddresses);

var mockLoginDiscontinuationConfiguration = new Mock<ILoginDiscontinuationConfiguration>();
mockLoginDiscontinuationConfiguration
.Setup(x => x.IsTenantIdPolicySupportedForOrganization(It.IsAny<string>(), It.IsAny<string>()))
Expand All @@ -1900,7 +1957,7 @@ public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg()

var org = await InvokeAddOrganization();

AssertNewOrganizationReturned(org, subscribedToPolicy: true);
AssertNewOrganizationReturned(org, subscribedToPolicy: true, confirmEmailAddresses: confirmEmailAddresses);
}

private Task<Organization> InvokeAddOrganization(string orgName = OrgName, string orgEmail = OrgEmail, User admin = null)
Expand All @@ -1925,14 +1982,26 @@ private Task<Organization> InvokeAddOrganization(string orgName = OrgName, strin
return _service.AddOrganizationAsync(orgName, orgEmail, admin);
}

private void AssertNewOrganizationReturned(Organization org, bool subscribedToPolicy)
private void AssertNewOrganizationReturned(Organization org, bool subscribedToPolicy, bool confirmEmailAddresses)
{
Assert.Equal(OrgName, org.Username);
Assert.Equal(OrgEmail, org.UnconfirmedEmailAddress);

if (confirmEmailAddresses)
{
Assert.Null(org.EmailAddress);
Assert.Equal(OrgEmail, org.UnconfirmedEmailAddress);
Assert.NotNull(org.EmailConfirmationToken);
}
else
{
Assert.Null(org.UnconfirmedEmailAddress);
Assert.Equal(OrgEmail, org.EmailAddress);
Assert.Null(org.EmailConfirmationToken);
}

Assert.Equal(OrgCreatedUtc, org.CreatedUtc);
Assert.True(org.EmailAllowed);
Assert.True(org.NotifyPackagePushed);
Assert.True(!string.IsNullOrEmpty(org.EmailConfirmationToken));

// Both the organization and the admin must have a membership to each other.
Func<Membership, bool> hasMembership = m => m.Member.Username == AdminName && m.Organization.Username == OrgName && m.IsAdmin;
Expand All @@ -1949,6 +2018,13 @@ private void AssertNewOrganizationReturned(Organization org, bool subscribedToPo
ar.AffectedMemberIsAdmin == true));
_service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Once());
}

private void SetUpConfirmEmailAddressesConfig(bool confirmEmailAddresses)
{
_service.MockConfig
.Setup(x => x.ConfirmEmailAddresses)
.Returns(confirmEmailAddresses);
}
}
public class TheRejectTransformUserToOrganizationRequestMethod
{
Expand Down