Gallery constraints to prevent Organization authentication (#4915)

src/NuGetGallery.Core/Auditing/AuditedAuthenticatedOperationAction.cs

@@ -19,5 +19,10 @@ public enum AuditedAuthenticatedOperationAction
         /// Login failed, user exists but password is invalid
         /// </summary>
         FailedLoginInvalidPassword,
+
+        /// <summary>
+        /// Login failed, user is an organization and should not have credentials.
+        /// </summary>
+        FailedLoginUserIsOrganization
     }
 }

src/NuGetGallery/Authentication/AuthenticationService.cs

@@ -127,6 +127,17 @@ await Auditing.SaveAuditRecordAsync(
                     return new PasswordAuthenticationResult(PasswordAuthenticationResult.AuthenticationResult.BadCredentials);
                 }
 
+                if (user.IsOrganization())
+                {
+                    _trace.Information($"Cannot authenticate organization account'{userNameOrEmail}'.");
+
+                    await Auditing.SaveAuditRecordAsync(
+                        new FailedAuthenticatedOperationAuditRecord(
+                            userNameOrEmail, AuditedAuthenticatedOperationAction.FailedLoginUserIsOrganization));
+
+                    return new PasswordAuthenticationResult(PasswordAuthenticationResult.AuthenticationResult.BadCredentials);
+                }
+
                 int remainingMinutes;
 
                 if (IsAccountLocked(user, out remainingMinutes))
@@ -201,7 +212,21 @@ private async Task<AuthenticatedUser> AuthenticateInternal(Func<Credential, Cred
                     _trace.Information("No user matches credential of type: " + credential.Type);
 
                     await Auditing.SaveAuditRecordAsync(
-                        new FailedAuthenticatedOperationAuditRecord(null, AuditedAuthenticatedOperationAction.FailedLoginNoSuchUser, attemptedCredential: credential));
+                        new FailedAuthenticatedOperationAuditRecord(null,
+                            AuditedAuthenticatedOperationAction.FailedLoginNoSuchUser,
+                            attemptedCredential: credential));
+
+                    return null;
+                }
+
+                if (matched.User.IsOrganization())
+                {
+                    _trace.Information($"Cannot authenticate organization account '{matched.User.Username}'.");
+
+                    await Auditing.SaveAuditRecordAsync(
+                        new FailedAuthenticatedOperationAuditRecord(null,
+                            AuditedAuthenticatedOperationAction.FailedLoginUserIsOrganization,
+                            attemptedCredential: credential));
 
                     return null;
                 }
@@ -465,6 +490,11 @@ public virtual ActionResult Challenge(string providerName, string redirectUrl)
 
         public virtual async Task AddCredential(User user, Credential credential)
         {
+            if  (user.IsOrganization())
+            {
+                throw new InvalidOperationException(Strings.OrganizationsCannotCreateCredentials);
+            }
+
             await Auditing.SaveAuditRecordAsync(new UserAuditRecord(user, AuditedUserAction.AddCredential, credential));
             user.Credentials.Add(credential);
             await Entities.SaveChangesAsync();
@@ -616,6 +646,11 @@ public static ClaimsIdentity CreateIdentity(User user, string authenticationType
 
         private async Task ReplaceCredentialInternal(User user, Credential credential)
         {
+            if (user.IsOrganization())
+            {
+                throw new InvalidOperationException(Strings.OrganizationsCannotCreateCredentials);
+            }
+
             // Find the credentials we're replacing, if any
             var toRemove = user.Credentials
                 .Where(cred =>
@@ -810,6 +845,8 @@ public virtual bool ValidatePasswordCredential(IEnumerable<Credential> creds, st
 
         private async Task MigrateCredentials(User user, List<Credential> creds, string password)
         {
+            // Authenticate already validated that user is not an Organization, so no need to replicate here.
+
             var toRemove = creds.Where(c =>
                 !string.Equals(
                     c.Type,

src/NuGetGallery/ExtensionMethods.cs

@@ -7,7 +7,6 @@
 using System.Globalization;
 using System.Linq;
 using System.Linq.Expressions;
-using System.Net.Mail;
 using System.Security;
 using System.Security.Claims;
 using System.Security.Principal;
@@ -259,16 +258,6 @@ public static IQueryable<T> SortBy<T>(this IQueryable<T> source, string sortExpr
             return source.Provider.CreateQuery<T>(methodCallExpression);
         }
 
-        public static MailAddress ToMailAddress(this User user)
-        {
-            if (!user.Confirmed)
-            {
-                return new MailAddress(user.UnconfirmedEmailAddress, user.Username);
-            }
-
-            return new MailAddress(user.EmailAddress, user.Username);
-        }
-
         public static bool IsError<TModel, TProperty>(this HtmlHelper<TModel> htmlHelper, Expression<Func<TModel, TProperty>> expression)
         {
             var metadata = ModelMetadata.FromLambdaExpression(expression, htmlHelper.ViewData);
@@ -552,17 +541,6 @@ public static User GetCurrentUser(this IOwinContext self)
             return user;
         }
 
-        /// <summary>
-        /// Get the current API key credential, if available.
-        /// </summary>
-        public static Credential GetCurrentApiKeyCredential(this User user, IIdentity identity)
-        {
-            var claimsIdentity = identity as ClaimsIdentity;
-            var apiKey = claimsIdentity.GetClaimOrDefault(NuGetClaims.ApiKey);
-
-            return user.Credentials.FirstOrDefault(c => c.Value == apiKey);
-        }
-
         private static User LoadUser(IOwinContext context)
         {
             var principal = context.Authentication.User;

src/NuGetGallery/Extensions/UserExtensions.cs

@@ -0,0 +1,51 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System.Linq;
+using System.Net.Mail;
+using System.Security.Claims;
+using System.Security.Principal;
+using NuGetGallery.Authentication;
+
+namespace NuGetGallery
+{
+    /// <summary>
+    /// Extension methods for the NuGetGallery.User entity.
+    /// </summary>
+    public static class UserExtensions
+    {
+        /// <summary>
+        /// Convert a User's email to a System.Net MailAddress.
+        /// </summary>
+        public static MailAddress ToMailAddress(this User user)
+        {
+            if (!user.Confirmed)
+            {
+                return new MailAddress(user.UnconfirmedEmailAddress, user.Username);
+            }
+
+            return new MailAddress(user.EmailAddress, user.Username);
+        }
+
+        /// <summary>
+        /// Get the current API key credential, if available.
+        /// </summary>
+        public static Credential GetCurrentApiKeyCredential(this User user, IIdentity identity)
+        {
+            var claimsIdentity = identity as ClaimsIdentity;
+            var apiKey = claimsIdentity.GetClaimOrDefault(NuGetClaims.ApiKey);
+
+            return user.Credentials.FirstOrDefault(c => c.Value == apiKey);
+        }
+
+        /// <summary>
+        /// Determines if the User (account) belongs to an organization.
+        /// </summary>
+        /// <param name="account">User (account) instance.</param>
+        /// <returns>True for organizations, false for users.</returns>
+        public static bool IsOrganization(this User account)
+        {
+            return account.Organization != null;
+        }
+    }
+}

src/NuGetGallery/NuGetGallery.csproj

@@ -772,6 +772,7 @@
     <Compile Include="Controllers\AppController.cs" />
     <Compile Include="Controllers\ErrorsController.cs" />
     <Compile Include="Controllers\PagesController.cs" />
+    <Compile Include="Extensions\UserExtensions.cs" />
     <Compile Include="Filters\ApiScopeRequiredAttribute.cs" />
     <Compile Include="GlobalSuppressions.cs" />
     <Compile Include="Helpers\GravatarHelper.cs" />

src/NuGetGallery/Strings.resx

@@ -606,4 +606,7 @@ For more information, please contact '{2}'.</value>
   <data name="RemoveOwner_NotAllowed" xml:space="preserve">
     <value>The user '{0}' does not have permission to remove the owner '{1}'.</value>
   </data>
+  <data name="OrganizationsCannotCreateCredentials" xml:space="preserve">
+    <value>Organization accounts cannot create credentials.</value>
+  </data>
 </root>

tests/NuGetGallery.Core.Facts/Auditing/AuditedAuthenticatedOperationActionTests.cs

@@ -14,6 +14,7 @@ public void Definition_HasNotChanged()
             {
                 "FailedLoginInvalidPassword",
                 "FailedLoginNoSuchUser",
+                "FailedLoginUserIsOrganization",
                 "PackagePushAttemptByNonOwner"
             };
 

tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs

@@ -67,6 +67,16 @@ public async Task GivenUserNameDoesNotMatchPassword_ItReturnsFailure()
                 Assert.Equal(PasswordAuthenticationResult.AuthenticationResult.BadCredentials, result.Result);
             }
 
+            [Fact]
+            public async Task GivenAnOrganization_ItReturnsFailure()
+            {
+                // Act
+                var result = await _authenticationService.Authenticate(_fakes.Organization.Username, Fakes.Password);
+
+                // Assert
+                Assert.Equal(PasswordAuthenticationResult.AuthenticationResult.BadCredentials, result.Result);
+            }
+
             [Fact]
             public async Task WritesAuditRecordWhenGivenUserNameDoesNotMatchPassword()
             {
@@ -197,6 +207,21 @@ public async Task GivenInvalidApiKeyCredential_ItReturnsNull()
                 Assert.Null(result);
             }
 
+            [Fact]
+            public async Task GivenAnOrganizationApiKeyCredential_ItReturnsNull()
+            {
+                // Arrange
+                var organization = _fakes.Organization;
+                var apiKey = Guid.Parse("1fdc96fe-0b41-4607-bc85-b6533b42d3f8");
+                organization.Credentials.Add(TestCredentialHelper.CreateV2ApiKey(apiKey, TimeSpan.FromDays(1)));
+
+                // Act
+                var result = await _authenticationService.Authenticate(apiKey.ToString());
+
+                // Assert
+                Assert.Null(result);
+            }
+
             [Fact]
             public async Task WritesAuditRecordWhenGivenInvalidApiKeyCredential()
             {
@@ -720,6 +745,19 @@ public async Task ThrowsExceptionIfNoUserWithProvidedUserName()
                 Assert.Equal(Strings.UserNotFound, ex.Message);
             }
 
+            [Fact]
+            public async Task GivenAnOrganization_ThrowsInvalidOperationException()
+            {
+                // Arrange
+                var fakes = Get<Fakes>();
+                var authService = Get<AuthenticationService>();
+
+                // Act & Assert
+                await Assert.ThrowsAsync<InvalidOperationException>(async () => {
+                    await authService.ReplaceCredential("testOrganization", fakes.Organization.Credentials.First());
+                });
+            }
+
             [Fact]
             public async Task AddsNewCredentialIfNoneWithSameTypeForUser()
             {
@@ -1214,6 +1252,20 @@ public async Task AddsTheCredentialToTheDataStore()
                 authService.Entities.VerifyCommitChanges();
             }
 
+            [Fact]
+            public async Task GivenAnOrganization_ThrowsInvalidOperationException()
+            {
+                // Arrange
+                var fakes = Get<Fakes>();
+                var credential = new CredentialBuilder().CreatePasswordCredential("password");
+                var authService = Get<AuthenticationService>();
+
+                // Act & Assert
+                await Assert.ThrowsAsync<InvalidOperationException>(async () => {
+                    await authService.AddCredential(fakes.Organization, credential);
+                });
+            }
+
             [Fact]
             public async Task WritesAuditRecordForTheNewCredential()
             {

tests/NuGetGallery.Facts/Framework/Fakes.cs

@@ -21,13 +21,15 @@ public class Fakes
 
         public Fakes()
         {
+            var credentialBuilder = new CredentialBuilder();
+
             User = new User("testUser")
             {
                 Key = 40,
                 EmailAddress = "[email protected]",
                 Credentials = new List<Credential>
                 {
-                    new CredentialBuilder().CreatePasswordCredential(Password),
+                    credentialBuilder.CreatePasswordCredential(Password),
                     TestCredentialHelper.CreateV1ApiKey(Guid.Parse("669e180e-335c-491a-ac26-e83c4bd31d65"),
                         ExpirationForApiKeyV1),
                     TestCredentialHelper.CreateV2ApiKey(Guid.Parse("779e180e-335c-491a-ac26-e83c4bd31d87"),
@@ -37,6 +39,31 @@ public Fakes()
                 }
             };
 
+            Organization = new User("testOrganization")
+            {
+                Key = 41,
+                EmailAddress = "[email protected]",
+                Organization = new Organization()
+                {
+                    Key = 1
+                },
+                // invalid credentials for testing authentication constraints
+                Credentials = new List<Credential>
+                {
+                    credentialBuilder.CreatePasswordCredential(Password)
+                }
+            };
+
+            Organization.Organization.Memberships = new List<Membership>()
+            {
+                new Membership
+                {
+                    Organization = Organization.Organization,
+                    Member = User,
+                    IsAdmin = true
+                }
+            };
+
             Pbkdf2User = new User("testPbkdf2User")
             {
                 Key = 41,
@@ -90,6 +117,8 @@ public Fakes()
 
         public User User { get; }
 
+        public User Organization { get; }
+
         public User ShaUser { get; }
 
         public User Pbkdf2User { get; }