In og_field_widget_form() we check for group IDs associated with the content that may not be accessible to the user so that those groups are saved along with any changes in selection the user makes.

Currently, og_get_entity_groups() is used to compare the entity groups with the user's groups. However, it is possible for the user to be legitimately given options in the group audience field, by action of other modules, to groups they may not be part of. If a user is not part of a group, but is allowed access to that group in the group audience field, they can add but not remove, that group from the group audience as the validation will add it back if the user de-selects it.

I propose that OgSelectionHandler::getReferenceableEntities be used instead of og_get_entity_groups(). By using the selection handler, we compare the entity groups with all options given to the user in the audience field.

Patch to follow.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bulldozer2003’s picture

Status: Active » Needs review
FileSize
1.62 KB
amitaibu’s picture

Can you explain a bit more about your use case.

bulldozer2003’s picture

This is related to my og_subgroup_roles module. I use hook_entity_query_alter() to, depending on the user's permissions, present groups in the audience field that the user is not a member of.

This is a little complicated so let me provide the following definitions for my explanation:

  • user: normal user without "administer groups" permission
  • user_groups: group entities the user is a member of
  • more_groups: groups entities the user is not a member of, but is allowed to add and/or edit the content of. These are added to the group audience list by external modules
  • other_groups: group entities the user is not a member of, and has no permissions over

When adding and/or editing a group content entity a user is presented with a group audience field composed of user_groups and more_groups. When adding user_groups and more_groups, the group audience field is saved appropriately. Editing existing content and attempting to de-select more_groups from the audience field fails.


The code inside og_field_widget_form gets user_groups with $user_gids = og_get_entity_groups(); and compares those to the group content entity's groups: $entity_gids = og_get_entity_groups($entity_type, $entity); The difference between those arrays are theoretically groups that need to remain selected and were not displayed in the group audience field, and are stored as $other_groups_ids = array_diff($entity_gids, $user_gids); The problem is that $other_groups_ids will contain both more_groups and other_groups. The more_groups are displayed in the group audience field for the user to select and deselect. If, however, the user tries to de-select some more_groups from the group audience field, they were stored in $other_groups_ids and re-saved in the field instead of being de-selected.


If, instead of using $user_gids = og_get_entity_groups() you were to use

    $mocked_instance = og_get_mocked_instance($instance, 'default');
    $ids = entityreference_get_selection_handler($field, $mocked_instance)->getReferencableEntities();
    $field_gids = array();
    $field_gids = !empty($ids) ? $field_gids[$target_type] = array_keys($ids) : array();

Then get your other_groups with $other_groups_ids = array_diff($entity_gids, $field_gids); your other_groups will be formed by comparing the entity's groups with the groups the user is actually presented in the audience field.

bulldozer2003’s picture

Issue tags: +Needs committer feedback

FYI, I just checked that this does not cause a privledge escalation where a non-member could de-select a group that has given the og_anonymous role edit privledges. Those are added to the field for display directly rather than through the getReferenceableEntities() query.

amitaibu’s picture

Category: bug » feature
Status: Needs review » Needs work
Issue tags: -Needs committer feedback

I'm not 100% sure about it, as this kind of logic change maybe deserves it own widget -- where you can apply you own logic.

Anyway:

+++ b/includes/og.field.incundefined
@@ -70,10 +70,12 @@ function og_field_widget_form(&$form, &$form_state, $field, $instance, $langcode
+    $ids = entityreference_get_selection_handler($field, $mocked_instance)->getReferencableEntities();

Explain why we don't use og_get_entity_groups().

+++ b/includes/og.field.incundefined
@@ -70,10 +70,12 @@ function og_field_widget_form(&$form, &$form_state, $field, $instance, $langcode
+    $field_gids = array();

That's a weird name, at least needs some docs. "The group IDs associated with the field", or something.

bulldozer2003’s picture

Issue tags: +Needs committer feedback

Maybe implementing my own widget is the best solution but I also think using og_get_entity_groups() may not be the best way.


In the function og_field_widget_form(), og_get_entity_groups() gets the groups the user is a member of, that is then compared to the entity's groups to determine which of the entity's groups the user has permission over. Then after the entity is submitted, the entity's groups that the user does not have selection over are added to the user's selection.


The "problem" with that is if the field query has been altered by an external module to allow the user to select groups that they are not a member of. The external module wants to give the user permission to add or remove content from those groups. By only comparing the user's memberships to the entity's groups, the user cannot make those additional selections because the code doesn't care what options have been given in the field, only what memberships the user has.


I'm sure you can appreciate a use case where an external module uses hook_entity_field_query() to expand the results to give user the option of adding content to additional groups they may not be a member of. As I said, implementing a new widget is an option, but it is a somewhat onerous one compared to altering the query.


The functionality for a vanilla organic groups installation is the same, but this expands the usefullness of organic groups to respect changes made via the query alter hook.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
1.9 KB

Here is a patch with some documentation inline.

Renee S’s picture

Just as an example, bulldozer2003, there's this issue: #1796914: Allow non members to create content in group where we'd like to leverage the "Create ___ content" permission within the group (currently greyed out) to allow non-group-members to add group content. Our use case is a department intranet where supervisors will be creating documentation and handing it off to staff groups. Even if they aren't members of those groups (though they are members of parent and sibling groups), they should be able to add content if the group has the right permission.

bulldozer2003’s picture

@Renee S
Thanks for the comment, are you also using query_alter to add additional groups to the audience field? I too had to handle giving non-members create permissions, you can view my query_alter here:
http://drupalcode.org/sandbox/briankpeterson/1506730.git/blob/refs/heads...

Renee S’s picture

Brilliant, that change to subgroup is exactly what we need for the use-case described. Thank you =) We'd just started working on it. Is there an issue where I can report that I've tested this? (and does it require the patch above?)

bulldozer2003’s picture

To alter the query and have OG respect those changes you will need to apply my patch.

Renee S’s picture

Right. I did check out the og_subgroups module, but this permissions functionality is the one thing it lacks. Thanks for og_subgroup_roles; great little module :)

Renee S’s picture

Status: Needs work » Needs review

Testing out this patch, a few things:

1. As admin, when I go in and edit a group post that had a group selected, the "Your groups" audience field widget has no group(s) selected anymore. "Other groups" has nothing selected either.
2. As a group member it shows me the correct group selected. However, on save, if a post was a member of two groups of which the member only belonged to one, it only saves the one group back (losing the other group membership which the user is not a member of.)

casey’s picture

bulldozer2003’s picture

@casey
No, I believe that would require some additional options/filtering done within the og field widget, and then some I imagine.

@Renee S
I will need to test your scenario but are you doing this on a clean install? I have some theories as to how my patch could cause that, but will need to test if it actually is. My test install is unclean atm.

bulldozer2003’s picture

Status: Needs review » Needs work
Renee S’s picture

Confirmed on a clean install:

1. As admin, same behaviour as reported above.
2. Actually, the only-saving-the-group-user-is-members-of only happens once I've made the group a subgroup, using og_subgroup_roles, so it's related to something it's doing. So it's not this patch, filed issue #1872334: Non-member group audience being lost .

bulldozer2003’s picture

@Renee S
Ok, so your issue does not happen with OG only?

Renee S’s picture

Only #1 does after applying this patch -- after a bit more testing, "Other groups" works fine for Admin, it's only "Your groups" that's showing everything as unselected on edit. This is with a clean install, and it only affects the Admin/User 1 on groups they created/are managers for. A bit of an edge case, but anyway.

bulldozer2003’s picture

Status: Needs review » Needs work

Needs to be updated to the new format getReferenceableEntities() spits out results in an array of bundle types.

bulldozer2003’s picture

Thats odd, something reverted in getReferencableEntities() because the result is not keyed by the bundle again. I'll role a new patch against the latest head shortly.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
Renee S’s picture

Applied this patch to the latest OG, but it still doesn't solve the Admin-gets-an-unselected-OG-list-on-existing-group-content problem. That's on my dev site; I'll try it on a bare install shortly, and test whether it's UID 1 or anybody with admin rights.

bulldozer2003’s picture

@Renee S
Test for the problem with and without the patch on a bare install without og_subgroup_roles installed.

If you do not get the problem with this patch alone, then you should open a ticket in my queue :-)

I will try and test too, but have been rather busy.

bulldozer2003’s picture

Status: Needs review » Needs work

Looks like I need to update this patch after all.

Needs to be updated to the new format getReferenceableEntities() spits out results in an array of bundle types.

bulldozer2003’s picture

bulldozer2003’s picture

Status: Needs work » Needs review

The last submitted patch, og-use_field_gids_other_groups-1865944-26.patch, failed testing.

bulldozer2003’s picture

Status: Needs work » Needs review
Issue tags: +Needs committer feedback
Renee S’s picture

Yes, this fixes it =)

Works great for me.

amitaibu’s picture

@bulldozer2003, I'd really be happy if you can add a simpletest, that will make sure this functionality isn't lost along the way.

bulldozer2003’s picture

@Amitaibu
Sure, but I am trying to think of what to test and how. To truly test it, I'd have to simulate a third party module altering the entity query and make sure the user can add and remove the added group. Then check the same thing for the admin interface.

I'll see what I come up with but any general tips would be appreciated.

amitaibu’s picture

> I'd have to simulate a third party module altering the entity query and make sure the user can add and remove the added group

Indeed, for that you have og_test module that can be used as the "implementing" module.

amitaibu’s picture

Issue tags: -Needs committer feedback

I'm soon going to tag RC3, which I hope to be the last one. So, If you want this in ... :)

bulldozer2003’s picture

Ugh, too late... I was on vacation since the 17th :(

amitaibu’s picture

Nothing is too late, you just missed the RC, not the final release...

bulldozer2003’s picture

I'll have this ready to review tomorrow at the latest.

bulldozer2003’s picture

This patch will fail but I am posting for feedback/help.

My test utilizes og_test_entity_query_alter to populate a re-named audience field with all node groups.
Click links for screenshots, edit page screenshots show what the page looks like before saving.

  1. Two groups are created, one for user1 and one for user2. Users have "edit any $type content" permission.
  2. A node is created and the altered group audience field is added.
  3. The node is saved by user2 with both grops selected.
  4. We test for success.
  5. The node is saved again by user 2 with no groups selected.
  6. We test for success.

The problem is the final test fails, og_get_entity_groups is returning two memberships still, even though the memberships no longer exist (judging by the screenshot).

    // Save node with membership in both groups.
    $edit = array("og_group_ref_altered[und][0][default][]" => array(
      $group1->nid => $group1->nid,
      $group2->nid => $group2->nid,
    ));
    $this->drupalPost("node/$common1->nid/edit", $edit, 'Save');

    // Assert user was able to post to both groups.
    $gids = og_get_entity_groups('node', $common1);
    $this->assertEqual(count($gids['node']), 2, 'Both groups selected.');

    // Save node with no group memberships.
    $edit = array("og_group_ref_altered[und][0][default][]" => array(
      '_none' => '_none',
    ));
    $this->drupalPost("node/$common1->nid/edit", $edit, 'Save');

    // Assert user was able to remove both groups.
    $this->assertFalse(og_get_entity_groups('node', $common1), 'No groups selected.');

Status: Needs review » Needs work

The last submitted patch, og-use_field_gids_other_groups-1865944-38.patch, failed testing.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
5.52 KB

Ok, I added og_membership_invalidate_cache(); before getting the entity groups and everything works as expected.

Renee S’s picture

Status: Needs review » Needs work

The screenshots don't seem to be showing up for me; Page Not Found.

eta: Oh! I guess less relevant if it works now ;)

bulldozer2003’s picture

Status: Needs work » Needs review

They still work for me... Maybe because I submitted them.

They should be attached to the thread, but drupal.org file upload is broken (for me) half the time. I can upload one or two files if I am lucky, but then clicking upload just waits until the page times out.

Anyway, setting back to needs review.

amitaibu’s picture

+++ b/og.testundefined
@@ -1067,6 +1067,60 @@ class OgComplexWidgetTestCase extends DrupalWebTestCase {
+    og_create_field(OG_AUDIENCE_FIELD . '_altered', 'node', 'common', $og_field);

+++ b/tests/og_test.moduleundefined
@@ -89,3 +89,16 @@ function og_test_form_behavior_validate($element, &$form_state) {
+function og_test_entity_query_alter($query) {

Let's add an IF to do those cahnges only if the field name == OG_AUDIENCE_FIELD . '_altered' - so it won't be applied on other tests.

+++ b/og.testundefined
@@ -1067,6 +1067,60 @@ class OgComplexWidgetTestCase extends DrupalWebTestCase {
+    // Save node with membership in both groups.

expand a bit more comment, to explain the user is actually a member of only a single group.

+++ b/tests/og_test.moduleundefined
@@ -89,3 +89,16 @@ function og_test_form_behavior_validate($element, &$form_state) {
+ * Implements hook_entity_query_alter().

Please add some docs on what we are doing here.

amitaibu’s picture

Status: Needs review » Needs work

Correct status.

bulldozer2003’s picture

Status: Needs work » Needs review
FileSize
5.83 KB

Let's add an IF to do those cahnges only if the field name == OG_AUDIENCE_FIELD . '_altered' - so it won't be applied on other tests.

This is already present.

expand a bit more comment, to explain the user is actually a member of only a single group.

Added more verbose comments in testAlteredAudienceField().

Please add some docs on what we are doing here.

I've added more verbose comments within og_test_entity_query_alter(). The other functions on og_test.module point to their test, and I was following that convention.

/**
 * Implements hook_entity_query_alter().
 *
 * @see OgComplexWidgetTestCase::testAlteredAudienceField()
 */
amitaibu’s picture

bulldozer2003’s picture

@Amitaibu
You're right.
I believe @ezra-g's problem can be fully solved using the methods this patch exemplifies. I have posted a comment there.

amitaibu’s picture

Status: Needs review » Fixed

Committed, thanks!

@bulldozer2003, bonus points if you add some docs under the advanced section of OG... :)

bulldozer2003’s picture

I wrote up this page in the docs, it should give anyone who knows what they want to do a good start.
https://drupal.org/node/1914476

amitaibu’s picture

Status: Fixed » Active

I reopen because I thinked a bit about this. If the problem is that other-gids is resaved, why not just form-alter and change the other-gids value?

amitaibu’s picture

Sorry I meant change the hidden-group-ids value

bulldozer2003’s picture

Status: Active » Needs review
FileSize
4.4 KB

Here is an updated patch that adds additional tests for precisely why the query should be altered rather than altering the node edit form.

Since og_node_access() calls entityreference_get_selection_handler($field, $instance)->getReferencableEntities(). If we are editing the query, this takes care of both the groups audience field, and allowing create access when OG strict node access is activated.

I've added three additional tests:

  1. check that a non-member can create content while OG node access strict is TRUE.
  2. check that non-member cannot edit that same node if node access is strict after they've saved it with group audiences.
  3. turn strict back to FALSE and check that the user can edit the node

The first test is most important because it tests that og_node_access() allows a non-member to post group content because the non-member is offered groups to select in the audience field.

The last two tests seem to redundantly check the node access strict permission, but they are needed to ensure the query alter is being respected.

I've thought about this use case many times, and altering the query is the cleanest, most integrated approach because one function can handle altering many different aspects of OG to the way the end-user needs.

amitaibu’s picture

Status: Needs review » Active

@bulldozer2003, I'll start with a big apology as I've reverted this commit. I really feel bad about it, as I've guided you in what I think is the wrong path :/ To show my appreciation to your work I'm willing to have a skype meeting with you to go over your needs. Sorry.

The proper solution for you is to create your own custom Entity reference selection handler and move your logic there.
As for the hidden_gids that are added, you can form alter the widget and change the values.

bulldozer2003’s picture

@Amitaibu
When are you active in IRC?
Since you have offered assistance, please consider the arguments I make below.
Thank you.

  1. When I saw the new selection handler in OG I thought it was a very complete solution.
  2. Entity provides hook_entity_query_alter(), I felt that was a natural way to change OG behavior.
  3. An audience field is populated by OgSelectionHandler::buildEntityFieldQuery() where a user's groups are retrieved $user_groups = og_get_groups_by_user(NULL, $group_type); then filtered by create and edit permissions.
  4. Within og_field_widget_form(), there are two options to determine the available groups:
    • The current use of $user_gids = og_get_entity_groups();
    • Using getReferencableEntities() as my patch does, which in turn uses the code in OgSelectionHandler::buildEntityFieldQuery()
  5. Since the field was originally populated with the code in OgSelectionHandler::buildEntityFieldQuery(), why don't we use the same method in og_field_widget_form() to determine the groups?
  6. My patch's method has the benefit of allowing altered queries to work, but that is NOT the only argument I have in favor of doing it this way.
  7. If you change the code in OgSelectionHandler::buildEntityFieldQuery(), to support #1902086: Allow group-audience widget to allow adding new content to groups a user doesn't belong to or otherwise, you will inevitably have to change the code in og_field_widget_form(). Likely in the same way my patch has.
  8. This patch does not change the default behavior of OG in anyway, but I think it makes OG "smarter".
  9. My patch puts OG on the path to supporting not only #1902086: Allow group-audience widget to allow adding new content to groups a user doesn't belong to but possibly also #1796914: Allow non members to create content in group through alterations to OgSelectionHandler::buildEntityFieldQuery() or third-party modules that are easier to implement by not "re-creating the wheel" (selection handlers, field widgets, etc.)
Renee S’s picture

I definitely have a need a way to move towards implementing the issues mentioned in points 7 and in 9. I think they would go a really long way towards making OG a complete solution for a social or educational website or user portal. All of which I'm building ;)

amitaibu’s picture

@Renee S,
The question is not about if the feature is needed, but the implementation details. I think that writing a new OG-selection handler, and possible a widget is the right approach.

Renee S’s picture

Well, Ok: IMO @bulldozer2003's solution applies the principles of parsimony and code reuse, and I think those are important implementation concepts. As he says, it makes OG smarter, and that's useful for all sorts of future developers/development.

bulldozer2003’s picture

Writing my own selection handler does open up some possibilities, but it also complicates things a lot.

Being able to alter a query to change the selection behavior allow intermediate, if not novice, Drupal/PHP users to suit OG to their liking.

For instance, when making group content types, I'll need to somehow offer the user the option to use either the native handler or the subgroup handler.

Also, writing a new handler requires a lot of duplicate code and maintaining it for bugs, whereas the code was already in OG.

So far, I haven't felt like I've gotten a truly good reason to reject this patch. It is a rather simple patch and does not alter the default behavior of OG. I'm still wondering what Amitai is primarily concerned about by committing it.

amitaibu’s picture

@bulldozer2003 ,
Haven't forgotten about this issue, just swamped, I hope I'll get a chance to revisit it in a few days, and give you a the proper answer / solution you deserve.

amitaibu’s picture

Title: Use the field's gids rather than user gids for "other groups" checking » Allow implementing modules to change the My/Other groups selection
Status: Active » Needs review
FileSize
5.97 KB
21 KB

Ok, here's what I have in mind, for allowing others to change the "My groups"/ "Other groups" logic. Note that this patch some fixes for OG core, to allow it to be properly extended, implementing modules will need only the code that appears now under og_example module.

To test it, enable og-example, and set your group-audience field to "Organic groups example" selection handler, and create 10 group nodes.
Now, when you create a new post you will see "My groups" shows group with nid below 5, and "Other groups" the rest.

og-example.png

This implementation IMO is cleaner as the logic is incapsulated in the selection handler (i.e. OgExampleSelectionHandler)

amitaibu’s picture

Status: Needs review » Fixed

Committed.
@bulldozer2003, if something not clear, please ping me.

rv0’s picture

Great!

RIght on time now I need this functionality too ;)

The code from the og_example module gave error screen for normal users.
But this seems like a known bug, also handled in the normal og selection handler.
So for anyone reading and needing this, add the following tags to the query (copy pasted from OgSelectionHandler.class)

// FIXME: http://drupal.org/node/1325628
 unset($query->tags['node_access']);

// FIXME: http://drupal.org/node/1413108
unset($query->tags['entityreference']);

Thanks.

amitaibu’s picture

Status: Fixed » Needs review
FileSize
797 bytes

Patch for #62

bulldozer2003’s picture

@Amitaibu
Thank you for addressing my concerns.

I'll test this as soon as I can.

amitaibu’s picture

Status: Needs review » Fixed

Committed #63

> I'll test this as soon as I can.

Great.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

xjm’s picture

Issue summary: View changes
Issue tags: -Needs committer feedback