Postponed on #2809177: Introduce entity permission providers

Problem/Motivation

Content moderation is not all about nodes since it is quite easy creating a custom content entity and make sure content moderation works on that. The module also introduces a "Latest version" local task on moderated entities, but in the related \Drupal\content_moderation\Access\LatestRevisionCheck access checker only validates if the current user has view (any/own) unpublished content permission, which only applies to nodes...

Proposed solution

Check the type of the entity object and only apply view (any/own) unpublished content when the type of the object is NodeInterface, otherwise use the view (any/own) unpublished [entity_type] permission, which may or may not exists.

Update: assumption: it should also respect the "administer [entity_type]" permission.

Related issues

There is a currently open patch in the Entity API module that would introduce a view any unpublished [entity_type] permission, view own is already supported. Add: "View any unpublished [entity_type]" permission [#3023527]

There are several open content moderation issues related to this problem as of today:

  1. Move permission "view any unpublished content" from Content Moderation to Node [#273595]
  2. Add per-bundle unpublished content permissions
    [#2875867]
  3. Add a "view any unpublished media" permission
    [#2936652]
CommentFileSizeAuthor
#6 interdiff_3_6.txt2.82 KBmxr576
#6 content_moderation-support-view-any-unpublished-entity-type-permission-3160748-6.patch2.08 KBmxr576
#3 interdiff_2_3.txt3.22 KBmxr576
#3 content_moderation-support-view-any-unpublished-entity-type-permission-3160748-3.patch3.5 KBmxr576
#2 content_moderation-support-view-any-unpublished-entity-type-permission-3160748-2.patch2.64 KBmxr576

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Issue tags: +Needs tests
StatusFileSize
new content_moderation-support-view-any-unpublished-entity-type-permission-3160748-2.patch2.64 KB
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
Title: Support "View any unpublished [entity_type]" permission » Support "View any unpublished [entity_type]" and "Administer [entity_type]" permissions
Issue summary: View changes
StatusFileSize
new content_moderation-support-view-any-unpublished-entity-type-permission-3160748-3.patch3.5 KB
new interdiff_2_3.txt3.22 KB

Just realised that the access control should also respect the "Administer [entity_type]" permission as well, should not it?

This patch breaks tests for sure.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented

It seems the Administer [entity_type] permission is not respected by edit forms either, so having it does not mean "bypass content moderation". Besides this permission, the user must have at least one [entity_type] moderation workflow: Use [transition] transition. permisison to have access (again) to the edit form. Is it by design?

sam152’s picture

sam152 commented
Issue tags: -content entity type

Before adding tags read the issue tag guidelines.

I can see what you mean, 'view any unpublished content' is implied to be a generic permission for all entity types, since it's defined in content_moderation, but 'view own unpublished content' is defined in node and you'd be forgiven for assuming this would apply to nodes only.

Two issues:

  1. There is no guarantee permissions will be defined in the pattern you've specified, so we should probably look at moving this to the moderation handlers.
  2. Sites may be depending on this permission to provide latest-version access to entity types, leaning on the permissions defined in node to facilitate access. Pushing this change through would break those sites. So this would either need a change record communicating the change or some BC layer.
mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented
StatusFileSize
new content_moderation-support-view-any-unpublished-entity-type-permission-3160748-6.patch2.08 KB
new interdiff_3_6.txt2.82 KB

There is no guarantee permissions will be defined in the pattern you've specified,

I tried to provide some sane defaults that works better for most of the case compared with the current one. These patterns are based on permissions provided by the Entity API module. Anybody who is building a custom entity without the Entity API module, for sure, it needs a custom moderation handler.

I also had a conversation with Berdir on Slack and based on that, probably it would be better to check if the user has edit access to the entity, which should cover any custom logic, including administer/edit any(own) permissions, etc.

berdir 21 days ago
because the administer content permission is not what you think it is

berdir 21 days ago
pretty much all it does is allow access to some fields on the node edit, like author, promote/sticky and so on. the permission that allows to actually edit content is bypass node access

berdir 21 days ago
it's a weird leftover that should be split into specific, dedicated permissions and deprecated

mxr576 20 days ago
So the logic in content moderation should respect the "bypass node access" permission? (I haven't checked if it does, but I will if you say yes)

mxr576 20 days ago
it's a weird leftover that should be split into specific, dedicated permissions and deprecated
just to be clear, are you referring to administer or bypass? :slightly_smiling_face:
Because entity API does not provide "bypass" permissions, it only provides "Administer" so I think in general it makes sense that if someone has Administer [entity_type] has full access to all entity operations all the time.

berdir 20 days ago
I mean administer nodes

berdir 20 days ago
and yes, most node types only have a single administer permission, but that's not how nodes work

berdir 20 days ago
administer nodes has been split into bypass node access and access content overview in D7 and administer nodes is just used for the remaining bits

berdir 20 days ago
\Drupal\content_moderation\Access\LatestRevisionCheck::access()

berdir 20 days ago
https://www.drupal.org/project/drupal/issues/3010770
Drupal.org
LatestRevisionCheck should assume that if a user can edit an entity, they should have access to the latest-version route
Problem/Motivation I'm using content moderation in combination with the group.module, so I'm relying on node grants from that for edit access and normal editors do not have "view any unpublished entity". As a result, /latest works fine for content that the user himself creates as he does have view own unpublished, but it doesn't work for entities that someone else created.
Nov 1st, 2018

berdir 20 days ago
the access check should be more flexible, but not administer nodes, my proposal was that it should check node edit access

mxr576 20 days ago
and what about other entity types that are not nodes? Should the access check respect (besides edit?) the administer permission specific to that entity type?

berdir 20 days ago
that's the point of my proposal

berdir 20 days ago
each entity type has its existing system on how to decide who can edit

berdir 20 days ago
with access control handler and hooks and so on. this would just use that api

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
commented 
1) Drupal\Tests\content_moderation\Unit\LatestRevisionCheckTest::testLatestAccessPermissions with data set #0 ('Drupal\node\Entity\Node', 'node', true, array('view latest version', 'view any unpublished content'), false, 'Drupal\Core\Access\AccessResultAllowed')
TypeError: Argument 1 passed to Drupal\Core\Access\AccessResult::orIf() must implement interface Drupal\Core\Access\AccessResultInterface, null given, called in /var/www/html/core/modules/content_moderation/src/Access/LatestRevisionCheck.php on line 61

So a few mocks needs to be adjusted, but otherwise it is looking promising...

(Plus we probably need an extended test coverage because the above described issue was not discovered by the current test coverage.)

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

pameeela’s picture

pameeela commented
Status: Active » Postponed (maintainer needs more info)
Issue tags: +Needs issue summary update

@mxr576 is this something you are still interested in? I think the issue summary needs to be updated based on your most recent comments, but it also sounds like this might be a task or two rather than a bug?

Either way if you could provide some more information this is more likely to progress.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Postponed

I think this should be postponed on #2809177: Introduce entity permission providers for now.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.