From 5f88eecceb1f5c6bd641bb9bf74ac943a38f971d Mon Sep 17 00:00:00 2001 From: Christy Henriksson Date: Thu, 26 Oct 2017 15:26:45 -0700 Subject: [PATCH] Gallery constraints to prevent Organization authentication (#4915) --- .../AuditedAuthenticatedOperationAction.cs | 5 ++ .../Authentication/AuthenticationService.cs | 39 +++++++++++++- src/NuGetGallery/ExtensionMethods.cs | 22 -------- src/NuGetGallery/Extensions/UserExtensions.cs | 51 ++++++++++++++++++ src/NuGetGallery/NuGetGallery.csproj | 1 + src/NuGetGallery/Strings.Designer.cs | 9 ++++ src/NuGetGallery/Strings.resx | 3 ++ ...uditedAuthenticatedOperationActionTests.cs | 1 + .../AuthenticationServiceFacts.cs | 52 +++++++++++++++++++ tests/NuGetGallery.Facts/Framework/Fakes.cs | 31 ++++++++++- 10 files changed, 190 insertions(+), 24 deletions(-) create mode 100644 src/NuGetGallery/Extensions/UserExtensions.cs diff --git a/src/NuGetGallery.Core/Auditing/AuditedAuthenticatedOperationAction.cs b/src/NuGetGallery.Core/Auditing/AuditedAuthenticatedOperationAction.cs index 4e5c7ec896..91f6cdc336 100644 --- a/src/NuGetGallery.Core/Auditing/AuditedAuthenticatedOperationAction.cs +++ b/src/NuGetGallery.Core/Auditing/AuditedAuthenticatedOperationAction.cs @@ -19,5 +19,10 @@ public enum AuditedAuthenticatedOperationAction /// Login failed, user exists but password is invalid /// FailedLoginInvalidPassword, + + /// + /// Login failed, user is an organization and should not have credentials. + /// + FailedLoginUserIsOrganization } } \ No newline at end of file diff --git a/src/NuGetGallery/Authentication/AuthenticationService.cs b/src/NuGetGallery/Authentication/AuthenticationService.cs index d40c687750..72eef63c45 100644 --- a/src/NuGetGallery/Authentication/AuthenticationService.cs +++ b/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 AuthenticateInternal(Func @@ -810,6 +845,8 @@ public virtual bool ValidatePasswordCredential(IEnumerable creds, st private async Task MigrateCredentials(User user, List 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, diff --git a/src/NuGetGallery/ExtensionMethods.cs b/src/NuGetGallery/ExtensionMethods.cs index 7d50dfd62d..3fdadeb716 100644 --- a/src/NuGetGallery/ExtensionMethods.cs +++ b/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 SortBy(this IQueryable source, string sortExpr return source.Provider.CreateQuery(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(this HtmlHelper htmlHelper, Expression> expression) { var metadata = ModelMetadata.FromLambdaExpression(expression, htmlHelper.ViewData); @@ -552,17 +541,6 @@ public static User GetCurrentUser(this IOwinContext self) return user; } - /// - /// Get the current API key credential, if available. - /// - 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; diff --git a/src/NuGetGallery/Extensions/UserExtensions.cs b/src/NuGetGallery/Extensions/UserExtensions.cs new file mode 100644 index 0000000000..64555f65e8 --- /dev/null +++ b/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 +{ + /// + /// Extension methods for the NuGetGallery.User entity. + /// + public static class UserExtensions + { + /// + /// Convert a User's email to a System.Net MailAddress. + /// + public static MailAddress ToMailAddress(this User user) + { + if (!user.Confirmed) + { + return new MailAddress(user.UnconfirmedEmailAddress, user.Username); + } + + return new MailAddress(user.EmailAddress, user.Username); + } + + /// + /// Get the current API key credential, if available. + /// + 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); + } + + /// + /// Determines if the User (account) belongs to an organization. + /// + /// User (account) instance. + /// True for organizations, false for users. + public static bool IsOrganization(this User account) + { + return account.Organization != null; + } + } +} \ No newline at end of file diff --git a/src/NuGetGallery/NuGetGallery.csproj b/src/NuGetGallery/NuGetGallery.csproj index ffdb15e446..da11128f26 100644 --- a/src/NuGetGallery/NuGetGallery.csproj +++ b/src/NuGetGallery/NuGetGallery.csproj @@ -772,6 +772,7 @@ + diff --git a/src/NuGetGallery/Strings.Designer.cs b/src/NuGetGallery/Strings.Designer.cs index 8d10befefa..79728b2d71 100644 --- a/src/NuGetGallery/Strings.Designer.cs +++ b/src/NuGetGallery/Strings.Designer.cs @@ -755,6 +755,15 @@ public static string NuGetPackagePropertyTooLong { } } + /// + /// Looks up a localized string similar to Organization accounts cannot create credentials.. + /// + public static string OrganizationsCannotCreateCredentials { + get { + return ResourceManager.GetString("OrganizationsCannotCreateCredentials", resourceCulture); + } + } + /// /// Looks up a localized string similar to Package created from API.. /// diff --git a/src/NuGetGallery/Strings.resx b/src/NuGetGallery/Strings.resx index 83ac2bcab9..17b35f1777 100644 --- a/src/NuGetGallery/Strings.resx +++ b/src/NuGetGallery/Strings.resx @@ -606,4 +606,7 @@ For more information, please contact '{2}'. The user '{0}' does not have permission to remove the owner '{1}'. + + Organization accounts cannot create credentials. + \ No newline at end of file diff --git a/tests/NuGetGallery.Core.Facts/Auditing/AuditedAuthenticatedOperationActionTests.cs b/tests/NuGetGallery.Core.Facts/Auditing/AuditedAuthenticatedOperationActionTests.cs index 51cb5a548c..5c746a1f74 100644 --- a/tests/NuGetGallery.Core.Facts/Auditing/AuditedAuthenticatedOperationActionTests.cs +++ b/tests/NuGetGallery.Core.Facts/Auditing/AuditedAuthenticatedOperationActionTests.cs @@ -14,6 +14,7 @@ public void Definition_HasNotChanged() { "FailedLoginInvalidPassword", "FailedLoginNoSuchUser", + "FailedLoginUserIsOrganization", "PackagePushAttemptByNonOwner" }; diff --git a/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs b/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs index 2669fa18b4..3ade0ad7fe 100644 --- a/tests/NuGetGallery.Facts/Authentication/AuthenticationServiceFacts.cs +++ b/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(); + var authService = Get(); + + // Act & Assert + await Assert.ThrowsAsync(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(); + var credential = new CredentialBuilder().CreatePasswordCredential("password"); + var authService = Get(); + + // Act & Assert + await Assert.ThrowsAsync(async () => { + await authService.AddCredential(fakes.Organization, credential); + }); + } + [Fact] public async Task WritesAuditRecordForTheNewCredential() { diff --git a/tests/NuGetGallery.Facts/Framework/Fakes.cs b/tests/NuGetGallery.Facts/Framework/Fakes.cs index da74024d87..09227086ac 100644 --- a/tests/NuGetGallery.Facts/Framework/Fakes.cs +++ b/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 = "confirmed0@example.com", Credentials = new List { - 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 = "confirmedOrganization@example.com", + Organization = new Organization() + { + Key = 1 + }, + // invalid credentials for testing authentication constraints + Credentials = new List + { + credentialBuilder.CreatePasswordCredential(Password) + } + }; + + Organization.Organization.Memberships = new List() + { + 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; }