🇪🇸Spain @penyaskito

Marking for preliminary review. We might want to do the trait for checking the field as Lee suggested, but could perfectly be part of the follow-up we wanted to open anyway and clear another critical beta blocker.

Leaving for Wim to evaluate.

larowlan → credited penyaskito → .

@Wim at #8: This is exactly what I was envisioning in #3452581-54: [META] XB Permissions → !!!

📌 Disallow deleting an XB-enabled content entity if it's currently the homepage Active just landed

♻️

+1. Added a test.

wim leers → credited penyaskito → .

Oopsie.

ResourceLinkCollection is not cacheable.

tldr: Coming from 📌 Disallow deleting an XB-enabled content entity if it's currently the homepage Active where we are using similar code to jsonapi for resource links + collection, but we are adding cacheability there, which is not a feature in jsonapi. I thought we were replicating a bug from jsonapi, while we were introducing a bugged feature 🥸

penyaskito → created an issue.

Part of 🐛 Setting default values does work for boolean props Active was merged since then, which I think is the same issue.

Can you verify with HEAD or using the committed MR as a patch?

I assume we want to tackle permissions directly here instead of a follow-up. Added permissions requirements here. We can split to a beta-blocker follow-up if it makes more sense.

Amazing 😍
NW per @larowlan review and I think I spotted a wrong permission check, but this looks amazing already and covers more scope than I expected.

I can't merge 😉

Ship it!

Given that at this point makes little sense to fix this in Drupal 7 date, could we at least have an alert notifying anonymous users that the time is not to be trusted, pointing to this known issue?

wim leers → credited penyaskito → .

(typo)

I don't feel that strongly, though, but a) there's no UI for creating regions; b) those are created when they are enabled for the theme in its settings, so feels safer to ensure they can't be created.

The cypress errors look legit.

Wondering why they happened here and not in #3529622 tho.

LGTM, don't even think this needs to run by Wim.

Glad I didn't add elseif in the suggestion, nice branch name 🤣

Looks pretty straightforward. Surprised it didn't affect more tests.

📌 Add field and entity access check on `ApiAutoSaveController::post()` Active landed

Too good to be true, if we can do !1086-note_540820 this is RTBC.

Which version of XB are you using? If HEAD, what's the last commit?

Split field access checks from 📌 Add field and entity access check on `ApiAutoSaveController::post()` Active to 📌 Add field access check on `ApiAutoSaveController::post()` Active .

Repurpose for only entity access check. Field access checks moved to 📌 Add field access check on `ApiAutoSaveController::post()` Active

penyaskito → created an issue.

RTBCing.

I contributed significantly to this, so @mglaman will do another review pass before passing this to Wim.

Removing needs follow-up, ✨ Create React Permission Utilities Active exists already.

Created 📌 [11.2.0-and-up] Add support for SDC variants Postponed and added to the table.

penyaskito → created an issue. See original summary → .

This is ready for further reviews.

Just in the back of my head for now. I don't think we create issues for updating docs, but might be wrong.

Even if I might agree this could happen in contrib first, strings are added/removed in different module versions without only being a result of bad practice per se.

Sooner than later, with the different page builders being built like XB, we will have SDC components strings here too, and SDC components will evolve too.
Also recipes will have translations (hopefully) soon, which might be affected by the same.
So it's not a priority for me, but Close (Won't fix) is probably excessive :-)

Updated IS with front-end issues now that we expose most required permissions/operations.
TBD: Create the issues.

re: zero routes should be using `access administration pages`

We have experience_builder.api.auto-save.get and experience_builder.api.log_error still using `access administration pages`. I think we can't remove that until we can depend on the new access handler in 📌 Add permission for "Use Experience Builder" Active , so I suggest we unpospone that one.

When that lands, I think it will be great replacing _user_is_logged_in with that new access handler, which will reduce the exposure of end-points for just those using XB.

I verified all routes as of now have access checks in place (aside of the known 🐛 `api.config.auto-save.get` route returns data without any authentication Active )

Pretty trivial, but unblocks work on the frontend.

Added tests for anonymous GET at assertAuthenticationAndAuthorization based on openapi examples, so we can generalize to all XB config entity types.

Glad that we solved the mistery.

This has been still useful for expanding test coverage + detecting a bug in our openapi definition. So MR should be merged IMHO.

Follow-up MR moving the contentEntityCreateOperations outside of xb.permissions

I need to self-review this yet, but feels close. Bálint changes look good to me, thanks!

Added 🐛 `api.config.auto-save.get` route returns data without any authentication Active

Follow-up at 🐛 `api.config.auto-save.get` route returns data without any authentication Active

And given I probed 🐛 After an error it’s possible to get trapped in an unrecoverable error state Active that 🐛 After an error it’s possible to get trapped in an unrecoverable error state Active is not related to permissions but client-side navigation, I'm not sure what needs to be done here then.

Can we add steps to reproduce to the IS?

Updated/expanded the MR with our actual test expectations for assets and patterns.

Back to Wim per !1178.
I'm 50% on each option, so your call. The client UI will probably need to check the absence of the key anyway.

penyaskito → created an issue.

Created 📌 Expose Publish Experience Builder Content permission to the client UI Active

Created 📌 Expose Publish Experience Builder Content permission to the client UI Active as a follow-up. Expected that to happen here, but I didn't add it to the IS.

penyaskito → created an issue.

#47: Right, we got to the same conclusion in the document Jesse created for making an overview on what needs to be changed in the UI app for having permission-awareness.

I'm wondering if they should be able to see the review panel though, as that's the way to see error messages on validation at the moment.

But agree that ideally we wouldn't have any reference to "access administration pages" in routing.yml. That should be a check for the permission added on 📌 Add a Publish Experience Builder Content permission Postponed , or even better, the check access in 📌 Add permission for "Use Experience Builder" Active .

(I don't think this needs extra test coverage as we had a) unit test, b) XbConfigEntityHttpApiTest functional test already)

Exposing code components even when disabled if we have access means that we also will expose unpublished asset libraries.

Which showed us that our openapi schema for the assets listing was wrong, and untested.

Per #13, unpostponing.

I can reproduce #8 and has nothing to do with the other issue mentioned in #9.

See my amend to #8:

1. Create a new code component.
2. Add the component to the library and place it on the canvas.
3. Go to Layers, click on the three dots, and select Edit code.
4. After that, try to delete ANY component from Layers.
   You will see the following error:

An unexpected error has occurred in a route. 404 Not Found

This happens even if I'm deleting something that is not what I'm seeing in the editor.
But surprisingly only happens if I have that component in the editor.

The difference is that if I create a new editor, my url is: xb_page/1/code-editor/code/test.
But if I edit a placed component code, my url is: xb_page/1/code-editor/component/jjj

(see component instead of code).

When I delete a component, even if it's not the one selected, i go from

xb/xb_page/1/editor/component/0b914052-f057-4a94-81db-bb64a09850e3 to xb/xb_page/1/editor (removing the component/{uuid}).

I think we are trying to do the same here, but ending in xb_page/1/code-editor which is a wrong url (you can just go directly in the browser and will see the same error Majur indicated)

Maybe there's something checking a regex component/{whatever} without checking a) that whatever is a uuid or, b) that we are at /editor/component/{whatever} (ensuring no match with /code-editor/component/{whatever}

@wim.leers That's a different error not affected by this. Will comment there.

We will benefit from having a Functional test, but tbh the unit test showed the misunderstanding (even in the test name).

The client UI will need

a) a listing of all entity types that have any bundle supported, so the navigator can list the entities of a given type, and check permissions for which content can be created.
b) a unique end-point (/xb/api/v0/content?) for getting the entities to list in the navigator.

I'll work on this if Matt doesn't beat me to it.

penyaskito → created an issue.

⚠️ This is HORRIBLY HACKY way to provide a XB link for articles using `field_xb_demo` and will go away! ☺️

Given this comment I don't think we need to invest time on testing, but the MR fixes the issue.

I reproduced this. Happens after publishing, viewing the content, outside of XB UI.

The toolbar item doesn't update. Clearing cache is a workaround, so a cache tag might be missing.

All tests passing 🎉

Something that doesn't have e2e tests is the code editor, but we need to fix it to at least send the same values in enum as meta:enum.
Bálint volunteered to write this.

@balintbrews For now I'd expect to send the same added values as labels. The only thing to take into account is replacing dots with underscores. Something like:

const getMetaEnumValue = (x: string) => x.replace('.', '_');

const enumValues = [
      '3.14',
      'b',
      'c',
    ];
const metaEnums = Object.fromEntries(new Map(enumValues.map(value => [getMetaEnumValue(value), value])))
console.log(metaEnums);


//{
//  "3_14": "3.14",
//  "b": "b",
//  "c": "c"
//} 

Fixed MR by rebasing back with 11.x. Didn't really review, just git magic.

Development happens on 11.x, not 11.2.x.

The datetime portion might be covered by 🐛 Default values for properties based on datetime fields are not schema-conformant Active

Posthumous review:

Fine with this change, but the menu id change might need to be covered in a change record/release notes (as it's 2.x only I think is fine) if people might be using it in recipes extending Drupal CMS?

I'd try to avoid unsetting hooks.

Recently we heard about this redirect causing issues with SSO modules.

I'd implement keepin the destination if set on the referrer to ensure the destination is also sent from the SSO source. Or implement a hook_user_login with a lower priority that overrides the redirect after hitting this.

just formatting

Added the different scenarios of permissions that need to be exposed to the client UI.

Discussed this with Ted + Alex. Even if we accomplish 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active , we shouldn't postpone/won't fix this because regardless this is an entity or form data, we want to be able to prevent you to publish something you don't have access to.

Even if we think we might be able to relax this checks to make possible to publish things people might have no access to edit, we don't want to support this at this time because it's hard to reason about because the difference with workspaces/content_moderation is that they perform an actual entity save, while we are creating + serializing the entity and not completing the entire entity-save-flow.

E.g. This won't trigger hook_entity_insert/update or whatever other custom code might do whatever they need. E.g. maybe they are throwing an exception on preSave (yes, people might do non-standard things)

So, IOW:

* We don't think this is a requirement and could be relaxed in the future.
* But it's more costly to verify, validate and document why we don't do the access checks, than actually just do then even if they look paranoid.
* The safest and actually easiest to implement is just verify the publisher has access to everything and the entity (or form data) is valid.

If this is too restrictive, we actually will be able to alleviate this once selective publishing is in place.

TODO: Create a follow-up to explore if we can relax these requirements.

LGTM

Given #3525829-8: If user with less/different permissions edits a page after a user with more/different permissions data lost could occur → , confirmed today by Lauri, I feel like we need a sync on this.

Basically, if I got this right, this is the same than 📌 Add field and entity access check on `ApiAutoSaveController::post()` Active with `ApiLayoutController`.

The difference is that for publishing, we want to throw an AccessDeniedException if we don't have access to the field.

For the layout, we just want to ignore the field, to prevent losing the changes other user would have made. As anyone publishing this would require to have all the needed permissions, they should be able to validate all field changes.

@Ted, can you confirm I'm right, and what's missing from the MR? (I think it is ready for review?)

I think we should still do this for page + node--article, even if 📌 Allow XB to be used on all node types Active would definitely need to refactor this.

Assigning to @tedbow for reviews.