Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
In times of decoupled drupal, site builders may decide that they don't really need the node module in their sites.
But there is a problem the access content
permission is provided by the node
module and there are several places where it is used to check access for non-node-related things.
In particular:
- File module:
'/file/progress/{key}'
- System module:
/machine_name/transliterate'
- User module: The
Who's new
view. - Taxonomy module: The taxonomy module use it in
TermAccessControlHandler
which makes complicated to decouple it from the node module #2339235: Remove taxonomy hard dependency on node module
Proposed resolution
If the node module is not enabled the declare the access content
permission in the system module.
This keeps the access content
in the Node section of the permission where the other content permission are.
Remaining tasks
None.
User interface changes
None
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#53 | 2892821-2-53.patch | 5.72 KB | alexpott |
#53 | 45-53-interdiff.txt | 6.49 KB | alexpott |
#46 | 2892821-45.patch | 3.78 KB | alexpott |
#46 | 43-45-interdiff.txt | 1.8 KB | alexpott |
#43 | 2892821-43.patch | 3.95 KB | Wim Leers |
Comments
Comment #2
dagmarLet's see is this breaks something.
Comment #3
dagmarComment #5
dagmarMaybe this can be answered by the node maintainer. Btw, this is blocking my progress with: #2339235: Remove taxonomy hard dependency on node module
Comment #6
Wim LeersThis is a blocker of #2843139, see #2843139-78: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler.
A duplicate of this was created: #2907121: Move 'access content' permission to system module.
Comment #7
Wim LeersIS refined.
Comment #8
Wim LeersComment #9
dawehner❓ I'm wondering whether we could add a description which makes it clearer that this is not just about nodes
Comment #10
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'm not really sure we want to move the permission from node.module to system.module as #2 proposes. I haven't caught up on everything that's been written in #2870018: Should the 'access content' permission be used to control access to viewing configuration entities via REST yet, but for anyone else reviewing this, that would be a good place to start.
I do agree that it's currently a bug though for the places identified in this issue's Problem/Motivation section to be checking a permission that's defined in node.module. So we need to either move the permission, or else figure out one or more new permissions that would be more appropriate for those cases.
In trying to test what happens to those cases currently when node.module is uninstalled, I ran into the following quirks:
However, if I locally edit
minimal.info.yml
, comment out the- node
dependency and install the profile that way, then I can get the following bugs:since node module isn't installed.
/machine_name/transliterate
returning an access denied.Comment #11
Wim LeersThis would be a BC break.
Comment #12
tedbowI agree with #11 that creating new permissions for the current use cases would be BC break.
If we created new permissions we could in an update assign the new permissions to any role that currently role has 'access content' permission the new permissions we create. But I don't think that would cover BC concerns. Custom code and contrib could already have logic that checks for 'access content' permission for interacting with portions of their site that have to deal with viewing taxonomy with correct expectation that if they have that permission they will be able to view taxonomy terms.
Although I don't like the fact that we use 'access content' and we label nodes on the UI as 'content' and we use this permission for other things that aren't nodes I feel like we are stuck with it.
Comment #13
Wim LeersPinged yoroy on IRC. I'm tempted to move this back to RTBC, but #10 means we need sign-off from a usability maintainer.
Comment #14
yoroy CreditAttribution: yoroy at Roy Scholten commentedNot sure what to review. What's the decision or trade-off we're looking to make here?
Comment #15
seanBDidn't even know all of the cases this permission was used. It is totally unclear that you grant access to other things than nodes when adding the permission for a user.
I agree separating the permission and creating new ones for the use cases that 'Access content' was abused is the real solution. I also see how this breaks BC.
If we are choosing to keep this permission and move it, I think we need to at least update the title as mentioned in #9.
One a side note:
Using 'Access content' for anything other than checking access to content (nodes) is wrong imho, so I wonder if we could think of a way to make this clear and fix this before Drupal 9.
Comment #16
Wim Leers#14: short version: the reality is that the
node
module'saccess content
permission is being used for a lot more than just granting access to nodes (== "content"). Therefore this issue proposes to move that permission from thenode
module to thesystem
module, to reflect reality.The question for you: is this okay from a usability POV?
#15: RE sidenote: no, fixing this before Drupal 9 would be a BC break. And fixing it not before 9, but in 9, would also be a BC break, per https://dri.es/making-drupal-upgrades-easy-forever.
Comment #17
yoroy CreditAttribution: yoroy at Roy Scholten commentedThe issue summary says no user interface changes. If that's correct then I don't see how moving code around needs usability review. If it does change the UI or changes the meaning of what the permissions (don't) do or where people can find them then the issue summary needs an update.
Comment #18
tedbow@yoroy re #17 I have updated summary to include the UI changes, namely a change on
admin/people/permissions
Comment #19
Bojhan CreditAttribution: Bojhan as a volunteer commentedI am not convinced this is good from a usability point of view. All content related permissions are there, and we would suddenly be splitting of just one - but the most binary one, to a different category - to support a >10% usecase.
I suggest we add logic, that in the case the "node" module is disabled, that we move it to "system".
Comment #20
tedbowUpdating with @Bojhan's suggestion.
I like this because it won't have an effect on vast majority of sites because most will have Node enabled.
Not providing a interdiff because no changes left from #2
Updated IS to reflect no "User interface changes" changes.
Comment #21
yoroy CreditAttribution: yoroy at Roy Scholten commentedThanks for the review @bojhan, it captured my thoughts as well. Glad to see we can accomodate the suggested added logic, thanks @tedbow.
Comment #23
tedbowWhoops copy/paste error forgot to update to reference new class in System module
Comment #24
Wim LeersBojhan++ — that strikes a nice middle ground! ❤️
s/enabled/not installed/
Comment #25
tedbow@Wim Leers thanks for review. Fixed
Comment #26
tedbowUpdate the summary to reflect the change to leave the permission in the node module but provide it in the system module if the node module is not enabled.
Comment #27
Wim LeersComment #28
Wim LeersThis blocks #2843139: EntityResource: Provide comprehensive test coverage for File entity, and tighten access control handler, which is the last blocker for #2824572: Write EntityResourceTestBase subclasses for every other entity type.!
Comment #29
alexpottI like the approach but it looks as though we're missing some test coverage.
Can of worms but the word "published" is extremely problematic here. I have no idea what to do about that.
Comment #30
Wim LeersYou'd like test coverage that shows that the
access content
permission is now available even when thenode
module is not yet installed, I think? Or is there something else you'd like to see tested?Yes :( This is already problematic in HEAD. Legacy. Hard to fix. I do think this is out of scope here, since it's already a problem in HEAD. This issue/patch does not make the problem worse.
Comment #31
alexpottYep a test that shows the perm is still there when node is not is exactly what we need. And yes I agree that not dealing with the published can of worms is acceptable here.
Comment #32
zaporylieOn it
Comment #33
zaporylieHere you go.
Comment #35
tedbow@zaporylie thanks for the patch!
Instead of the checking for the check we could just check for
$permissions['access content']['provider'] === 'system'
. Then we would still only need 1 assert but also confirm that system is providing it.Comment #36
zaporylieAs you wish.
Probably no need to upload test-only patch again so I'm skipping it here for sake of DrupalCI.
Comment #37
tedbowI think @group should be "system" here since
Permissions.php
is system module..should we add @covers here?
Comment #38
zaporylieI've used
@group Drupal
because neighbour test file within the same dir was using it (PathHooksTest.php). But I see now most of system Kernel tests use@group system
so that should be probably changed. I'll add issue to address incorrect test group in PathHooksTest.php file.Adding @covers for now - it can be removed on commit if not needed.
Comment #39
tedbow@zaporylie looks great. RTBC again since was RTBC in #27
We now have tests per @alexpott's request in #29.
I agree with @Wim Leers in #29 that the 'View published content' permission string is existing problem and out of scope here
Comment #40
Wim LeersI think this is a good start, but we want a bit more.
After this, install the
node
module, then check that the provider is'node'
, not'system'
.And then uninstall the
node
module, and check that the provider is again'system'
.Comment #41
tedbow@Wim Leers good point.
Here is this test changes. They aren't passing for me locally. I am not sure if I need to do anything besides install the module to get the permissions to reset.
Comment #43
Wim LeersThe reason #41 fails is pretty simple: you're using services from before the module was installed, i.e. without a rebuilt container. Converting this to a
BrowserTestBase
test will make that easier. Confusing indeed — I've pulled my hair out over this in the past.Also fixed a bunch of nits.
Comment #44
Wim LeersOh one more nit: s/existence/provider/
Comment #45
zaporylieSince you transformed Kernel test into Functional test, namespace should reflect that change.
Comment #46
alexpottIt can still be a KernelTestbBase - in fact it is slightly easier with a KTB because its container gets updated when the container is rebuilt via the module installer.
Fixed #44 too. And #45 - because it is a KTB again :)
Comment #47
alexpottJust a comment on the earlier patches:
This type of storing a service on the test class is not a great pattern. It is super vulnerable to container rebuilds and things not being in-sync and not testing what you think it does.
Comment #48
zaporylieThanks @alexpott for your patch (#46), and even more for taking the time to comment on container handling in tests classes (#47) - much appreciated!
Comment #49
Wim Leers@alexpott++++++ I tried to achieve that for 10 minutes, but failed, figured that #43 was still a step forward :) Thanks for the explanation! ❤️
Comment #50
tedbowditto @alexpott++ thanks for the explanations!
Comment #51
alexpottThis is going to be tricky for #2026983: [regression] Grants for permissions defined by a module are not revoked / removed from Role config entities when that module is uninstalled / #2571235: [regression] Roles should depend on objects that are building the granted permissions since they are removing permissions when modules are uninstalled. At least for the version that uses proper config dependencies to manage this I think we might have to hard code something special.
It'd be best if the permission could just be in the system module - the whole moving to the node module so that to keep the tickbox in the node section when the node module is installed is a bit icky. Maybe we could move it to system and change the title? Not sure. Very tricky. Or maybe just hack the form to special case it and not do the whole shimmy with the code. In fact, thinking about it I'd way prefer the permission to always by provided by system and the weird logic to list in the node permissions to be in the form,
Comment #52
alexpottThe reason for my preference is once roles get proper config dependencies this will make everything much simpler.
Comment #53
alexpottHere's a patch that maintains the position but ensures that the permission is always provided by system.
Comment #54
Wim LeersWFM!
Comment #55
xjmI'd ask for node access maintainer signoff; fortunately that's me. +1 for this approach.
I don't think there should be any BC impacts from this as System module can't be disabled, but let's provide a CR to inform developers?
Comment #56
alexpottCreated the change record - https://www.drupal.org/node/2921484
This is at least a major since it is blocking #2571235: [regression] Roles should depend on objects that are building the granted permissions
Comment #57
xjmCommitted and pushed to 8.5.x, and published the change record.
Comment #58
Wim LeersYAYYYYYYY!!!!!!!!!!!! I'm working on the follow-ups.
Comment #60
Anonymous (not verified) CreditAttribution: Anonymous commented🙏 Great idea, implementation and commit! 🚀🚀🚀
Comment #61
Wim LeersComment #62
Wim Leers