Add Culture Picker for The Admin Site

[hishamco]

Fixes #9720

[hishamco]

Seems the build failed, because the feature needs to be enabled

[hishamco]

It was my bad after you suggested to change the feature Id. Now everything working as it was

[jtkech]

@hishamco

Didn't have so much time to think about it but will let you know if I find something. Yes at the end the same things would be done but maybe not from the same places, my feeling is that the RazorPage is not the right place to check for an enabled feature, but why not I'm open to everything ;)

Maybe @ns8482e had a good idea when saying that this is the feature that needs to drive the shape (that could be overridden). Maybe an IShapeTableProvider registered if the feature is enabled and that would check if in the admin and then enlist this shape in the admin NavbarTop zone of the current theme Layout.

[hishamco]

but why not I'm open to everything ;)

;)

@ns8482e if you can push some commits on my PR or open a a new open it will be more valuable, so we can use it for this PR and many things inside and outide OC

[hishamco]

Hope if the solution can be used for any feature, not for particular one

[jtkech]

@hishamco

Just a first test that I will complete, only to show the idea, the IShapeTableProvider would be registered when the feature is enabled, I will add a check for the admin, build the shape to add ...

public class Shapes : IShapeTableProvider
{
    public void Discover(ShapeTableBuilder builder)
    {
        builder.Describe("Layout")
            .OnDisplaying(async context =>
            {
                if (context.Shape is IZoneHolding layout)
                {
                    await layout.Zones["NavbarTop"].AddAsync(new HtmlString("Hello world"));
                }
            });

[hishamco]

How we can make such this extensible and work for any shape without need to rewrite the entire IShapeTableProvider?

[jtkech]

Any feature can implement its own IShapeTableProvider, many examples in the source

[jtkech]

@hishamco

Something like this

// Registered in the startup executed when the feature is enabled.
public class AdminCulturePickerShapes : IShapeTableProvider
{
    public void Discover(ShapeTableBuilder builder)
    {
        builder.Describe("Layout")
            .OnDisplaying(async context =>
            {
                if (context.Shape is IZoneHolding layout)
                {
                    var httpContextAccessor = context.ServiceProvider.GetRequiredService<IHttpContextAccessor>();
                    if (!AdminAttribute.IsApplied(httpContextAccessor.HttpContext))
                    {
                        return;
                    }

                    var shapeFactory = context.ServiceProvider.GetRequiredService<IShapeFactory>();
                    var culturePickerShape = await shapeFactory.CreateAsync("CulturePicker");
                    await layout.Zones["NavbarTop"].AddAsync(culturePickerShape);
                }
            });

[hishamco]

Just a small question, if there are many shapes will be rendered in the same zone, how can I arrange them?

[jtkech]

Currently there is nothing in this zone, in TheAdmin layout the darkmode button and the user menu are explicitly rendered after this zone, so the culture picker will always be rendered before.

If another feature adds a shape in this zone in the same way it will depend on the startup registration ordering, so on the feature dependency ordering, if your feature depends on another feature your provider will be registered later.

Then you can tweak the startup ordering by overriding the startup Order property.

Finally you can use placement to define for the culture CP shape a location in the NavbarTop layout zone

[jtkech]

@hishamco

Just did some first tests, can't continue right now but will continue this week-end ;)

First on the Default tenant, it was not setting the cookie so it was not working

I needed to remove this part ;path=/@ShellSettings.Name.ToLower()

Yes the culture was sticky, always the same on different page, but I was not able to change the culture.

Then I also tested ;path=/, it was working

Then just for testing I tried the following directly in the script (didn't take the time for now to use a data attribute): ;path=@(Context.Request.PathBase.HasValue ? Context.Request.PathBase : "/")

It was working and I could see the path /tenant1 for the tenant1 and / for the Default tenant in place of /default.

That said, for now as I have tested it (will retry this week-end) it was not sufficient to differentiate/isolate the cookie accross tenant, for now only tested with the Default and Tenant1 tenants.

So for now looks like we will have to also differentiate the cookie name per tenant.

---

Then not a big issue but when going to "/tenant1", not "/tenant1/", it fails in the CustomRequestCultureProvider.

ArgumentOutOfRangeException: startIndex cannot be larger than length of string. (Parameter 'startIndex')
string.Substring(int startIndex, int length)
OrchardCore.Localization.CulturePickerStartup.<ConfigureServices>b__3_1(HttpContext context) in 
Startup.cs
+
            var path = context.Request.Path.Value[1..];

because here the request path has no value (path.HasValue is false)

There are pathString extensions to check if it starts with a given segment, but always annoying when comparing escaped pathString with e.g a non escaped AdminUrlPrefix from the options. Hmm what we could do is to build once 2 pathString (or still strings), one starting with a slash "/adminprefix" and one ending with a slash "/adminprefix/", the goal being to not index the current request path.

[hishamco]

Thanks again @jtkech no worry I will test all the cases that you mentioned. BTW I started this PR as draft just to show a new localization feature, nothing but seems setting cookies at client-side is tricky ;)

[jtkech]

@hishamco

No problem, otherwise I like how it works, but yes not so easy to take into account all the things.

But I think you are very close to make it fully working and consistent ;)

I will still continue to test it this w-end as it is interesting!

[jtkech]

@hishamco

I made a separate PR based on your branch to show you what I meant, see #12113.

Feel free to use or not what you like or not ;)

[hishamco]

@jtkech I updated the PR with some of your changes, BTW I just test the cases that you mentioned above using one cookie name and controlling the path with the help of the tenant name

The issue previously when we used / for the path, coz it will affect the entire domain, but I really forgot that I need to target /Admin or {tenant}/Admin. We could use the tenant version as you suggested but while we can do it with one cookie without collision I suggest the solution is fine unless there's a case I didn't aware of

Please let me your feedback

[jtkech]

See my comments in #12113

What you did may work or not

E.g. "/" + ShellSettings.Name.ToLower() is not necessarily equal to "/" + tenantUrlPrefix and is not well escaped. For example if a tenant Tenant1 has an url prefix TenantA it will just not work.

That's why I used PathBase and checked if it has a value.

[hishamco]

That's why I used PathBase and checked if it has a value.

This is the last thing I will change, but I need to add a sufix "/Admin". BTW I was talking preiously about cookie name and path which work great in the test cases that I made

[jtkech]

@hishamco For info as I remember

The shapeTableProvider needs to be decorated with the [Feature()] attribute, as I did in my branch, otherwise when you have 1st enabled the feature and then disable it the culture picker is still rendered. This is because of some caching by the shape table manager at the module level.

Yes good idea to also use ".../admin" in the cookie path but 1st it would need to not be hardcoded but retrieved from the options, and it would need to be escaped, here the usage of PathString is helpful. But I tried it, the problem is that our urls may contain ".../admin" or ".../Admin" which are not considered as same cookie paths, so you lose the culture and another cookie is created on selection.

So it seems to not be so good to use too specialized paths, I was tempted to always set the path to "/" and only differentiate by name, as we do for some cookies, but using the request pathBase is a good compromise, as it is done by aspnetcore for the orchauth_ and AFT cookies.

we have some place where we set the cookie path to the tenant url prefix as is, e.g. for twitter, this is not totally right, first it would need to be escaped, and it would not work if we are under a virtual folder.

Here an example of a cookie builder using the request pathBase which is used I think for the the orchauth_ and AFT cookies for which we don't set any path ourselves.

https://github.com/dotnet/aspnetcore/blob/c85baf8db0c72ae8e68643029d514b2e737c9fae/src/Security/Authentication/Core/src/RequestPathBaseCookieBuilder.cs#L22-L38

[hishamco]

The shapeTableProvider needs to be decorated with the [Feature()] attribute, as I did in my branch, otherwise when you have 1st enabled the feature and then disable it the culture picker is still rendered. This is because of some caching by the shape table manager at the module level.

Thanks for mentioning that, I forgot to comment in that line in your PR, coz I surrprised why it works with me without it ;) I will update the PR soon

Yes good idea to also use ".../admin" in the cookie path but 1st it would need to not be hardcoded but retrieved from the options, and it would need to be escaped, here the usage of PathString is helpful. But I tried it, the problem is that our urls may contain ".../admin" or ".../Admin" which are not considered as same cookie paths, so you lose the culture and another cookie is created on selection.

Ya using AdminOptions is good for this case. Regarding the casing I didn't expect it's case sensitive, let me double check

[hishamco]

@jtkech please last review, seems we forgot this one, I suggest to unify cookie prefix after this PR

[hishamco]

@jtkech your approval if you don't have any comments, may be we triage this today

[jtkech]

Sorry didn't have the time to look at it again but LGTM

Did you retry it completely?

[hishamco]

Sure, I tried last time when I was made my last changes, no problem I will try it one more time

[hishamco]

Everything works as expected