URL Rewriting Module

[arkadiuszwojcik]

This PR partially addresses issue #9027. It introduces a new Rewrite module with, as of now, basic functionality that allows providing rewrite/redirect rules using Apache mod_rewrite syntax. This functionality is very basic but also very flexible, providing a good starting point for more features that might be requested in the future.

Few remaining questions are:

1. Is proposed name good? Maybe it should be: OrchardCore.Rewrites or OrchardCore.Rewriter
2. I am not sure about module priority, currently it is: InfrastructureService
3. Looks like RewriteRules have some limitations regarding absolute addresses. I created issue related to that in aspnetcore project.

[MikeAlhayek]

@arkadiuszwojcik Thank you for this PR.

I am not a fan of the suggested UI. I think it would be much better and user friendly to have a UI similarity to the Tenants, Features, or Notifications UI where user can add a new rule "as a new entry" form.

Also add a way to edit one rule at a time. I also think it would be better to prompt the user with at the following fields:

• Title
• Description
• Pattern
• Replacement
• SkipRemainingRules.

Or the following fields:

• Title
• Description
• Pattern
• SkipRemainingRules.

This way the user can provide meaning title/description for what the rules should actually do.

And even better, you can then easily implement sorting on the UI where one can drag/drop the entries which will change the order of the entries in the options.

Updated

I would rename the module to UrlRewriting

Also, how would this work with other tenants?

For example, if every tenant on my setup uses a subsdomain, can one tenant put a rule that would redirect all of the traffic to their instance instead?

.AddRedirect("^(?!www\\.).*", "https://site1.example.com", statusCode: StatusCodes.Status301MovedPermanently)

Or of every tenant is setup using a prefix, can I just write a rule that would redirect all other tenants to my instances?

.AddRedirect(@"^(.+)/$", "/site1/")

Please confirm that the two rules I shared above, are only applicable on one tenant only and will not impact other tenants.

[arkadiuszwojcik]

@MikeAlhayek, thank you for your comments. Regarding the UI, I think the module should ultimately support both types of URL rewrites: UI-based and text-based. Text-based is more advanced because RewriteRule can be preceded by multiple RewriteCond, which allow for numerous checks based on variables like Cookie, DateTime, UserAgent, etc. These checks are complex to model in a graphical UI. I suggest having two tabs: one for "Simple Mode" with a user-friendly interface, and another for "Advanced Mode". In this PR I just added only advance one.

Regarding multitenancy, the URL rewrite middleware is registered separately for each tenant (each tenant has its own IApplicationBuilder instance), so when a request hits the rewrite middleware, it has already been routed to the proper tenant by OC. I tested a simple case with two tenants to verify that the rewrite rules from one do not affect the other. With this it is impossible to intercept and redirect traffic from other tenant.

[MikeAlhayek]

@arkadiuszwojcik yes I understand. You can also model your UI just like Queries UI. This UI allows you to add different types of queries. In your case you can provide the advance rule, but will allow another person to also define a different rule.

Yea the renaming should work as long as the middleware is added after the tenant middle ware.

[arkadiuszwojcik]

Regarding the module name, what do you think about 'UrlRewrites' instead of 'UrlRewriting'? To me, it seems much more aligned with the current module names, although I might be wrong.

[MikeAlhayek]

Regarding the module name, what do you think about 'UrlRewrites' instead of 'UrlRewriting'? To me, it seems much more aligned with the current module names, although I might be wrong.

Personally, I would go with UrlRewriting.

[hishamco]

Is there any reason not to use the URL Rewrite module available in ASP.NET Core? Regardless the issue that you mentioned

[arkadiuszwojcik]

Is there any reason not to use the URL Rewrite module available in ASP.NET Core? Regardless the issue that you mentioned

Can you elaborate? This OC module is using ASP.NET Core Rewrite module internally, it just expose way to provide rewrite rules in OC admin menu.

Update:
@hishamco ASP.NET Rewrite middleware allows to define rewrite/redirect rules in few text formats: Apache and IIS, I chosen to use Apache as more popular.

[hishamco]

Good, also I have another module on top of my head that might be related URL redirection

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[MikeAlhayek]

just to elaborate on query strings issue: maybe we should consider changing the name to something more descriptive. When creating a substitution URL (for rewrite or redirect), it can contain a query string. The question is: what should we do with the query string from the original request? Should we append it to substitution url with new query string or drop it? In both cases operation that have to be completed is the same, this is way I think common name for such operation is more intuitive.

I am okay with changing it from a checkbox to a drop down menu with multiple options "Exclude Query String from Original Request", "Append Query String from Original Request"

Regarding the order of rules in the Recipe, I still believe it's much safer and more desirable to place the new rules (without explicit order) at the very end. Otherwise, they might break the current behavior if there are already some rules defined. If the user believes such rules should have higher priority, he should explicitly specify the priority in the recipe or manually adjust it later in the UI.

I suggest that you try to import the above recipe couple times so you can see the behavior in place which I think it's the same of what you are describing. What I expect to happen is when a new rule being imported via recipe, we look to see the largest order value and we add 1 to it.

So if you run this recipe "with no existing rules"

{
  "steps": [
    {
      "name": "UrlRewriting",
      "Rules": [
        {
          "Source": "Redirect",
          "Name": "Rule 1",
          "Pattern": "^/about-us$",
          "SubstitutionPattern": "/about",
          "IsCaseInsensitive": true,
          "AppendQueryString": false,
          "RedirectType": "MovedPermanently"
        },
        {
          "Source": "Rewrite",
          "Name": "Rule 2",
          "Pattern": "^/img/(.*)$",
          "SubstitutionPattern": "/media/$1",
          "IsCaseInsensitive": true,
          "IgnoreQueryString": false,
          "SkipFurtherRules": true
        }
      ]
    }
  ]
}

Then run this recipe again from a second request

{
  "steps": [
    {
      "name": "UrlRewriting",
      "Rules": [
        {
          "Source": "Redirect",
          "Name": "Rule 3",
          "Pattern": "^/about-us$",
          "SubstitutionPattern": "/about",
          "IsCaseInsensitive": true,
          "AppendQueryString": false,
          "RedirectType": "MovedPermanently"
        },
        {
          "Source": "Rewrite",
          "Name": "Rule 4",
          "Pattern": "^/img/(.*)$",
          "SubstitutionPattern": "/media/$1",
          "IsCaseInsensitive": true,
          "IgnoreQueryString": false,
          "SkipFurtherRules": true
        }
      ]
    }
  ]
}

The order will be at follow:

1. Rule 1 << Order 0
2. Rule 2 << Order 0
3. Rule 3 << Order 1
4. Rule 4 << Order 1

As you can see the order will be 0 for the first 2 rules (from the first recipe run). But 1 comes before 2 because of the create data is slightly smaller.

So by default we add to the bottom of the list "newer to the end". But if you want to control the behavior, then you would specify a different order from the recipe.

[arkadiuszwojcik]

The drop-down menu idea sounds good; it expresses the intent more clearly.

Regarding the order, yeah, I know it's sorted by order first and then by date. But in the example above, we're relying on knowing how many rules already exist to assign the correct order numbers. In most cases, though, we don’t know how many rules aleready exist or what their numbers are if we execute some non bootrstrap recipe. So, the priority numbers end up being pretty arbitrary, which can lead to new rules showing up at the beginning, the end, or randomly. It’s much safer to assume that new rules should go at the very end. Such assumption is used in the UI when creating a new rule. Having the UI suggest the highest order number for a new rule while the recipe suggests a lower number feels odd.

[MikeAlhayek]

The drop-down menu idea sounds good; it expresses the intent more clearly.

Feel free to implement this.

But in the example above, we're relying on knowing how many rules already exist to assign the correct order numbers.

Not really the case. We don't do a count. Instead of fix the largest Order available and then add one to it. We could add some logic that would keep track of added items per-request to ensure unique order but not sure if this is needed.

[MikeAlhayek]

I just added a counter per request so that the order is more explicit even when importing multiple rules from recipe.

[arkadiuszwojcik]

@MikeAlhayek Ok, at this point, I am ditching this PR. I think the current solution is over-engineered and bloated. It is three times larger in terms of code compared to my second iteration with the table UI and spreads across three separate assemblies, without adding any real value. Instead of expanding it carefully with time to meet the new incoming requirements, it is designed upfront like other, more complex modules. The same issue applies to the UI — it displays a lot of information I'm not interested in, while hiding the information I actually need. Lastly, the bizarre ordering should be resolved using a simple list pattern, as is done in every other solution to this problem that I reviewed. At this stage, I am overwhelmed by the current implementation, and I prefer to go back to my custom module, which does the same thing but in a much simpler way. Good luck.

[MikeAlhayek]

@arkadiuszwojcik

Perhaps you are overlooked the idea behind extensible UI. This module is part of a framework, and providing an extensible architecture will empower other users to extend both the code and the UI simultaneously.

User experience is crucial, and we aim to deliver the best possible experience for our users. To that end, I made some adjustments to facilitate a more user-friendly sorting mechanism that eliminates the need to manually specify the order. I have removed pagination in favor of a more user-friendly approach that allows you to drag and drop rules to change their order easily. Additionally, any new rules will be automatically added to the end of the list.

[MikeAlhayek]

I noticed some of my code was removed before the commit. I'll fix that later.

[MikeAlhayek]

Fixed the last issue

[arkadiuszwojcik]

Perhaps you are overlooked the idea behind extensible UI. This module is part of a framework, and providing an extensible architecture will empower other users to extend both the code and the UI simultaneously.

@MikeAlhayek I understand extensible UI well, and I know that you don't build it upfront when all the requirements and usecases are not yet clear. It's better to start small and expand the abstraction later, based on the requirements, rather than providing abstraction too early. Doing so can avoid the need to support it later and extend without breaking the functionality that others depend on.

[MikeAlhayek]

@arkadiuszwojcik did you evaluate the latest changes? Any other issue you are seeing?

[arkadiuszwojcik]

@MikeAlhayek looks good. Except one minor thing: when adding new rule main header says "New rule" but probably it should be "New rediredt/rewrite rule". The same is with Edit window but there it should be "Edit '' rule". At least this is how it works in Queries module as I can see.

[MikeAlhayek]

@arkadiuszwojcik done.

@sebastienros I am debating weather or not we should make OrchardCore.UrlRewriting a dependency only feature. Then create OrchardCore.UrlRewriting.ApacheModRewrite feature in the same module that depends on OrchardCore.UrlRewriting and expose only the ApacheMod rules. This way if we ever want to use other rule from IIS for example, we would just add another feature that would expose different set of rules.

[MikeAlhayek]

I am debating weather or not we should make OrchardCore.UrlRewriting a dependency only feature. Then create OrchardCore.UrlRewriting.ApacheModRewrite feature in the same module that depends on OrchardCore.UrlRewriting and expose only the ApacheMod rules. This way if we ever want to use other rule from IIS for example, we would just add another feature that would expose different set of rules.

I think the debate is over, I don't think we need to create another feature for this. If someone want to use IIS rule, they can just call them other than "Rewrite" and "Redirect".

I am done with this PR. @arkadiuszwojcik if you see no more issues, I'll merge it and let people try it out.

[MikeAlhayek]

@arkadiuszwojcik anything to add here?