Problem/Motivation
By default content moderation module gives View any unpublished content
permission to view the all the unpublished content in the site. But we shouldn't able to restrict the unpublished content view access of particular content type. So this will be too permissive when complex workflows enanled.
Proposed resolution
Add view any unpublished [ENTITY_TYPE:BUNDLE] content
permissions.
Remaining tasks
Make the code changeAdd tests
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#56 | 2875867-56.patch | 19.69 KB | kevin.dutra |
#49 | Screenshot from 2020-01-03 10-04-46.png | 14.91 KB | iyyappan.govind |
#46 | interdiff-42-46.txt | 1.39 KB | iyyappan.govind |
#46 | 2875867-46.patch | 19.67 KB | iyyappan.govind |
Issue fork drupal-2875867
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
timmillwoodThere's also talk about moving the
view any unpublished content
to core rather than content_moderation. #273595: Move permission "view any unpublished content" from Content Moderation to NodeComment #3
jhedstromThis will be more readily implementable once #2865498: Latest revision tab should respect 'view own unpublished content' permission is in place.
Comment #4
jhedstromHere's a first attempt at this.
Comment #6
jhedstromThis updates the previously failing test class to utilize the class resolver service instead of directly instantiating the permissions class.
Comment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedReview as follows. I know there has been some friction in other issues that introduce a lot of permissions. I think it makes sense though because node allows you to customise everything per-bundle.
We could just inject the result of getDefinitions() here if we aren't using all of entityTypeManager.
It would be good to have a test for an entity without bundles and ensure we don't get two permissions that do that same thing (wouldn't want a permission for "entity_test" and "entity_test:entity_test").
Missing braces and indent.
Comment #8
jhedstromI think this addresses #7.
I was going to add additional entity/bundle tests to
ContentModerationPermissionsTest
, but wanted to use the test entity types, and discovered #2886203: Clarify scope of each entity_test entity type. Rather than block this on that issue, we could use node to test here...Comment #9
timmillwoodI think this looks pretty good, very tempted to RTBC, but two very minor comments first, so leaving as Needs Review:
Something does feel right about doing this same check twice. I guess it makes sense, if access is not allowed check this, if access is still not allowed check this, etc. Although I wonder if there is a better solution.
I'm surprised we don't have one or more get methods in ModerationInformation that would do this. Like getBundlesThatShouldModerate() or something.
Comment #10
jhedstromThis level of verbosity for access-related things was initially introduced in this case in #2865498: Latest revision tab should respect 'view own unpublished content' permission, so it seemed to make sense to continue that pattern.
Comment #11
jhedstromSince the test entity with bundle isn't currently revisionable, this adds node types to the kernel test so that both bundle and non-bundle entity types are tested.
I didn't see anything like that... we could add it in a follow-up?
Comment #13
timmillwoodComment #14
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #15
vijaycs85Comment #16
jhedstromPatch no longer applies due to some coding standards fixes.
Also, it needs to be updated to include the new permission in
content_moderation_entity_access()
:Comment #17
jhedstromGoing to work on #16.
Comment #18
jhedstromThis updates the entity access hook to add the new permission, and adds test coverage of that code path.
Comment #19
dawehnerJust a general comment. I would expect
\Drupal\node\Plugin\views\filter\Status::query
to be extended in some way or another here.Comment #20
jhedstromGood point @dawehner! I hadn't even thought of that. Since it doesn't currently factor in the content moderation
view any unpublished content
permission either, perhaps this could be done in a follow-up issue?Comment #21
timmillwood\Drupal\entity_test\Entity\EntityTestNoBundle
just to make sure when the$bundle
is added to a permission name / title it still works ok when there is no bundle entity key on the entity.Comment #22
oakulm CreditAttribution: oakulm as a volunteer and at Drontti Oy commentedHi,
I applied patch in #18 to clean Drupal 8.5.x
Made 3 roles, 2 content types and 2 users. As far as I can see the patch does the job.
Few observations:
Users can comment on unpublished nodes if they have "view any unpublished x content". This probably is as it should?
You can create permission set where user can create nodes (unpublished) but can't see them after creating them. This is also fine, I think.
There was one small glitch while patching:
Comment #23
timmillwoodI still think we need the extra test, as stated in #21.
Comment #24
jhedstromThanks for the review @timmillwood.
This adds the no-bundle test entity. I've also added #2935732: The node views status filter is not aware of content moderation's unpublished permissions for the views issue @dawehner mentioned in #19.
For #273595: Move permission "view any unpublished content" from Content Moderation to Node, I'd say if this goes in first, it can be updated to move these as well (and the other way around should that go in sooner).
Comment #26
jhedstromThe fails in #24 were due to the change to this test in #2932154: ModerationInformation::getLatestRevisionId returns access-specific results which turned on
node_access_test
which was making the nodes private in the failing test. Since this test is about per-bundle permissions, the fix was to disable private nodes for the new test.Comment #28
Georgii CreditAttribution: Georgii as a volunteer commentedI have just applied the patch in #26. Worked for me in my system with 4 content type. Tried different combinations. Viewed
node/%nid/latest
having different permissions selected. Changed permissions and tried again. All works as expected.Thank you @jhedstrom !!!
Comment #29
jhedstromRe-roll of #26 after #2572613: Fix 'Drupal.Array.Array.CommaLastItem' coding standard went in.
Comment #30
timmillwoodLooks good, think it's ready for committer review.
Comment #31
tstoecklerShould we check whether
permission_granularity
is set tobundle
for a given entity type before adding this to all entity types?Comment #32
jhedstromI had forgotten this existed at all. That makes a ton of sense. Working on that now.
Comment #33
jhedstromThis adds a check for permission granularity.
Since we don't have any test entity types that specify
bundle
as the permission granularity, this change also changes the test expectations, but I think that's ok.Comment #35
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation for Pfizer, Inc. commentedShould we focus instead on #2809177: Introduce entity permission providers ?
Comment #37
alisonHmm, I'm not sure #2809177: Introduce entity permission providers will cover this functionality...? It doesn't look like it -- the closest I'm seeing so far is "view own unpublished ENTITY_TYPE" -- not "view own unpublished (BUNDLE) ENTITY_TYPE," or "view unpublished ENTITY_TYPE" or "view unpublished (BUNDLE) ENTITY_TYPE".
I'll post a comment over there, too, just to see.
Comment #39
Wim LeersLGTM, just nits 😅
🤓 Nit:
$entity_type_definitions
would be more accurate.🤓 Nit: "per entity type", not "per entity"
🤓 s/Access/Assess/ or perhaps "assert".
Comment #40
Madhura BK CreditAttribution: Madhura BK at UniMity Solutions Pvt Limited for Drupal India Association commentedHave made changes suggested in #39.
Comment #42
iyyappan.govindHi Folks, I have updated the patch to fix the coding standard and using the deprecated class usage. Please review it. Thanks
Comment #43
iyyappan.govindComment #44
iyyappan.govindComment #45
Wim LeersGreat! Just one detail was forgotten 😅
This should be named
$entityTypeDefinitions
.Comment #46
iyyappan.govindHi Wim,
I have updated patch with your suggestions mentioned in #45. Please review the patch. Thanks
Comment #47
Wim LeersLGTM!
To reach RTBC, we need some meta work to be done too.
Comment #48
iyyappan.govindComment #49
iyyappan.govindComment #50
iyyappan.govindHi All, I have added the change record for this issue. Please review. Change record URL - https://www.drupal.org/node/3104146
Comment #51
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedReviewing this with the benefit of some foresight, two things stand out to me as potential issues or caveats:
node
permission "view own unpublished content" doesn't have the same per-bundle support.\Drupal\node\Plugin\views\filter\Status
, I don't know how these new permissions will interact with this existing feature.I also think it's cleaner if all these test cases are additive instead of modified.
Comment #53
ressa CreditAttribution: ressa at Ardea commentedAdding related view_unpublished issue.
Comment #56
kevin.dutra CreditAttribution: kevin.dutra at Workday, Inc. commentedSimple re-roll of #46 for 9.2.x
Comment #57
ressa CreditAttribution: ressa at Ardea commentedThanks @kevin.dutra! Perhaps an interdiff can be added and Status changed to "Needs review", to get it tested?
Comment #58
kevin.dutra CreditAttribution: kevin.dutra at Workday, Inc. commented@ressa, that was just a re-roll of the patch in #46, so it doesn't address the comments in #51 and as a result it's not ready for review. There's no interdiff because there are no code changes from the #46 patch, just adjustments to the code context so that Git can find the correct places to apply the changes.
Comment #59
ressa CreditAttribution: ressa at Ardea commentedI see, thanks for clarifying. I originally just did a quick comparison between the two patches, and saw a bigger change, which I thought was the removal of a service. Looking closer, I now see that that change is in the current 9.3.x-dev version.
Comment #61
TMWagner CreditAttribution: TMWagner at Tableau commented@kevin.dutra For what it's worth, #56 applies cleanly to core 9.2.5