Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Organizations] Clone existing API keys to org admin on account transform #5589

Merged
merged 4 commits into from
Mar 9, 2018
Merged

[Organizations] Clone existing API keys to org admin on account transform #5589

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e5e2421
move api keys to organization admin
Mar 9, 2018
4e6f5ce
fix collection was modified
Mar 9, 2018
aeb517c
unit tests
Mar 9, 2018
72cd33a
Merge branch 'dev' into sb-transapi
Mar 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/NuGetGallery/Services/IUserService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ public interface IUserService

Task<bool> TransformUserToOrganization(User accountToTransform, User adminUser, string token);

Task TransferApiKeysScopedToUser(User userWithKeys, User userToOwnKeys);

Task<Organization> AddOrganizationAsync(string username, string emailAddress, User adminUser);
}
}
35 changes: 34 additions & 1 deletion src/NuGetGallery/Services/UserService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Threading.Tasks;
using NuGetGallery.Auditing;
using NuGetGallery.Configuration;
using NuGetGallery.Infrastructure.Authentication;
using NuGetGallery.Security;
using Crypto = NuGetGallery.CryptographyService;

Expand Down Expand Up @@ -46,7 +47,8 @@ public UserService(
IEntitiesContext entitiesContext,
IContentObjectService contentObjectService,
ISecurityPolicyService securityPolicyService,
IDateTimeProvider dateTimeProvider)
IDateTimeProvider dateTimeProvider,
ICredentialBuilder credentialBuilder)
: this()
{
Config = config;
Expand Down Expand Up @@ -370,10 +372,41 @@ public async Task<bool> TransformUserToOrganization(User accountToTransform, Use
{
return false;
}

await TransferApiKeysScopedToUser(accountToTransform, adminUser);

return await EntitiesContext.TransformUserToOrganization(accountToTransform, adminUser, token);
}

public async Task TransferApiKeysScopedToUser(User userWithKeys, User userToOwnKeys)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also do this in MigrateUserToOrganization.sql, which woudl allow you to keep it in the same transaction as the transform.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering that, but given that
1 - I'm less knowledgeable about SQL so this would have been harder for me to write
2 - we're trying to get this change out quickly for the next deployment
3 - if cloning the keys succeeds but transforming the org fails, the worst case scenario is that the admin user has some useless keys
I thought this was fine for now.

{
var eligibleApiKeys = userWithKeys.Credentials
.Where(c => c.IsApiKey() && c.Scopes.All(k => k.Owner == null || k.Owner == userWithKeys)).ToArray();
Copy link
Contributor

@shishirx34 shishirx34 Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can there be api keys for a user with the scope owned by someone else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in case the user has API keys scoped to another user or organization.

E.g.
User A is a member of organization B.
A makes API keys scoped to its own packages.
A makes API keys scoped to B's packages.
A leaves B.
A transforms into an organization with user C as admin.
We copy the API keys that A has that are scoped to its own packages but not B's.

foreach (var originalApiKey in eligibleApiKeys)
{
var scopes = originalApiKey.Scopes.Select(s =>
new Scope(userWithKeys, s.Subject, s.AllowedAction));

var clonedApiKey = new Credential(originalApiKey.Type, originalApiKey.Value)
{
Description = originalApiKey.Description,
ExpirationTicks = originalApiKey.ExpirationTicks,
Expires = originalApiKey.Expires,
Scopes = scopes.ToArray(),
User = userToOwnKeys,
UserKey = userToOwnKeys.Key,
Value = originalApiKey.Value
};

userToOwnKeys.Credentials.Add(clonedApiKey);
}

if (eligibleApiKeys.Any())
{
await EntitiesContext.SaveChangesAsync();
}
}

public async Task<Organization> AddOrganizationAsync(string username, string emailAddress, User adminUser)
{
if (!ContentObjectService.LoginDiscontinuationConfiguration.AreOrganizationsSupportedForUser(adminUser))
Expand Down
202 changes: 194 additions & 8 deletions tests/NuGetGallery.Facts/Services/UserServiceFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Threading.Tasks;
using Moq;
using NuGetGallery.Auditing;
using NuGetGallery.Authentication;
using NuGetGallery.Framework;
using NuGetGallery.Infrastructure.Authentication;
using NuGetGallery.Security;
Expand Down Expand Up @@ -947,8 +949,192 @@ private Task<bool> InvokeTransformUserToOrganization(int affectedRecords, User a
return service.TransformUserToOrganization(account, admin, "token");
}
}

public class TheTransferApiKeysScopedToUserMethod
{
public static IEnumerable<object[]> TransfersApiKeysAsExpected_Data
{
get
{
foreach (var hasExternalCredential in new[] { false, true })
{
foreach (var hasPasswordCredential in new[] { false, true })
{
foreach (var hasUnscopedApiKeyCredential in new[] { false, true })
{
foreach (var hasApiKeyScopedToUserCredential in new[] { false, true })
{
foreach (var hasApiKeyScopedToDifferentUser in new[] { false, true })
{
yield return MemberDataHelper.AsData(
hasExternalCredential,
hasPasswordCredential,
hasUnscopedApiKeyCredential,
hasApiKeyScopedToUserCredential,
hasApiKeyScopedToDifferentUser);
}
}
}
}
}
}
}

[Theory]
[MemberData(nameof(TransfersApiKeysAsExpected_Data))]
public async Task TransfersApiKeysAsExpected(
bool hasExternalCredential,
bool hasPasswordCredential,
bool hasUnscopedApiKeyCredential,
bool hasApiKeyScopedToUserCredential,
bool hasApiKeyScopedToDifferentUser)
{
// Arrange
var originalOwner = new User("originalOwner") { Key = 11111 };
var randomUser = new User("randomUser") { Key = 57576768 };
var newOwner = new User("newOwner") { Key = 69785, Credentials = new List<Credential>() };

var credentials = new List<Credential>();

var externalCredential = TestCredentialHelper.CreateExternalCredential("cred", null);
AddFieldsToCredential(externalCredential, "externalCredential", "value1", originalOwner, expiration: null);

var passwordCredential = TestCredentialHelper.CreateSha1Password("password");
AddFieldsToCredential(passwordCredential, "passwordCredential", "value2", originalOwner, expiration: null);

var unscopedApiKeyCredential = TestCredentialHelper.CreateV4ApiKey(new TimeSpan(5, 5, 5, 5), out var key1);
AddFieldsToCredential(unscopedApiKeyCredential, "unscopedApiKey", "value3", originalOwner, expiration: new DateTime(2018, 3, 9));

var scopedToUserApiKeyCredential = TestCredentialHelper.CreateV4ApiKey(new TimeSpan(5, 5, 5, 5), out var key2)
.WithScopes(new[] { new Scope { Owner = originalOwner, OwnerKey = originalOwner.Key } });
AddFieldsToCredential(scopedToUserApiKeyCredential, "scopedToUserApiKey", "value4", originalOwner, expiration: new DateTime(2018, 3, 10));

var scopedToDifferentUserApiKeyCredential = TestCredentialHelper.CreateV4ApiKey(new TimeSpan(5, 5, 5, 5), out var key3)
.WithScopes(new[] { new Scope { Owner = randomUser, OwnerKey = randomUser.Key } });
AddFieldsToCredential(scopedToDifferentUserApiKeyCredential, "scopedToDifferentUserApiKey", "value5", originalOwner, expiration: new DateTime(2018, 3, 11));

if (hasExternalCredential)
{
credentials.Add(externalCredential);
}

if (hasPasswordCredential)
{
credentials.Add(passwordCredential);
}

if (hasUnscopedApiKeyCredential)
{
credentials.Add(unscopedApiKeyCredential);
}

if (hasApiKeyScopedToUserCredential)
{
credentials.Add(scopedToUserApiKeyCredential);
}

if (hasApiKeyScopedToDifferentUser)
{
credentials.Add(scopedToDifferentUserApiKeyCredential);
}

originalOwner.Credentials = credentials;
var originalCredentialCount = credentials.Count();

var service = new TestableUserService();

// Act
await service.TransferApiKeysScopedToUser(originalOwner, newOwner);

// Assert
service.MockEntitiesContext.Verify(
x => x.SaveChangesAsync(),
hasUnscopedApiKeyCredential || hasApiKeyScopedToUserCredential ? Times.Once() : Times.Never());

Assert.Equal(originalCredentialCount, originalOwner.Credentials.Count());

Assert.Equal(
(hasUnscopedApiKeyCredential ? 1 : 0) + (hasApiKeyScopedToUserCredential ? 1 : 0),
newOwner.Credentials.Count());

AssertCredentialInOriginalOnly(externalCredential, originalOwner, newOwner, hasExternalCredential);
AssertCredentialInOriginalOnly(passwordCredential, originalOwner, newOwner, hasPasswordCredential);
AssertCredentialInOriginalOnly(scopedToDifferentUserApiKeyCredential, originalOwner, newOwner, hasApiKeyScopedToDifferentUser);

AssertCredentialInNew(unscopedApiKeyCredential, originalOwner, newOwner, hasUnscopedApiKeyCredential);
AssertCredentialInNew(scopedToUserApiKeyCredential, originalOwner, newOwner, hasApiKeyScopedToUserCredential);
}

private void AddFieldsToCredential(Credential credential, string description, string value, User originalOwner, DateTime? expiration)
{
credential.Description = description;
credential.Value = value;
credential.User = originalOwner;
credential.UserKey = originalOwner.Key;

if (expiration.HasValue)
{
credential.ExpirationTicks = expiration.Value.Ticks;
credential.Expires = expiration.Value;
}
}

private void AssertCredentialInOriginalOnly(Credential credential, User originalOwner, User newOwner, bool hasCredential)
{
var credentialEquals = CredentialEqualsFunc(credential);
Assert.Equal(hasCredential, originalOwner.Credentials.Any(
hasCredential ? CredentialEqualsWithOwnerFunc(credential, originalOwner) : CredentialEqualsFunc(credential)));
Assert.False(newOwner.Credentials.Any(CredentialEqualsFunc(credential)));
}

private void AssertCredentialInNew(Credential credential, User originalOwner, User newOwner, bool hasCredential)
{
Assert.Equal(hasCredential, originalOwner.Credentials.Any(
hasCredential ? CredentialEqualsWithOwnerFunc(credential, originalOwner) : CredentialEqualsFunc(credential)));
Assert.Equal(hasCredential, newOwner.Credentials.Any(
hasCredential ? CredentialEqualsWithOwnerAndScopeFunc(credential, newOwner, originalOwner) : CredentialEqualsFunc(credential)));
}

private bool CredentialEquals(Credential expected, Credential actual)
{
return
expected.Description == actual.Description &&
expected.ExpirationTicks == actual.ExpirationTicks &&
expected.Expires == actual.Expires &&
expected.Type == actual.Type &&
expected.Value == actual.Value;
}

private bool CredentialEqualsWithOwner(Credential expected, Credential actual, User owner)
{
return CredentialEquals(expected, actual) &&
owner == actual.User &&
owner.Key == actual.UserKey;
}

private bool CredentialEqualsWithOwnerAndScope(Credential expected, Credential actual, User owner, User scopeOwner)
{
return CredentialEqualsWithOwner(expected, actual, owner) &&
expected.Scopes.All(s => s.Owner == scopeOwner && s.OwnerKey == scopeOwner.Key);
}

private Func<Credential, bool> CredentialEqualsFunc(Credential expected)
{
return (c) => CredentialEquals(expected, c);
}

private Func<Credential, bool> CredentialEqualsWithOwnerFunc(Credential expected, User owner)
{
return (c) => CredentialEqualsWithOwner(expected, c, owner);
}

private Func<Credential, bool> CredentialEqualsWithOwnerAndScopeFunc(Credential expected, User owner, User scopeOwner)
{
return (c) => CredentialEqualsWithOwnerAndScope(expected, c, owner, scopeOwner);
}
}

public class TheCreateOrganizationAccountMethod
public class TheAddOrganizationAccountMethod
{
private const string OrgName = "myOrg";
private const string OrgEmail = "[email protected]";
Expand All @@ -962,7 +1148,7 @@ public class TheCreateOrganizationAccountMethod
public async Task WithUserNotSupportedForOrganizations_ThrowsEntityException()
{
SetupOrganizationsSupportedForUser(supported: false);
var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeCreateOrganization());
var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeAddOrganization());
Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.Organizations_NotInDomainWhitelist, AdminName), exception.Message);

_service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny<Organization>()), Times.Never());
Expand All @@ -981,7 +1167,7 @@ public async Task WithUsernameConflict_ThrowsEntityException()

SetupOrganizationsSupportedForUser();

var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeCreateOrganization(orgName: conflictUsername));
var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeAddOrganization(orgName: conflictUsername));
Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.UsernameNotAvailable, conflictUsername), exception.Message);

_service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny<Organization>()), Times.Never());
Expand All @@ -1000,7 +1186,7 @@ public async Task WithEmailConflict_ThrowsEntityException()

SetupOrganizationsSupportedForUser();

var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeCreateOrganization(orgEmail: conflictEmail));
var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeAddOrganization(orgEmail: conflictEmail));
Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.EmailAddressBeingUsed, conflictEmail), exception.Message);

_service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny<Organization>()), Times.Never());
Expand All @@ -1017,7 +1203,7 @@ public async Task WhenAdminHasNoTenant_ThrowsEntityException()

var adminUsername = "adminWithNoTenant";
SetupOrganizationsSupportedForUser(adminUsername);
var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeCreateOrganization(admin: new User(adminUsername)));
var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeAddOrganization(admin: new User(adminUsername)));
Assert.Equal(String.Format(CultureInfo.CurrentCulture, Strings.Organizations_AdminAccountDoesNotHaveTenant, adminUsername), exception.Message);

_service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny<Organization>()), Times.Once());
Expand All @@ -1037,7 +1223,7 @@ public async Task WhenSubscribingToPolicyFails_ThrowsUserSafeException()
.Returns(Task.FromResult(false));
SetupOrganizationsSupportedForUser();

var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeCreateOrganization());
var exception = await Assert.ThrowsAsync<EntityException>(() => InvokeAddOrganization());
Assert.Equal(Strings.DefaultUserSafeExceptionMessage, exception.Message);

_service.MockOrganizationRepository.Verify(x => x.InsertOnCommit(It.IsAny<Organization>()), Times.Once());
Expand All @@ -1057,7 +1243,7 @@ public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg()
.Returns(Task.FromResult(true));
SetupOrganizationsSupportedForUser();

var org = await InvokeCreateOrganization();
var org = await InvokeAddOrganization();

Assert.Equal(OrgName, org.Username);
Assert.Equal(OrgEmail, org.UnconfirmedEmailAddress);
Expand All @@ -1077,7 +1263,7 @@ public async Task WhenSubscribingToPolicySucceeds_ReturnsNewOrg()
_service.MockEntitiesContext.Verify(x => x.SaveChangesAsync(), Times.Once());
}

private Task<Organization> InvokeCreateOrganization(string orgName = OrgName, string orgEmail = OrgEmail, User admin = null)
private Task<Organization> InvokeAddOrganization(string orgName = OrgName, string orgEmail = OrgEmail, User admin = null)
{
// Arrange
admin = admin ?? new User(AdminName)
Expand Down