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.
Comment | File | Size | Author |
---|---|---|---|
#63 | 1865944-set-tags-63.patch | 797 bytes | amitaibu |
#60 | og-example.png | 21 KB | amitaibu |
#60 | 1865944-og-example-subgroups-60.patch | 5.97 KB | amitaibu |
#52 | og-use_field_gids_other_groups-1865944-52.patch | 4.4 KB | bulldozer2003 |
#45 | og-use_field_gids_other_groups-1865944-45.patch | 5.83 KB | bulldozer2003 |
Comments
Comment #1
bulldozer2003Comment #2
amitaibuCan you explain a bit more about your use case.
Comment #3
bulldozer2003This 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:
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
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.Comment #4
bulldozer2003FYI, 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.
Comment #5
amitaibuI'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:
Explain why we don't use og_get_entity_groups().
That's a weird name, at least needs some docs. "The group IDs associated with the field", or something.
Comment #6
bulldozer2003Maybe 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.
Comment #7
bulldozer2003Here is a patch with some documentation inline.
Comment #8
Renee S CreditAttribution: Renee S commentedJust 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.
Comment #9
bulldozer2003@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...
Comment #10
Renee S CreditAttribution: Renee S commentedBrilliant, 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?)
Comment #11
bulldozer2003To alter the query and have OG respect those changes you will need to apply my patch.
Comment #12
Renee S CreditAttribution: Renee S commentedRight. 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 :)
Comment #13
Renee S CreditAttribution: Renee S commentedTesting 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.)
Comment #14
casey CreditAttribution: casey commentedWould this solve #1872016: Widget does not filter by configure membership type?
Comment #15
bulldozer2003@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.
Comment #16
bulldozer2003Comment #17
Renee S CreditAttribution: Renee S commentedConfirmed 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 .
Comment #18
bulldozer2003@Renee S
Ok, so your issue does not happen with OG only?
Comment #19
Renee S CreditAttribution: Renee S commentedOnly #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.
Comment #20
bulldozer2003Needs to be updated to the new format getReferenceableEntities() spits out results in an array of bundle types.
Comment #21
bulldozer2003Thats 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.
Comment #22
bulldozer2003Comment #23
Renee S CreditAttribution: Renee S commentedApplied 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.
Comment #24
bulldozer2003@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.
Comment #25
bulldozer2003Looks like I need to update this patch after all.
Comment #26
bulldozer2003Fixed to account for #1821060: Return values from getReferencableEntities() keyed by bundle
Comment #27
bulldozer2003Comment #29
bulldozer2003#26: og-use_field_gids_other_groups-1865944-26.patch queued for re-testing.
Comment #30
Renee S CreditAttribution: Renee S commentedYes, this fixes it =)
Works great for me.
Comment #31
amitaibu@bulldozer2003, I'd really be happy if you can add a simpletest, that will make sure this functionality isn't lost along the way.
Comment #32
bulldozer2003@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.
Comment #33
amitaibu> 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.
Comment #34
amitaibuI'm soon going to tag RC3, which I hope to be the last one. So, If you want this in ... :)
Comment #35
bulldozer2003Ugh, too late... I was on vacation since the 17th :(
Comment #36
amitaibuNothing is too late, you just missed the RC, not the final release...
Comment #37
bulldozer2003I'll have this ready to review tomorrow at the latest.
Comment #38
bulldozer2003This 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.
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).
Comment #40
bulldozer2003Ok, I added og_membership_invalidate_cache(); before getting the entity groups and everything works as expected.
Comment #41
Renee S CreditAttribution: Renee S commentedThe screenshots don't seem to be showing up for me; Page Not Found.
eta: Oh! I guess less relevant if it works now ;)
Comment #42
bulldozer2003They 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.
Comment #43
amitaibuLet'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.
expand a bit more comment, to explain the user is actually a member of only a single group.
Please add some docs on what we are doing here.
Comment #44
amitaibuCorrect status.
Comment #45
bulldozer2003This is already present.
Added more verbose comments in testAlteredAudienceField().
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.
Comment #46
amitaibuSeems #1902086: Allow group-audience widget to allow adding new content to groups a user doesn't belong to is related, @bulldozer2003 would you mind giving it a look?
Comment #47
bulldozer2003@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.
Comment #48
amitaibuCommitted, thanks!
@bulldozer2003, bonus points if you add some docs under the advanced section of OG... :)
Comment #49
bulldozer2003I 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
Comment #50
amitaibuI 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?
Comment #51
amitaibuSorry I meant change the hidden-group-ids value
Comment #52
bulldozer2003Here 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:
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.
Comment #53
amitaibu@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.
Comment #54
bulldozer2003@Amitaibu
When are you active in IRC?
Since you have offered assistance, please consider the arguments I make below.
Thank you.
$user_groups = og_get_groups_by_user(NULL, $group_type);
then filtered by create and edit permissions.$user_gids = og_get_entity_groups();
Comment #55
Renee S CreditAttribution: Renee S commentedI 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 ;)
Comment #56
amitaibu@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.
Comment #57
Renee S CreditAttribution: Renee S commentedWell, 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.
Comment #58
bulldozer2003Writing 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.
Comment #59
amitaibu@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.
Comment #60
amitaibuOk, 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.
This implementation IMO is cleaner as the logic is incapsulated in the selection handler (i.e.
OgExampleSelectionHandler
)Comment #61
amitaibuCommitted.
@bulldozer2003, if something not clear, please ping me.
Comment #62
rv0 CreditAttribution: rv0 commentedGreat!
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)
Thanks.
Comment #63
amitaibuPatch for #62
Comment #64
bulldozer2003@Amitaibu
Thank you for addressing my concerns.
I'll test this as soon as I can.
Comment #65
amitaibuCommitted #63
> I'll test this as soon as I can.
Great.
Comment #67
xjm