Added an OrchardCore Https module #1277
Conversation
| @@ -0,0 +1,29 @@ | |||
| using Microsoft.AspNetCore.Mvc.TagHelpers; | |||
There was a problem hiding this comment.
I assume this spilled in this PR
There was a problem hiding this comment.
I used this tag helper in the settings editor view to disable an input field based on a condition. Maybe this should be put somewhere else, or there is another solution?
There was a problem hiding this comment.
@{
var disabled = Model.IsHttpsRequest ? "disabled" : (string) null;
}
<input disabled="@disabled"/>
|
|
||
| namespace OrchardCore.Https.Services | ||
| { | ||
| public class MvcOptionsHttpsConfiguration : IConfigureOptions<MvcOptions> |
There was a problem hiding this comment.
Maybe I missed this in the comments, but could we not just have a middleware do the redirection instead of relying on MVC?
|
|
||
| public async Task<HttpsSettings> GetSettingsAsync() | ||
| { | ||
| var siteSettings = await _siteService.GetSiteSettingsAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
Remove ConfigureAwait, it's not necessary in ASP.NET Core.
|
@sebastienros now that I have change this. I think this should be refactored into a Rewrite module that the HTTPS module then depends on. That way other rewrite rules could be added, and a single RewriteOptions object can be configure by both modules. |
| <fieldset class="form-group" asp-validation-class-for="RequireHttps"> | ||
| <div class="form-check"> | ||
| <label class="form-check-label"> | ||
| <input asp-for="RequireHttps" class="form-check-input" asp-is-disabled="@(!Model.IsHttpsRequest)" /> |
There was a problem hiding this comment.
Why is it disabled when the current request is https ?
There was a problem hiding this comment.
I shot myself in the foot and clicked enable require https over http when https settings were incorrect. This is only disabled if the current request is NOT https.
There was a problem hiding this comment.
So this way you can only enable it when the settings are correctly configured and working, meaning you are on HTTPS already? That's clever.
| { | ||
| public override void Configure(IApplicationBuilder app, IRouteBuilder routes, IServiceProvider serviceProvider) | ||
| { | ||
| var rewriteOptions = new RewriteOptions(); |
There was a problem hiding this comment.
That looks complicated, maybe you are right but I would have expected it to be simpler. Like just resolving the HttpsSettings here and registering the rewrite rule right there.
There was a problem hiding this comment.
Yeah I thought it would be just like MvcOptions and all the OpenId Options, where any registered IConfigureOptions would be invoked. Maybe a bug on ASP.NET Core Rewrite?
|
That looks better now ... still some questions. Have you seen this issue about adding more security features in OC? #854 Once this SSL one is done, we might be able to add the suggested ones as other features. That would make a nice Security module. |
| <ProjectReference Include="..\..\OrchardCore\OrchardCore.Entities.DisplayManagement\OrchardCore.Entities.DisplayManagement.csproj" /> | ||
| <ProjectReference Include="..\..\OrchardCore\OrchardCore.Environment.Navigation\OrchardCore.Environment.Navigation.csproj" /> | ||
| </ItemGroup> | ||
|
|
There was a problem hiding this comment.
@petedavis just for infos, you need to reference OrchardCore.Module.Targets.csproj to make it working when the module will be packed and referenced as a package.
|
FYI, there will be a middleware for that very soon. It will simplify the code and also add support for HsTs |
|
@sebastienros Should I rename the module to be OrchardCore.Security, and make this the place to add security features mentioned in #854? |
Added a module that provides a configuration wrapper around the MVC RequiresHttps filter.