Problem/Motivation
I want to be able to create a role in Drupal that doesn't have the permission to bypass node access, but can create, edit, and delete content (including unpublished content) and be able to add menu links to them.
At the moment a user MUST have the bypass node access permission to be able to create a menu link as a child of an unpublished node via the node form
Steps to reproduce
Login as an editor without the bypass node access permission but ability to create menu links
Create a parent page leaving in draft
Create a child page and notice in the menu settings you can't select the parent.
Proposed resolution
Only add the query condition on node status from DefaultMenuLinkTreeManipulators::checkNodeAccess if the user doesn't have the 'view any unpublished content' permission or the project has no node_grants implementations. The query still checks access so should still filter the list for nodes that the user doesn't have access to.
This could be considered a bug as I find it odd that a user is able to create, edit, and delete content and menus/menu items but can't set a parent to an unpublished node that even they created.
Comment | File | Size | Author |
---|---|---|---|
#107 | 2807629-107-d9.patch | 8.23 KB | Mirjam Dissel |
Issue fork drupal-2807629
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:
- 2807629-allow-menu-items changes, plain diff MR !920
Comments
Comment #2
aditya.n CreditAttribution: aditya.n as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commented@acbramley, hi, I followed your instructions, and regenerated the issue. Yes I believe, this might be a bug and the user might feel limited with the permissions provided. I went through with your provided resolution, and am adding a patch for the same. After applying this patch, everything seems to work fine, and the user is able to add menu links as a child to the unpublished node.
Still the API and documentation changes are required Here, if this patch passes the tests.
Comment #4
aditya.n CreditAttribution: aditya.n as a volunteer and at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedPatch with revisions.
Comment #5
acbramley CreditAttribution: acbramley commented@androiditya thanks for that! I'll be interested to see what the response on this is as it does seem odd to set a hard condition on the published status of the node when the query has access checking anyway.
Comment #7
galooph CreditAttribution: galooph at Code Enigma commentedRe-rolled the patch so I could use it with 8.3.2.
Comment #10
idebr CreditAttribution: idebr at ezCompany commentedLet's add test coverage to show the intended behavior
Comment #11
idebr CreditAttribution: idebr at iO commentedSetting to 'Needs work' per #10
Comment #12
robpowell@idebr I am trying to determine the best way to write a phpunit test to cover the expected method result. Here is the test that was removed.
And the error message:
This makes sense to me as we removed the code the would make the query add that condition. However, I don't understand what we can add in that test's place. Can you expand what you would expect?
Comment #16
idebr CreditAttribution: idebr at iO commented#12 Attached patch adds test coverage to check unpublished nodes can be selected as a menu parent item.
Comment #18
idebr CreditAttribution: idebr at iO commentedThe NodeInterface use statement is now unused, so it can be removed.
Comment #19
LendudeSo we now have test coverage that we can add a menu link to an unpublished node we have access to, but I don't see any test coverage for this statement in the IS:
Pretty sure that it's true, but looking though
\Drupal\Tests\menu_ui\Functional\MenuUiNodeTest::testMenuNodeFormWidget
I don't see any explicit coverage for this. There is some coverage for inaccessible menu's, but not for inaccessible nodes as far as I can see.And bug vs feature...hmm sounds buggy to me, but since there is a specific line of code that explicitly adds this (but has no functional test coverage ¯\_(ツ)_/¯ ), it's probably a feature to remove it.
Comment #21
DamienMcKennaRerolled against the latest 8.9.x, and it applies cleanly to 9.0.x and 9.1.x.
Comment #22
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #23
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have fixed failed test of patch #21.
Comment #24
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs commentedComment #25
idebr CreditAttribution: idebr at iO commentedStill needs additional test coverage per #19
Comment #27
thatguy CreditAttribution: thatguy at Siili Solutions commentedPatch in #23 applies correctly in 9.0.7 and fixes the issue
Comment #31
pameeela CreditAttribution: pameeela commentedAdding credit from #3068508: Cannot save draft if menu parent is unpublished. and #3059790: If pages in the menu are unpublished, changes made in the menu UI can be lost when node is saved which are duplicates.
Comment #32
amateescu CreditAttribution: amateescu as a volunteer commentedComment #33
kalvis CreditAttribution: kalvis commentedThe patch in #18 can't be applied to Drupal 8.9.10. Here's an updated version based on #18 and latest 8.9.x branch.
Comment #34
seanBRerolled for 9.1.
Comment #35
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedFixed Custom Commands Failed.
Comment #36
KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commentedComment #38
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented@KapilV can you please add interdiff?
Comment #39
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #41
nicrodgersPatch no longer applies - needs a re-roll
Comment #44
pookmish CreditAttribution: pookmish commentedI re-rolled and did a PR instead of a patch for easier updating if needed.
I was successful on this update working on the latest 9.2.x and 9.3.x versions.
Comment #45
acbramley CreditAttribution: acbramley at PreviousNext commentedAdds test coverage as per #19
Comment #46
Bohus UlrychThank you @acbramley
patch #45 works with Drupal 9.2.1
Comment #47
mpp CreditAttribution: mpp at AmeXio for District09 commented#45 works on drupal/core (9.2.4).
Comment #51
SpokjeBack to RTBC per #47 after #3255836: Test fails due to Composer 2.2 solved the unrelated test failure.
Comment #53
SpokjeComment #54
cpierce42I pulled down patch 45 and this fixes the issue for me.
Comment #55
cpierce42After further testing this patch does fix the issue for me but creates another.
Draft content links do appear in menu for authenticated users who have permissions to see draft content.
Problem: Draft content links now show in menus for unauthenticated users.
Comment #56
thomas.crouch CreditAttribution: thomas.crouch as a volunteer commentedI've rerolled the patch and made it so users that are not logged in cannot view unpublished menu items, but logged in users with permissions can.
Comment #57
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedFixed CS errors.
Comment #59
acbramley CreditAttribution: acbramley at PreviousNext commentedBack to RTBC and please use patch #45. The permission check in #56 is not needed. If users can see unpublished nodes it's because they have access to them. This is proven by the tests added i #45 as the query is running through node access hooks.
If there is a bug, please post a failing test patch or provide steps to reproduce from a clean install.
Comment #67
larowlanI feel like this will leak into the anonymous user if you don't have a module that implements hook_node_grants
Comment #68
larowlanI think we almost need a lesser permission here, something like 'Reference unpublished nodes' which could also be used in the Node selection handler
There needs to be something between bypass node access and no access to unpublished content in ER fields, menu UI etc.
Also, I think this is a major bug, because it causes data loss as per #3059790: If pages in the menu are unpublished, changes made in the menu UI can be lost when node is saved which was closed in favour of this.
Comment #69
larowlanComment #71
alienzed CreditAttribution: alienzed commentedAny news on this? My site's editorial workflow is severely disrupted here :S
Comment #72
GaëlGComment #73
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded reroll of patch #57 on Drupal 10.1.x., keeping the status needs work for comment #68.
Comment #74
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedFixed failed commands on #73
Attached interdiff patch
The comment #68 needs to be addressed
Comment #75
szloredan CreditAttribution: szloredan at 1xINTERNET commentedReroll for 9.5x
Comment #76
pameeela CreditAttribution: pameeela at Technocrat commentedComment #77
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedComment #78
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Updated IS with steps to reproduce.
Tested manually following those steps and can confirm I can now add child nodes to unpublished parents without the bypass node access permission.
The patch contains valid tests.
So +1 for me.
Comment #79
larowlanThis introduces a security issue, we're now not limiting access to menu items that point to unpublished nodes for anonymous users
And the test is adding a hook to check specifically for the actual item.
My comment/suggestion at #68 hasn't been addressed either.
Please don't reroll or use patches #73 through #75 otherwise you're introducing a security risk on your site.
Comment #80
larowlanSorry, the issue is with patch #75 only, #73 and #74 have an extra check.
Comment #81
idebr CreditAttribution: idebr at iO commented#68
Introducing a new permission does not seem very sensible at this point, since this issue already requires checking for specific permissions for generic use cases, for example #2845144: User can't reference unpublished content even when they have access to it
As a result, the access check can easily result in incorrect access cache metadata: #3278759: Access cacheability is not correct when "view own unpublished content" is in use
As a more general solution, we can rely on either node access or the broad 'view any unpublished content' permission. Note that the 'view own unpublished nodes' permission was explicitly ignored to not require cache entries per user in #2250315: Regression: Bring back node access optimization to menu trees. This requirement is untouched.
#79.1 Added explicit coverage to test unpublished content is not available in the 'Parent link' select list
#79.2 Removed the hook from the test that checked for an actual item.
The patch implements the following changes:
Comment #82
idebr CreditAttribution: idebr at iO commented#81 rerolled for 9.5.x
Comment #83
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeem to have CI errors in #81.
Comment #84
idebr CreditAttribution: idebr at iO commentedAdded a property for $moduleHandler, fixing the PHPstan error.
Comment #86
LendudeSo we now have the check for node_grants being used, but we don't have test coverage for this. There should be some test modules around that we could install in the test to cover this I think
First I was worried that we only end on negative assertions, but optionNotExists does check if the specified select element exists and fails otherwise, so that is great.
Also the current proposed fix no longer lines up with the proposed fix in the IS, so the IS could use an update
Comment #87
idebr CreditAttribution: idebr at iO commented#86.1 Added a test with the node_grants implementation from node_access_test
#86.2 Updated the issue summary proposed solution in line with the patch
Comment #88
LendudeNitpick: Double login
Unrelated changes, if these were the only bad testing namespaces left in core I'd be ok with sneaking it in here, but they are not, so let's not fix them here, but do a new issue to fix this everywhere.
If I search core with regex "\\Drupal\\(.*)\\Tests\\" I find a whole bunch of these all over core.
Did a quick look but couldn't find an existing issue for it.
Edit: better regex "\\Drupal\\(\w+)\\Tests\\"
Comment #89
idebr CreditAttribution: idebr at iO commented#88.1 Removed the duplicate login
#88.2 Removed the unrelated changes and filed a follow-up issue at #3337653: Update outdated class references from Drupal\*\Tests to Drupal\Tests\*
Comment #92
LendudeLooks ready to me, thanks!
Comment #93
Fernly CreditAttribution: Fernly at Dropsolid for District09 commentedOnly patch #82 worked for me (D 9.5.3).
Patches #84, #87 and #89 did not apply.
Edit: Didn't notice that those patches are for D10. All is good.
Comment #94
larowlanCan we confirm that this test has existing coverage for the unpublished status filter.
It looks like node 3 in that instance covers the unpublished case.
But I can't see a functional test in core to ensure that an unpublished node link isn't output to the page.
If we confirm we have existing coverage for that I'll be happy we're not accidentally creating an access bypass here.
The fact tests didn't fail on #75 makes me think that we only have this unit test, and it is too tightly bound to the current implementation to fail when we make this behaviour change.
So to be clear, we need to confirm we have a test that renders a menu with a link in it that points to an unpublished node, that confirms the link isn't output to users without access.
Sorry for being pedantic here, but security is something we've always prided ourselves on, and I don't want to be the one to commit something that harms that.
Comment #95
idebr CreditAttribution: idebr at iO commentedExisting functional test coverage is available \Drupal\Tests\menu_ui\Functional\MenuUiNodeTest::testMenuNodeFormWidget(). This test implements a scenario where an editor is able to add/edit an unpublished node, but not view it:
Comment #96
larowlanQueued a 9.5 test
Just to be super cautious, I did some manual testing of this and it works well.
Steps I took:
* Install umami with patch applied
* Modify permissions for Editor role to allow editing menu
* As admin create some pages, mix of published and unpublished and place them in the main menu
* As editor edit a page (cannot create pages in Umami as Editor which feels odd, but is not related), verify you can see the unpublished items as menu parent
* View page as editor, unpublished items show up in the menu
* View page as anonymous, unpublished items do not show up
* Confirm that both pages are still cacheable (for editor with DPC, for anon with page cache and DPC)
In my screenshots, rubbish and recycling is the unpublished item
Screenshots are for anon, and for editor
My screenshot tool won't capture the open select field, but the unpublished item is in the list
Saving issue credits
Comment #98
larowlanWhilst this is a bug report, we're introducing a new deprecation and changing the parameters to the menu tree manipulators service - so that means it can only be in a new minor.
I've committed this to 10.1.x and published the change notice.
Thanks folks
Comment #99
larowlanOh, and I meant to say thanks to @idebr for doing the work to find the test coverage I was asking about, much appreciated.
Comment #100
idebr CreditAttribution: idebr at iO commentedNice to have this committed, thanks!
Comment #101
karenann CreditAttribution: karenann commentedRecently updated one of my instances to 9.5.2 and PHP 8.1.14.
I removed patch #42 from my codebase and the problem I was having reappeared. The problem I have is that, on the edit screen, the "Menu Settings > Parent link" dropdown will omit any unpublished items and all of it's children.
I applied #82 and my problem is resolved.
Comment #102
acbramley CreditAttribution: acbramley at PreviousNext commented@karenann the fix has not been backported to 9.5, it is only in 10.1.x. You will need the patch until upgrading to 10.1
Comment #104
douggreen CreditAttribution: douggreen at Peak Digital, LLC commentedAttached is the backport for 9.x.
Comment #105
sraLton CreditAttribution: sraLton at Previon Plus AG commentedHere is a patch for Drupal 9.5.10
Comment #106
mahdeCannot apply the latest patch on Drupal 9.5.10!
Comment #107
Mirjam Dissel CreditAttribution: Mirjam Dissel at Flink [NL] commentedI've looked into why the 2807629-105.patch wouldn't apply on D9.5.10 and it was just a small fix in de core.services.yml part.
Here it is again!