Skip to content

Commit 46740f0

Browse files
zhhyuzhhyu
authored
Fix email verification issue for AAD/MSA registration (#6548)
* Fix email verification issue for AAD/MSA registration * Add email match logic * Use default comparison for emails * Refactor logic and add unit tests * Add unit tests
1 parent 2ea385d commit 46740f0

5 files changed

Lines changed: 247 additions & 15 deletions

File tree

‎src/NuGetGallery/Authentication/AuthenticateExternalLoginResult.cs‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4-
using System;
5-
using System.Collections.Generic;
64
using System.Security.Claims;
75
using NuGetGallery.Authentication.Providers;
86

@@ -15,6 +13,7 @@ public class AuthenticateExternalLoginResult
1513
public Authenticator Authenticator { get; set; }
1614
public Credential Credential { get; set; }
1715
public ExternalLoginSessionDetails LoginDetails { get; set; }
16+
public IdentityInformation UserInfo { get; set; }
1817
}
1918

2019
public class ExternalLoginSessionDetails

‎src/NuGetGallery/Authentication/AuthenticationService.cs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ private async Task<Claim[]> GetUserLoginClaims(AuthenticatedUser user, bool wasM
299299
return claims.ToArray();
300300
}
301301

302-
public virtual async Task<AuthenticatedUser> Register(string username, string emailAddress, Credential credential)
302+
public virtual async Task<AuthenticatedUser> Register(string username, string emailAddress, Credential credential, bool autoConfirm = false)
303303
{
304304
if (_config.FeedOnlyMode)
305305
{
@@ -332,7 +332,7 @@ public virtual async Task<AuthenticatedUser> Register(string username, string em
332332
// Add a credential for the password
333333
newUser.Credentials.Add(credential);
334334

335-
if (!_config.ConfirmEmailAddresses)
335+
if (!_config.ConfirmEmailAddresses || autoConfirm)
336336
{
337337
newUser.ConfirmEmailAddress();
338338
}
@@ -651,7 +651,8 @@ public virtual async Task<AuthenticateExternalLoginResult> ReadExternalLoginCred
651651
ExternalIdentity = externalIdentity,
652652
Authenticator = authenticator,
653653
Credential = _credentialBuilder.CreateExternalCredential(userInfo.AuthenticationType, userInfo.Identifier, identity, userInfo.TenantId),
654-
LoginDetails = new ExternalLoginSessionDetails(userInfo.Email, userInfo.UsedMultiFactorAuthentication)
654+
LoginDetails = new ExternalLoginSessionDetails(userInfo.Email, userInfo.UsedMultiFactorAuthentication),
655+
UserInfo = userInfo
655656
};
656657
}
657658
catch (Exception ex)

‎src/NuGetGallery/Controllers/AuthenticationController.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,9 @@ public virtual async Task<ActionResult> Register(LogOnViewModel model, string re
271271
user = await _authService.Register(
272272
model.Register.Username,
273273
model.Register.EmailAddress,
274-
result.Credential);
274+
result.Credential,
275+
(result.Credential.IsExternal() && string.Equals(result.UserInfo?.Email, model.Register.EmailAddress))
276+
);
275277
}
276278
else
277279
{

‎tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs‎

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -759,7 +759,8 @@ public async Task GivenPlainTextPassword_ItSaltsHashesAndPassesThru()
759759
It.Is<Credential>(c => VerifyPasswordHash(
760760
c.Value,
761761
CredentialBuilder.LatestPasswordType,
762-
password))))
762+
password)),
763+
It.IsAny<bool>()))
763764
.CompletesWithNull()
764765
.Verifiable();
765766

@@ -844,6 +845,57 @@ public async Task WillSaveTheNewUserAsConfirmedWhenConfigured()
844845
auth.Entities.VerifyCommitChanges();
845846
}
846847

848+
[Theory]
849+
[InlineData("MicrosoftAccount")]
850+
[InlineData("AzureActiveDirectory")]
851+
public async Task WillSaveTheNewUserWithExternalCredentialAndMatchEmailAsConfirmed(string credType)
852+
{
853+
// Arrange
854+
var configurationService = GetConfigurationService();
855+
configurationService.Current.ConfirmEmailAddresses = true;
856+
857+
var auth = Get<AuthenticationService>();
858+
859+
// Act
860+
var authUser = await auth.Register(
861+
"newUser",
862+
"theEmailAddress",
863+
new CredentialBuilder().CreateExternalCredential(credType, "blorg", "Bloog"),
864+
true);
865+
866+
// Assert
867+
Assert.True(auth.Entities.Users.Contains(authUser.User));
868+
Assert.True(authUser.User.Confirmed);
869+
Assert.True(string.Equals(authUser.User.EmailAddress, "theEmailAddress"));
870+
auth.Entities.VerifyCommitChanges();
871+
}
872+
873+
[Theory]
874+
[InlineData("MicrosoftAccount")]
875+
[InlineData("AzureActiveDirectory")]
876+
public async Task WillSaveTheNewUserWithExternalCredentialAndNotMatchEmailAsNotConfirmed(string credType)
877+
{
878+
// Arrange
879+
var configurationService = GetConfigurationService();
880+
configurationService.Current.ConfirmEmailAddresses = true;
881+
882+
var auth = Get<AuthenticationService>();
883+
884+
// Act
885+
var authUser = await auth.Register(
886+
"newUser",
887+
"theEmailAddress",
888+
new CredentialBuilder().CreateExternalCredential(credType, "blorg", "Bloog"),
889+
false);
890+
891+
// Assert
892+
Assert.True(auth.Entities.Users.Contains(authUser.User));
893+
auth.Entities.VerifyCommitChanges();
894+
Assert.True(string.Equals(authUser.User.UnconfirmedEmailAddress, "theEmailAddress"));
895+
Assert.NotNull(authUser.User.EmailConfirmationToken);
896+
Assert.False(authUser.User.Confirmed);
897+
}
898+
847899
[Fact]
848900
public async Task SetsAConfirmationToken()
849901
{

‎tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs‎

Lines changed: 186 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -720,7 +720,7 @@ public async Task WillShowTheViewWithErrorsIfTheModelStateIsInvalid()
720720
public async Task WillInvalidateModelStateAndShowTheViewWhenAnEntityExceptionIsThrow()
721721
{
722722
GetMock<AuthenticationService>()
723-
.Setup(x => x.Register(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Credential>()))
723+
.Setup(x => x.Register(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<Credential>(), It.IsAny<bool>()))
724724
.Throws(new EntityException("aMessage"));
725725

726726
var controller = GetController<AuthenticationController>();
@@ -756,7 +756,7 @@ public async Task WillCreateAndLogInTheUserWhenNotLinking()
756756
var authenticationService = GetMock<AuthenticationService>();
757757
var controller = GetController<AuthenticationController>();
758758

759-
authenticationService.Setup(x => x.Register(authUser.User.Username, authUser.User.UnconfirmedEmailAddress, It.IsAny<Credential>()))
759+
authenticationService.Setup(x => x.Register(authUser.User.Username, authUser.User.UnconfirmedEmailAddress, It.IsAny<Credential>(), It.IsAny<bool>()))
760760
.CompletesWith(authUser);
761761

762762
authenticationService
@@ -807,7 +807,7 @@ public async Task WillNotSendConfirmationEmailWhenConfirmEmailAddressesIsOff()
807807
configurationService.Current.ConfirmEmailAddresses = false;
808808

809809
GetMock<AuthenticationService>()
810-
.Setup(x => x.Register("theUsername", "[email protected]", It.IsAny<Credential>()))
810+
.Setup(x => x.Register("theUsername", "[email protected]", It.IsAny<Credential>(), It.IsAny<bool>()))
811811
.CompletesWith(authUser);
812812

813813
var controller = GetController<AuthenticationController>();
@@ -830,6 +830,182 @@ public async Task WillNotSendConfirmationEmailWhenConfirmEmailAddressesIsOff()
830830
}, "/theReturnUrl", linkingAccount: false);
831831

832832
// Assert
833+
GetMock<AuthenticationService>()
834+
.Verify(x => x.Register("theUsername", "[email protected]", It.IsAny<Credential>(), false));
835+
836+
GetMock<IMessageService>()
837+
.Verify(x => x.SendNewAccountEmailAsync(
838+
It.IsAny<User>(),
839+
It.IsAny<string>()), Times.Never());
840+
}
841+
842+
[Fact]
843+
public async Task WillNotAutoConfirmAndWillSendConfirmationEmailWhenNotExternalCredential()
844+
{
845+
// Arrange
846+
var authUser = new AuthenticatedUser(
847+
new User("theUsername")
848+
{
849+
UnconfirmedEmailAddress = "[email protected]",
850+
EmailConfirmationToken = "t0k3n"
851+
},
852+
new Credential());
853+
854+
var authenticationServiceMock = GetMock<AuthenticationService>();
855+
var controller = GetController<AuthenticationController>();
856+
authenticationServiceMock
857+
.Setup(x => x.Register(authUser.User.Username, authUser.User.UnconfirmedEmailAddress, It.IsAny<Credential>(), It.IsAny<bool>()))
858+
.CompletesWith(authUser);
859+
authenticationServiceMock
860+
.Setup(x => x.CreateSessionAsync(controller.OwinContext, authUser, false))
861+
.Returns(Task.FromResult(0))
862+
.Verifiable();
863+
authenticationServiceMock
864+
.Setup(x => x.ReadExternalLoginCredential(controller.OwinContext))
865+
.CompletesWith(new AuthenticateExternalLoginResult()
866+
{
867+
ExternalIdentity = new ClaimsIdentity(),
868+
Credential = new Credential(),
869+
UserInfo = new IdentityInformation("", "", authUser.User.UnconfirmedEmailAddress, "")
870+
});
871+
872+
// Act
873+
var result = await controller.Register(
874+
new LogOnViewModel()
875+
{
876+
Register = new RegisterViewModel
877+
{
878+
Username = "theUsername",
879+
EmailAddress = authUser.User.UnconfirmedEmailAddress,
880+
}
881+
}, "/theReturnUrl", linkingAccount: true);
882+
883+
// Assert
884+
authenticationServiceMock.VerifyAll();
885+
886+
GetMock<AuthenticationService>()
887+
.Verify(x => x.Register(authUser.User.Username, authUser.User.UnconfirmedEmailAddress, It.IsAny<Credential>(), false));
888+
889+
GetMock<IMessageService>()
890+
.Verify(x => x.SendNewAccountEmailAsync(
891+
authUser.User,
892+
TestUtility.GallerySiteRootHttps + "account/confirm/" + authUser.User.Username + "/" + authUser.User.EmailConfirmationToken));
893+
894+
ResultAssert.IsSafeRedirectTo(result, "/theReturnUrl");
895+
}
896+
897+
[Fact]
898+
public async Task WillNotAutoConfirmAndWillSendConfirmationEmailWhenModelRegisterEmailAndExternalCredentialEmailNotMatch()
899+
{
900+
// Arrange
901+
var authUser = new AuthenticatedUser(
902+
new User("theUsername")
903+
{
904+
UnconfirmedEmailAddress = "[email protected]",
905+
EmailConfirmationToken = "t0k3n"
906+
},
907+
new Credential());
908+
909+
var externalCred = new CredentialBuilder().CreateExternalCredential("MicrosoftAccount", "blorg", "Bloog");
910+
911+
var authenticationServiceMock = GetMock<AuthenticationService>();
912+
var controller = GetController<AuthenticationController>();
913+
authenticationServiceMock
914+
.Setup(x => x.Register(authUser.User.Username, "[email protected]", externalCred, It.IsAny<bool>()))
915+
.CompletesWith(authUser);
916+
authenticationServiceMock
917+
.Setup(x => x.CreateSessionAsync(controller.OwinContext, authUser, false))
918+
.Returns(Task.FromResult(0))
919+
.Verifiable();
920+
authenticationServiceMock
921+
.Setup(x => x.ReadExternalLoginCredential(controller.OwinContext))
922+
.CompletesWith(new AuthenticateExternalLoginResult()
923+
{
924+
ExternalIdentity = new ClaimsIdentity(),
925+
Credential = externalCred,
926+
UserInfo = new IdentityInformation("", "", "[email protected]", "")
927+
});
928+
929+
// Act
930+
var result = await controller.Register(
931+
new LogOnViewModel()
932+
{
933+
Register = new RegisterViewModel
934+
{
935+
Username = "theUsername",
936+
EmailAddress = "[email protected]",
937+
}
938+
}, "/theReturnUrl", linkingAccount: true);
939+
940+
// Assert
941+
authenticationServiceMock.VerifyAll();
942+
943+
GetMock<AuthenticationService>()
944+
.Verify(x => x.Register(authUser.User.Username, "[email protected]", externalCred, false));
945+
946+
GetMock<IMessageService>()
947+
.Verify(x => x.SendNewAccountEmailAsync(
948+
authUser.User,
949+
TestUtility.GallerySiteRootHttps + "account/confirm/" + authUser.User.Username + "/" + authUser.User.EmailConfirmationToken));
950+
951+
ResultAssert.IsSafeRedirectTo(result, "/theReturnUrl");
952+
}
953+
954+
[Fact]
955+
public async Task WillAutoConfirmAndWillNotSendConfirmationEmailWhenIsExternalCredentialAndModelRegisterEmailAndExternalCredentialEmailMatch()
956+
{
957+
// Arrange
958+
var authUser = new AuthenticatedUser(
959+
new User("theUsername")
960+
{
961+
UnconfirmedEmailAddress = "[email protected]",
962+
EmailConfirmationToken = "t0k3n"
963+
},
964+
new Credential());
965+
966+
var externalCred = new CredentialBuilder().CreateExternalCredential("MicrosoftAccount", "blorg", "Bloog");
967+
968+
var authenticationServiceMock = GetMock<AuthenticationService>();
969+
var controller = GetController<AuthenticationController>();
970+
authenticationServiceMock
971+
.Setup(x => x.Register(authUser.User.Username, authUser.User.UnconfirmedEmailAddress, externalCred, It.IsAny<bool>()))
972+
.CompletesWith(authUser)
973+
.Callback(() =>
974+
{
975+
authUser.User.EmailAddress = authUser.User.UnconfirmedEmailAddress;
976+
authUser.User.UnconfirmedEmailAddress = null;
977+
});
978+
979+
authenticationServiceMock
980+
.Setup(x => x.CreateSessionAsync(controller.OwinContext, authUser, false))
981+
.Returns(Task.FromResult(0))
982+
.Verifiable();
983+
authenticationServiceMock
984+
.Setup(x => x.ReadExternalLoginCredential(controller.OwinContext))
985+
.CompletesWith(new AuthenticateExternalLoginResult()
986+
{
987+
ExternalIdentity = new ClaimsIdentity(),
988+
Credential = externalCred,
989+
UserInfo = new IdentityInformation("", "", authUser.User.UnconfirmedEmailAddress, "")
990+
});
991+
992+
// Act
993+
var result = await controller.Register(
994+
new LogOnViewModel()
995+
{
996+
Register = new RegisterViewModel
997+
{
998+
Username = "theUsername",
999+
EmailAddress = authUser.User.UnconfirmedEmailAddress,
1000+
}
1001+
}, "/theReturnUrl", linkingAccount: true);
1002+
1003+
// Assert
1004+
authenticationServiceMock.VerifyAll();
1005+
1006+
GetMock<AuthenticationService>()
1007+
.Verify(x => x.Register(authUser.User.Username, authUser.User.EmailAddress, externalCred, true));
1008+
8331009
GetMock<IMessageService>()
8341010
.Verify(x => x.SendMessageAsync(
8351011
It.IsAny<NewAccountMessage>(), It.IsAny<bool>(), It.IsAny<bool>()),
@@ -868,7 +1044,7 @@ public async Task GivenExpiredExternalAuth_ItRedirectsBackToLogOnWithExternalAut
8681044
GetMock<AuthenticationService>()
8691045
.Verify(x => x.CreateSessionAsync(It.IsAny<IOwinContext>(), It.IsAny<AuthenticatedUser>(), false), Times.Never());
8701046
GetMock<AuthenticationService>()
871-
.Verify(x => x.Register("theUsername", "theEmailAddress", It.IsAny<Credential>()), Times.Never());
1047+
.Verify(x => x.Register("theUsername", "theEmailAddress", It.IsAny<Credential>(), It.IsAny<bool>()), Times.Never());
8721048
}
8731049

8741050
[Fact]
@@ -888,7 +1064,7 @@ public async Task GivenValidExternalAuth_ItCreatesAccountAndLinksCredential()
8881064
var authenticationServiceMock = GetMock<AuthenticationService>();
8891065
var controller = GetController<AuthenticationController>();
8901066
authenticationServiceMock
891-
.Setup(x => x.Register(authUser.User.Username, authUser.User.UnconfirmedEmailAddress, externalCred))
1067+
.Setup(x => x.Register(authUser.User.Username, authUser.User.UnconfirmedEmailAddress, externalCred, It.IsAny<bool>()))
8921068
.CompletesWith(authUser);
8931069
authenticationServiceMock
8941070
.Setup(x => x.CreateSessionAsync(controller.OwinContext, authUser, false))
@@ -900,7 +1076,8 @@ public async Task GivenValidExternalAuth_ItCreatesAccountAndLinksCredential()
9001076
.CompletesWith(new AuthenticateExternalLoginResult()
9011077
{
9021078
ExternalIdentity = new ClaimsIdentity(),
903-
Credential = externalCred
1079+
Credential = externalCred,
1080+
UserInfo = new IdentityInformation("", "", "", "")
9041081
});
9051082

9061083
// Simulate the model state error that will be added when doing an external account registration (since password is not present)
@@ -959,7 +1136,7 @@ public async Task GivenAdminLogsInWithExternalIdentity_ItChallengesWhenNotUsingR
9591136
externalCred);
9601137

9611138
GetMock<AuthenticationService>()
962-
.Setup(x => x.Register("theUsername", "theEmailAddress", externalCred))
1139+
.Setup(x => x.Register("theUsername", "theEmailAddress", externalCred, It.IsAny<bool>()))
9631140
.CompletesWith(authUser);
9641141

9651142
EnableAllAuthenticators(Get<AuthenticationService>());
@@ -985,7 +1162,8 @@ public async Task GivenAdminLogsInWithExternalIdentity_ItChallengesWhenNotUsingR
9851162
.CompletesWith(new AuthenticateExternalLoginResult()
9861163
{
9871164
ExternalIdentity = new ClaimsIdentity(),
988-
Credential = externalCred
1165+
Credential = externalCred,
1166+
UserInfo = new IdentityInformation("", "", "theEmailAddress", "")
9891167
});
9901168

9911169
// Act

0 commit comments

Comments
 (0)