Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
I have installed nodeaccess on drupal 7.12
when I create a new content, save it (not published)
then any user with the url can view the node !
If I edit again the node, click on the "grant" tab and "save grants" and flush cache,
then it's ok, access becomes denied for anonymous users.
This seems a very serious security problem to me.
Comments
Comment #1
sanette CreditAttribution: sanette commentedthis is probably not the best thing to do but this seems to work: insert
if (!$node->status) {
// Don't create grants for unpublished nodes.
return NULL;
}
in function nodeaccess_node_access_records
(taken from https://drupal.org/node/1294566)
Comment #2
bkosborneThis seems like a critical bug to me, and probably a pretty significant security issue unless I'm overlooking something here. When this module is enabled, it examines your Drupal role permissions for to see if "anonymous" and "authenticated" user roles have the "access content" permission. Then, for every node on the site, it adds a node_access record that allows those roles to view the content (assuming that role had "access content" perm).
It has no regard for the nodes current published status. Meaning, once you enable this module, any unpublished content you have becomes visible to everyone.
Comment #3
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedI will have a look at this immediately.
Comment #4
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedWho is the author of your nodes? Is it anonymous (ie: uid=0)?
Comment #6
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedI have pushed a fix to dev version already and have attached a patch against 1.3 that will fix this issue.
Instead of just not setting the grants for anonymous or authenticated users, I have based it on the role's permission to 'bypass node access'.
You will have to rebuild your access in order to see the change. Let me know if you run into any new issue.
Comment #7
bkosborneNo the author was not anon, but I think you already figured that out. The patch seems to have fixed the issue.
Comment #8
sanette CreditAttribution: sanette commentedThe patch #6 doesn't work for me. The unpublished content is still visible
I'll try to look into it when I have some time ... :(
Comment #9
sanette CreditAttribution: sanette commentedMore precisely:
If I'm loggued as another user, then I cannot see the unpublished content
If I'm anonymous, then I can see the unpublished content :(
Comment #10
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedPlease make sure to rebuild the permissions after the patch.
Comment #11
sanette CreditAttribution: sanette commentedyes I did it, + cache clear
Comment #12
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedThat must mean that anonymous users either have permission to 'bypass node access' or some other content access module (ex: Content Access, Domain access, some other one?) is giving access. Nodeaccess module no longer creates any entry into the node_access table for anonymous users (unless 'bypass node access' is granted).
Comment #13
sanette CreditAttribution: sanette commentedno, anomymous users don't have this permission.
But I should mention that the node I'm testing contains some user defined grants
(sorry for this, I should have mentionned)
so it's logical that the patch doesn't affect this, since it applies only in the case the node has no own grants.
for a new content created without its own grant, the patch seems to work
So now the question is changed compared to #1. Here is the value of $grants for my node:
Array
(
[0] => Array
(
[gid] => 1
[realm] => nodeaccess_rid
[grant_view] => 1
[grant_update] => 0
[grant_delete] => 0
[priority] => 0
)
[1] => Array
(
[gid] => 9
[realm] => nodeaccess_rid
[grant_view] => 1
[grant_update] => 0
[grant_delete] => 0
[priority] => 0
)
[2] => Array
(
[grant_view] => 1
[grant_update] => 1
[grant_delete] => 1
[gid] => 1
[realm] => nodeaccess_author
[priority] => 0
)
)
Comment #14
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedNot sure what you mean by some user defined grants. But logically, if you are editing a node and want to grant access to a user and check the checkbox that this is what you want to do. That's what the module should do, regardless of what the permissions are for published/unpublished. This way you can granularly set access to a particular node. So if you don't want anon to have access to all unpublished nodes, but want to grant access to one unpublished node, you can do that.
This issue and the fix that followed repaired the condition where a node that is not published was granted access to a user upon install because the defaults were set to 'view' node for anonymous users.
If there is another issue, please create a separate issue in the queue so that it can be logged properly and will show up in other users' searches. Thanks.
Comment #15
sanette CreditAttribution: sanette commentedI think my problem is now the same as
https://drupal.org/node/1444040
(I create a new node, give 'view' access to anonymous (using the grant tab), unpublish it --> it is visible by anonymous, which I don't want).
Thanks for the help.
Comment #16
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedYeah that's exactly what I meant above. What if you didn't want anon to have access to all unpublished nodes, but wanted them to have access to one particular unpublished node. If I made the change to work as you request, it would be impossible to do so.
Sorry, I don't think that I will be making the change beyond what is already done.
Comment #17
sanette CreditAttribution: sanette commentedI see what you mean. Let me just try and make my case.
* I use this great module because it lets me have a group of people (=role) who can modify a set of articles.
* of course these articles are visible by anonymous, and not editable by other users.
* sometimes they want to unpublish an article to work on it.
* but the unpublished article is still visible by anonymous users !
I believe my situation is very standard, and I continue to think that this is a serious issue, as when the node is not published, the authors think nobody can view it and some confidential text might be present in the article.
Did I miss something ? If not, of course I respect your decision, but I think I will hack my own copy (in my situation I don't see the point of letting anonymous users having access to any unpublished content.)
Comment #19
manuel.adanThis major issue should be solved in stable version of the module. Current 7.x-1.3 still allow access to unpublished contents!.
Comment #20
MorganL CreditAttribution: MorganL commentedI second arcovia's comment. This is a HUGE security issue. It needs to be pushed into the stable release immediately.
Comment #21
willzyx CreditAttribution: willzyx commentedThe patch solve the problem partially and introduced new issue;
Problem:
1) it isn't retroactive. All nodes with before-patch "user defined grants" may be visible by anonymous. This is a HUGE security issue.
2) Users can fall in error. They have not evidence that nodes are visible by anonymous even if unpublished.
New issue:
1) Modify grant on unpublished node with default settings break default configuration. see https://www.drupal.org/node/2140819
and big part of the patch is useless (as you can see here https://www.drupal.org/node/270000)
so all the "else" part of the patch can be safely deleted.
Proposed solution:
1) Only write grants for published nodes or write grant_view = false if node is unpublished like a lot of content access modules does (content_access (per node), taxonomy_access, tac_lite, domain_access etc)
2) Add grant "view unpublished" to the allowed grants and manage it if node is unpublished (if "you didn't want anon to have access to all unpublished nodes, but wanted them to have access to one particular unpublished node" have sense)
Comment #22
willzyx CreditAttribution: willzyx commentedComment #23
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedAgreed, adding patch that will remove the else, if it passes tests I will commit immediately to dev.
Comment #25
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedComment #29
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedComment #30
willzyx CreditAttribution: willzyx commentedok code now is more clean.. but the patch don't solve the issue
It applies only for nodes without saved grants. The unpublished node with saved grants are still visible from anonymous user.
patch for solve this problem is attached
Note with this patch applied :
Only published nodes will get nodeaccess grants written
In order to maintain the consistency of the nodes grants the configuration is read from the table "nodeaccess" and not directly from "node_access" (which is subject to change) otherwise the grant tab would not show the correct grants configuration.
needs some serious test!
Comment #31
willzyx CreditAttribution: willzyx commentedComment #32
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedPlease don't post patches that will not fix the issue that is being discussed in this thread, it creates confusion, especially when they are duplicate patches from other issues #2342327: node_access_write_grants should not be called directly .
Comment #33
willzyx CreditAttribution: willzyx commentedthe patch will fix the issue
check it out!
the core is in nodeaccess_node_access_records
reroll of nodeaccess-2342327-read-config-from-nodeaccess-table.patch in this patch is needed for show the correct grants configuration on grant tab
Comment #34
vlad.pavlovic CreditAttribution: vlad.pavlovic commentedI see what you mean. The logic from your solution has been ported to and released to dev as part of #2037899: Expose operations as API. I have also added tests that confirm that things are working as they should. Let me know if there are issues.
Comment #36
SunnyGambino CreditAttribution: SunnyGambino commentedDear Guys,
It is a really good feature and it is working. BUT.. :D
In my own custom Drupal 7 system I am using both published and unpublished nodes on an absolutely different way. Could this feature be a switchable option on the module configuration page?
Thanks a lot!
Comment #37
ElusiveMind CreditAttribution: ElusiveMind commentedPatch lines 132-150 of nodeaccess.module.
Module was defaulting to NODE_ACCESS_IGNORE if user did not own content, did not have 'view own unpublished content' and 'bypass node access' set. Note that "bypass node access" causes hook_access_control to not be fired, and as such would have no impact on user's ability to control content for that permission
Comment #38
ElusiveMind CreditAttribution: ElusiveMind commentedComment #39
ElusiveMind CreditAttribution: ElusiveMind commentedThis needs community review. I cannot update the status as I am not yet the maintainer.