-
Notifications
You must be signed in to change notification settings - Fork 643
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
ApiKey owner (org) scope UI work #4965
Conversation
|
.OrderBy(i => i) | ||
// Get package owners (user's self or organizations) | ||
var owners = user.Organizations | ||
.Select(o => CreateApiKeyOwnerViewModel(o.Organization, canPushNew: o.IsAdmin)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should determine the possible actions for a scope using the PermissionsService
.
In #4963 I added some more AccountActions
for API actions.
We can add another PermissionsService
method that takes in a Membership
and checks it against an AccountAction
so that performance isn't a concern.
var owners = new [] { CreateApiKeyOwnerViewModel(user, canPushNew: true, canPushVersion: true, canUnlist: true) };
owners.Concat(user.Organizations.Select(membership =>
{
var canPushNew = PermissionsService.IsActionAllowed(membership, AccountActions.ApiPushOnBehalfOf);
var canPushVersion = PermissionsService.IsActionAllowed(membership, AccountActions.ApiPushVersionOnBehalfOf);
var canUnlist = PermissionsService.IsActionAllowed(membership, AccountActions.ApiUnlistOnBehalfOf);
return CreateApiKeyOwnerViewModel(user, canPushNew, canPushVersion, canUnlist);
}));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will integrate w/ PermissionsService in a follow-up PR.
@@ -558,14 +568,24 @@ public virtual ActionResult PasswordChanged() | |||
[Authorize] | |||
[HttpPost] | |||
[ValidateAntiForgeryToken] | |||
public virtual async Task<JsonResult> GenerateApiKey(string description, string[] scopes = null, string[] subjects = null, int? expirationInDays = null) | |||
public virtual async Task<JsonResult> GenerateApiKey(string description, string owner, string[] scopes = null, string[] subjects = null, int? expirationInDays = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change scopes
to be actions
here and elsewhere. It's incredibly confusing that a Scope
is an object that contains an owner, a set of actions, and a list of subjects AND a "scope" is an action that the API key allows on the package. Now we're also referring to the owner as a "scope" too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. The term "scope" is used interchangeably with scope action throughout the code.
I'm ok with fixing, but prefer to do it in a separate PR as I don't want to inflate this one.
}, this); | ||
|
||
// Do not default to push new scope if disabled (organization collaborator) | ||
this.PushNewEnabled = ko.pureComputed(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will probably want to do this for all three possible actions (push new, push version, and unlist) because the PermissionsService
allows for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Push version and unlist are always available, even to organization collaborators. I only need to determine when to enable/disable push new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
always available NOW
in the future they may not be, so we use PermissionsService
to centralize this logic
@@ -174,6 +174,10 @@ | |||
<!-- /ko --> | |||
</li> | |||
</ul> | |||
<!-- ko if: Owner --> | |||
<b>Package owner:</b> <span data-bind="text: Owner"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any thoughts on showing the current user if there is no owner scope?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided that no owner scope means that it can match any, not just the current user. That's why I chose to show "*"... but am open to a different suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh my bad, I will need to update my PR to understand this logic--currently it only uses the current user if an API key has no owner scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to @anangaur, we decided that no owner scope means only match current user
personally, I prefer this because it forces users to use organizations correctly
{ | ||
if (string.IsNullOrWhiteSpace(description)) | ||
{ | ||
Response.StatusCode = (int)HttpStatusCode.BadRequest; | ||
return Json(Strings.ApiKeyDescriptionRequired); | ||
} | ||
|
||
// Get the owner scope | ||
User scopeOwner = string.IsNullOrEmpty(owner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check that the current user actually has the permissions to create an API key with this user as their owner scope. From what I can see here, someone could create an API key with any user as the owner in the scope.
On a similar note, we should check that the user has permissions to create an API key with these scopes (actions) on the owner (e.g. organization collaboration shouldn't have push new).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, client side validation is a "hint" to the user. Server side validation is the real enforcement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding server side validation, but will integrate w/ PermissionsService in follow-up PR since it's under flux right now.
@@ -293,6 +297,23 @@ | |||
</div> | |||
} | |||
</div> | |||
|
|||
@if (Model.PackageOwners.Count > 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Model.PackageOwners.Count == 1
you may want to pass the single option as a hidden item on the form.
It's not needed because you have the GetCurrentUser
call in GenerateApiKey
, but in case some code changes make it possible for the only package owner to be NOT the current user, this is more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can just hide the drop-down and still populate the PackageOwner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/NuGet/NuGetGallery/pull/4963/files#diff-5058b8e804fd7b6e9e60a29de7f6fff6R60 is how I did it for upload/edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified to still populate PackageOwner observable when single owner (self), but will hide the drop-down.
{ | ||
Owner = owner ?? "*"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"*"
-> NuGetPackagePattern.AllInclusivePattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a package pattern, but will add AllInclusivePattern const to the view model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed per @anangaur feedback that legacy keys match only current user.
return self.PackageOwner() && self.PackageOwner().CanPushNew; | ||
}, this); | ||
this.PushNewEnabled.subscribe(function (newValue) { | ||
var defaultPushId = (newValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why parens around newValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
: _userService.FindByUsername(owner); | ||
if (scopeOwner == null) | ||
{ | ||
Response.StatusCode = (int)HttpStatusCode.NotFound; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
404 is a bit weird here. The endpoint is found but the input is invalid. I think this should be a 400.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
User scopeOwner = string.IsNullOrEmpty(owner) | ||
? GetCurrentUser() | ||
: _userService.FindByUsername(owner); | ||
if (scopeOwner == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure -- this code is saying all future API keys must be scoped to a single owner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, API keys will be scoped to a single owner.
We need a final sign-off from all feature owners and @anangaur that:
I keep forgetting our conclusion on these discussions 😊. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⌚️
@joelverhagen |
Both cases are for existing API keys and they should get associated to the account owner's account, UI or otherwise. The spec has this discussed here: https://github.com/NuGet/Home/wiki/Organizations-on-NuGet.org#what-happens-to-authors-existing-api-keys |
@@ -139,7 +154,7 @@ | |||
return id; | |||
}, self); | |||
} | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary whitespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted
@@ -174,6 +174,10 @@ | |||
<!-- /ko --> | |||
</li> | |||
</ul> | |||
<!-- ko if: Owner --> | |||
<b>Package owner:</b> <span data-bind="text: Owner"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to @anangaur, we decided that no owner scope means only match current user
personally, I prefer this because it forces users to use organizations correctly
ce04393
to
207cda5
Compare
@scottbommarito @joelverhagen Addressed feedback. I will integrate it with PermissionsService in a separate PR, to avoid taking dependency for now on #4963. There's still a test I'd like to add for server-side validation, but I'm currently blocked there by a VS bug with test discovery. |
@@ -686,7 +722,32 @@ public virtual ActionResult PasswordChanged() | |||
return newCredential; | |||
} | |||
|
|||
private static IList<Scope> BuildScopes(string[] scopes, string[] subjects) | |||
// todo: integrate verification logic into PermissionsService. | |||
private bool VerifyScopes(User scopeOwner, IEnumerable<Scope> scopes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that when you integrate this with PermissionsService
, this will become a one-liner
scopes.All(s => NuGetScopes.IsActionAllowedOnOwnerByCurrentUser(s.Owner ?? currentUser, currentUser, s.AllowedAction))
I was going to suggest that you modify this method to enumerate through all scopes, but since you'll be changing to use PermissionsService
soon anyway there's no need.
@@ -524,7 +527,11 @@ public virtual CredentialViewModel DescribeCredential(Credential credential) | |||
// Set the description as the value for legacy API keys | |||
Description = credential.Description, | |||
Value = kind == CredentialKind.Token && credential.Description == null ? credential.Value : null, | |||
Scopes = credential.Scopes.Select(s => new ScopeViewModel(s.Subject, NuGetScopes.Describe(s.AllowedAction))).ToList(), | |||
Scopes = credential.Scopes.Select(s => new ScopeViewModel( | |||
scopeOwner.Username, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't cache the value and use it for each scope in the list when you're enumerating through each scope anyway.
s.Owner?.Username ?? credential.User
@@ -70,6 +71,19 @@ public static bool HasOwnerScope(this Scope scope) | |||
|
|||
return scope.OwnerKey.HasValue; | |||
} | |||
|
|||
public static User GetOwnerScope(this IEnumerable<Scope> scopes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest removing this function and using scopes.SingleOrDefault(s => s.Owner)
instead in all the places that you use it for the following reasons.
- it can be reduced into the one-liner above
- it will need to be removed when we allow multiple owners per scope
- its existence makes it very easy to not treat scopes appropriately (e.g. by assuming that the first owner in the scope applies to ALL scopes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, if we eventually allow multiple scope owners, having this in a central location would actually help identify callers that need updating.
src/NuGetGallery/Strings.resx
Outdated
@@ -618,4 +618,7 @@ For more information, please contact '{2}'.</value> | |||
<data name="AccountDelete_Success" xml:space="preserve"> | |||
<value>The account:{0} was deleted succesfully.</value> | |||
</data> | |||
<data name="ApiKeyOwnerRequired" xml:space="preserve"> | |||
<value>Can't generate an API key without a package owner.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must specify a package owner to generate an API key.
might be better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was consistent with ApiKeyDescriptionRequired, but I have no strong preference. Updated.
@@ -20,6 +20,13 @@ public ApiKeyViewModel(CredentialViewModel cred) | |||
throw new ArgumentNullException(nameof(cred)); | |||
} | |||
|
|||
// Currently ApiKeys.cshtml has single Owner per ApiKey restriction. | |||
var owner = cred |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use GetOwnerScope
extension method here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No... code looks the same, but this is querying on view models not entities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b866401
to
a95a650
Compare
Issue #4872.
Work remaining for API key owner (org) scopes: