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

Create OwnerDetailsUriTemplateResourceV3 and provider #5763

Merged
merged 6 commits into from
Apr 27, 2024
Merged

Create OwnerDetailsUriTemplateResourceV3 and provider #5763

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2fe8167
Create Owner Details URI Template resources
donnie-msft Apr 23, 2024
7469b33
Remove public ctor
donnie-msft Apr 24, 2024
ab7626e
Add tests for OwnerDetailsUriResourceV3 and Provider
donnie-msft Apr 24, 2024
f711821
Remove empty Uri test
donnie-msft Apr 24, 2024
0b5db64
Use _template in test
donnie-msft Apr 26, 2024
0e21635
Make resource throw if not found instead of null
donnie-msft Apr 27, 2024
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
39 changes: 39 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/Providers/OwnerDetailsUriResourceV3Provider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// 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.

#nullable enable

using System;
using System.Threading;
using System.Threading.Tasks;
using NuGet.Protocol.Core.Types;
using NuGet.Protocol.Resources;

namespace NuGet.Protocol.Providers
{
/// <summary>NuGet.Protocol resource provider for <see cref="OwnerDetailsUriTemplateResourceV3"/> in V3 HTTP feeds.</summary>
/// <remarks>When successful, returns an instance of <see cref="OwnerDetailsUriTemplateResourceV3"/>.</remarks>
public class OwnerDetailsUriResourceV3Provider : ResourceProvider
{
public OwnerDetailsUriResourceV3Provider()
: base(typeof(OwnerDetailsUriTemplateResourceV3),
nameof(OwnerDetailsUriTemplateResourceV3),
NuGetResourceProviderPositions.Last)
{
}

/// <inheritdoc cref="ResourceProvider.TryCreate(SourceRepository, CancellationToken)"/>
public override async Task<Tuple<bool, INuGetResource?>> TryCreate(SourceRepository source, CancellationToken token)
{
OwnerDetailsUriTemplateResourceV3? resource = null;
ServiceIndexResourceV3? serviceIndex = await source.GetResourceAsync<ServiceIndexResourceV3>(token);
if (serviceIndex != null)
{
Uri uriTemplate = serviceIndex.GetServiceEntryUri(ServiceTypes.OwnerDetailsUriTemplate);
donnie-msft marked this conversation as resolved.
Show resolved Hide resolved
resource = OwnerDetailsUriTemplateResourceV3.CreateOrNull(uriTemplate);
}

return new Tuple<bool, INuGetResource?>(resource != null, resource);
}
}
}
6 changes: 6 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/PublicAPI/net472/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
#nullable enable
NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider
NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider.OwnerDetailsUriResourceV3Provider() -> void
NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3
NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3.GetUri(string! owner) -> System.Uri!
override NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider.TryCreate(NuGet.Protocol.Core.Types.SourceRepository! source, System.Threading.CancellationToken token) -> System.Threading.Tasks.Task<System.Tuple<bool, NuGet.Protocol.Core.Types.INuGetResource?>!>!
static NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3.CreateOrNull(System.Uri? uriTemplate) -> NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3?
~NuGet.Protocol.Core.Types.IPackageSearchMetadata.OwnersList.get -> System.Collections.Generic.IReadOnlyList<string>
~NuGet.Protocol.Core.Types.PackageSearchMetadataBuilder.ClonedPackageSearchMetadata.OwnersList.get -> System.Collections.Generic.IReadOnlyList<string>
~NuGet.Protocol.Core.Types.PackageSearchMetadataBuilder.ClonedPackageSearchMetadata.OwnersList.set -> void
Expand Down
6 changes: 6 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/PublicAPI/netcoreapp5.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
#nullable enable
NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider
NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider.OwnerDetailsUriResourceV3Provider() -> void
NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3
NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3.GetUri(string! owner) -> System.Uri!
override NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider.TryCreate(NuGet.Protocol.Core.Types.SourceRepository! source, System.Threading.CancellationToken token) -> System.Threading.Tasks.Task<System.Tuple<bool, NuGet.Protocol.Core.Types.INuGetResource?>!>!
static NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3.CreateOrNull(System.Uri? uriTemplate) -> NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3?
~NuGet.Protocol.Core.Types.IPackageSearchMetadata.OwnersList.get -> System.Collections.Generic.IReadOnlyList<string>
~NuGet.Protocol.Core.Types.PackageSearchMetadataBuilder.ClonedPackageSearchMetadata.OwnersList.get -> System.Collections.Generic.IReadOnlyList<string>
~NuGet.Protocol.Core.Types.PackageSearchMetadataBuilder.ClonedPackageSearchMetadata.OwnersList.set -> void
Expand Down
6 changes: 6 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
#nullable enable
NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider
NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider.OwnerDetailsUriResourceV3Provider() -> void
NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3
NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3.GetUri(string! owner) -> System.Uri!
override NuGet.Protocol.Providers.OwnerDetailsUriResourceV3Provider.TryCreate(NuGet.Protocol.Core.Types.SourceRepository! source, System.Threading.CancellationToken token) -> System.Threading.Tasks.Task<System.Tuple<bool, NuGet.Protocol.Core.Types.INuGetResource?>!>!
static NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3.CreateOrNull(System.Uri? uriTemplate) -> NuGet.Protocol.Resources.OwnerDetailsUriTemplateResourceV3?
~NuGet.Protocol.Core.Types.IPackageSearchMetadata.OwnersList.get -> System.Collections.Generic.IReadOnlyList<string>
~NuGet.Protocol.Core.Types.PackageSearchMetadataBuilder.ClonedPackageSearchMetadata.OwnersList.get -> System.Collections.Generic.IReadOnlyList<string>
~NuGet.Protocol.Core.Types.PackageSearchMetadataBuilder.ClonedPackageSearchMetadata.OwnersList.set -> void
Expand Down
1 change: 1 addition & 0 deletions src/NuGet.Core/NuGet.Protocol/Repository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public virtual IEnumerable<Lazy<INuGetResourceProvider>> GetCoreV3()
yield return new Lazy<INuGetResourceProvider>(() => new PluginResourceProvider());
yield return new Lazy<INuGetResourceProvider>(() => new RepositorySignatureResourceProvider());
yield return new Lazy<INuGetResourceProvider>(() => new VulnerabilityInfoResourceV3Provider());
yield return new Lazy<INuGetResourceProvider>(() => new OwnerDetailsUriResourceV3Provider());

// Local repository providers
yield return new Lazy<INuGetResourceProvider>(() => new FindLocalPackagesResourceUnzippedProvider());
Expand Down
76 changes: 76 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/Resources/OwnerDetailsUriTemplateResourceV3.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// 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.

#nullable enable

using System;
using NuGet.Protocol.Core.Types;

namespace NuGet.Protocol.Resources
{
/// <summary>Owner Details Uri Template for NuGet V3 HTTP feeds.</summary>
/// <remarks>Not intended to be created directly. Use <see cref="SourceRepository.GetResourceAsync{T}(CancellationToken)"/>
/// with <see cref="OwnerDetailsUriTemplateResourceV3"/> for T, and typecast to this class.
public class OwnerDetailsUriTemplateResourceV3 : INuGetResource
{
private readonly string _template;

private OwnerDetailsUriTemplateResourceV3(string template)
{
_template = template ?? throw new ArgumentNullException(nameof(template));
}

/// <summary>
/// Creates the specified Owner Details Uri template provided by the server if it exists and is valid.
/// </summary>
/// <param name="uriTemplate">The Absolute Uri template provided by the server.</param>
/// <returns>A valid Owner Details Uri template, or null.</returns>
public static OwnerDetailsUriTemplateResourceV3? CreateOrNull(Uri? uriTemplate)
donnie-msft marked this conversation as resolved.
Show resolved Hide resolved
{
if (uriTemplate == null || uriTemplate.OriginalString.Length == 0 || !uriTemplate.IsAbsoluteUri)
{
return null;
}

string absoluteUri = uriTemplate.OriginalString;
if (string.IsNullOrWhiteSpace(absoluteUri)
|| !IsValidUriTemplate(absoluteUri))
{
return null;
}

return new OwnerDetailsUriTemplateResourceV3(absoluteUri);
}

private static bool IsValidUriTemplate(string absoluteUri)
Copy link
Member

Choose a reason for hiding this comment

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

You have a Uri type in the method calling this, so you shouldn't need to pass a string.

I'd just all the validations in 1 method and use the Uri type that's already there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I was trying not to invent any new validation strategy, as mentioned in #5763 (comment), PackageDetailsUriResourceV3 does this custom validation, and it's why I copied that for the OwnerDetailsUriTemplateResourceV3 as well.

I'll see if I can reduce this validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I don't like this any better than what was there, but now I'm throwing if a null is provided, and now must check for null in the Provider. We don't want providers to throw when they can't find the resource, because it's not required for a package source to implement this resource.

{
Uri? uri;
var isValidUri = Uri.TryCreate(absoluteUri, UriKind.Absolute, out uri);
Copy link
Contributor

Choose a reason for hiding this comment

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

We are converting the URI parameter value to a string in the CreateOrNull method, which then calls the IsValidUriTemplate method, passing in that string. I am not sure why we are converting that string again to verify if it is a valid URI. How about just passing the URI object that was passed to the CreateOrNull method instead of creating another URI object here?

Security considerations section in docs suggest that, You can check a URI string for validity by calling the Uri.IsWellFormedOriginalString method. Can this method be used in this case?

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 noticed that method as well, but unfortunately, it returns false because of the braces in the URI template provided by the server resource, as shown below.

My suspicion is this is why the PackageDetailsUriResourceV3 does this custom validation, and it's why I copied that for the OwnerDetailsUriTemplateResourceV3 as well.

If you have other suggestions, let me know. I'm a little hesitant to modify the PackageDetailsUriResourceV3 in this PR, but could consider refactoring it as well separately.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

CreateOrNull method in PackageDetailsUriResourceV3 takes a string as parameter but in this resource the same method has Uri as parameter. Is this could be a reason why we are converting the Uri to string and then string to Uri for validation purposes?

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've reduced some of the validation logic there, have a look and see if that addresses your concern.


// Only allow HTTPS owner details URLs.
if (isValidUri && uri?.Scheme.Equals("https", StringComparison.OrdinalIgnoreCase) != true)
{
return false;
}

return isValidUri;
}

/// <summary>
/// Gets a URL for viewing package Owner URL outside of Visual Studio. The URL will not be verified to exist.
/// </summary>
/// <param name="owner">The owner username.</param>
/// <returns>The first URL from the resource, with the URI template applied.</returns>
public Uri GetUri(string owner)
donnie-msft marked this conversation as resolved.
Show resolved Hide resolved
{
var uriString = _template
#if NETCOREAPP
.Replace("{owner}", owner, StringComparison.OrdinalIgnoreCase);
kartheekp-ms marked this conversation as resolved.
Show resolved Hide resolved
#else
.Replace("{owner}", owner);
#endif

return new Uri(uriString);
}
}
}
2 changes: 2 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/ServiceTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public static class ServiceTypes
public static readonly string Version500 = "/5.0.0";
public static readonly string Version510 = "/5.1.0";
internal const string Version670 = "/6.7.0";
internal const string Version6110 = "/6.11.0";

public static readonly string[] SearchQueryService = { "SearchQueryService" + Versioned, "SearchQueryService" + Version340, "SearchQueryService" + Version300beta };
public static readonly string[] RegistrationsBaseUrl = { $"RegistrationsBaseUrl{Versioned}", $"RegistrationsBaseUrl{Version360}", $"RegistrationsBaseUrl{Version340}", $"RegistrationsBaseUrl{Version300rc}", $"RegistrationsBaseUrl{Version300beta}", "RegistrationsBaseUrl" };
Expand All @@ -29,5 +30,6 @@ public static class ServiceTypes
public static readonly string[] RepositorySignatures = { "RepositorySignatures" + Version500, "RepositorySignatures" + Version490, "RepositorySignatures" + Version470 };
public static readonly string[] SymbolPackagePublish = { "SymbolPackagePublish" + Version490 };
internal static readonly string[] VulnerabilityInfo = { "VulnerabilityInfo" + Version670 };
internal static readonly string[] OwnerDetailsUriTemplate = { "OwnerDetailsUriTemplate" + Version6110 };
}
}
65 changes: 65 additions & 0 deletions ...NuGet.Core.Tests/NuGet.Protocol.Tests/Providers/OwnerDetailsUriResourceV3ProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// 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.

#nullable enable

using System;
using System.Threading;
using System.Threading.Tasks;
using FluentAssertions;
using NuGet.Configuration;
using NuGet.Packaging;
using NuGet.Protocol.Core.Types;
using NuGet.Protocol.Providers;
using Xunit;

namespace NuGet.Protocol.Tests.Providers
{
public class OwnerDetailsUriResourceV3ProviderTests
{
[Fact]
public async Task TryCreate_NoResourceInServiceIndex_ReturnsFalseAsync()
{
// Arrange
var serviceIndexProvider = MockServiceIndexResourceV3Provider.Create();
var target = new OwnerDetailsUriResourceV3Provider();
var providers = new INuGetResourceProvider[] { serviceIndexProvider, target };

PackageSource packageSource = new PackageSource("https://nuget.test/v3/index.json");
SourceRepository sourceRepository = new SourceRepository(packageSource, providers);

// Act
Tuple<bool, INuGetResource?> result = await target.TryCreate(sourceRepository, CancellationToken.None);

// Assert
bool providerHandlesInputSource = result.Item1;
INuGetResource? resource = result.Item2;

providerHandlesInputSource.Should().BeFalse();
resource.Should().BeNull();
}

[Fact]
public async Task TryCreate_ResourceInServiceIndex_ReturnsTrueAsync()
{
// Arrange
var ownerDetailsResourceEntry = new ServiceIndexEntry(new Uri("https://nuget.test/profiles/{owner}?_src=template"), ServiceTypes.OwnerDetailsUriTemplate[0], MinClientVersionUtility.GetNuGetClientVersion());
var serviceIndexProvider = MockServiceIndexResourceV3Provider.Create();
var target = new OwnerDetailsUriResourceV3Provider();
var providers = new INuGetResourceProvider[] { serviceIndexProvider, target };

PackageSource packageSource = new PackageSource("https://nuget.test/v3/index.json");
SourceRepository sourceRepository = new SourceRepository(packageSource, providers);

// Act
Tuple<bool, INuGetResource?> result = await target.TryCreate(sourceRepository, CancellationToken.None);

// Assert
bool providerHandlesInputSource = result.Item1;
INuGetResource? resource = result.Item2;

providerHandlesInputSource.Should().BeFalse();
resource.Should().BeNull();
}
}
}
2 changes: 1 addition & 1 deletion test/NuGet.Core.Tests/NuGet.Protocol.Tests/RepositoryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public void Provider_WithDefaultProvider_ReturnsDefaultResourceProviders()

int actualCount = resourceProviders.Count();

Assert.Equal(47, actualCount);
Assert.Equal(48, actualCount);
}
}
}
119 changes: 119 additions & 0 deletions ...NuGet.Core.Tests/NuGet.Protocol.Tests/Resources/OwnerDetailsUriTemplateResourceV3Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
// 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.

#nullable enable

using System;
using FluentAssertions;
using NuGet.Protocol.Resources;
using Xunit;

namespace NuGet.Protocol.Tests.Resources
{
public class OwnerDetailsUriTemplateResourceV3Tests
{
private readonly Uri _template = new Uri("https://nuget.test/profiles/{owner}?_src=template");
donnie-msft marked this conversation as resolved.
Show resolved Hide resolved

[Fact]
public void CreateOrNull_WhenNullTemplate_CreatesNullResource()
{
// Arrange
Uri? template = null;

// Act
var target = OwnerDetailsUriTemplateResourceV3.CreateOrNull(template);

// Assert
target.Should().BeNull();
}

[Fact]
public void CreateOrNull_WhenTemplateNotAbsoluteUri_CreatesNullResource()
{
// Arrange
var template = new Uri("/owner/profile", UriKind.Relative);

// Act
var target = OwnerDetailsUriTemplateResourceV3.CreateOrNull(template);

// Assert
target.Should().BeNull();
}

[Fact]
public void CreateOrNull_WhenTemplateNotHttps_CreatesNullResource()
{
// Arrange
var template = new Uri("http://nuget.test/profiles/{owner}?_src=template");
donnie-msft marked this conversation as resolved.
Show resolved Hide resolved

// Act
var target = OwnerDetailsUriTemplateResourceV3.CreateOrNull(template);

// Assert
target.Should().BeNull();
}

[Fact]
public void CreateOrNull_WhenValidTemplateHttps_CreatesResource()
{
// Arrange & Act
var target = OwnerDetailsUriTemplateResourceV3.CreateOrNull(_template);

// Assert
target.Should().NotBeNull();
}

[Fact]
public void GetUri_WithSpacesInOwnerParameter_CreatesValidOwnerUriWithEncoding()
{
// Arrange
string owner = "Microsoft Microsoft Microsoft";
string formattedOwner = "Microsoft%20Microsoft%20Microsoft";

var target = OwnerDetailsUriTemplateResourceV3.CreateOrNull(_template);

// Act
Uri ownerUri = target!.GetUri(owner);

// Assert
ownerUri.Should().NotBeNull();
ownerUri.IsAbsoluteUri.Should().BeTrue();
ownerUri.AbsoluteUri.Should().Be($"https://nuget.test/profiles/{formattedOwner}?_src=template");
}

[Theory]
[InlineData("microsoft")]
[InlineData("MiCroSoFT")]
public void GetUri_WithValidOwnerParameter_CreatesValidOwnerUriWithSameCasing(string owner)
{
// Arrange
var target = OwnerDetailsUriTemplateResourceV3.CreateOrNull(_template);

// Act
Uri ownerUri = target!.GetUri(owner);

// Assert
ownerUri.Should().NotBeNull();
ownerUri.IsAbsoluteUri.Should().BeTrue();
ownerUri.AbsoluteUri.Should().Be($"https://nuget.test/profiles/{owner}?_src=template");
}

[Theory]
[InlineData(null)]
[InlineData("")]
public void GetUri_WithInvalidOwnerParameter_ReturnsOriginalTemplate(string owner)
{
// Arrange
var target = OwnerDetailsUriTemplateResourceV3.CreateOrNull(_template);
string templateWithoutOwner = "https://nuget.test/profiles/?_src=template";

// Act
Uri ownerUri = target!.GetUri(owner);

// Assert
ownerUri.Should().NotBeNull();
ownerUri.IsAbsoluteUri.Should().BeTrue();
ownerUri.AbsoluteUri.Should().Be(templateWithoutOwner);
}
}
}