Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pages MetaData Editing Mode Support #29

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

robearlam
Copy link
Member

This PR adds support for Pages MetaData Editing.

Description / Motivation

  • Introduction of a new Library to be referenced in XMC projects
  • Changes required in LayoutService.Client project to introduce concept of Field Chromes, which weren't required previously.
  • Tests updated to reflect new and updated code

Testing

  • The Unit & Intergration tests are passing.
  • I have added the necessary tests to cover my changes.

Terms

Closes #17

@robearlam robearlam added the enhancement New feature or request label Feb 17, 2025
@robearlam robearlam self-assigned this Feb 17, 2025
{
logger.LogDebug("Processing valid Pages Render request");
PagesRenderArgs args = ParseQueryStringArgs(Request);
return Redirect($"{args.Route}?mode={args.Mode}&sc_itemid={args.ItemId}&sc_version={args.Version}&sc_lang={args.Language}&sc_site={args.Site}&sc_layoutKind={args.LayoutKind}&secret={args.EditingSecret}&tenant_id={args.TenantId}&route={args.Route}");

Check warning

Code scanning / CodeQL

URL redirection from remote source Medium

Untrusted URL redirection due to
user-provided value
.
Untrusted URL redirection due to
user-provided value
.
Untrusted URL redirection due to
user-provided value
.
Untrusted URL redirection due to
user-provided value
.
Untrusted URL redirection due to
user-provided value
.
Untrusted URL redirection due to
user-provided value
.
Untrusted URL redirection due to
user-provided value
.

Copilot Autofix

AI 10 days ago

To fix the problem, we need to validate the user input before using it in the URL redirect. One way to do this is to maintain a list of authorized routes and ensure that the user-provided route is in this list before performing the redirection. Alternatively, we can check that the target URL does not redirect to a different host by ensuring that the URL is either relative or on a known good host.

In this case, we will implement a validation method that checks if the route is in a predefined list of valid routes. This method will be called before performing the redirection.

Suggested changeset 1
src/Sitecore.AspNetCore.SDK.Pages/Controllers/PagesSetupController.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Sitecore.AspNetCore.SDK.Pages/Controllers/PagesSetupController.cs b/src/Sitecore.AspNetCore.SDK.Pages/Controllers/PagesSetupController.cs
--- a/src/Sitecore.AspNetCore.SDK.Pages/Controllers/PagesSetupController.cs
+++ b/src/Sitecore.AspNetCore.SDK.Pages/Controllers/PagesSetupController.cs
@@ -52,3 +52,7 @@
                 PagesRenderArgs args = ParseQueryStringArgs(Request);
-                return Redirect($"{args.Route}?mode={args.Mode}&sc_itemid={args.ItemId}&sc_version={args.Version}&sc_lang={args.Language}&sc_site={args.Site}&sc_layoutKind={args.LayoutKind}&secret={args.EditingSecret}&tenant_id={args.TenantId}&route={args.Route}");
+                if (IsValidRoute(args.Route))
+                {
+                    return Redirect($"{args.Route}?mode={args.Mode}&sc_itemid={args.ItemId}&sc_version={args.Version}&sc_lang={args.Language}&sc_site={args.Site}&sc_layoutKind={args.LayoutKind}&secret={args.EditingSecret}&tenant_id={args.TenantId}&route={args.Route}");
+                }
+                return BadRequest("Invalid route.");
             }
@@ -73,2 +77,9 @@
         }
+
+        private bool IsValidRoute(string route)
+        {
+            // Define a list of valid routes
+            var validRoutes = new List<string> { "/home", "/about", "/contact" };
+            return validRoutes.Contains(route);
+        }
 
EOF
@@ -52,3 +52,7 @@
PagesRenderArgs args = ParseQueryStringArgs(Request);
return Redirect($"{args.Route}?mode={args.Mode}&sc_itemid={args.ItemId}&sc_version={args.Version}&sc_lang={args.Language}&sc_site={args.Site}&sc_layoutKind={args.LayoutKind}&secret={args.EditingSecret}&tenant_id={args.TenantId}&route={args.Route}");
if (IsValidRoute(args.Route))
{
return Redirect($"{args.Route}?mode={args.Mode}&sc_itemid={args.ItemId}&sc_version={args.Version}&sc_lang={args.Language}&sc_site={args.Site}&sc_layoutKind={args.LayoutKind}&secret={args.EditingSecret}&tenant_id={args.TenantId}&route={args.Route}");
}
return BadRequest("Invalid route.");
}
@@ -73,2 +77,9 @@
}

private bool IsValidRoute(string route)
{
// Define a list of valid routes
var validRoutes = new List<string> { "/home", "/about", "/contact" };
return validRoutes.Contains(route);
}

Copilot is powered by AI and may make mistakes. Always verify output.
@jballe
Copy link

jballe commented Mar 31, 2025

I have tried this and it is working (almost) as expected 👍

When I have the SXA LinkList rendering inserted on a page then editing that page in Pages is failing:
image

I think the reason is that this rendering has an Integrated GraphQL query specified in Sitecore.
This is rendering in the layout response as an object, similar like the standard fields.

Layout response with ContentResolvers are properly handled, because it is rendered as an array. When parsing the layout in

and using the CustomContrentFieldKey and hereby handled in
if (field.Value is JsonSerializedField serialisedField && field.Key != "CustomContent")

However this result from the Integrated Query is not going into this code path but is ending up as a standard field with the serialization error shown.
The LinkList is working with ordinary rendering.

I completely acknowledge that this is something we need to handle in the ASP.NET SDK because we are using model binding with (real) strong types while the node based SDK is (more or less) just ignoring this difference 😄

@robearlam
Copy link
Member Author

robearlam commented Apr 1, 2025

I have tried this and it is working (almost) as expected 👍

When I have the SXA LinkList rendering inserted on a page then editing that page in Pages is failing: image

I think the reason is that this rendering has an Integrated GraphQL query specified in Sitecore. This is rendering in the layout response as an object, similar like the standard fields.

Layout response with ContentResolvers are properly handled, because it is rendered as an array. When parsing the layout in

and using the CustomContrentFieldKey and hereby handled in

if (field.Value is JsonSerializedField serialisedField && field.Key != "CustomContent")

However this result from the Integrated Query is not going into this code path but is ending up as a standard field with the serialization error shown. The LinkList is working with ordinary rendering.

I completely acknowledge that this is something we need to handle in the ASP.NET SDK because we are using model binding with (real) strong types while the node based SDK is (more or less) just ignoring this difference 😄

Good catch @jballe, I'd forgotten about the iGQL in use on that control.

I've updated the error handling in the GraphQLEditingServiceHandler so that this doesn't blow up the page. The component now renders on the page and is draggable within the placeholders, however while it's fields are rendered correctly they're not inline editable.

I think this will work for now, and we can say that anything implemented with a custom Rendering Contents Resolvers, or with iGQL that changes the structure of the data returned will no longer be editable.

We can then have a look at how to better handle deserialisation of these custom types, as the strong typing model in dotnet will make this more challenging than in node as you mentioned!

@jballe
Copy link

jballe commented Apr 6, 2025

Thank you @robearlam that was really fast! I have tried it and it is now working with those changes.

I have two other comments - with the risk of delaying the PR (and especially when I have been poking @IvanLieckens it might not be the best timing) - so let me just give the input and then you can decide if it should actually be part of this PR or a future improvement - or maybe even something unrelated to this...

When using metadata editing in Pages there is a request to config endpoint, implemented in the PagesSetupController. I had to make two changes here to get rid of console/CORS errors, so I have added [HttpOptions] and I have added httpResponse.Headers.AccessControlAllowHeaders = "Authorization";. I don't see the need for the Authorization header but Pages include it in the request (at least currently) so without the request is blocked.
However, it might as well "just" be incorrect behavior in Pages, I also see this CORS error in a newly created JSS based Play Summit demo instance...

More important:
The config endpoint returns a list of (supported) components.
I can see that the PagesSetupController returns the ComponentName value from ComponentRendererDescriptor instances in renderingEngineOptions.RendererRegistry.
However, the values here are not the component name value used to match with the value from the rendering item in Sitecore (that value is compiled to a Predicate and stored in the Match property).
When using the AddModelBoundView extension method, the value stored in ComponentName is actually the view name. So with this implementation you cannot edit components with a view name different than the field in Sitecore, even though the extension method has an overload to specify this.

In this example I have moved the Promo view to a subfolder and changed the view name with a registration like this:

renderingEngineOptions.AddModelBoundView<Promo>("Promo", "HeadlessExperienceAccelerator/Promo")

image
There is not really any indication in the UI, why the rendering is "disabled" but the rendered source code give a clue
image
The response from the config endpoint is:
image

For ViewComponents it is actually worse, the extension method to register the view component

ComponentRendererDescriptor descriptor = ViewComponentComponentRenderer.Describe(match, viewComponentName);

But the method signature is changed to have a locator as a second paramter, so we end up with a registration with an empty componentName
public static ComponentRendererDescriptor Describe(Predicate<string> match, string locator, string componentName = "")

I can see you have had a bit back and forth on how this actually should be so it was probably not changed everywhere.
I also notice that there is a commit "Changed to no longer persist component names for predicate registrations as its not supported by MetaData Editing" 2 weeks ago so there might be details I don't see (and I don't get what's not supported)

But, from what I see, I think we need to store the actual component name that is used to build the predicate so that it can be exposed in the config endpoint. Right now I think there is a bit confusion on what is the component name used in the layout response and what is the name used to render the component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for XMC Editor Metadata integration
2 participants