[META] XB Permissions

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 #3477164: Lift all methods out of `SdcController` into separate invokable services-as-controllers.

Still relevant!

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

Refinements:

• Added: Administer Components (as in: XB Component config entities)
• Put Administer Component Overrides on hold, see issue summary for details.
• Put Administer Content Type Templates on hold, see issue summary for details.

Content Manager is a persona that did not exist until now. It's the Digital Strategist in the Drupal CMS personas.

Closed #3513569: Gate editing global regions behind 'administer blocks' permission in the UI 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 #3454677: Diagram tying the product requirements + decisions together introduced.)

Oversights in #3502048: Permissions for XB Page content entities: edit/delete/create, reuse core's `access content` for viewing 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.

#3508694: Permissions for XB config entity types is in 🥳

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

is unblocked!

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

Added #3519992: Previewing the Layout causes an error when the user does not have access to all fields in the auto-save data, 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). → #3529892: Add a Publish Experience Builder Content permission, use it on `experience_builder.api.auto-save.(delete|post)` routes
• Add permission for "Use Experience Builder", so we can access XB without an actual content (e.g. so we can create the first page). → #3529924: Add access check for using Experience Builder at all: if >=1 content entity type with an XB field can be created or edited.

Edited for links.

#3516657: Update `ApiContentControllers::list()` to expose available content entity operations in `meta` 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.

#3516432: Update all XB routes to respect content entity update/field edit access of edited XB field is in :)

Created #3529895: Provide the client with `create` operation access information similar to #3516657, #3529892: Add a Publish Experience Builder Content permission, use it on `experience_builder.api.auto-save.(delete|post)` routes

Created #3529924: Add access check for using Experience Builder at all: if >=1 content entity type with an XB field can be created or edited.

#3516657: Update `ApiContentControllers::list()` to expose available content entity operations in `meta` 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 #3513566: [later phase] [PP-1] Add generic create entity support (currently just `Page` content entities) will need to be supported (not by beta).

EDIT: I missed #42 😅🙈

#3505224: XbConfigEntityHttpApiTest missing test coverage for individual GET requests for JavaScriptComponent + AssetLibrary 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 #3529892: Add a Publish Experience Builder Content permission, use it on `experience_builder.api.auto-save.(delete|post)` routes.)

Thoughts?

I expanded the scope at #3529892: Add a Publish Experience Builder Content permission, use it on `experience_builder.api.auto-save.(delete|post)` routes to also cover the route that was just added by #3529207: For selective reverting add DELETE auto-save endpoint 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 Review changes 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).

#3529892: Add a Publish Experience Builder Content permission, use it on `experience_builder.api.auto-save.(delete|post)` routes 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 #3529892: Add a Publish Experience Builder Content permission, use it on `experience_builder.api.auto-save.(delete|post)` routes, or even better, the check access in #3529924: Add access check for using Experience Builder at all: if >=1 content entity type with an XB field can be created or edited..

Created #3531841: Expose Publish Experience Builder Content permission to the client UI

Added #3531913: `api.config.get` route returns data without any authentication due to broad `view` access

#3531858: CanvasController's response must vary by access result cacheability is a stable-blocking follow-up for #3529895: Provide the client with `create` operation access information similar to #3516657, because without it an information disclosure is possible in principle, if contrib/custom modules are installed that alter access control.

#3529895: Provide the client with `create` operation access information similar to #3516657 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? 🙏

For example, zero routes should be using access administration pages, and anonymous/unauthenticated access should be prevented (see #3531913: `api.config.get` route returns data without any authentication due to broad `view` access for example).

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 #3529924: Add access check for using Experience Builder at all: if >=1 content entity type with an XB field can be created or edited., 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 #3531913: `api.config.get` route returns data without any authentication due to broad `view` access)

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

Removed [blocked/on hold: content type templates] because #3455629: [PP-1] [META] 7. Content Templates — aka "default layouts" — affects the tree+props data model is not happening before beta1.

#3531841: Expose Publish Experience Builder Content permission to the client UI is in.

#3531913: `api.config.get` route returns data without any authentication due to broad `view` access landed yesterday.

Split field access checks from #3529426: Add entity access check on `ApiAutoSaveController::post()` to #3532454: Add field access check on `ApiAutoSaveController::post()`.

#3529426: Add entity access check on `ApiAutoSaveController::post()` landed

♻️

#3506434: Disallow deleting an XB-enabled content entity if it's currently the homepage just landed

#3506434: Disallow deleting an XB-enabled content entity if it's currently the homepage is in! 🎉

Reviewed all of the remaining server-side ones:

1. #3532454: Add field access check on `ApiAutoSaveController::post()`
2. #3527244: Add test coverage that ensure fields values the user has view but not edit access to can NOT be saved to AutoSave
3. #3527156: Add test coverage for access exception in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess()

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

Bumped #3529924 to beta blocking per #3529924-19: Add access check for using Experience Builder at all: if >=1 content entity type with an XB field can be created or edited..

#3527244: Add test coverage that ensure fields values the user has view but not edit access to can NOT be saved to AutoSave landed 🎉

Update meta with some non-priority low-hanging fruit.
Tempted to remove #3519737: ADR for dynamic code components from the meta.

Added #3533781: Allow users with edit permissions to see and manage revisions of `Page` content entities

♻️

Just landed:

1. #3532454: Add field access check on `ApiAutoSaveController::post()`
2. #3516641: Make the XB UI show only UI elements/operations available to the current user

Oh, and #3527156: Add test coverage for access exception in \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess(), too, last week!

#3529924: Add access check for using Experience Builder at all: if >=1 content entity type with an XB field can be created or edited. and #3533728: Duplicate operation should require 'create' permission, not 'update' permission landed.

Bugs reported relating to this meta:

1. #3535132: Show descriptive error message instead of generic 403 for unauthorised actions in the navigator
2. #3535134: XB UI should gracefully handle access changing mid-session
3. #3535142: Edit option incorrectly visible in Layers panel for users without "administer code components" permission

4. #3535221: Pending changes leaking entities that user might have no access to

I think this actually all was finished back in #75 🤓

Yay, thanks everyone! 🥳