OrchardCore.Cors

[MatthijsKrempel]

Cors module
Fixes #2680

Feedback is welcome

[sebastienros]

/cc @petedavis @jrestall @PinpointTownes @rserj

Feel free to criticize as much as you want, I didn't write it 😄

[jrestall]

Thanks for making the changes, module looks great and will be very useful!

[Skrypt]

I'm going to test this branch ; I want to see how the UI/data is working 😉

[Skrypt]

The UI should be changed in favor of using a sortable to reorder each policy. Each sortable element should be containing the form elements to edit each policy. The "Add policy button" would need to be set underneath that sortable list just like we did with the Predefined List Vue.js component for Text fields. We also could add a radio button to be able to select the default policy to make it more evident than just using the first policy in that list by default (optional). Also, each sortable elements could be collapsible so that we can just see a list of policies by name and if we need to edit it we could expand it to see the actual form details. Also optionally, we could have a "edit data" icon set on top of that list to be able to actually just manually edit/copy/paste the json data and also to be able to change the default policy by typing it's name.

[jtkech]

@Skrypt @MatthijsKrempel

For infos about the distributed cache, based on what i'm working on in the distributed branch.

• Currently IDistributedCache is mainly used with ITagCache so that when a tag is invalidated the related cache entries are invalidated. We will have distributed implementations based on redis for both IDistributedCache and ITagCache, but currently this pattern is mainly used for rendering.

• Another pattern is to use IMemoryCache and ISignal whose implementation will be distributed so that when a signal token has changed local caches are invalidated. Some services, e.g TemplateManager, use this pattern that we adapted (see the distributed branch) by using signal tokens for cache entries.

• Finally, for settings many times we use a warning message saying that the tenant will be restarted on saving, and when updating the settings we restart the tenant. And, because we will have a feature to sync tenants / shells states in a distributed system, this is another way to sync data among instances.

So here, for the settings driver, if possible maybe better to use the last pattern.

[Skrypt]

So remove the IMemoryCache and rework when the warning message displays.

[jtkech]

My suggestion is to do, if possible, as in some other drivers e.g OpenIdValidationSettingsDisplayDriver.

Which uses the following warning in the edit view.

<p class="alert alert-warning">@T["The current tenant will be reloaded when the settings are saved."]</p>

And force the reloading when updating by doing something like this.

            // If the settings are valid, reload the current tenant.
            if (context.Updater.ModelState.IsValid)
            {
                await _shellHost.ReloadShellContextAsync(_shellSettings);
            }

Same suggestion for the Https and ReverseProxy modules.

[MatthijsKrempel]

Sorry for the lack of updates, been really swamped the last couple of months.

Going to pick this up as soon as I get some time.

[agriffard]

Can you please resolve the conflicts ?

[MatthijsKrempel]

@agriffard will do, have to upgrade the UI as well.

[MatthijsKrempel]

@agriffard done ;)

Out of date.

[scleaver]

Yeah... it threw me cause I felt like the first save should have been enough and then I moved onto another screen which of course meant my policy was not there when I went back.

[scleaver]

@MatthijsKrempel - I think you should add policies the same way you add steps to deployment plans. Once you add and save a step you don't have to click save on the deployment plan separately to have the step saved to the plan. Just a thought.

[rserj]

Hello all, any updates on this PR?

[agriffard]

Please merge dev and put the title into the Title zone.

[agriffard]

@MatthijsKrempel Can you please merge dev and finalize the PR?

[jtkech]

LGTM

[jtkech]

@agriffard I did a review, LGTM

But maybe need a review by others for the UI part as many changes were done around UI design

[Skrypt]

If it works. UI can wait. If I remember, there's a lot in this PR about UI that could be done.
Can't block it because of it.

[MatthijsKrempel]

I'm open to suggestions 👍

________________________________ From: Sébastien Ros <[email protected]> Sent: Tuesday, December 29, 2020 9:26:46 PM To: OrchardCMS/OrchardCore <[email protected]> Cc: Matthijs Krempel <[email protected]>; Mention <[email protected]> Subject: Re: [OrchardCMS/OrchardCore] OrchardCore.Cors (#2682) @sebastienros commented on this pull request.

________________________________ In src/OrchardCore.Modules/OrchardCore.Cors/Services/CorsOptionsConfiguration.cs<#2682 (comment)>:

+ {

+ _corsService = corsService; + _logger = logger; + } + + public void Configure(CorsOptions options) + { + var corsSettings = _corsService.GetSettingsAsync().GetAwaiter().GetResult(); + if (corsSettings?.Policies == null || !corsSettings.Policies.Any()) + return; + + foreach (var corsPolicy in corsSettings.Policies) + { + if (corsPolicy.AllowCredentials && corsPolicy.AllowAnyOrigin) + { + _logger.LogWarning($"AllowCredentials and AllowAnyOrigin is considered a security risk, policy {corsPolicy.Name} not loaded"); Bad logging usage — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2682 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAMGIKVWRQTDAQ7VUSYKA4LSXI3QNANCNFSM4GD2AE7A>.

[sebastienros]

Ue data- attribute to pass the value

[MatthijsKrempel]

I can do that, no problem. 👍 Will have a look at all this feedback tomorrow.

________________________________ From: Jean-Thierry Kéchichian <[email protected]> Sent: Tuesday, December 29, 2020 11:49:10 PM To: OrchardCMS/OrchardCore <[email protected]> Cc: Matthijs Krempel <[email protected]>; Mention <[email protected]> Subject: Re: [OrchardCMS/OrchardCore] OrchardCore.Cors (#2682) @jtkech commented on this pull request.

________________________________ In src/OrchardCore.Modules/OrchardCore.Cors/Services/CorsOptionsConfiguration.cs<#2682 (comment)>:

+ {

+ _corsService = corsService; + _logger = logger; + } + + public void Configure(CorsOptions options) + { + var corsSettings = _corsService.GetSettingsAsync().GetAwaiter().GetResult(); + if (corsSettings?.Policies == null || !corsSettings.Policies.Any()) + return; + + foreach (var corsPolicy in corsSettings.Policies) + { + if (corsPolicy.AllowCredentials && corsPolicy.AllowAnyOrigin) + { + _logger.LogWarning($"AllowCredentials and AllowAnyOrigin is considered a security risk, policy {corsPolicy.Name} not loaded"); @MatthijsKrempel<https://github.com/MatthijsKrempel> @sebastienros<https://github.com/sebastienros> means that in place of using string interpolation, the logger allows to pass parameter and parse them efficiently when it is called. _logger.LogWarning("AllowCredentials ... policy {PolicyName} not loaded", corsPolicy.Name); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2682 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAMGIKUYC7K73BFU5RC4ZFLSXJMGNANCNFSM4GD2AE7A>.

[deanmarcussen]

Missing reference to OrchardCore.ResourceManagement busted in production as it won't find the Taghelpers. Easy to fix @scleaver

[hishamco]

Nah no one notice this ;)

[MikeAlhayek]

@MatthijsKrempel Do you remember why a UI for this module was created? I find it kind of odd that we have a CORS UI when the policies created there still need to be consumed in code. If you're already writing code to apply a policy, it seems like defining it in code would make more sense. Maybe a recipe to set up policies would be helpful, but I'm curious—does anyone actually use the UI? Do we even need it?

Thank you