Security: Set Instance access handler

This is a proposal for the logic:

• for the current display_builder_instance entity of the route, check the profileId property and load the display_builder_profile entity (this may create problems until #3544545: Profile changed in config don't update the instance entity is solved. If yes, let's move this ticket to beta1 scope)
• check if the current user has the permission of using the Profile enity

The logic from comment #4 may not be enough.

We may also need to add to the API checks related to the display we are editing:

• For entity view display, check also if user has administer {entity_type} display permission
• For entity view overrides, check also if the user has the permission to edit the content entity
• For page layout, check also if user has administer page_layout permission
• For view, check also if user has administer views permission

How do we implement this in a standardized way? A new method in DisplayBuildableInterface?

thanks. i will have a look

Hi Sviatoslav,

Alongside the feedbacks from @mogtofu33

Check access to builder controllers

"builder controllers" are controllers extending IntegrationControllerBase:

• modules/display_builder_entity_view/src/Controller/EntityViewController.php
• modules/display_builder_entity_view/src/Controller/EntityViewOverridesController.php
• modules/display_builder_page_layout/src/Controller/PageLayoutController.php
• modules/display_builder_views/src/Controller/ViewsController.php

You haved added modules/display_builder_page_layout/src/AccessControlHandler.php and modules/display_builder_views/display_builder_views.routing.yml, that's OK.

Check access to ApiController

That's the core of the issue.

The goal is to check the requests to ApiContoller. Each endpoint has an "autocast" of display_builder_instance entity where InstanceAccessControlHandler is supposed to be checked. For example:

display_builder.api_root_attach:
  path: '/api/display-builder/{builder}'
  methods: [POST]
   ....
  options:
    parameters:
      builder:
        type: 'entity:display_builder_instance'

You have added a check to $entity->getProfile()->getPermissionName(), that's great.

Can you also add to the API checks related to the display we are editing:

• For entity view display, check also if user has administer {entity_type} display permission
• For entity view overrides, check also if the user has the permission to edit the content entity
• For page layout, check also if user has administer page_layout permission
• For view, check also if user has administer views permission

I guess you need a new DisplayBuildableInterface>getPermissionName() method.

Don't hesitate to share your thoughts about this proposal.

I take it to do the remaining part and send to review.

Work in progress.

Scope

• For entity view display, check also if user has administer {entity_type} display permission
• For entity view overrides, check also if the user has the permission to edit the content entity >> ❌ It was not done
• For page layout, check also if user has administer page_layout permission
• For view, check also if user has administer views permission

Review with 3 different roles

After pushing an extra commit renaming the permissions.

Limited profile is a profile created for this tests in order to check the users can't access the default profile.

Role 1: with all permissions (bit not the administrator)

Permissions:

• Use Display Builder (Display Builder module): YES
• Use the Limited Display Builder profile (Display Builder module): YES
• Use the Default Display Builder profile (Display Builder module): NO
• {entity_type}: Administer display (Field UI module): YES
• Administer views (Views module): YES
• Administer page layouts (Display Builder for page layout module): YES

Testing ConfigFormBuilder:

• Can create/switch a display with Limited profile. Expected: YES. Results: ✅
• Can create/switch a display with Default profile. Expected: NO. Results: ✅
• Can switch a display from limited to default Limited profile. Expected: NO. Results: ✅
• Can switch a display from default to limited Limited profile. Expected: ???. Results: ⚠️yes I can. This behavior was not discussed until now, and need to have its own issue

Page Layout:

• Can create a page layout. Expected: YES. Results: ✅
• Can load Display Builder for page layout with Limited profile. Expected: YES. Results: ✅
• Can load Display Builder for page layout with Default profile. Expected: NO. Results: ❌ i was able to load it
• Can alter the display builder instance with Limited profile. Expected: YES. Results: ❌ Got HTTP 403
• Can alter the display builder instance with Default profile. Expected: No. Results: ✅ i got HTTP 403

Entity view display:

• Can switch an entity view display to Display Builder. Expected: YES. Results: ✅
• Can load Display Builder for entity view display with Limited profile. Expected: YES. Results: ✅
• Can load Display Builder for entity view display with Default profile. Expected: NO. Results: ❌ i was able to load it
• Can alter the display builder instance with Limited profile. Expected: YES. Results: ✅
• Can alter the display builder instance with Default profile. Expected: NO. Results: ❌ i was able to do some changes

Views:

• Can switch a views display to Display Builder. Expected: YES. Results: ✅
• Can load Display Builder for views display with Limited profile. Expected: YES. Results: ✅
• Can load Display Builder for views display with Default profile. Expected: NO. Results: ❌ i was able to load it
• Can alter the display builder instance with Limited profile. Expected: YES. Results: ✅
• Can alter the display builder instance with Default profile. Expected: NO. Results: ❌ i was able to do some changes

Role 2: has the "classic" permissions of Drupal display building, but not the ones from Display Builder

Permissions:

• Use Display Builder (Display Builder module): NO
• Use the Limited Display Builder profile (Display Builder module): NO
• Use the Default Display Builder profile (Display Builder module): NO
• {entity_type}: Administer display (Field UI module): YES
• Administer views (Views module): YES
• Administer page layouts (Display Builder for page layout module): YES

Page Layout:

• Can create a page layout. Expected: NO. Results: ⚠️ no because i got
  Error: Call to a member function id() on null. We will consider this OK for is and fix it in another issue.
• Can load Display Builder for page layout. Expected: NO. Results: ❌ i was able to load it
• Can alter the display builder instance. Expected: NO. Results: ✅ my changes are not saved

Entity view display:

• Can switch an entity view display to Display Builder. Expected: NO. Results: ✅ i can't because no profile available for me.
• Can load Display Builder for entity view display. Expected: NO. Results: ❌ i was able to load it
• Can alter the display builder instance. Expected: NO. Results: : ❌ i am able to do some changes

Views:

• Can switch a views display to Display Builder. Expected: NO. Results: ✅ i can't because no profile available for me.
• Can load Display Builder for views display. Expected: NO. Results: ❌ i was able to load it
• Can alter the display builder instance. Expected: NO. Results: ✅ my changes are not saved

Role 3: has the Display Builder permissions, but not the ones from classic Drupal display building

Permissions:

• Use Display Builder (Display Builder module): YES
• Use the Limited Display Builder profile (Display Builder module): YES
• Use the Default Display Builder profile (Display Builder module): NO
• {entity_type}: Administer display (Field UI module): NO
• Administer views (Views module): NO
• Administer page layouts (Display Builder for page layout module): NO

Page Layout:

• Can create a page layout. Expected: NO. Results: ???
• Can load Display Builder for page layout. Expected: NO. Results: ???
• Can alter the display builder instance. Expected: NO. Results: ???

Entity view display:

• Can switch an entity view display to Display Builder. Expected: NO. Results: ???
• Can load Display Builder for entity view display. Expected: NO. Results: ???
• Can alter the display builder instance. Expected: NO. Results: ???

Views:

• Can switch a views display to Display Builder. Expected: NO. Results: ???.
• Can load Display Builder for views display. Expected: NO. Results: ???
• Can alter the display builder instance. Expected: NO. Results: ???

Next steps

What about entity view overrides ?

Also, do we really need "Use Display Builder" permission? It seems this is the same has no "Use the {Profile} Display Builder profile" permission. Let's remove it if not useful, but we have to ask Jean first.

Hi Sviatoslav,

Here is my review for Role 1 & Role 2. I will review Role 3 once the needed work is done.

Thanks for your time. As every security related task, this issue is very valuable.

i will have a look

I will rebase, add a kernel test and do some change in the logic.

work in progress.

Review with 3 different roles

Limited profile is a profile created for this tests in order to check the users can't access the default profile.

Role 1: with all permissions (bit not the administrator)

Permissions:

• Use Display Builder (Display Builder module): YES.
• Use the Limited Display Builder profile (Display Builder module): YES
• Use the Default Display Builder profile (Display Builder module): NO
• {entity_type}: Administer display (Field UI module): YES
• Administer views (Views module): YES
• Administer page layouts (Display Builder for page layout module): YES
• Article: Edit any content (Node module, to test entity view display overrides): YES

Testing ConfigFormBuilder:

• Can create/switch a display with Limited profile. Expected: YES. Results: ✅
• Can create/switch a display with Default profile. Expected: NO. Results: ✅
• Can add a new profile. Expected: NO. Results: ✅ No link and forbidden access.
• Can switch a display from limited to defaultprofile. Expected: NO. Results: ✅
• Can switch a display from default to limited profile. Expected: ???. Results: ⚠️yes I can. This behavior was not discussed until now, and need to have its own ticket

Page Layout:

• Can create a page layout. Expected: YES. Results: ✅
• Can load Display Builder for page layout with Limited profile. Expected: YES. Results: ✅
• Can load Display Builder for page layout with Default profile. Expected: NO. Results: ✅<
• Can alter the display builder instance with Limited profile. Expected: YES. Results: ✅
• Can alter the display builder instance with Default profile. Expected: No. Results: ✅ i got HTTP 403 (to do this test, we need to remove Profile access check first)

Entity view display:

• Can switch an entity view display to Display Builder. Expected: YES. Results: ✅
• Can load Display Builder for entity view display with Limited profile. Expected: YES. Results: ✅
• Can load Display Builder for entity view display with Default profile. Expected: NO. Results: ✅<
• Can alter the display builder instance with Limited profile. Expected: YES. Results: ✅
• Can alter the display builder instance with Default profile. Expected: No. Results: ✅ i got HTTP 403 (to do this test, we need to remove Profile access check first)

Entity view override:

• Can pick an existing field. Expected: YES. Results: ✅
• Can create a new field. Expected: NO. Results: ⚠️ We can't but the link is visible anyway. Let's hide it.
• Can switch a views display to Display Builder. Expected: YES. Results:✅
• Can load Display Builder for override with Limited profile. Expected: YES. Results: ⚠️ tab visible and access OK, but tab as "display" label instead of the display label
• Can load Display Builder for override with Default profile. Expected: NO. Results: ✅ tab not visible + HTTP 403
• Can alter the display builder instance with Limited profile. Expected: YES. Results: ✅
• Can alter the display builder instance with Default profile. Expected: No. Results: ??? (to do this test, we need to remove Profile access check first)

Views:

• Can switch a views display to Display Builder. Expected: YES. Results: ✅
• Can load Display Builder for views display with Limited profile. Expected: YES. Results: ✅
• Can load Display Builder for views display with Default profile. Expected: NO. Results: ???
• Can alter the display builder instance with Limited profile. Expected: YES. Results: ✅
• Can alter the display builder instance with Default profile. Expected: NO. Results: ✅ i got HTTP 403 (to do this test, we need to remove Profile access check first)

Role 2: has the "classic" permissions of Drupal display building, but not the ones from Display Builder

Permissions:

• Use Display Builder (Display Builder module): NO
• Use the Limited Display Builder profile (Display Builder module): NO
• Use the Default Display Builder profile (Display Builder module): NO
• {entity_type}: Administer display (Field UI module): YES
• Administer views (Views module): YES
• Administer page layouts (Display Builder for page layout module): YES
• Article: Edit any content (Node module, to test entity view display overrides): YES

Page Layout:

• Can create a page layout. Expected: NO. Results: ❌ i can create and i have a fatal error: Call to a member function id()
• Can load Display Builder for page layout. Expected: NO. Results: ✅
• Can alter the display builder instance. Expected: NO. Results: ??? (to do this test, we need to remove Profile access check first)

Entity view display:

• Can switch an entity view display to Display Builder. Expected: NO. Results: ???
• Can load Display Builder for entity view display. Expected: NO. Results: ✅
• Can alter the display builder instance. Expected: NO. Results: ??? (to do this test, we need to remove Profile access check first)

Entity view override:

• Can pick an existing field. Expected: YES. Results: ❌ yes i can, must be hidden
• Can create a new field. Expected: NO. Results: ⚠️ I can't, but it is an empty select.
• Can switch a views display to Display Builder. Expected: YES. Results: ⚠️ I can't, but it is an empty select.
• Can load Display Builder for override. Expected: NO. Results: : ✅ tab not visible and access forbidden
• Can alter the display builder instance. Expected: YES. Results: ??? (to do this test, we need to remove Profile access check first)

Views:

• Can switch a views display to Display Builder. Expected: NO. Results:⚠️ I can't, but it is an empty dialog. A message would be better.
• Can load Display Builder for views display. Expected: NO. Results: ✅
• Can alter the display builder instance. Expected: NO. Results: ??? (to do this test, we need to remove Profile access check first)

Role 3: has the Display Builder permissions, but not the ones from classic Drupal display building

Permissions:

• Use Display Builder (Display Builder module): YES
• Use the Limited Display Builder profile (Display Builder module): YES
• Use the Default Display Builder profile (Display Builder module): NO
• {entity_type}: Administer display (Field UI module): NO
• Administer views (Views module): NO
• Administer page layouts (Display Builder for page layout module): NO
• Article: Edit any content (Node module, to test entity view display overrides): NO

Page Layout:

• Can create a page layout. Expected: NO. Results: ✅
• Can load Display Builder for page layout. Expected: NO. Results: ✅
• Can alter the display builder instance. Expected: NO. Results: ???

Entity view display:

• Can switch an entity view display to Display Builder. Expected: NO. Results: ✅
• Can load Display Builder for entity view display. Expected: NO. Results: ✅
• Can alter the display builder instance. Expected: NO. Results: ???

Entity view override:

• Can pick an existing field. Expected: NO. Results: ✅
• Can create a new field. Expected: NO. Results: ✅
• Can switch a views display to Display Builder. Expected: YES. Results: ✅
• Can load Display Builder for override. Expected: NO. Results: ✅
• Can alter the display builder instance. Expected: YES. Results: ???

Views:

• Can switch a views display to Display Builder. Expected: NO. Results: ✅
• Can load Display Builder for views display. Expected: NO. Results: ✅
• Can alter the display builder instance. Expected: NO. Results: ???

@jean: Do we really need "Use Display Builder" permission? It seems this is the same has no "Use the {Profile} Display Builder profile" permission. Let's remove it if not useful.

Gitlab CI all green :)

Remaining work in the current issue:

• Can create a page layout without profile.
• Entity view override: Can't pick a field when no profile.
• We can switch profile even if we have no access to the actual profile. Do we turn the select in readonly?

Follow-up for post-beta1:

• #3548769: Security: availability of endpoints according to islands
• Do we really need "Use Display Builder" permission? It seems this is the same has no "Use the {Profile} Display Builder profile" permission. Let's remove it if not useful.
• UI tweaks:
  • Entity view override: no empty select when no access to profiles
  • Entity view override: hide "create field" link when no access to profiles
  • Entity view override: show display label in tab when only one display available
  • Views: no empty dialog when no access to profiles

DONE:

• We can switch profile even if we have no access to the actual profile >> The select is now disabled.
• Entity view override: no empty select when no access to profiles >> The select is now disabled.
• Can create a page layout without profile >> The submit button is now disabled
• Entity view override: Can't pick a field when no profile >> The select are now disabled, but there is still some tweaks to do
• Entity view override: hide "create field" link when no access to profiles >> Hidden
• Views: no empty dialog when no access to profiles >> We have a message now.

Needs review.

Only 128 lines of PHP code (tests, blank lines & comments removed) to add this very important mechanism ;)

Follow-up for post-beta1:

• #3548769: Security: availability of endpoints according to islands
• #3556166: UI tweaks for users with limited permissions

rename ConfigFormBuilderTest tests

done

Jean, on your 4 comments:

• I have fixed 2 of them
• I have answered the 2 others

Need to fix test ConfigFormBuilderTest, split AccessControlTest for each entity types and use data provider to have lighter and easier to maintain tests.

I would be happy to do this and fix the phpunit fail probably introduced by the rebase: https://git.drupalcode.org/project/display_builder/-/jobs/7264278

Don't worry. I will rebase your stuff and keep working ConfigFormBuilderTest

pushed. Pipeline in progress

For the split and data provider let me know if you want me to do it.

That would be nice if you do it because I am not sure about the way to do it:

• split: the 2 access handlers are working together, checking 2 layers of the same access, so it seems odd to me to split them (it may be because I am new to kernel testing)
• data provider: i don't see data providers in the other kernel tests we have, I was beliveing it was more a unit test thing

An other point is you assumed the 'profile' key in the tests, I changed it for the const ConfigFormBuilderInterface::PROFILE_PROPERTY.

oh! sure! 👍

You left some empty third arguments in the self::assert methods, it's used to describe the test, it makes it easier to read, I personally don't use it enough but that's good practice.

Good to know.