Users can add content to groups where they don't have "create <type> content" permission [#1816752]

Users can add content to groups where they don't have "create content" permission.

Setup

Create user with "create article content" permission in group A and only "update article content" permission in another group B.
As this user, create an article in group A.
Edit this article and change group selection to group B.

(I'm unable to test form submission at this moment due to another bug)

Fix

In OgSelectionHandler, only include a group in which the user has "update" permission if the node is already in that group.

Comment	File	Size	Author
#10	1816752-og-update-10.patch	7.23 KB	amitaibu
#10
#8	1816752-og-update-8.patch	6.42 KB	amitaibu
#8
#7	update-testonly.patch	2.9 KB	ezheidtmann
#7
#7	update-withtest.patch	4.36 KB	ezheidtmann
#7
#3	og-test.2012-10-22.patch	2.83 KB	ezheidtmann
#3
	og-update-permission-fix.patch	1.46 KB	ezheidtmann

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

amitaibu

Hebrew

Israel

CreditAttribution: amitaibu commented 18 October 2012 at 21:40

+++ b/plugins/entityreference/selection/OgSelectionHandler.class.phpundefined
@@ -116,9 +116,15 @@ class OgSelectionHandler extends EntityReference_SelectionHandler_Generic {
+            $node_groups = isset($node_groups) ? $node_groups : og_get_entity_groups('node', $node->nid);

Why isset($node_groups) ? -- it's not declared before.

Also, can you add a test for it under OgNodeAccess

Comment #2

ezheidtmann CreditAttribution: ezheidtmann commented 19 October 2012 at 00:50

Yea, I'll try to write a test.

The isset($node_groups) saves repeated calls to og_get_entity_groups() within that loop.

Comment #3

ezheidtmann CreditAttribution: ezheidtmann commented 22 October 2012 at 01:57

File	Size
og-test.2012-10-22.patch	2.83 KB

Here's a test that, if it works right, should pass in current code but fail with the fix applied. Not sure at the moment how to invert the logic ...

Comment #4

22 October 2012 at 02:01

Status:

Needs review

» Needs work

The last submitted patch, og-test.2012-10-22.patch, failed testing.

Comment #5

ezheidtmann CreditAttribution: ezheidtmann commented 22 October 2012 at 02:05

Status:

Needs work

» Needs review

#3: og-test.2012-10-22.patch queued for re-testing.

Comment #6

22 October 2012 at 02:13

Status:

Needs review

» Needs work

The last submitted patch, og-test.2012-10-22.patch, failed testing.

Comment #7

ezheidtmann CreditAttribution: ezheidtmann commented 23 October 2012 at 19:06

Status:

Needs work

» Needs review

File	Size
update-withtest.patch	4.36 KB

update-testonly.patch	2.9 KB

First patch has test only; should fail. Second patch should pass.

Comment #8

amitaibu

Hebrew

Israel

CreditAttribution: amitaibu commented 23 October 2012 at 21:15

File	Size
1816752-og-update-8.patch	6.42 KB

Thanks. I've changed the test to something more simple, as we don't need to create new roles; and it's always better to test the select list itself using XPath.