Add RequireHttpsMetadata to ProviderOptions to allow modify Discovery policy RequireHttps value

[alexfdezsauco]

Sample of usage

services.AddOidcAuthentication(
                        options =>
                            {
                                /*...*/
                                options.ProviderOptions.RequireHttpsMetadata = false;
                                /*...*/
                            });

This is a fix for this #321

[Eilon]

Hi @alexanderkyte this seems like a reasonable thing to add.

Tagging @jspuij who wrote this code initially.

@jspuij - any thoughts or concerns about this? The one thing that strikes me is that these options are JSON serializable because it seems they can be retrieved remotely. Is there anything we need to be concerned about when adding another option? Presumably we need to set the right JSON metadata for this new property? Or does this set of JSON data need to match some pre-existing spec, and we shouldn't add new options to it?

[jspuij]

Hi @alexanderkyte this seems like a reasonable thing to add.

Tagging @jspuij who wrote this code initially.

@jspuij - any thoughts or concerns about this? The one thing that strikes me is that these options are JSON serializable because it seems they can be retrieved remotely. Is there anything we need to be concerned about when adding another option? Presumably we need to set the right JSON metadata for this new property? Or does this set of JSON data need to match some pre-existing spec, and we shouldn't add new options to it?

You hit the nail on the head. These options are a selection of the oidc options that the oidc-client javascript library provides. There is support within asp.net core to provide these options from the server when you use API authentication. I tried my best mapping them to the oidc-client .NET library, but not all options are supported this way.

I'm not sure whether adding this boolean and not providing it from the server would actually break deserialization or that we could add a default value through an attribute to solve this.

The whole authentication mechanism is defined in a way though that it is extensible enough that subclassing this options class and the actual service would achieve this without extending the original OidcProviderOptions class.

[Eilon]

I'm not sure whether adding this boolean and not providing it from the server would actually break deserialization or that we could add a default value through an attribute to solve this.

I don't think it would break deserialization; it should just get ignored.

The whole authentication mechanism is defined in a way though that it is extensible enough that subclassing this options class and the actual service would achieve this without extending the original OidcProviderOptions class.

Hmm this would be nice if we don't have to add anything. I guess you derive from the service and override CreateOidcClientFromOptions to do whatever you want?

[jspuij]

I'm not sure whether adding this boolean and not providing it from the server would actually break deserialization or that we could add a default value through an attribute to solve this.

I don't think it would break deserialization; it should just get ignored.

The whole authentication mechanism is defined in a way though that it is extensible enough that subclassing this options class and the actual service would achieve this without extending the original OidcProviderOptions class.

Hmm this would be nice if we don't have to add anything. I guess you derive from the service and override CreateOidcClientFromOptions to do whatever you want?

Correct again 👌 There are overloads in the service registration methods that allow you to register your own TOidcProviderOptions type: https://github.com/dotnet/MobileBlazorBindings/blob/master/src/Microsoft.MobileBlazorBindings.Authentication/MobileBlazorBindingsAuthenticationServiceExtensions.cs

The actual additions for the AuthenticationStateProvider are TryAddScoped calls, so if you register a derived class yourself first, the base class won't be added.

I realize this is pretty advanced stuff for a simple property, but this is the way javier envisioned it, I merely mirrored the original auth design of blazor webassembly.

[Eilon]

Thank you for all the details @jspuij !

Given this, I'm closing the PR and issue. If we get broader feedback in the future that this is a common scenario that we need to address, we will reconsider making this scenario simpler.

@alexfdezsauco - please let us know if you're able to use the suggested workaround or if you'd like any help getting that to work. Thank you very much for the PR and I hope to collaborate more in the future!

[alexfdezsauco]

Since you use OidcProviderOptions instead OidcClientOptions, I added the unique property I needed to solve my issue. But, why don't you pass the "orginal" OidcClientOptions to the AddOidcAuthentication method?

[Eilon]

@jspuij any idea on that?

[alexfdezsauco]

Adding this is also about to simplify the development time requirements. For instance, I use keycloak all the time locally as identity provider, and could be "complex" provide a valid certificate and its not full required for «development purposes». I though that you use a custom provider options for reasons that also include the simplification.

[alexfdezsauco]

... there are overloads in the service registration methods that allow you to register your own TOidcProviderOptions type

Yes, but this is the way how the OidcClient is created:

            return new OidcClient(new OidcClientOptions()
            {
                Authority = oidcProviderOptions.Authority,
                ClientId = oidcProviderOptions.ClientId,
                PostLogoutRedirectUri = oidcProviderOptions.PostLogoutRedirectUri,
                RedirectUri = oidcProviderOptions.RedirectUri,
                ResponseMode = responseMode,
                LoadProfile = false,
                Scope = string.Join(" ", oidcProviderOptions.DefaultScopes),
            });

So, the only parameters in use are those, the others are ignored. The easiest way to solve my development needs was to add a single property.

[alexfdezsauco]

@Eilon This PR is actually the workaround for my issue.

[Eilon]

Right, you could derive from that service, override the CreateOidcClientFromOptions method, call base.CreateOidcClientFromOptions(), then adjust the settings as needed.

But let me think about this some more. Using a self-signed cert during dev perhaps seems reasonably common. Now I'm just trying to think whether the way it's done in the PR is the best way forward (given the serialization issue) or if there's some other way.

[alexfdezsauco]

Using a self-signed cert during dev perhaps seems reasonably common.

Yes, and also have to install it on the phone or the emulator. The self-signed cert is not for my app, is for the identity server. There is a way to do it, but I got an error on the emulator cause for the validation. So, also for development purposes I have to skip the cert validation.

[Eilon]

Dinner time now, but I will revisit this and I hope we can come up with something that everyone likes!

[jspuij]

Since you use OidcProviderOptions instead OidcClientOptions, I added the unique property I needed to solve my issue. But, why don't you pass the "orginal" OidcClientOptions to the AddOidcAuthentication method?

Because then we would lose the ability to communicate the OIDC options from the server through ASP.NET core identity. This is to preserve compatibility with ASP.NET core identity and the WebAssembly variant of OIDC authentication. If this compatibility is deemed unnecessary I'm fine to diverge and remove OidcProviderOptions.

My hope is that as MBB is integrated into dotnet itself that in due time these codebases can be merged together as webassembly is 90% the same code as the MBB version. (which is logical as I based it upon it).

What I would do is this: Verify that the deserialization of OidcPoviderOptions { just generate one before adding the property} is still possible with the added boolean property. If this works, I'm fine with the PR.

We are a bit hindered by the fact that there is zero unit testing in this repo, otherwise I'd ask to write a test to verify and accept the PR.