Refactor ReverseProxy settings access and cleanup #18805
Conversation
Removed ReverseProxyService in favor of direct ISiteService usage in settings configuration. Updated namespaces for configuration classes, cleaned up Startup registrations, and improved appsettings.json KnownNetworks example. No functional changes beside the admin menu.
hishamco
left a comment
hishamco
left a comment
There was a problem hiding this comment.
I disagree with this PR, unless you change the namespaces for configuration in every module. Same this for the ReverseProxyService
| // "ForwardedHeaders": "None", | ||
| // "KnownNetworks": ["192.168.1.100/4", "192.168.1.101/4"], | ||
| // "KnownProxies": ["192.168.1.200", "192.168.1.201"], | ||
| // "KnownNetworks": ["192.168.1.0/24"], |
There was a problem hiding this comment.
What's th wrong with this example :)
There was a problem hiding this comment.
From a network perspective, this example doesn't make any sense. In addition, the change adjusts the example to match the documentation.
...dCore.Modules/OrchardCore.ReverseProxy/Configuration/ForwardedHeadersOptionsConfiguration.cs
Show resolved
Hide resolved
src/OrchardCore.Modules/OrchardCore.ReverseProxy/Services/ReverseProxyService.cs
Show resolved
Hide resolved
|
I never mind changing the menu location. TBH, I lost too many times in different modules after the menu changes :) |
| .Permission(Permissions.ManageReverseProxySettings) | ||
| .LocalNav() | ||
| ) | ||
| .Add(S["Reverse Proxy"], S["Reverse Proxy"].PrefixPosition(), proxy => proxy |
Before commenting, please look at the other modules. This PR actually aligns the layout to the other modules that have the same behavior concerning appsettings.json. Additionally, the affected types are Concerning the service: For what purpose should that stay? It's just wasting CPU and mental capacity. It has no additional behavior at all. |
|
This is just one sample, if you want more, I will share them :) TBH I'm with you with the naming, but we need to do it everywhere |
|
Thanks, I don't need these examples. I can give you other examples, but I know that OC is not consistent, so that is not an argument. This whole debate is pointless. I only improved this module while we were working on PR #18739, and I don't want to lose even these small improvements. Changes to other modules don't belong in this PR. To make the intent of the affected types clearer, I have now made them internal. |
@gvkries I knew your intent, and as you said, OC is inconsistent in many places, so let's merge this PR and start pushing PRs that align other modules to follow the same cleanup |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
3 participants
Removed
ReverseProxyServicein favor of directISiteServiceusage in settings configuration. Updated namespaces for configuration classes, cleaned up Startup registrations, and improvedappsettings.json KnownNetworksexample. No functional changes beside the admin menu.@MikeAlhayek I struggled to find the reverse proxy in the admin menu multiple times and I think it doesn't make sense below "Security". Reverse proxy is primarily an infrastructure/networking concern, not exclusively a security feature. So I moved it out of security, hope you agree.