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

Added V5 short lived credential type and excluded it from the API key… #10322

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Added V5 short lived credential type and excluded it from the API key… #10322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions src/NuGetGallery.Core/CredentialTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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;
Expand All @@ -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";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.

public const string VerifyV1 = Prefix + "verify.v1";
}

Expand Down Expand Up @@ -63,7 +64,8 @@ public static bool IsType(this Credential c, string type)
ApiKey.V1,
ApiKey.V2,
ApiKey.V3,
ApiKey.V4
ApiKey.V4,
ApiKey.V5
};

/// <summary>
Expand Down
7 changes: 6 additions & 1 deletion src/NuGetGallery/Scripts/gallery/page-api-keys.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,8 +646,13 @@
function ApiKeyListViewModel(initialData) {
var self = this;

// Excluding v5
var typeToOmit = "apikey.v5";

var apiKeys = $.map(initialData.ApiKeys, function (data) {
return new ApiKeyViewModel(self, initialData.PackageOwners, data);
Copy link
Member

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();
}

if (data.Type !== typeToOmit) {
return new ApiKeyViewModel(self, initialData.PackageOwners, data);
}
});
var newApiKey = new ApiKeyViewModel(self, initialData.PackageOwners);

Expand Down