In Drupal Commons an end user needs access to approve or ignore his/her trusted contacts.

Details

Maybe this can be solved in general.

CommentFileSizeAuthor
#14 2583303-og_membership-access_check-14.patch483 bytesnplowman
#8 2583303-og_membership_access_check-8.patch514 bytesslv_
#4 2583303-og_membership_access_check-4.patch699 bytesslv_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slowflyer created an issue. See original summary.

slowflyer’s picture

slowflyer CreditAttribution: slowflyer commented
Issue summary: View changes
slowflyer’s picture

slowflyer CreditAttribution: slowflyer commented
Title: Drupal Commons "Trusted contact" and og_mebership_access » Drupal Commons "Trusted contact" and og_membership_access
slv_’s picture

slv_ CreditAttribution: slv_ commented
Title: Drupal Commons "Trusted contact" and og_membership_access » og_membership_access() doesn't take into account actual group permissions.
Category: Feature request » Bug report
Status: Active » Needs review
FileSize
2583303-og_membership_access_check-4.patch699 bytes

The problem seems a general one to me, indeed. Moving to bug instead of Feature Request. As it stands, the function goes against the permissions available in the UI for groups. Particularly:

Manage members
Users may remove group members and alter member status and roles. Warning: Give to trusted roles only; this permission has security implications in the group context.

So according to that, any user with that permission should be granted access to the membership entity.

Patch attached to solve it.

Status: Needs review » Needs work

The last submitted patch, 4: 2583303-og_membership_access_check-4.patch, failed testing.

slv_’s picture

slv_ CreditAttribution: slv_ commented

Doh... patch correct, but badly created so it won't apply, I'll attach the right one asap T_T.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@slv_ just need --relative flag inside the og folder I guess for git diff?

slv_’s picture

slv_ CreditAttribution: slv_ commented
Status: Needs work » Needs review
FileSize
2583303-og_membership_access_check-8.patch514 bytes

Let's see now!

Status: Needs review » Needs work

The last submitted patch, 8: 2583303-og_membership_access_check-8.patch, failed testing.

slv_’s picture

slv_ CreditAttribution: slv_ commented
Status: Needs work » Needs review

Right, I'm confident the patch is correct now, and was applied successfully. Problem is there's already a test failing in the branch, which is not related to this patch, but makes the tests fail.

The failing test seems to be there since January -> https://www.drupal.org/pift-ci-job/584179.

This is the test for the patch uploaded in previous comment: https://www.drupal.org/pift-ci-job/605815. Same failure. Marking as needs review, as I suppose it can be RTBC regardless of the other issue.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Is there an issue for that failure?

slv_’s picture

slv_ CreditAttribution: slv_ commented

I've just created it https://www.drupal.org/node/2859078.

It also seems that one was in place from some time ago? Just found this (https://www.drupal.org/node/2770927#comment-11429029).

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Thanks. Yeah, it would be great if you could tackle the failing test. Also, I'd love to have a simpleTest to assert the access change introduced in this patch

nplowman’s picture

nplowman CreditAttribution: nplowman at FFW commented
FileSize
2583303-og_membership-access_check-14.patch483 bytes

I'm attaching a re-rolled patch that will apply cleanly on the newest version of the module.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

@nplowman that doesn't quite look like a re-roll. Did you mean to remove the user_access check?

danyg’s picture

danyg CreditAttribution: danyg at Cheppers commented
Status: Needs review » Reviewed & tested by the community

@joelpittet Actually og_user_access() does the checking of user_access('administer group'), so I think checking og_user_access is enough and working. (Tested with OG 2.10)