MobileBlazorBindings icon indicating copy to clipboard operation
MobileBlazorBindings copied to clipboard

Add RequireHttpsMetadata to ProviderOptions to allow modify Discovery policy RequireHttps value

Open alexfdezsauco opened this issue 4 years ago • 14 comments
trafficstars

Sample of usage

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

This is a fix for this #321

alexfdezsauco avatar Feb 10 '21 01:02 alexfdezsauco

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?

Eilon avatar Feb 16 '21 22:02 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?

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.

jspuij avatar Feb 16 '21 22:02 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?

Eilon avatar Feb 16 '21 22:02 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?

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.

jspuij avatar Feb 16 '21 22:02 jspuij

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!

Eilon avatar Feb 16 '21 23:02 Eilon

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?

alexfdezsauco avatar Feb 16 '21 23:02 alexfdezsauco

@jspuij any idea on that?

Eilon avatar Feb 16 '21 23:02 Eilon

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 avatar Feb 16 '21 23:02 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 avatar Feb 16 '21 23:02 alexfdezsauco

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

alexfdezsauco avatar Feb 17 '21 00:02 alexfdezsauco

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.

Eilon avatar Feb 17 '21 00:02 Eilon

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.

alexfdezsauco avatar Feb 17 '21 00:02 alexfdezsauco

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

Eilon avatar Feb 17 '21 01:02 Eilon

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.

jspuij avatar Feb 17 '21 07:02 jspuij