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.

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

Conversation

scottbommarito
Copy link
Contributor

With this PR, when an account is transformed into an organization, its API keys are now cloned to the new admin.

This prevents breaking existing CI scenarios upon organization transformation.

public async Task TransferApiKeysScopedToUser(User userWithKeys, User userToOwnKeys)
{
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

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.


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.

@scottbommarito scottbommarito merged commit d0e1ed9 into dev Mar 9, 2018
@scottbommarito scottbommarito deleted the sb-transapi branch March 9, 2018 22:32
@scottbommarito scottbommarito restored the sb-transapi branch July 22, 2019 23:19
@scottbommarito scottbommarito deleted the sb-transapi branch July 22, 2019 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants