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

Without this, we need to grant anonymous users the administer code components permission for them to be visible 😅

Solid start, thanks! Posted a first round of feedback 😊

JS Components is mostly ready. Would love another review before starting with asset libraries as they should be basically the same.

Very much on the right track! 🤩 Lots of suggestions, but loving where this is going! 👏🙏

I need to check cypress failures (if I get to run them...) but phpunit is passing and this is ready for another round of reviews.

updated the summary based on @lauriii comment https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...

I assumed a single permission for both asset libraries and code components but asked for clarification here https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...

Thanks for the clarifications, @lauriii and @tedbow! 🙏👍

I made a few further clarifications — do you agree?

I'm terribly sorry, @penyaskito, but turns out that pretty much everything you've done on this issue/MR turns out to be unnecessary now that @lauriii has provided precise product requirements 🫣😅

But … I'm pretty sure this issue has helped @lauriii arrive at those clear conclusions!

AFAICT given the refined product requirements for XB permissions (see the new meta: 📌 SdcController cleanup tasks Active ) means that this issue can just:

• Use a tiny subclass of default \Drupal\Core\Entity\EntityAccessControlHandler for the Component and Pattern config entity types, aka all XB config entity types that provide the basic building blocks for the XB UI.

  For now, simply require the user to be authenticated (i.e. vary by the user.roles:authenticated) cache context).

• Use the default \Drupal\Core\Entity\EntityAccessControlHandler for the JavaScriptComponent, AssetLibrary and PageRegion config entity types, aka all XB config entity types that do not need to be available to all XB users.

  So that's basically just:

  diff --git a/src/Entity/PageRegion.php b/src/Entity/PageRegion.php
  index e384258ed..773f4d2ee 100644
  --- a/src/Entity/PageRegion.php
  +++ b/src/Entity/PageRegion.php
  @@ -23,7 +23,7 @@ use Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItemInstantiat
    *    label_singular = @Translation("page region"),
    *    label_plural = @Translation("page region"),
    *    label_collection = @Translation("Page region"),
  - *    admin_permission = "access administration pages",
  + *    admin_permission = "administer page template",
    *    entity_keys = {
    *      "id" = "id",
    *      "status" = "status",
  diff --git a/src/Entity/Pattern.php b/src/Entity/Pattern.php
  index 12e61d717..fb23d7895 100644
  --- a/src/Entity/Pattern.php
  +++ b/src/Entity/Pattern.php
  @@ -22,7 +22,7 @@ use Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItemInstantiat
    *    label_singular = @Translation("pattern"),
    *    label_plural = @Translation("patterns"),
    *    label_collection = @Translation("Patterns"),
  - *    admin_permission = "access administration pages",
  + *    admin_permission = "administer patterns",
    *    entity_keys = {
    *      "id" = "id",
    *      "label" = "label",
  

• … but then the bulk of the remaining work is in updating the routes/controllers to respect that. Plus all the testCannotDeleteWhenThereAreDependents() work also remains relevant.

@lauriii to confirm

1. The permissions listed in 📌 SdcController cleanup tasks Active do not include , so I added that in 📌 SdcController cleanup tasks Active .

   there is one more permission, which would be required to access the /admin/structure/component/status UI. Please either confirm you agree, or propose an alternative.

2. • Use a tiny subclass of default \Drupal\Core\Entity\EntityAccessControlHandler for the Component and Pattern config entity types, aka all XB config entity types that provide the basic building blocks for the XB UI. These must respect the admin_permission, except that viewing must always be allowed.

     For now, simply require the user to be authenticated (i.e. vary by the user.roles:authenticated) cache context).

   In fact, it could be implemented more simply stil:

   •   public function access(EntityInterface $entity, $operation, ?AccountInterface $account = NULL, $return_as_object = FALSE) {
         $account = $this->prepareUser($account);
         if ($operation === 'view') {
           $result = AccessResult::allowed();
           return $return_as_object ? $result : $result->isAllowed();
         }
         return parent::access($entity, $operation, $account, $return_as_object);
     

   • Add the
     

         _user_is_logged_in: 'TRUE'
     

     route requirement to the experience_builder.api.config.list route.

     without this relaxation, the administer patterns and administer components permissions would be required for a regular Content Creator to be able to use XB at all.

The permissions listed in #3452581: [META] XB Permissions do not include Administer Components, so I added that in #3452581: [META] XB Permissions

This seems too granular for now. Could we use administer themes permission for this? It might make sense to even move this under "Appearance" but that's off-topic for this issue 🤔

Use a tiny subclass of default \Drupal\Core\Entity\EntityAccessControlHandler for the Component and Pattern config entity types, aka all XB config entity types that provide the basic building blocks for the XB UI. These must respect the admin_permission, except that viewing must always be allowed.
For now, simply require the user to be authenticated (i.e. vary by the user.roles:authenticated) cache context).

Seems fine to allow viewing these to be tied to whatever parent context (e.g., access content).

Could we use administer themes permission for this?

Sure, that's fine.

@lauriii to confirm

1. It might make sense to even move this under "Appearance" but that's off-topic for this issue 🤔

   That's a trivial move: instead of being listed at /admin/structure, add 2 new tabs at /admin/appearance. It'd also allow us to clean up the long obsolete UI label.

   (Patch that implements this attached.)

2. Seems fine to allow viewing these to be tied to whatever parent context (e.g., access content).

   There is no parent context when using the XB UI and fetching the list of Component and Pattern config entities. The XB UI just fetches them regardless of which thing you're editing, no matter if it's a Page or Node content entity, or a ContentTypeTemplate config entity (once 🌱 [META] 7. Content type templates — aka "default layouts" — clarify the tree+props data model Active is implemented).

   Either:

   1. it is acceptable that Component and Pattern config entities are retrieved context-free from the server to populate the choices available in the client.
   2. the client
      1. first retrieves the edited XB component tree
      2. after it got a (2xx) response, it passes this context to the server again when fetching the Component and Pattern config entities
      3. … while it'd return the exact same API response regardless of the context, it'd perform different access checking

      This means that for now, it's oddly using different access checks to fetch the same data, but in the future it'd make it trivial to add context-aware (specifically, component tree host-aware) filtering logic.

      Note too that this would cause worse front-end performance.

   IMHO the context-free choice is the simplest, sanest choice for now.

That's a trivial move: instead of being listed at /admin/structure, add 2 new tabs at /admin/appearance. It'd also allow us to clean up the long obsolete Page Builder Components UI label.

Could we add a new tab called "Components" and then add the two tabs under it?

There is no parent context when using the XB UI and fetching the list of Component and Pattern config entities. The XB UI just fetches them regardless of which thing you're editing, no matter if it's a Page or Node content entity, or a ContentTypeTemplate config entity (once #3455629: [Needs design] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model is implemented).

Let's implement this based on the context-free proposal for now. We can add more complex permissions to the components later.

#20: ACK, context-free it is 👍 That means you've +1'd the _user_is_logged_in proposal.

Could we add a new tab called "Components" and then add the two tabs under it?

Sure! :)

Created 📌 Move components listing to Appearance Active for #19, confirmed in #20.

That works too :)

Ready for another round of reviews.

Managed to do a partial review, will review the rest later today :)

Looking great! 😄 I think we're in the final stretch here now :)

Tantalizingly close! 😄

penyaskito → credited larowlan → .

Granting credit to @larowlan for previous reviews now that I can.
NR again, maybe last time? 😁

Looking great now! 😄 But … all PHPUnit CI jobs were failing. It seems to have been a CI infra issue with the DB. Rerunning those CI jobs, feel free to merge if they pass :)

(Please merge using the d.o "merge" button, not the GitLab one. That ensures the proper commit message is applied.)

Needs review for https://git.drupalcode.org/project/experience_builder/-/merge_requests/7....

I'm pretty sure that's a good fix, but want to be sure that we aren't just hiding another issue, as might be related to 🐛 ComponentTreeHydrated prevents serialization Active .
Assigning to larowlan for that.

The trace of the test failure was:

/var/www/html/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:262
/var/www/html/web/core/lib/Drupal/Core/Entity/Plugin/DataType/EntityReference.php:74
/var/www/html/web/core/lib/Drupal/Core/TypedData/DataReferenceBase.php:37
/var/www/html/web/core/modules/serialization/src/Normalizer/EntityReferenceFieldItemNormalizer.php:44
/var/www/html/vendor/symfony/serializer/Serializer.php:152
/var/www/html/web/core/modules/serialization/src/Normalizer/ListNormalizer.php:24
/var/www/html/vendor/symfony/serializer/Serializer.php:152
/var/www/html/web/core/modules/serialization/src/Normalizer/ContentEntityNormalizer.php:25
/var/www/html/vendor/symfony/serializer/Serializer.php:152
/var/www/html/web/modules/contrib/experience_builder/tests/src/Kernel/DataType/ComponentTreeHydratedTest.php:957

The normalization is trying to load the anonymous user from the db, and the users table doesn't exist.

Back to RTBC then 🎉

This unblocked 📌 Pass current user's XB permissions to the XB UI Active !