-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
#808 Add Cors feature #849
Conversation
@rserj, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great start, thanks rserj! I've made a few comments intended to find the most restrictive policy. However, regardless of my comments and even though I like the "config free", automatic setup of the policy based on audiences, I'm leaning more and more towards just having policy names defined in modules and the actual configuration left to appsettings.json:
"Cors": {
"Policies": [
{
"Name": "Orchard.OpenId.OpenIdConnectCorsPolicy",
"Origins": [ "https://my.externaldomain.com" ],
"Methods": [ "GET", "POST", "OPTIONS" ],
"Headers": [ "accept", "content-type", "authorization" ],
"PreflightMaxAge": 86400
},
{
"Name": "Orchard.SomeOtherModule.SomeOtherCorsPolicy",
"Origins": [ "https://my.externaldomain.com" ],
"Methods": [ "GET", "POST", "PATCH", "OPTIONS" ],
"Headers": [ "accept", "content-type", "authorization" ],
"AllowCredentials": true
"PreflightMaxAge": 43200
}
]
}
@@ -24,6 +26,7 @@ | |||
namespace Orchard.OpenId.Controllers | |||
{ | |||
[Authorize] | |||
[EnableCors(Constants.OpenIdConnectPolicy)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way of restricting this to only those actions that may require CORS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, I will apply it per Action which requires CORS, thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @anoordende. We need to make sure CORS is only applied to non-interactive endpoints, which excludes both the authorization and logout endpoints.
Not doing that may represent a security risk (e.g an evil application could send a XHR request to the authorization endpoint with all the authentication cookies automatically appended to the headers, which would be terribad)
.WithOrigins(openIdSettings.Audiences.ToArray()) | ||
.AllowAnyHeader() | ||
.AllowAnyMethod() | ||
.AllowCredentials() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is required for any of the flows, we should consider configuring the policy based on the enabled flows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I didn't think about flows.
If Admin set some external Audience, He assuming that External host can connect to API. However, I missed one case, where Audience will be the same as current host
If you are providing tokens for accessing resources on this site Audiences should be equal to Authority url
Probably we should just check if Audience equals to current Host then ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flows based on redirects don't require Cors for the AccessController
, only for the UserInfo
controller. We are also assuming that every configured App requires UserInfo. So, we'd have to enable/disable Cors at the App level (or for each audience) and have 2 policies: one for the AccessController and another for the UserController. Also, not all flows require AllowCredentials
.
if (openIdSettings.Audiences != null && openIdSettings.Audiences.Any()) | ||
options.AddPolicy(Constants.OpenIdConnectPolicy, builder => builder | ||
.WithOrigins(openIdSettings.Audiences.ToArray()) | ||
.AllowAnyHeader() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most restrictive policy should be applied, ie. only the verbs and headers that are required, something along the lines of GET, POST, OPTIONS and accept, content_type and authorization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, Will do
Cool stuff happening here! 👍 Two remarks, tho':
For these reasons, you should instead consider implementing CORS at the middleware level. /cc @sebastienros |
Thanks @PinpointTownes, great explanation. That does raise a very fundamental question, unless I'm mistaken, this means throughout Orchard we cannot use MVC Cors (they are mutually exclusive). By default, we can only apply a single policy using the middleware Thinking out loud:
And something along the lines of:
The naming of the policies can follow a pattern that identifies the requests to apply each policy on, increasingly more specific (or conversely, more generic), based on Area, Controller, Action. For example, the policy provider would look for a policy for
Lastly, having gone full circle, I'm really not so sure that the OpenId module should generate these policies dynamically. I'd much prefer a single point of truth and very granular control instead of assumptions by modules. |
If we decided stick with Middleware approach, probably we should use MVC RouteTemplate (eg {controller}/{Action}/{arg1}/{arg2}) instead of "Name"? Basicaly it means we are trying implement the same filtration approach which we have in MVC but in middleware level. I see two big conserns
I'm not an export in OpenIddict but I tried "Password flow" with MVC Origin Filtration ([EnableCors]) with Cross Origin requests, I could retrieve Token and Discovery End-points worked as well. |
I just made a quick PoC in one of our internal projects using the This is an alternative approach to the current PR, but if anyone's interested, I can put this in a separate PR over the next few days so we can compare the two, pick & mix and arrive at a solution to move forward with. Simply 👍 or 👎 me here :) |
PS: Sorry @rserj our comments got cross-wired.
|
I added AllowedOrigins Property to OpenId Application and made some changes according to your notes. |
I have one concern here, In case if I made changes to OpenId Application's AllowedOrigins, in order to changes take effect, I have to restart Tenant, I guess I have to Warn about it as we do in global OpenId settings by showing Warning message, what do you think? |
{ | ||
var valueAsString = valueProviderResult.FirstValue; | ||
|
||
if (string.IsNullOrEmpty(valueAsString)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if
could be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed, thanks
&& app.AllowedOrigins != null && app.AllowedOrigins.Any()) | ||
.SelectMany(app => app.AllowedOrigins) | ||
.ToArray(); | ||
if (!appOrigins.Any()) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (appOrigins.Count == 0)
{
return;
}
public void Configure(CorsOptions options) | ||
{ | ||
var openIdApplications = _openIdApplicationStore.GetAllApps().GetAwaiter().GetResult(); | ||
if (openIdApplications == null || !openIdApplications.Any()) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.Count == 0
@@ -22,6 +22,7 @@ | |||
--> | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="..\..\OrchardCore.Modules\Orchard.Cors\Orchard.Cors.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add PrivateAssets="none" here please
Nicholas and Wojciech I made commit regarding to your reviews, thanks |
Sorry, holidays. Will get back to this thread shortly. |
I'll close this PR, better to separate CORS from "OpenId Application" settings, please see discussion in |
Feature allows to make Cross Origin Requests for Open Id, please see #808 ticket for more information