AzureAD/microsoft-identity-abstractions-for-dotnet

[Bug] [DownstreamApiOptions should be readable entierely from the configuration

jmprieur opened this issue · 1 comments

Which version of Microsoft Identity Abstractions for dotnet are you using?
4.1.0

Repro

DownstreamApiOptions downstreamApiOptions = new DownstreamApiOptions();
builder.Configuration.GetSection("DownstreamApi").Bind(downstreamApiOptions);

Expected behavior
no exception thrown, and the properties can be read from the configuration.

Actual behavior

  1. Exceptions are thrown if the "ProtocolScheme" is not set in the configuration
  2. The HttpMethod property cannot be read from the configuration (there is no converter from string to HttpMethod)

Possible solution

  1. Change the HttpMethod property to be a string, instead of an HttpMethod (same for the backing field _httpMethod). This will be a breaking change (people will have to change their code to use httpMethod.ToString(), for instance HttpMethod.Post.ToString(). This impacts the public API:

    • AuthorizationHeaderProviderOptions.HttpMethod, AuthorizationHeaderProviderOptions._httpMethod which need to move to be a string
    • DownstreamApiOptionsReadOnlyHttpMethod(DownstreamApiOptions options, HttpMethod httpMethod) (the last parameter needs to be a string)
    • DownstreamApiOptionsReadOnlyHttpMethod.HttpMethod that needs to be a string too.
    • the tests (lines such as options.HttpMethod = HttpMethod.Patch; becomes options.HttpMethod = HttpMethod.Patch.ToString();)
  2. Remove the throw new ArgumentNullException in the properties that have a default value (ProtocolScheme that should return "Get", HttpMethod, that should return "Get"). Setting them to null (that is not setting them in the configuration), sets them to their default value.

    DefaultValue("Bearer")]
    public string ProtocolScheme
    {
     get
     {
      return _protocolScheme;
     }
     set
     {
    -  _protocolScheme = string.IsNullOrEmpty(value) ? throw new ArgumentNullException(_protocolScheme) : value;
    + _protocolScheme = string.IsNullOrEmpty(value) ? "Bearer" : value;
      }
    }

    This impacts tests as assigning null to these properties won't throw any longer (they will get a default value)

We'd need to move to Abstractions 5.0.0