Omit `PageRegion` representation from `ApiLayoutController::get()`, 403 if sending it in `::patch()` or `::post()`

Created on 1 April 2025, 24 days ago

Overview

Technically blocked on 📌 Add access control for "code components" and "asset libraries", special case: instantiated code components must be accessible to *all* Active , but work can begin ahead of that landing: because that will simply change the admin_permission of the PageRegion config entity type.

Proposed resolution

  1. ApiLayoutController::(get|patch)() must omit PageRegion component trees from the response by modifying ::addGlobalRegions() → no regions will appear in the XB UI
  2. ApiLayoutController::(patch|post) must validate that no component instances that are being modified are stored in a PageRegion config entity (a malicious user might try to bypass the above omission, or it might just be that the permissions for the user changed since they started editing)
  3. BUT we must ensure that a user who does not have the PageRegion admin_permissions still sees the rendered regions; without having access to manipulate them. This likely requires changes to ::buildPreviewRenderable()
  4. All of the above likely results in some refactoring of ::addGlobalRegions(), ::buildPreviewRenderable() etc. to be beneficial.

Out of scope:

User interface changes

Page regions no longer appear in the XB UI for users without the necessary permissions.

📌 Task
Status

Active

Version

0.0

Component

Internal HTTP API

Created by

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @wim leers
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Probably we don't even want users to focus on global regions (double-click). TODO: Confirm and create issue

  • 🇫🇮Finland lauriii Finland

    FYI, Non-empty global regions need to be highlighted when hovering with the green color Active is where we would allow users to double click a global region inside the preview to edit it.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • Pipeline finished with Failed
    17 days ago
    Total: 1472s
    #468340
  • Pipeline finished with Failed
    17 days ago
    Total: 1439s
    #468375
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • Pipeline finished with Success
    17 days ago
    Total: 1440s
    #468398
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • Pipeline finished with Failed
    15 days ago
    Total: 1235s
    #469475
  • Pipeline finished with Failed
    15 days ago
    Total: 388s
    #469483
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Oops some test failures.

  • Pipeline finished with Failed
    15 days ago
    Total: 3078s
    #469485
  • Pipeline finished with Failed
    15 days ago
    Total: 444s
    #469522
  • Pipeline finished with Failed
    15 days ago
    Total: 1293s
    #469526
  • Pipeline finished with Failed
    15 days ago
    Total: 1785s
    #469537
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Ready for another round of reviews.

  • Pipeline finished with Success
    15 days ago
    Total: 3455s
    #469555
  • Pipeline finished with Success
    15 days ago
    Total: 1629s
    #469917
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Canceled
    14 days ago
    Total: 1043s
    #470491
  • Pipeline finished with Failed
    14 days ago
    Total: 1984s
    #470511
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • Pipeline finished with Success
    14 days ago
    Total: 1510s
    #470531
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Canceled
    14 days ago
    Total: 69s
    #471164
  • Pipeline finished with Failed
    14 days ago
    Total: 1516s
    #471165
  • Pipeline finished with Canceled
    14 days ago
    Total: 626s
    #471212
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    🏓I forgot about openapi until now, added that 🙈

  • Pipeline finished with Canceled
    14 days ago
    Total: 316s
    #471218
  • Pipeline finished with Success
    14 days ago
    Total: 2004s
    #471225
  • Pipeline finished with Success
    14 days ago
    Total: 1776s
    #471298
  • Pipeline finished with Success
    9 days ago
    Total: 4277s
    #474744
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    ::getRegion() is still wrong, and the openapi.yml file was updated incorrectly.

    I recommend putting a breakpoint on \Drupal\Tests\experience_builder\Kernel\ApiLayoutControllerPostTest::testMissingSlot() and stepping through it — that's what I just did and it's how I observed the modified docs for ::getRegion() to be wrong in multiple ways 😅

    Let's spend the 10–20 mins it takes to get this right now, otherwise this will continue to cause confusion! 🙏

  • Pipeline finished with Canceled
    9 days ago
    Total: 144s
    #475097
  • Pipeline finished with Failed
    9 days ago
    Total: 299s
    #475102
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Fixed feedback, thanks!

  • Pipeline finished with Success
    9 days ago
    Total: 2384s
    #475108
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    This last bit now is perhaps genuinely nitpicky, but I find ::getRegionComponents() genuinely confusing 🙈

    I'd prefer to see https://git.drupalcode.org/project/experience_builder/-/merge_requests/8... get fixed, but if you disagree: I've approved the MR and marked this RTBC :)

  • Pipeline finished with Canceled
    9 days ago
    Total: 73s
    #475178
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    #20 suggestion is great, I'll fix that.

  • Pipeline finished with Success
    9 days ago
    Total: 1802s
    #475179
  • Pipeline finished with Failed
    9 days ago
    Total: 1478s
    #475238
  • Pipeline finished with Success
    8 days ago
    Total: 2859s
    #475267
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • Pipeline finished with Skipped
    8 days ago
    #475292
  • Pipeline finished with Skipped
    8 days ago
    #475293
  • Pipeline finished with Skipped
    8 days ago
    #475294
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Probably we don't even want users to focus on global regions (double-click). TODO: Confirm and create issue

    They won't be able to, because the regions will simply not appear in the information the server sends to the client; the client won't even know about these regions! 👍 So: no follow-up needed.

  • Issues identified for the Content Editor role:

    1. Experience Builder: Article Editor link is visible and clickable

      Steps to reproduce:

      1. Log in as a Content Editor.
      2. Go to the Content page.
      3. Click on the title of any Article created by an Admin (for example: Test Page).
      4. In the header, the Experience Builder: Test Page link is visible and clickable.
      5. Clicking this link redirects to the node editor page and shows the following error:
      An unexpected error has occurred while fetching layouts.
    2. Error while publishing an Article

      When creating a new Article page as a Content Editor and attempting to publish it, the following error appears:

      You do not have access to transition from Draft to Draft.
  • Issues identified for the Content Editor is not related to this issue.

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    @mayur-sose So what was it related to?

Production build 0.71.5 2024