dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.36k stars 4.75k forks source link

[API Proposal]: Add AuthenticationHeaderValue constructor accepting AuthenticationSchemes enum #104159

Open LWChris opened 4 months ago

LWChris commented 4 months ago

Background and motivation

APIs that accept a string value to be chosen out of a well-known set of valid values can be accompanied by a static class with const members that provide those string values. An example would be the System.Net.Mime.MediaTypeNames class/subclasses.

Given there is only a handful of valid authentication schemes, it would be user-friendly to augment the AuthenticationHeaderValue constructors as well.

Since the logical name "AuthenticationSchemes" for the static class is already taken by an enum, namely System.Net.AuthenticationSchemes, we cannot provide quite the same pattern of "string API augmented by a collection of relevant constants". But we could still add constructor overloads that accept the enum.

API Proposal

using System.Net;

namespace System.Net.Http.Headers;

public class AuthenticationHeaderValue : ICloneable
{
    public AuthenticationHeaderValue(AuthenticationSchemes scheme);
    public AuthenticationHeaderValue(AuthenticationSchemes scheme, string? parameter);
}

API Usage

using var client = new HttpClient();
client.DefaultRequestHeaders.Authorization = new(AuthenticationSchemes.Basic, authHeaderValue);

Alternative Designs

An alternative would be to create the collection of string constants under a different name.

Risks

Adding constructors with totally new signatures and only types from the same DLL should be risk-free.

dotnet-policy-service[bot] commented 4 months ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

KalleOlaviNiemitalo commented 4 months ago

System.Net.AuthenticationSchemes doesn't match the "HTTP Authentication Schemes" registry at IANA well at all. Only Basic, Digest, and Negotiate are listed in both. Although Ntlm is not in the registry, there is a specification at [MS-NTHT]: NTLM Over HTTP Protocol and the AuthenticationHeaderValue constructor could accept that. The constructor would have to reject Anonymous, and None, and bit combinations like IntegratedWindowsAuthentication.

It seems System.Net.AuthenticationSchemes is a legacy type, used only in HttpListener and WCF.

LWChris commented 4 months ago

Hm, so the proposal doesn't work, and the only real option would be the alternative design with a different class name, such as HttpAuthenticationSchemes for example.

KalleOlaviNiemitalo commented 4 months ago

Re discoverability of the static class, XML documentation of the .NET Framework 4.6.1 reference assemblies includes completionlist elements to let Visual Studio know which type has members for possible values:

    <member name="T:System.Drawing.Pen">
      <summary>Defines an object used to draw lines and curves. This class cannot be inherited.</summary>
      <filterpriority>1</filterpriority>
      <completionlist cref="T:System.Drawing.Pens" />
    </member>

However, reference assemblies of higher versions no longer include those elements; and git grep -e completionlist doesn't find anything from dotnet/runtime, dotnet/winforms, or dotnet/dotnet-api-docs. I get the impression that the completionlist element is no longer in fashion.

julealgon commented 4 months ago

Could the upcoming extension type mechanism be used for cases like this?

One could create an extension type for string and have the constants present in that type (for enhanced discoverability) while at the same time still accepting normal string input like before.

dotnet-policy-service[bot] commented 4 months ago

Tagging subscribers to this area: @dotnet/ncl See info in area-owners.md if you want to be subscribed.

RenderMichael commented 4 months ago

@julealgon Unfortunately the current plan is for extension types to be behind a Preview wall in .NET 9, so it would be unlikely for the BCL to use that feature until v10.

LWChris commented 4 months ago

Re discoverability of the static class, XML documentation of the .NET Framework 4.6.1 reference assemblies includes completionlist elements to let Visual Studio know which type has members for possible values:

    <member name="T:System.Drawing.Pen">
      <summary>Defines an object used to draw lines and curves. This class cannot be inherited.</summary>
      <filterpriority>1</filterpriority>
      <completionlist cref="T:System.Drawing.Pens" />
    </member>

This looks like an interesting concept, but it feels "backwards" for use-cases such as mine. You wouldn't want to annotate System.String with a list of all collections of string constants.

But I could really see value in the <param> doc tag getting an additional attribute or sub-tag to reference a completion list, like so:

/// <summary>
///   Initializes a new instance of the <see cref="System.Net.Mail.Attachment" /> class
///   with the specified content string and MIME type information.
/// </summary>
/// <param name="fileName">
///   A <see cref="System.String" /> that contains the content for this attachment.
/// </param>
/// <param name="mediaType">
///   A <see cref="System.String" /> that contains the MIME Content-Header information for this attachment.
///   This value can be <see langword="null" />.
///   <completionsource>
///     <see cref="System.Net.Mail.MediaTypeNames.Application" />
///     <see cref="System.Net.Mail.MediaTypeNames.Font" />
///     <see cref="System.Net.Mail.MediaTypeNames.Image" />
///     <see cref="System.Net.Mail.MediaTypeNames.Multipart" />
///     <see cref="System.Net.Mail.MediaTypeNames.Text" />
///   </completionsource>
/// </param>
/// <exception cref="System.ArgumentNullException">
///   <paramref name="fileName" /> is <see langword="null" />.
/// </exception>
/// <exception cref="System.FormatException">
///   <paramref name="mediaType" /> is not in the correct format.
/// </exception>
public Attachment (string fileName, string? mediaType)
[...]

But that would just help IntelliSense make clever suggestions which type's fields to suggest as completions. Still leaves us with the question whether we want to create such a constant collection class for authentication schemes.

julealgon commented 4 months ago

@julealgon Unfortunately the current plan is for extension types to be behind a Preview wall in .NET 9, so it would be unlikely for the BCL to use that feature until v10.

That's very unfortunate to hear... I would understand some aspects of extension types to be behind preview flags in the beginning, but not the entire thing... do you happen to have any links to where this decision was documented @RenderMichael ?

RenderMichael commented 4 months ago

@julealgon As with all language decisions, it was documented in the LDM meeting notes. This decision is here: https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-06-12.md#extensions

ManickaP commented 4 months ago

This will certainly not make it in 9.0, putting into future as I still see some questions here about the design.

LWChris commented 4 months ago

Just for clarification @ManickaP:

This will not certainly make it in 9.0

Is this meant to read as

  1. "It is uncertain whether or not it will make it into 9.0", or
  2. "It is certain this will not make it into 9.0", right?

Because I would assume it is 2 (certainly [not make it]), but it currently reads like 1 ([not certainly] make it). 😃