OpenId Cross-origin requests

[rserj]

I just wondering how Orchard Open Id handling Cross-origin requests. I tested "Resource Owner Password Credentials" flow. I have to add CORS policy in order to allow make requests from my host, but it is kinda hardcode.

I think Admin should be able to specify "Application Host" while creating "OpenID Connect Application" in Admin tool.
Then each request with clientId/bearToken should be validated for registered "Application Host", if validation get passed server should issue "Access-Control-Allow-Origin" header in response with registered Host, (like Access-Control-Allow-Origin: foo.com)

OpenIddict does not support it out of the box
openiddict/openiddict-core#28
Do we need to implement it by ourself?

[kevinchalet]

Do we need to implement it by ourself?

Yes.

Note: I'm considering adding such support directly in OpenIddict, but if I did, it would be only for the OpenIddict-managed endpoints and you'd have to create your own policy for your own APIs anyway.

[anoordende]

Perhaps this is a different issue altogether, but I ran into a CORS issue with the UserInfo endpoint called from an external SPA.

[anoordende]

I could not find any reference to Cors in O2, so I briefly thought about a possible solution, at least for the UserInfo endpoint but hopefully taking into account the bigger picture:

1. We add an OrchardCore.Modules/Orchard.Cors module that, when enabled, adds the Cors Middleware to the ServicesCollection, together with optionally appsettings-configurable policies.
2. Any module (like Orchard.OpenId) can specify a dependency on the Orchard.Cors module.
3. Add a new option to Orchard.OpenId "Enable CORS" (requires restart),
4. Orchard.OpenId adds an additional policy at startup for the relevant audiences,
5. This policy is declared on the UserInfo controller.

In fact, only step 1 and 5 are really needed to get going, the rest is just some automation.

I'd be happy to put together a PoC for this.

[sebastienros]

If cors is not mandatory for openid, then we can still have a cors module but no dependency. In the openid module then we create a Startup class with a [RequiredFeature] attribute which will then configure and enable some UI only if the cors module is enabled.

[anoordende]

The [RequiredFeature] sounds the way to go. Want me to put a CORS module together or is it already in-progress somewhere?

[jersiovic]

Cool

[sebastienros]

You can start a PR, I assume you need it so you are best at least solve your issues. Then we'll get some comments from Sergio and Kevin. On my side I will probably worry about how/when configuration is done, and flowed to the middleware which is itself quite configurable. Not sure how granular we need to be, and how to expose it (configuration files, site settings). Using controller attributes will then forcibly imply a direct dependency on the feature itself (if a controller uses this attribute, it's feature will enable CORS by default using the manifest).

[rserj]

It is not clear to me what the purpose of Orchard.Cors module will be, Will it expose some abstractions in order to allow to other modules register their Cors policies? Why each module can not just use standard services.UseCors(..) middleware?

[sebastienros]

If each module uses their own middleware, there will be multiple of them running at the same time. Also, the configuration might have to be defined by the user, and not hard coded in a module, that will depend.

Policies can be defined in services, so each feature could define some, but they are registered in the middleware declaration, so it's a dead end.

[rserj]

I will try to make a PR next week, thanks

[kevinchalet]

I'm no longer so sure it would be useful to have OpenID application-specific CORS rules.

I suggest a simpler plan:

• Introduce an OrchardCore.Cors module to centralize the CORS handling.

• Introduce a new ICorsHandler interface and replace DefaultCorsPolicyProvider by an implementation accepting IEnumerable<ICorsHandler> so modules can define their own policies (the problem with CorsMiddleware is that you can only have one ICorsPolicyProvider so we need a new implementation to accept an enumeration of policy providers, ideally order-based).

• Add a path-based OpenID CORS handler: return an empty policy for the interactive endpoints (i.e authorization and logout endpoints) and an allow-all policy for the OpenID API endpoints (these endpoints don't use any form of persistent authentication and don't have side effects so it's reasonably safe).

[rserj]

Introduce a new ICorsHandler interface and replace DefaultCorsPolicyProvider

I guess the problem might be in Origins, they should not be hardcoded. Probably we may create an Orchard Features with ICorsHandlers which may read CORS policies from "Configuration file" AND/OR "Database".

[rserj]

OR we may just leave DefaultCorsPolicyProvider as is, and allow Modules register their own CORS policies by standard way .AddCors + create a feature which will supply CORS policies via DB/Config file

This is the way how I did in my app

[Feature("MyFeature.Cors")]
    public class HovPalCorsConfiguration : IConfigureOptions<CorsOptions>
    {
        private readonly IHttpContextAccessor _httpContextAccessor;

        public HovPalCorsConfiguration(IHttpContextAccessor httpContextAccessor)
        {

            _httpContextAccessor = httpContextAccessor;
        }

        public void Configure(CorsOptions options)
        {
.....
            var corsSettings = corsPolicySettingsStore.GetAllCorsSettingsAsync(CancellationToken.None).GetAwaiter().GetResult().ToList();
            if (corsSettings == null || corsSettings.Count==0) return;
            foreach (var setting in corsSettings)
            {
                if(!setting.Enabled) continue;
                options.AddPolicy(setting.PolicyName, builder =>
                {
                    var policyBuilder = builder
                        .WithOrigins(setting.Origins)
                        .WithMethods(setting.Methods)
                        .WithHeaders(setting.Headers);

                    if (setting.SupportsCredentials)
                        policyBuilder.AllowCredentials();
                    policyBuilder.Build();
                });
            }
            
        }
    }

In code above, in case if "MyFeature.Cors" feature is enabled, I'm reading CORS policies from DB
+I have a Admin UI in order to manage CORS policies. In this approach, we do not need to override DefaultCorsPolicyProvider

[kevinchalet]

In this approach, we do not need to override DefaultCorsPolicyProvider

The problem with this approach is that you can't add path-based policies in CorsOptions, which is something we'll want to do for the OpenID module.

[rserj]

Can you please give an example of path-based CORS? this is not clear to me

[kevinchalet]

@rserj here's a concrete example:

public class OpenIdCorsProvider : ICorsPolicyProvider
{
    private readonly IOptionsMonitor<OpenIddictOptions> _options;

    public OpenIdCorsProvider(IOptionsMonitor<OpenIddictOptions> options)
    {
        _options = options;
    }

    public Task<CorsPolicy> GetPolicyAsync(HttpContext context, string policyName)
    {
        var options = _options.Get(OpenIdConnectServerDefaults.AuthenticationScheme);

        // Note: the authorization and logout endpoints are interactive endpoints that are
        // susceptible of using forms of persistent authentication like Basic, Cookies or
        // Windows Integrated Authentication (e.g NTLM, Kerberos/Negociate). As such, they
        // MUST NOT be included in a CORS policy. To prevent these endpoints from being
        // accidentally included in a CORS policy, an empty policy is returned here.
        if (context.Request.Path.IsEquivalentTo(options.AuthorizationEndpointPath) ||
            context.Request.Path.IsEquivalentTo(options.LogoutEndpointPath))
        {
            return Task.FromResult(new CorsPolicy());
        }

        // Note: the discovery/introspection/revocation/token/userinfo endpoints are
        // non-interactive endpoints that don't use persistent authentication and
        // are not allowed to return any authentication or session cookie. As such,
        // they can be safely included in a lenient allow-all-origin CORS policy.
        if (context.Request.Path.IsEquivalentTo(options.ConfigurationEndpointPath) ||
            context.Request.Path.IsEquivalentTo(options.CryptographyEndpointPath) ||
            context.Request.Path.IsEquivalentTo(options.IntrospectionEndpointPath) ||
            context.Request.Path.IsEquivalentTo(options.RevocationEndpointPath) ||
            context.Request.Path.IsEquivalentTo(options.TokenEndpointPath) ||
            context.Request.Path.IsEquivalentTo(options.UserinfoEndpointPath))
        {
            return Task.FromResult(new CorsPolicy
            {
                Methods = { HttpMethods.Get, HttpMethods.Post },
                Origins = { CorsConstants.AnyOrigin }
            });
        }

        // If the request path doesn't correspond to an OpenID endpoint, return null
        // to indicate that another policy might be selected by another provider.
        return Task.FromResult<CorsPolicy>(null);
    }
}

[kevinchalet]

Note: for internal applications (i.e applications that can't be accessed from Internet), we may want to make this allow-all policy opt-out, in case administrators would be concerned about the OpenID endpoints being directly reachable from a remote site via AJAX.

[rserj]

@PinpointTownes thanks for an example.
As I know Microsoft internally resolves only one instance of ICorsPolicyProvider, it means we have to keep default implementation by deriving from standard implementation
"Microsoft.AspNetCore.Cors.Infrastructure.DefaultCorsPolicyProvider" then delegate calls to the base class (or create a decorator). Anyway If any other custom module decides to override ICorsPolicyProvider they have to take in account that we already overridden it for OpenId unless OpenId Cors will be broken.
It creates not an obvious dependency, so I'm not sure if overriding standard ICorsPolicyProvider implementation is good idea.

[kevinchalet]

Anyway If any other custom module decides to override ICorsPolicyProvider they have to take in account that we already overridden it for OpenId unless OpenId Cors will be broken.

That's why I suggested introducing an ICorsHandler interface and replacing DefaultCorsPolicyProvider by a different implementation that takes an IEnumerable<ICorsHandler>.

It creates not an obvious dependency, so I'm not sure if overriding standard ICorsPolicyProvider implementation is good idea.

If we decide to go with "CORS policies as options" (implemented as an IConfigureOptions<CorsOptions>), we'll likely hit the same issue if the developer decides to override the default policy provider.

Honestly, I have no strong opinion about that. Both options have their pros/cons.

[stantoxt]

Is there any temporary method to deal with this?

[hyzx86]

Is there any update on this issue?

[kevinchalet]

We now have a CORS module that takes care of applying global CORS policies and the interactive/"non-API" OpenID endpoints - like the authorization endpoints - have been decorated with [DisableCors] to ensure a CORS policy is never applied to them.

Closing.