feat(securityscheme): add oauth2MetadataUrl support (OpenAPI 3.2)

[mdaneri]

Pull Request

Description

Adds first-class support for OpenAPI 3.2 oauth2MetadataUrl on OAuth2 security schemes, including serialization and OpenAPI 3.2 reader support.

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Other (please describe):

Related Issue(s)

Fixes #2694

Changes Made

• Add OAuth2MetadataUrl to OpenApiSecurityScheme/IOpenApiSecurityScheme and reference wrapper.
• Serialize oauth2MetadataUrl only for OpenAPI 3.2+ when type: oauth2.
• Parse oauth2MetadataUrl in the OpenAPI 3.2 reader.
• Add unit tests for serialization and parsing.

Testing

• Unit tests added/updated
• Integration tests added/updated
• Manual testing performed
• All existing tests pass

Checklist

• My code follows the code style of this project
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

Versions applicability

• My change applies to the version 1.X of the library, if so PR link:
• My change applies to the version 2.X of the library, if so PR link:
• My change applies to the version 3.X of the library, if so PR link:
• I have evaluated the applicability of my change against the other versions above.

Additional Notes

Unit tests executed locally:

• test/Microsoft.OpenApi.Tests/Models/OpenApiSecuritySchemeTests.cs
• test/Microsoft.OpenApi.Readers.Tests/V32Tests/OpenApiSecuritySchemeTests.cs

[baywet]

Thanks for the contribution!

In addition to the requested change, you'll need to refresh the public API export.

[mdaneri]

Always I forget about that 😓

[mdaneri]

That test looks interesting! I might just borrow it and incorporate it into my project.

[baywet]

That test looks interesting! I might just borrow it and incorporate it into my project.

The dotnet team actually made it an analyzer, so you get feedback on build and not on tests anymore. We just haven't taken the time to update things here. See this as an example.

https://github.com/BinkyLabs/openapi-overlays-dotnet/blob/b9635b903f375498673a3ef801d7d862879e2e3b/src/lib/BinkyLabs.OpenApi.Overlays.csproj#L45

[baywet]

This change is technically source breaking. Even though we recommend not to take a dependency on those interfaces. This still poses a problem with the transitive dependency aspect of things. I.E. if a dependency like aspnetcore.openapi or swashbuckle is accessing other properties in the DOM of type IOpenApiSecurityScheme and have not re-compiled after we ship that change, they'll get exceptions like MethodNotFound/ or that the interface is not implemented by the types that actually implement it.

[baywet]

One way we could try to mitigate this is through Default Interface Implementations, but they are only supported for net core 3.0 onwards. And we currently ship netstandard2.0 and net8.0. We'd need to figure out something for netstandard2.0

[baywet]

And even if we added the default implementation, it'd still remain binary breaking.

[baywet]

This effectively leaves us with the following choices:

• major rev, and the cost associated with supporting v3.
• major rev, and not support v3, only a valid approach as long as net11 does not have support for v3. And we should ensure we don't have any other breaking changes that need to be slated.
• decline this PR, and have an incomplete DOM.
• revert the interface change, but keep the rest. That degrades the experience quite a bit forcing type assertions/casts to access the property. And we'd clean this up in a future major version (OpenAPI 3.3?)
• introduce an additional interface to facilitate the type assertions. (variation of the above)

[baywet]

CC @martincostello in case you have a better suggestion here

[martincostello]

Introduce this via a new interface instead?

Thinking out loud, you could add an IOAuth2MetadataProvider interface:

public interface IOAuth2MetadataProvider
{
    Uri? OAuth2MetadataUrl { get; }
}

Then implement that interface on whatever concrete types need it, and require users to type cast to test for it for implementations that care about it:

if (thing is IOAuth2MetadataProvider oauth2Metadata)
{
    // whatever
}

As already noted, adding it to other interfaces in their inheritance chain is source breaking and DIMs are problematic for TFMs that are too old to support it (i.e. for .NET Framework via netstandard2.0).

You could potentially avoid that by making the new interface conditionally compiled for #if NET and use DIMs, but that would make the API surface non-symmetrical for different TFMs.

[baywet]

Thank you for the additional information.

Yes that was the last option I was listing even though I didn't explicitly document it. Let's go with that then.

@mdaneri, would you mind introducing a new interface please? Additionally please:

• leave a super explicit doc comment this is a "temporary" interface.
• leave a todo comment to remove and collapse the interface with the existing one on the next major version.
• update the doc comment of the existing interface to point to this one. Example with type assertions would be nice as well. So people are not surprised by this pattern, know that's mostly because we (Microsoft) messed up, and know where to find the missing property.

[baywet]

@mdaneri would you mind updating the benchmarks please?

cd performance/benchmark
dotnet run -c Release

(and commit the changes)