Follow up for #1871772-108: Convert custom blocks to content entities

Problem/Motivation

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockAccessController.php
@@ -0,0 +1,47 @@
+  public function viewAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
+    return user_access('view content', $account);

Hm. That user permission belongs to Node module.

We definitely need a (major) follow-up issue for that.

  • Custom block module re-uses 'view content' permission which is from Node module.
  • We need to have a discussion about whether we want a new 'view blocks' permission.

Proposed resolution

Determine if 'view blocks' permission is warranted and implement as required.

User interface changes

'View published content' permission goes from 'Node' module to 'System' module in the user permissions page.

CommentFileSizeAuthor
#9 block-access-1919928.9.fail_.patch3.19 KBlarowlan
#9 block-access-1919928.9.pass_.patch3.78 KBlarowlan
#4 block-access-1919928.4.interdiff.txt1.64 KBlarowlan
#4 block-access-1919928.4.patch1.67 KBlarowlan
#2 block-access-1919928.2.patch769 byteslarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Resolve usage of 'view content' in modules other than node » Resolve usage of 'view content' permission in modules other than Node
Status: Postponed » Active
Issue tags: -D8 upgrade path

This is a fairly hairy situation.

For one, I wouldn't have expected any kind of correlation to that Node module permission.

OTOH, a separate "view block" permission doesn't make sense either.

Perhaps we should move the Node module permission to somewhere else; e.g., System module or similar? So that it is perceived as a global "view content" permission?

Another alternative would be to remove the view permission entirely for custom blocks. AFAIK, custom blocks of D7 and below didn't have any permissions either?

Additionally, block plugins support an optional visibility constraint by roles already — which essentially duplicates this view permission.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Active » Needs review
FileSize
block-access-1919928.2.patch769 bytes

I like the idea of moving view content to system module however thats a UX change, and a big one - people are used to seeing that in the 'node' section on the permissions screen.

So I think the best bet is to make it return TRUE. If contrib or a particular site build dictates something different, they can just hook_entity_info_alter() in an alternate access controller. Access controllers ftw!.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Hm. I'd sooner move the permission, personally. A permission moving from one fieldset to the other is going to be the least of the changes in Drupal 8, UX-wise. ;)

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
block-access-1919928.4.patch1.67 KB
block-access-1919928.4.interdiff.txt1.64 KB

Works for me
This patch moves 'access content' permission to system module and updates access controller for blocks.
Turns out its 'access content' too

Updated issue summary to list UX changes

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

I'm really not sure whether I agree with @webchick... spent the past couple of days sleeping over it.

Associating the "access content" permission with blocks inherently means that a user who does not have that permission... will potentially see... Nothing at all.

Custom blocks are not really "content" in the "page content" sense — they consist of content that appears in sidebars, the header, or the footer. There's hardly a point in associating them with a user permission.

What's much more likely is that you want to control visibility of very specific / individual block instances to show up for certain roles only — and that's what we have in the block-instance-specific visibility options already.

All of that being said...:

   public function viewAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {
-    return user_access('view content', $account);
+    return user_access('access content', $account);
   }

WAT? :)

How are existing tests able to pass with this code in HEAD? ;)

Looks like we do not have any tests for custom block access? If so, we'd have to add tests in this patch, in case we keep the permission. If we drop it, then there's nothing to test.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes

Updated issue summary.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

I agree with sun. Blocks, by their nature should be access agnostic as much as possible, and allow for access and visibility to be defined for them by the site builder. I'm working very hard on visibility control mechanism for blocks that uses a pluggable architecture here. Ultimately, if users think they're setting visibility, and then find out they don't have access, that's going to be a wtf.

I would encourage us to actually remove any sort of access check here.

Eclipse

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Ok, so that means we're back to the patch at 2.
Re why aren't we getting failures in HEAD, entity_page_access isn't used for blocks as we don't have a page callback to view the block on. So agreed that we need tests if we do use moving omission to system module approach

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

Couldn't we just remove the access controller completely?

Eclipse

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
FileSize
block-access-1919928.9.pass_.patch3.78 KB
block-access-1919928.9.fail_.patch3.19 KB

We need the controller, no questions asked, it is being used for other operations - not just for 'view' yet.
Here is a fail patch to address the 'view content', 'access content' snafoo.
The test adds a view callback in the test module.

As well as the fix (same as 2).

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks for the additional test coverage! I still think this makes the most sense.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

The main thing I was concerned about here is whether or not we'd introduce a security regression in sites upgraded from D7 for blocks that expected to not be seen publicly in the case of e.g. intranet sites. However, it doesn't look like this code is present in D7 (the permission name seems to be wrong in any case? It's "access content" no?).

With that concern resolved, I think this is good to go.

Committed and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.