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

Scott Bommarito · web-flow · commit 8ac0bcbcb9ad · 2018-08-16T11:49:43.000-07:00
diff --git a/src/NuGetGallery.Core/Entities/User.cs b/src/NuGetGallery.Core/Entities/User.cs
@@ -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))
             {
diff --git a/src/NuGetGallery/Controllers/AccountsController.cs b/src/NuGetGallery/Controllers/AccountsController.cs
@@ -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);
             }
diff --git a/src/NuGetGallery/Services/UserService.cs b/src/NuGetGallery/Services/UserService.cs
@@ -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();
         }
 
@@ -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);
diff --git a/tests/NuGetGallery.Facts/Controllers/AccountsControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/AccountsControllerFacts.cs
@@ -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();
@@ -236,15 +252,15 @@ public virtual async Task WhenNewEmailIsDifferentAndWasConfirmed_SavesChanges(Fu
                 model.ChangeEmail.NewEmail = "account2@example.com";
 
                 // 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]
@@ -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
@@ -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)
                 {
diff --git a/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs b/tests/NuGetGallery.Facts/Services/UserServiceFacts.cs
@@ -1156,13 +1156,38 @@ public async Task SetsUnconfirmedEmailWhenEmailIsChanged()
                     Users = new[] { user }
                 };
 
+                service.MockConfig
+                    .Setup(x => x.ConfirmEmailAddresses)
+                    .Returns(true);
+
                 await service.ChangeEmailAddress(user, "new@example.org");
 
                 Assert.Equal("old@example.org", user.EmailAddress);
                 Assert.Equal("new@example.org", user.UnconfirmedEmailAddress);
                 service.FakeEntitiesContext.VerifyCommitChanges();
             }
 
+            [Fact]
+            public async Task AutomaticallyConfirmsWhenConfirmEmailAddressesConfigurationIsFalse()
+            {
+                var user = new User { Username = "Bob", EmailAddress = "old@example.org" };
+                var service = new TestableUserServiceWithDBFaking
+                {
+                    Users = new[] { user }
+                };
+
+                service.MockConfig
+                    .Setup(x => x.ConfirmEmailAddresses)
+                    .Returns(false);
+
+                await service.ChangeEmailAddress(user, "new@example.org");
+
+                Assert.Equal("new@example.org", 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
@@ -1178,6 +1203,10 @@ public async Task ModifiesConfirmationTokenWhenEmailAddressChanged()
                     Users = new User[] { user },
                 };
 
+                service.MockConfig
+                    .Setup(x => x.ConfirmEmailAddresses)
+                    .Returns(true);
+
                 await service.ChangeEmailAddress(user, "new@example.com");
                 Assert.NotNull(user.EmailConfirmationToken);
                 Assert.NotEmpty(user.EmailConfirmationToken);
@@ -1197,6 +1226,10 @@ public async Task DoesNotModifyAnythingWhenConfirmedEmailAddressNotChanged()
                     Users = new User[] { user },
                 };
 
+                service.MockConfig
+                    .Setup(x => x.ConfirmEmailAddresses)
+                    .Returns(true);
+
                 await service.ChangeEmailAddress(user, "old@example.com");
                 Assert.True(user.Confirmed);
                 Assert.Equal("old@example.com", user.EmailAddress);
@@ -1221,6 +1254,10 @@ public async Task DoesNotModifyConfirmationTokenWhenUnconfirmedEmailAddressNotCh
                     Users = new User[] { user },
                 };
 
+                service.MockConfig
+                    .Setup(x => x.ConfirmEmailAddresses)
+                    .Returns(true);
+
                 await service.ChangeEmailAddress(user, "old@example.com");
                 Assert.Equal("pending-token", user.EmailConfirmationToken);
             }
@@ -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);
@@ -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 = "ialreadyexist@existence.com";
 
+                SetUpConfirmEmailAddressesConfig(confirmEmailAddresses);
+
                 _service.MockEntitiesContext
                     .Setup(x => x.Users)
                     .Returns(new[] { new User("user") { EmailAddress = conflictEmail } }.MockDbSet().Object);
@@ -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>()))
@@ -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>()))
@@ -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>()))
@@ -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)
@@ -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;
@@ -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
         {

Original file line number	Diff line number	Diff line change
`@@ -154,7 +154,7 @@ public void CancelChangeEmailAddress()`
`154`	`154`	`UnconfirmedEmailAddress = null;`
`155`	`155`	`}`
`156`	`156`
`157`		`- public void UpdateEmailAddress(string newEmailAddress, Func<string> generateToken)`
	`157`	`+ public void UpdateUnconfirmedEmailAddress(string newEmailAddress, Func<string> generateToken)`
`158`	`158`	`{`
`159`	`159`	`if (!string.IsNullOrEmpty(UnconfirmedEmailAddress))`
`160`	`160`	`{`
Original file line number	Diff line number	Diff line change
`@@ -252,7 +252,7 @@ public virtual async Task<ActionResult> ChangeEmail(TAccountViewModel model)`
`252`	`252`	`return AccountView(account, model);`
`253`	`253`	`}`
`254`	`254`
`255`		`- if (account.Confirmed)`
	`255`	`+ if (account.Confirmed && !string.IsNullOrEmpty(account.UnconfirmedEmailAddress))`
`256`	`256`	`{`
`257`	`257`	`SendEmailChangedConfirmationNotice(account);`
`258`	`258`	`}`