At the Cornell Daily Sun, we do a lot of work with our articles or nodes before actually publishing them, and editors and others need access to these articles before they are all published at once. And its not just us; there are a lot of sites that do thing like publishing queues and whatnot.
However, the node grant system does not apply at all to unpublished articles. It only applies to published nodes; for unpublished nodes, node grants are entirely skipped and instead a user is given permission to update his own unpublished articles; otherwise access is denied. Quite narrowly tailored business logic for a CMS that is used by a very wide audience.
Thus, to work around this, all nodes have been marked as "published" (as in $node->status is 1) even if in reality they are unpublished, and node grants are coded to deny access to most users for articles which are "published" yet actually unpublished. And while this hack works, it also requires us to write a custom view filter to filter out these unpublished articles, and we have to alter any view which checks if the node is "published" or not (read: almost all of them). And there are certainly a lot of other places where we have to distinguish between published and unpublished nodes which are all "published" according to $node->status.
At its heart, it seems that the current method for access control for unpublished nodes is too restrictive. In many cases, developers need a lot of flexibility for access control and permissions when it comes to these nodes, too. If we could do that, then $node->status could then reflect whether or not the node was actually published or not. Thus, while the node grants could run some complicated logic to determine who can access these articles or not; it would be really easy to determine whether an article is published or not by checking the status field.
Also, this change would not require much extra work to port a module to Drupal 7. If modules wanted to have hook_node_access_records behave exactly like it did in Drupal 6, they only need to add the following code at the top of the hook.
if (!$node->status) {
return;
}
But in this case, the decision of whether to use node grants for unpublished nodes is left in the hands of the developers rather than Drupal.
Comment | File | Size | Author |
---|---|---|---|
#68 | node-view-own-unpublished.png | 36.15 KB | sun |
#68 | drupal.node-view-own-unpublished-perm.68.patch | 1012 bytes | sun |
#58 | drupal-452538-HEAD.patch | 10.14 KB | Mike Wacker |
#50 | drupal-452538-HEAD.patch | 10.09 KB | Mike Wacker |
#46 | drupal-452538-HEAD.patch | 9.94 KB | Mike Wacker |
Comments
Comment #1
Mike Wacker CreditAttribution: Mike Wacker commentedThis patch implements the proposed change. It removes the requirement that nodes be published in order to use the node grants stored in the node_access table, and it also adds a 'view own unpublished content' permission (used in node_access) for users who want to preserve the legacy behavior for Drupal 6 or earlier.
Comment #3
Mike Wacker CreditAttribution: Mike Wacker commentedOops, ran cvs diff -up from /modules/node instead of /. This patch should fix that (and thus not fail to be applied by testing.drupal.org).
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedThis looks like a very nice improvement. I will ask a few others to comment here and wait for test bot before RTBC.
Comment #6
Mike Wacker CreditAttribution: Mike Wacker commentedStrange. I did a fresh install of Drupal 7 with the patch already in place and reran the failed tests. They didn't fail this time. My local setup is a standard LAMP stack (installed via Ubuntu's package manager apt-get).
Comment #7
agentrickardI would go a step further and add a 'view unpublished content' alongside the 'view own unpublished content' permission. (Or even a 'view unpublished TYPE content' as well.)
Comment #8
meba CreditAttribution: meba commented+1 from me for "view unpublished TYPE content". I'll be happy to review a patch. This is a must for every more advanced site.
Comment #9
Mike Wacker CreditAttribution: Mike Wacker commentedHere is an updated patch. Is this what you had in mind? It just brings the 'view' op in line with the 'edit' and 'delete' ops.
Comment #10
agentrickardI'm not sure, and defer to moshe a bit here. I liked the 'view unpublished' string, because it clearly states why the permission is special. If we bring 'view' in line with 'edit' and 'delete', that makes some sense, but might confuse people using node access. And people may enable it by mistake, not realizing what it is really for.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedyeah, lets omit the 'view own' and 'view any' part of this patch - too confusing. also, if we are to keep old behavior then we need to grant this 'view own unpublished content' perm in our .profile files and we need an update function to grant it for existing sites.
Comment #12
Mike Wacker CreditAttribution: Mike Wacker commentedHere's a new patch. After reducing it to a simpler patch like moshe said, I added some code to insert the permission for authenticated users into the default profile (I assume experts can figure it out on their own), and I added a new update hook which does the same.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedLooks perfect to me. The test bot will smack it down if we have missed something.
Comment #14
Dries CreditAttribution: Dries commentedI don't understand why we'd want to have "view own unpublished content" if we have "view own content" ... that sounds very confusing to me.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commented@Dries - thats not in the most recent patch. We didn't like that either.
Comment #16
Mike Wacker CreditAttribution: Mike Wacker commentedYeah, we only add 'view own unpublished content' in this patch, and that's added not as a new feature, but strictly to support legacy behavior for 6.x and earlier, when users could always view their own unpublished content.
Comment #17
Dries CreditAttribution: Dries commented@moshe, it seems to be in the last patch.
@mike, care to explain the legacy stuff a bit more?
Comment #18
Mike Wacker CreditAttribution: Mike Wacker commented@Dries The patch in comment 12 just has a 'view own unpublished content' permission added; I just checked it again. If you look at the old code for node_access, you would see that if $node->status is 0, it would skip the section where you check the node_access table for node grants. It would then go down to a section of code that would check if the user owns the content and grant 'view' access if so.
The way the code is structured now, if you enable the 'view own unpublished content' permissions (as is done in default_profile_tasks and system_update_7024), the system will behave in exactly the same way in Drupal 6. But if you don't want to let users view their own unpublished content, you can remove that permission as well.
Comment #19
BerdirMinor DBTNG Coding Style stuff...
This should probably use db_insert(), especially since we are going to remove update_sql() ... ?
You need to wrap ->fields and ->execute() to their own lines, something like:
Comment #20
Mike Wacker CreditAttribution: Mike Wacker commentedHere's a new patch with style issues resolved.
Comment #21
moshe weitzman CreditAttribution: moshe weitzman commentedComment #22
sunDoesn't that mean that no node access grants are applied? So, for example, if a node is unpublished and belongs to an organic group I no longer belong to, I'm granted access to update it, although I shouldn't have access to the group in the first place?
Comment #23
Mike Wacker CreditAttribution: Mike Wacker commented@sun Only if that user has the 'view own unpublished content' perm. If they don't have the perm (or if the user is not the author, the op is not view, the node is published), that conditional check is skipped, dropping us down into the section that checks node grants.
Comment #24
moshe weitzman CreditAttribution: moshe weitzman commentedback to original status.
Comment #26
Mike Wacker CreditAttribution: Mike Wacker commentedUpdate to system.install caused function name conflict. Rerolled patch resolves this. Remarked as RTBC unless bot rejects the patch.
Comment #28
Mike Wacker CreditAttribution: Mike Wacker commentedChanging system_update_7025 to system_update_7026 in the patch...
Comment #29
martin_qExcellent. So glad to find that this issue has been addressed. I spent hours trying to work out why node grants were not being applied to a node I had in my test system, until I noticed that it was unpublished. In the meantime, I will submit a request for the documentation on D6 and earlier to make the existing behaviour clearer: #481822: Node grants only apply to published nodes, documentation doesn't make this clear. Look forward to seeing the changed behaviour in D7.
Comment #31
webchickHEAD broke.
Comment #33
moshe weitzman CreditAttribution: moshe weitzman commentedAny chance Mike can reroll?
Comment #34
Mike Wacker CreditAttribution: Mike Wacker commentedComment #36
Mike Wacker CreditAttribution: Mike Wacker commentedAnother reroll...
Comment #38
Mike Wacker CreditAttribution: Mike Wacker commentedReroll #5
Comment #40
Mike Wacker CreditAttribution: Mike Wacker commentedFailure caused by testing problems, not the patch
Comment #41
webchickThis looks sane to me, but it'd be nice to have some tests for this:
* Make sure a non-admin user w/ "view own unpublished content" permissions can see their unpublished content.
* Make sure a non-admin user cannot see another user's unpublished content.
* Make sure a non-admin user w/out "view own unpublished content" permissions can't see their own unpublished content.
Also, we should set the default permission here during installation in system_install() like the other permissions.
Comment #42
webchick.
Comment #43
Mike Wacker CreditAttribution: Mike Wacker commented(Note this patch will fail for now due to an issue reported in http://drupal.org/node/505014)
Tests added. I noticed the NodeAccessRecordsAlterUnitTest class was also testing hook_node_access_records to some extent, so I renamed it NodeAccessRecordsUnitTest and added in a snippet to test that node grants work on unpublished nodes.
Since this patch modifies node_access, and we don't really test that at all, I added in a test class (which cause the patch ordering to come out a bit weird) for the node_access function and included the tests for the 'view own unpublished content' permission in it.
The tests revealed a hard-to-spot bug where anyone can view unpublished content. That issue is fixed.
Comment #44
Mike Wacker CreditAttribution: Mike Wacker commentedAlso, we should set the default permission here during installation in system_install() like the other permissions.
I believe the reason we enabled the 'view own unpublished content' permission for authenticated uses in default.profile as opposed to system_install() was that we didn't want to turn it on for the expert profile. I'm not quite sure why users could always view their own unpublished content anyway, so part of me even thinks we should only have enable that permission in an upgrade and not an installation.
Comment #46
Mike Wacker CreditAttribution: Mike Wacker commentedSince I can't find a good workaround for the aforementioned SimpleTest bug, I'm just compressing my four test methods into one big test method for now.
Comment #47
moshe weitzman CreditAttribution: moshe weitzman commentednice improvement and thorough tests. thanks.
Comment #49
webchickThe reason this permission is enabled by default is to avoid the following situation happening:
If you have a content type that defaults to "unpublished" (common in situations where content is moderated before it is published), a user gets whacked in the face with a big ol' "Access denied" upon creating their content. Not very nice.
Now I, on the other hand, can't envision why you wouldn't want this permission enabled by default, almost always. :) Can you give me a counter-scenario that justifies only doing it in default.profile? If not, let's move it back to system_install().
Also, system_update_7029 is now in conflict with something that was committed since, so this will need a re-roll for that.
Other minor stuff I found...
+ * Not covered is hook_access and hook_node_access_records.
Let's convert this to a @todo please.
+ // Asserts functionality of node_access.
Let's convert that to a full PHPDoc signature.
t('node_access returns @result with operation \'@op\'.'
We should only escape 's when strictly necessary. Move the string to double quotes, unescaped single quotes surrounding @op.
+ // Clear permissions for authenticated users.
This should be moved inside setUp() above the db_delete() line.
+ function testNodeAccess() {
PHPDoc summary please.
+
Very minor, but the patch introduces trailing whitespace on the lines between the tests. Can you please make your editor not do that?
Great work on those tests! Once these minor issues are fixed, this should be ready to go.
Comment #50
Mike Wacker CreditAttribution: Mike Wacker commentedHere it is. Everything done as requested.
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedthx mike.
Comment #52
jhodgdonI think this may need an update to the doc section that starts with
in node.module, explaining the actual process used to check access in D7.
Comment #53
webchickActually, in looking at http://api.drupal.org/api/group/node_access/7 that is hopelessly out of date as it stands, regardless of this patch (still mentions db_rerite_sql(), doesn't mention the alter hooks, etc.). jhodgdon, could you file a separate follow-up issue to make this reflect reality?
Comment #54
jhodgdonOK: #508434: Doc section on Node Access Rights needs update for D7
Comment #56
Mike Wacker CreditAttribution: Mike Wacker commentedFailed to apply patch on a retest...probably a test bot failure.
Comment #58
Mike Wacker CreditAttribution: Mike Wacker commentedReroll...
Comment #59
rhouse CreditAttribution: rhouse commentedI might have misunderstood the purpose of this patch, but from following the above comments, it appears that someone suggested "View unpublished..." and "View own unpublished..." permissions. Then someone said that was too complicated, and so "View unpublished..." was removed, leaving only "View own unpublished...". Is that right?
Because if so, I am deeply mystified. "View own unpublished..." is as close to useless as anything is likely to be. (What site allows you to create stuff and not look to see what you created?) Maybe not actually useless, I am sure there must be a need for it somewhere - but the one you left out is a crying need. The module_grants module and its companions revisioning and workflow in D6 provide impressive publishing capabilities that absolutely hinge upon being able to grant access to non-own unpublished content. Many users of module_grants express amazement that core drupal staggers along without this capability.
Maybe I have the wrong end of the stick, if so, apologies, but I would strongly recommend that you look at the great work done by Rik in module_grants, and consider taking some of his key ideas on board here.
Ron.
Comment #60
Mike Wacker CreditAttribution: Mike Wacker commented@RTH
"View own unpublished..." is as close to useless as anything is likely to be.
1. See comment 49 by webchick
2. When the patch is applied, Drupal 7 permissions will work different than on Drupal 6 on a fresh install or update. This permission is turned on so Drupal 7's default behavior is the same as Drupal 6 (which was the original reason to add that permission).
3. The permission can be turned off if a user wants to use node grants and hook_node_access_records to do something much more sophisticaed.
Comment #61
webchickSorry, I got very swamped this week. This is committed to HEAD now. :) Great work, Mike!
Could you add a note about this to the 6.x -> 7.x upgrade page?
Comment #62
Mike Wacker CreditAttribution: Mike Wacker commentedDocumentation added to upgrade page.
Comment #63
webchickAwesome, thanks a lot! :)
Comment #64
rhouse CreditAttribution: rhouse commented@60, I think you misunderstood my comment. Yes, if you change the behaviour of D6, you need this in order to put back the D6 behaviour. I was saying that the new different behaviour (a creator not being able to see their own unpublished work) is the near-useless behaviour - not the need for rescuing the old D6 behaviour. But OTOH, what you have NOT added is the ability to grant a user the ability to see others' unpublished work - that is the really needed thing. Please, please check out the excellent work in module_grants. You guys simply haven't got the plot yet.
With apologies,
Ron House
Comment #65
Mike Wacker CreditAttribution: Mike Wacker commented@RTH
What you seem to describe is a separate issue. If so, file a new issue for it. If so many users of module_grants are "amazed" that we're completely missing the plot, they should be able to come up with a patch to submit if it's that important. These users have to be developers to care so much about something like that, so I don't think you'll be lacking manpower on this one.
The main purpose for turning off "view own unpublished content" is for advanced developers who want to start from scratch and don't want any hard-coded permissions for unpublished nodes. These code changes were not to completely resolve all permissions issues for unpublished nodes; they were meant to open up the node grant system to unpublished nodes as well for developers with complete flexibility and no hard-coded anything.
Comment #67
BenK CreditAttribution: BenK commentedJust need to keep track of this thread....
Comment #68
sunQuick follow-up fix:
Permissions are now presented in the order they are defined. Since "Access content" was renamed to "View published content", it looks very odd that "View own unpublished content" is deeply buried somewhere below and in between.
Comment #70
jhodgdonHmmm... I agree it looks odd to have them so separated. But I'm not sure I like the new order much better than the existing order, because it puts the two view permisssions in the middle of a bunch of admin permissions...
It seems to me that a better order would be:
Administer content types
Administer content
Bypass content access control
Access the content overview page
View published content
View own unpublished content
CRUD by content type
View/revert/delete revisions
Maybe? Or maybe there's a better order someone can come up with?
Also, what's the difference now between "administer content" and "bypass content access control"? Maybe it needs a description? That may be a separate issue, but since you brought up the order...
Comment #71
jhodgdonStatus change was unintentional.
Comment #72
Agileware CreditAttribution: Agileware commentedWhy no permissions for viewing other peoples unpublished nodes?
I would think that would be more useful (and at least as often required)
than just being able to remove the view own unpublished permission.
I know this allows other modules to make node grants that could accomplish
this but I would have thought this would be something that would be in core.
Comment #73
jhodgdonWell, the reason is that no one suggested it on an issue yet. At this point, I think that the idea of having a "View all unpublished nodes" would be a separate issue, so if you feel strongly enough about it, please file one.
Also, how about making a separate "node permissions need reordering" issue rather than discussing the #68 follow-up here, at the end of a 72-message thread?
Comment #74
agentrickard'bypass node access' allows one to view all unpublished nodes. But it may be too late to get a new permission in.
Comment #75
jhodgdon'bypass node access' also allows you to create, update, and delete any node.
Comment #76
sunSorry, I thought this would be a quick fix.
To be continued in #738512: Node permissions are ordered oddly
Comment #77
Agileware CreditAttribution: Agileware commented@jhodgdon:
#273595: Move permission "view any unpublished content" from Content Moderation to Node has been open for longer than this issue and asks for that.
It was closed as a duplicate of this issue.
I will reopen it.