[2FA] Enforce 2FA for new accounts. (#9035)

* Remove disable-enable 2FA button.

* Enable 2fa by default for new accounts.

* Link or replacing new accounts without 2FA enabled show error.

* Enable 2FA button

* fix current tests.

* Removed unecessary 2fa html.

* nit

* Added new tests for authentication controller.

* feature flag.

* fix enable/disable button logic.

* nit tests.

* fix Register enabling 2fa.

* improve UseAccountChangeExternalCredential and fix tests.

* nit.

* Removed non needed nullable references.

* Added test for enable on controller register method.

* Update EnableMultiFactorAuthentication on creation.

Author: Daniel Jacinto

src/AccountDeleter/EmptyFeatureFlagService.cs

@@ -141,6 +141,11 @@ public bool IsMarkdigMdRenderingEnabled()
             throw new NotImplementedException();
         }
 
+        public bool IsNewAccount2FAEnforcementEnabled()
+        {
+            throw new NotImplementedException();
+        }
+
         public bool IsODataDatabaseReadOnlyEnabled()
         {
             throw new NotImplementedException();

src/GitHubVulnerabilities2Db/Fakes/FakeFeatureFlagService.cs

@@ -274,5 +274,10 @@ public bool IsRecentPackagesNoIndexEnabled()
         {
             throw new NotImplementedException();
         }
+
+        public bool IsNewAccount2FAEnforcementEnabled()
+        {
+            throw new NotImplementedException();
+        }
     }
 }

src/NuGetGallery.Services/Authentication/AuthenticationService.cs

@@ -345,7 +345,7 @@ private async Task<Claim[]> GetUserLoginClaims(AuthenticatedUser user, bool wasM
                     .Where(cred => cred.IsExternal())
                     .Select(cred => cred.Identity)
                     .ToArray();
-                
+
                 var identityList = string.Join(" or ", externalIdentities);
                 ClaimsExtensions.AddExternalCredentialIdentityClaim(claims, identityList);
             }
@@ -365,7 +365,7 @@ private async Task<Claim[]> GetUserLoginClaims(AuthenticatedUser user, bool wasM
             return claims.ToArray();
         }
 
-        public virtual async Task<AuthenticatedUser> Register(string username, string emailAddress, Credential credential, bool autoConfirm = false)
+        public virtual async Task<AuthenticatedUser> Register(string username, string emailAddress, Credential credential, bool autoConfirm = false, bool enableMultiFactorAuthentication = false)
         {
             if (_config.FeedOnlyMode)
             {
@@ -392,7 +392,8 @@ public virtual async Task<AuthenticatedUser> Register(string username, string em
                 UnconfirmedEmailAddress = emailAddress,
                 EmailConfirmationToken = CryptographyService.GenerateToken(),
                 NotifyPackagePushed = true,
-                CreatedUtc = _dateTimeProvider.UtcNow
+                CreatedUtc = _dateTimeProvider.UtcNow,
+                EnableMultiFactorAuthentication = enableMultiFactorAuthentication
             };
 
             // Add a credential for the password
@@ -799,15 +800,15 @@ private async Task ReplaceCredentialInternal(User user, Credential credential)
             Func<Credential, bool> replacingPredicate;
             if (!string.IsNullOrEmpty(replaceCredPrefix))
             {
-                 replacingPredicate = cred => cred.Type.StartsWith(replaceCredPrefix, StringComparison.OrdinalIgnoreCase);
+                replacingPredicate = cred => cred.Type.StartsWith(replaceCredPrefix, StringComparison.OrdinalIgnoreCase);
             }
             else
             {
                 replacingPredicate = cred => cred.Type.Equals(credential.Type, StringComparison.OrdinalIgnoreCase);
             }
 
             var toRemove = user.Credentials
-                .Where(replacingPredicate) 
+                .Where(replacingPredicate)
                 .ToList();
 
             foreach (var cred in toRemove)

src/NuGetGallery.Services/Configuration/FeatureFlagService.cs

@@ -53,6 +53,7 @@ public class FeatureFlagService : IFeatureFlagService
         private const string DisplayTargetFrameworkFeatureName = GalleryPrefix + "DisplayTargetFramework";
         private const string ComputeTargetFrameworkFeatureName = GalleryPrefix + "ComputeTargetFramework";
         private const string RecentPackagesNoIndexFeatureName = GalleryPrefix + "RecentPackagesNoIndex";
+        private const string NewAccount2FAEnforcementFeatureName = GalleryPrefix + "NewAccount2FAEnforcement";
 
         private const string ODataV1GetAllNonHijackedFeatureName = GalleryPrefix + "ODataV1GetAllNonHijacked";
         private const string ODataV1GetAllCountNonHijackedFeatureName = GalleryPrefix + "ODataV1GetAllCountNonHijacked";
@@ -362,5 +363,10 @@ public bool IsRecentPackagesNoIndexEnabled()
         {
             return _client.IsEnabled(RecentPackagesNoIndexFeatureName, defaultValue: false);
         }
+
+        public bool IsNewAccount2FAEnforcementEnabled()
+        {
+            return _client.IsEnabled(NewAccount2FAEnforcementFeatureName, defaultValue: false);
+        }
     }
 }

src/NuGetGallery.Services/Configuration/IFeatureFlagService.cs

@@ -291,5 +291,10 @@ public interface IFeatureFlagService
         /// Whether or not recent packages has no index applied to block search engine indexing.
         /// </summary>
         bool IsRecentPackagesNoIndexEnabled();
+
+        /// <summary>
+        /// Whether or not to enforce 2FA for new external account link or replacement.
+        /// </summary>
+        bool IsNewAccount2FAEnforcementEnabled();
     }
 }

src/NuGetGallery/App_Data/Files/Content/flags.json

@@ -32,6 +32,7 @@
     "NuGetGallery.ImageAllowlist": "Enabled",
     "NuGetGallery.ShowReportAbuseSafetyChanges": "Enabled",
     "NuGetGallery.ComputeTargetFramework": "Enabled",
+    "NuGetGallery.NewAccount2FAEnforcement": "Disabled"
   },
   "Flights": {
     "NuGetGallery.TyposquattingFlight": {

src/NuGetGallery/Controllers/AccountsController.cs

@@ -56,6 +56,8 @@ public class ViewMessages
         protected IIconUrlProvider IconUrlProvider { get; }
 
         protected IGravatarProxyService GravatarProxy { get; }
+        
+        protected IFeatureFlagService FeatureFlagService { get; }
 
         private readonly DeleteAccountListPackageItemViewModelFactory _deleteAccountListPackageItemViewModelFactory;
 
@@ -71,7 +73,8 @@ public AccountsController(
             IMessageServiceConfiguration messageServiceConfiguration,
             IDeleteAccountService deleteAccountService,
             IIconUrlProvider iconUrlProvider,
-            IGravatarProxyService gravatarProxy)
+            IGravatarProxyService gravatarProxy,
+            IFeatureFlagService featureFlagService)
         {
             AuthenticationService = authenticationService ?? throw new ArgumentNullException(nameof(authenticationService));
             PackageService = packageService ?? throw new ArgumentNullException(nameof(packageService));
@@ -85,6 +88,7 @@ public AccountsController(
             DeleteAccountService = deleteAccountService ?? throw new ArgumentNullException(nameof(deleteAccountService));
             IconUrlProvider = iconUrlProvider ?? throw new ArgumentNullException(nameof(iconUrlProvider));
             GravatarProxy = gravatarProxy ?? throw new ArgumentNullException(nameof(gravatarProxy));
+            FeatureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
 
             _deleteAccountListPackageItemViewModelFactory = new DeleteAccountListPackageItemViewModelFactory(PackageService, IconUrlProvider);
         }
@@ -444,6 +448,7 @@ protected virtual void UpdateAccountViewModel(TUser account, TAccountViewModel m
 
             model.IsCertificatesUIEnabled = ContentObjectService.CertificatesConfiguration?.IsUIEnabledForUser(currentUser) ?? false;
             model.WasMultiFactorAuthenticated = User.WasMultiFactorAuthenticated();
+            model.IsNewAccount2FAEnforcementEnabled = FeatureFlagService.IsNewAccount2FAEnforcementEnabled();
 
             model.HasPassword = account.Credentials.Any(c => c.IsPassword());
             model.CurrentEmailAddress = account.UnconfirmedEmailAddress ?? account.EmailAddress;

src/NuGetGallery/Controllers/AuthenticationController.cs

@@ -34,15 +34,12 @@ public partial class AuthenticationController
         : AppController
     {
         private readonly AuthenticationService _authService;
-
         private readonly IUserService _userService;
-
         private readonly IMessageService _messageService;
-
         private readonly ICredentialBuilder _credentialBuilder;
-
         private readonly IContentObjectService _contentObjectService;
         private readonly IMessageServiceConfiguration _messageServiceConfiguration;
+        private readonly IFeatureFlagService _featureFlagService;
         private const string EMAIL_FORMAT_PADDING = "**********";
 
         // Prioritize the external authentication mechanism.
@@ -57,14 +54,16 @@ public AuthenticationController(
             IMessageService messageService,
             ICredentialBuilder credentialBuilder,
             IContentObjectService contentObjectService,
-            IMessageServiceConfiguration messageServiceConfiguration)
+            IMessageServiceConfiguration messageServiceConfiguration,
+            IFeatureFlagService featureFlagService)
         {
             _authService = authService ?? throw new ArgumentNullException(nameof(authService));
             _userService = userService ?? throw new ArgumentNullException(nameof(userService));
             _messageService = messageService ?? throw new ArgumentNullException(nameof(messageService));
             _credentialBuilder = credentialBuilder ?? throw new ArgumentNullException(nameof(credentialBuilder));
             _contentObjectService = contentObjectService ?? throw new ArgumentNullException(nameof(contentObjectService));
             _messageServiceConfiguration = messageServiceConfiguration ?? throw new ArgumentNullException(nameof(messageServiceConfiguration));
+            _featureFlagService = featureFlagService ?? throw new ArgumentNullException(nameof(featureFlagService));
         }
 
         /// <summary>
@@ -295,12 +294,13 @@ public virtual async Task<ActionResult> Register(LogOnViewModel model, string re
                     }
 
                     usedMultiFactorAuthentication = result.LoginDetails?.WasMultiFactorAuthenticated ?? false;
+                    var enableMultiFactorAuthentication = _featureFlagService.IsNewAccount2FAEnforcementEnabled() ? true : usedMultiFactorAuthentication;
                     user = await _authService.Register(
                         model.Register.Username,
                         model.Register.EmailAddress,
                         result.Credential,
-                        (result.Credential.IsExternal() && string.Equals(result.UserInfo?.Email, model.Register.EmailAddress))
-                        );
+                        autoConfirm: (result.Credential.IsExternal() && string.Equals(result.UserInfo?.Email, model.Register.EmailAddress)),
+                        enableMultiFactorAuthentication: enableMultiFactorAuthentication);
                 }
                 else
                 {
@@ -507,6 +507,15 @@ public virtual async Task<ActionResult> LinkOrChangeExternalCredential(string re
                 return SafeRedirect(returnUrl);
             }
 
+            // All new linking or replacing accounts should have 2FA enabled.
+            if (_featureFlagService.IsNewAccount2FAEnforcementEnabled() && !result.UserInfo.UsedMultiFactorAuthentication)
+            {
+                return ChallengeAuthentication(
+                    Url.LinkOrChangeExternalCredential(returnUrl),
+                    result.Authenticator.Name,
+                    new AuthenticationPolicy() { Email = result.LoginDetails.EmailUsed, EnforceMultiFactorAuthentication = true });
+            }
+
             var newCredential = result.Credential;
             if (await _authService.TryReplaceCredential(user, newCredential))
             {
@@ -518,6 +527,16 @@ public virtual async Task<ActionResult> LinkOrChangeExternalCredential(string re
                 var usedMultiFactorAuthentication = result.LoginDetails?.WasMultiFactorAuthenticated ?? false;
                 await _authService.CreateSessionAsync(OwinContext, authenticatedUser, usedMultiFactorAuthentication);
 
+                // Update the 2FA if used during login but user does not have it set on their account.
+                if (result?.LoginDetails != null
+                    && usedMultiFactorAuthentication
+                    && !user.EnableMultiFactorAuthentication
+                    && CredentialTypes.IsExternal(result.Credential))
+                {
+                    await _userService.ChangeMultiFactorAuthentication(user, enableMultiFactor: true, referrer: "Authentication");
+                    OwinContext.AddClaim(NuGetClaims.EnabledMultiFactorAuthentication);
+                }
+
                 // Get email address of the new credential for updating success message
                 var newEmailAddress = GetEmailAddressFromExternalLoginResult(result, out string errorReason);
                 if (!string.IsNullOrEmpty(errorReason))
@@ -627,6 +646,14 @@ await _authService.CreateSessionAsync(OwinContext,
 
                 return SafeRedirect(returnUrl);
             }
+            else if (_featureFlagService.IsNewAccount2FAEnforcementEnabled() && CredentialTypes.IsExternal(result.Credential) && !result.LoginDetails.WasMultiFactorAuthenticated)
+            {
+                // Invoke the authentication again enforcing multi-factor authentication for the same provider.
+                return ChallengeAuthentication(
+                    Url.LinkExternalAccount(returnUrl),
+                    result.Authenticator.Name,
+                    new AuthenticationPolicy() { Email = result.LoginDetails.EmailUsed, EnforceMultiFactorAuthentication = true });
+            }
             else
             {
                 // Gather data for view model
@@ -703,13 +730,15 @@ internal bool ShouldEnforceMultiFactorAuthentication(AuthenticateExternalLoginRe
             // Enforce multi-factor authentication only if:
             // 1. The authenticator supports multi-factor authentication, otherwise no use.
             // 2. The user has enabled multi-factor authentication for their account.
-            // 3. The user authenticated with the personal microsoft account. AAD 2FA policy is controlled by the tenant admins.
-            // 4. The user did not use the multi-factor authentication for the session, obviously.
+            // 3. The user did not use the multi-factor authentication for the session, obviously.
+            // 4. The user authenticated with an external account (currently only MSA and AAD are supported).
+            // 5. If the 2FA enforcement for new accounts is enabled all external account types should be enforced (step 4 validated this).
+            //    If not, only user authenticated with a personal microsoft account is enforced. AAD 2FA policy is controlled by the tenant admins.
             return result.Authenticator.SupportsMultiFactorAuthentication()
                 && result.Authentication.User.EnableMultiFactorAuthentication
                 && !result.LoginDetails.WasMultiFactorAuthenticated
                 && result.Authentication.CredentialUsed.IsExternal()
-                && (CredentialTypes.IsMicrosoftAccount(result.Authentication.CredentialUsed.Type));
+                && (_featureFlagService.IsNewAccount2FAEnforcementEnabled() || CredentialTypes.IsMicrosoftAccount(result.Authentication.CredentialUsed.Type));
         }
 
         private string FormatEmailAddressForAssistance(string email)

src/NuGetGallery/Controllers/OrganizationsController.cs

@@ -48,7 +48,8 @@ public OrganizationsController(
                   messageServiceConfiguration,
                   deleteAccountService,
                   iconUrlProvider,
-                  gravatarProxy)
+                  gravatarProxy,
+                  features)
         {
             _features = features ?? throw new ArgumentNullException(nameof(features));
         }

src/NuGetGallery/Controllers/UsersController.cs

@@ -69,7 +69,8 @@ public UsersController(
                   messageServiceConfiguration,
                   deleteAccountService,
                   iconUrlProvider,
-                  gravatarProxy)
+                  gravatarProxy,
+                  featureFlagService)
         {
             _packageOwnerRequestService = packageOwnerRequestService ?? throw new ArgumentNullException(nameof(packageOwnerRequestService));
             _config = config ?? throw new ArgumentNullException(nameof(config));