-
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
Added V5 short lived credential type and excluded it from the API key… #10322
base: dev
Are you sure you want to change the base?
Conversation
@@ -25,6 +25,7 @@ public static class ApiKey | |||
public const string V2 = Prefix + "v2"; | |||
public const string V3 = Prefix + "v3"; | |||
public const string V4 = Prefix + "v4"; | |||
public const string V5 = Prefix + "v5"; |
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.
public const string V5 = Prefix + "v5"; | |
public const string V5ShortLived = Prefix + "v5.shortlived"; |
Maybe we can call this shortlived
for now. The V5 format using HISv2 (my draft PR I sent you) could be used for short lived or long-lived API keys in the future. Right now, we are focused on short lived which should be hidden from the UI and we can use the type string to differentiate between them. v5.longlived
could be added later and may not be excluded from the UI.
var apiKeys = $.map(initialData.ApiKeys, function (data) { | ||
return new ApiKeyViewModel(self, initialData.PackageOwners, data); |
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.
This filtering is done on the client side. This could work, but I think another option that is more consistent with existing code is filtering on the server side. It looks like we already filter out the verify.v1
type already.
See
NuGetGallery/src/NuGetGallery/Controllers/UsersController.cs
Lines 1192 to 1202 in b55d335
private Dictionary<CredentialKind, List<CredentialViewModel>> GetCredentialGroups(User user) | |
{ | |
return user | |
.Credentials | |
.Where(CredentialTypes.IsViewSupportedCredential) | |
.OrderByDescending(c => c.Created) | |
.ThenBy(c => c.Description) | |
.Select(AuthenticationService.DescribeCredential) | |
.GroupBy(c => c.Kind) | |
.ToDictionary(g => g.Key, g => g.ToList()); | |
} |
which calls
NuGetGallery/src/NuGetGallery.Core/CredentialTypes.cs
Lines 86 to 91 in b55d335
public static bool IsViewSupportedCredential(this Credential credential) | |
{ | |
return | |
SupportedCredentialTypes.Any(credType => credential.IsType(credType)) || | |
credential.IsExternal(); | |
} |
…s page
Summary of the changes (in less than 80 characters):
Addresses #123