This commit
http://drupalcode.org/project/og.git/commit/400ff38654b4f1d66d07e12ac791...

Introduced a new way for organic groups to check access to group content.

The commit in question introduces a check of node_node_access on line 763

  // We call node_node_access() directly as we just want to check the
  // permissions using user_acces().
  if (node_node_access($node, $op, $account)) {...

The problem is that node_node_access, simply checks the user role against their enabled permissions. It does not check if the user has bypass node access privilege checked or not.

So right now, I have a user that has bypass node access privilege. He can create a group and any group content where the group field is populated, but in the case where a node is a group content of a private group and the group field is not populated this validation won't allow the user to create the node.

The only way to get around this currently would be to check the permission for the role. But when they have bypass node access that would be silly.

CommentFileSizeAuthor
#16 og-check-node-access-bypass-2192669-16.patch3.82 KBr-mo
#14 og-check-node-access-bypass-2192669-14.patch563 bytesr-mo
#12 check-node-access-bypass-2192669-10.patch800 bytesdjdevin
#10 check-node-access-bypass-2192669-10.patch1.18 KBdjdevin
#8 check-node-access-bypass-2192669-5.patch806 bytesiamjon
#5 check-node-access-bypass-2192669-4.patch801 bytesiamjon
#2 check-node-access-bypass-2192669-1.patch806 bytesiamjon
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Hi.

It sound like we could just do if (node_node_access($node, $op, $account) || user_access('node bypass..., no?

iamjon’s picture

iamjon CreditAttribution: iamjon commented
FileSize
check-node-access-bypass-2192669-1.patch806 bytes

Patch Attached.

iamjon’s picture

iamjon CreditAttribution: iamjon commented
Status: Active » Needs review

Change status. Patch also changes the inline commenting a bit.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Thanks.

+++ b/og.module
@@ -741,10 +741,9 @@ function og_form_group_reference_validate($form, &$form_state) {
+    if (user_access('bypass node access', $account) || node_node_access($node, $op, $account)) {     ¶

Remove trailing space, here and above.

Also, since we are dealing with access this should be followed by a test.

iamjon’s picture

iamjon CreditAttribution: iamjon commented
FileSize
check-node-access-bypass-2192669-4.patch801 bytes

Reroll.
I would love to write the test..but need help in that dept. Any suggestions? I'm on irc.

Status: Needs review » Needs work

The last submitted patch, 5: check-node-access-bypass-2192669-4.patch, failed testing.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented 
+++ b/og.module
@@ -741,10 +741,9 @@ function og_form_group_reference_validate($form, &$form_state) {
+    if (user_access('bypass node access', $account) || node_node_access($node, $op, $account)) {  ¶

Still trailing spaces...

As for the tests, look at \OgNodeAccess::testNoStrictAccessNodeUpdate and \OgNodeAccess::testNoStrictAccessNodeCreate

iamjon’s picture

iamjon CreditAttribution: iamjon commented
FileSize
check-node-access-bypass-2192669-5.patch806 bytes
amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Seems we are back to the 2 trailing spaces ;)

djdevin’s picture

djdevin
Location Philadelphia
CreditAttribution: djdevin commented
Status: Needs work » Needs review
FileSize
check-node-access-bypass-2192669-10.patch1.18 KB

Status: Needs review » Needs work

The last submitted patch, 10: check-node-access-bypass-2192669-10.patch, failed testing.

djdevin’s picture

djdevin
Location Philadelphia
CreditAttribution: djdevin commented
Status: Needs work » Needs review
FileSize
check-node-access-bypass-2192669-10.patch800 bytes
Ghostthinker’s picture

Ghostthinker CreditAttribution: Ghostthinker commented

Can you please explain to me why this check is made? Why not usw node_access instead of node_node_access? With this code users that are able to edit a node (via node access realms - not og_access/subscriber) get the "You must select one or more groups for this content." error when updaing ogs with group

I just replaced it with node_access( $op, $node, $account) and now it works for me as it should. Did I raise up a security problem?

r-mo’s picture

r-mo CreditAttribution: r-mo at CTI Digital commented
FileSize
og-check-node-access-bypass-2192669-14.patch563 bytes

Ran into this issue when using a custom node_access hook. I don't see why it shouldn't be checking node_access.

Status: Needs review » Needs work

The last submitted patch, 14: og-check-node-access-bypass-2192669-14.patch, failed testing.

r-mo’s picture

r-mo CreditAttribution: r-mo at CTI Digital commented
FileSize
og-check-node-access-bypass-2192669-16.patch3.82 KB

reimplemented node_access without the call to og_node_access to check for system wide permissions.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented
Status: Needs work » Needs review

Setting status

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

@r-mo, thanks. Access issues require a simpleTest - would you be able to work on it?