SdcController cleanup tasks

Thanks for the review!

• I think everybody would agree all of the things you listed should happen eventually.
• But they should not be required to happen now.

We need to be able to iterate quickly, without accumulating enormous MRs like we tend to do in Drupal core. In Drupal core this makes sense because it's already a consistent whole, but that's not yet true for Experience Builder.
In other words: for XB, we intentionally want to avoid the "every commit must be perfect" strategy that Drupal core uses.

I acknowledge this means outside participation is more difficult: what is in a "ready for detailed review" state vs "slapped together, don't bother to review" state? So I think that is the challenge we should solve rather than disallowing imperfect/incomplete MRs to be reviewed.

So I propose to introduce a way to convey this. Ideas:

1. an impossible to miss
   

   // ⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️
   // ⚠️ 🔨🧹  This file is an early iteration. Do not review in detail yet.   🧹🔨 ⚠️
   // ⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️⚠️
   

   at the top of the file

2. Issues like the one you created here make for excellent Novice tasks! 😊 So it's not like this is a wasted effort at all. Perhaps we should even intentionally call this out, by adding comments like // @todo Novice: inject service, // @todo Novice: introduce new permission, et cetera?
3. introduce src-rough (back end) and ui/src-rough (front end) directories
4. … something else?

Started adopting #4.1: https://git.drupalcode.org/project/experience_builder/-/merge_requests/2... 🤠

I'm going to update the issue summary to outline, to make this a great issue to sprint on at DrupalCon Barcelona :)

Nobody picked this up during DrupalCon. Narrowing scope. Removing dead routes should happen in 📌 Lift all methods out of `SdcController` into separate invokable services-as-controllers Needs work .

Still relevant!

Discussed this in detail with @lauriii. Converting this to a meta issue instead.

Refinements:

• Added: (as in: XB Component config entities)
• Put on hold, see issue summary for details.
• Put on hold, see issue summary for details.

is a persona that did not exist until now. It's the in the Drupal CMS personas → .

Closed 📌 Gate editing global regions behind 'administer blocks' permission in the UI Active in favor of this issue.

Updating XB's docs per #12: https://git.drupalcode.org/project/experience_builder/-/merge_requests/828

(This updates the docs that 📌 [PP-1] Diagram tying the product requirements + decisions together Postponed introduced.)

wim leers → credited lauriii → .

Oversights in 📌 Provide granular permissions for pages Active clarified by @lauriii. 👍

Another important oversight spotted while refining the issue summary for #3508694: #3508694-14: Permissions for XB config entity types → , asked @lauriii to confirm.

Outline of remaining plan.

📌 Add access control for "code components" and "asset libraries", special case: instantiated code components must be accessible to *all* Active is in 🥳

Hide parts of UI current user cannot access instead of always showing everything

is unblocked!

wim leers → credited penyaskito → .

Added 🐛 ContentCreatorVisibleXbConfigEntityAccessControlHandler's `view` access must refuse access to disabled config entities Active — great find, @penyaskito!

wim leers → credited tedbow → .

Added 🐛 ApiLayoutController::buildPreviewRenderable unnecessarily call \ClientDataToEntityConverter::converts causing errors Active , found by @tedbow.

If I got this right, we are missing issues for:

• Add permission for "Publish Experience Builder Content" (see point 12 in the IS).
• Add permission for "Use Experience Builder", so we can access XB without an actual content (e.g. so we can create the first page).

📌 Update `ApiContentControllers::list()` to expose available content entity operations in `meta` Active landed, which means this meta is very much in need of an overhaul.

I did the simple first pass, but I think this would benefit a lot from identifying next steps — @penyaskito, what else do you think is needed for all the BE pieces here to be done, to allow the FE (the XB UI) to start respecting permissions throughout?

Per #3508694-18: Permissions for XB config entity types → and #23 here.

📌 [PP-1] Update `experience_builder.(experience_builder|api.layout.get) routes` to respect content entity update/field edit access of edited XB field Active is in :)

Created 📌 Provide the client with permissions flags for create content Active , 📌 Add a Publish Experience Builder Content permission Postponed

Created 📌 Add permission for "Use Experience Builder" Active

📌 Update `ApiContentControllers::list()` to expose available content entity operations in `meta` Active informs the client which operations are available on existing entities. But what about informing the client of when creating a new entity is supported?

i.e. conveying when this should be visible:

Note that this too must be done in a way that is not specific to Page content entities, even though that's the only type of content entity that can currently be created via XB. Because eventually 📌 Add generic create entity support Active will need to be supported (not by beta).

🐛 XbConfigEntityHttpApiTest Active is in.

Concern: we're blanket granting all permissions in XBTestSetup. This makes it less clear which functionality requires which permissions, and makes it so that we don't end up using the extraPermissions infrastructure that exists in the e2e tests.

I'm not blocking merges for that because of the time pressure we're under, but it is this meta that is introducing many permissions, so it SHOULD (IMHO) also be up to the MRs for this meta to refine the e2e tests as needed. (In particular when testing the "test functionality when permission is not granted" case, which is true for 📌 Add a Publish Experience Builder Content permission Postponed .)

Thoughts?

I expanded the scope at 📌 Add a Publish Experience Builder Content permission Postponed to also cover the route that was just added by 📌 For selective reverting add DELETE auto-save endpoint Active under that new permission.

But … IMHO the following should also be covered by it:

experience_builder.api.auto-save.get:
  path: '/xb/api/v0/auto-saves/pending'
  defaults:
    _controller: 'Drupal\experience_builder\Controller\ApiAutoSaveController::get'
  requirements:
    _permission: 'access administration pages'
  methods: ['GET']
  options:
    _format: 'json'

👆 Will mean that you won't be able to see unless you have the necessary permissions. That will need a XB UI logic update too, to either avoid making that request at all (which would then require the UI being informed via drupalSettings.xb.permissions and hence requiring an update to ExperienceBuilderController), or to allow making that request but then NOT erroring (which would also harden against the case where the permission is revoked after the XB UI has been loaded).

📌 Add a Publish Experience Builder Content permission Postponed landed. 🎉

#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 .

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

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

📌 Add access handler cache metadata to ExperienceBuilderController Active is a stable-blocking follow-up for 📌 Provide the client with permissions flags for create content Active , because without it an information disclosure is possible in principle, if contrib/custom modules are installed that alter access control.

📌 Provide the client with permissions flags for create content Active is in.

@penyaskito in #51: thanks! Can you please check every route in experience_builder.routing.yml and ensure they have the appropriate access control, and if not, create corresponding 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 )

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

Removed because 🌱 [META] 7. Content type templates — aka "default layouts" — clarify the tree+props data model Active is not happening before beta1.

📌 Expose Publish Experience Builder Content permission to the client UI Active is in.

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

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

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

♻️

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

📌 Disallow deleting an XB-enabled content entity if it's currently the homepage Active is in! 🎉

Reviewed all of the remaining ones:

1. 📌 Add field access check on `ApiAutoSaveController::post()` Active
2. 🐛 Fields values the user has view but not edit access can be saved to AutoSave Needs work
3. 📌 Add test coverage for access exception in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess Active

They're all in the hands of @penyaskito and/or @tedbow.