#8167 closed defect (bug) (fixed)
Nouveau: Inviter cannot remove group invitation
Reported by: | dcavins | Owned by: | dcavins |
---|---|---|---|
Milestone: | 5.1.0 | Priority: | normal |
Severity: | normal | Version: | 5.0.0 |
Component: | Groups | Keywords: | has-patch commit |
Cc: | dcavins |
Description
As reported here:
https://buddypress.org/support/topic/cannot-remove-group-invites/
Using the Nouveau Template, the inviter cannot delete an invitation they have extended. The issue is that the ajax.php
file specifically only allows group admins to delete invitations. My opinion is that site admins, group admins and the inviter should be able to delete invitations. I'll attach a patch that allows those three cases.
Attachments (2)
Change History (14)
#1
@
5 years ago
Hi @dcavins thanks a lot for your patch, I believe this might also be the case for BP Legacy. Could you check if it is the case ?
About the patch it looks good, but I’d feel safer if :
$current_user
was renamed$current_user_id
- the if inside the foreach would use strict comparison
===
, - we had a unit test for this.
Is this a side effect of the BP Invites API we should fix in 5.1.0 ?
#2
follow-up:
↓ 4
@
5 years ago
As the original bug poster I tried to merge the patch (manually) into my test environment. After merging, the group view /groups/ does not load, it stays in "loading group" state. I tried twice to rule out c/p errors. There is, though, no PHP error (what I'd expect when I break the php file). Can anybody reproduce this? Unfortunately I don't have a git environment here to do proper merges.
#3
@
5 years ago
- Owner set to dcavins
- Status changed from new to assigned
Hi @imath-
This issue is limited to Nouveau--Legacy works as expected.
The issue was exposed in 5.0, but only through a series of quirks. In BP 4.4, the user who sent the invite couldn't access the delete button, but should have been able to. In a commit from two years ago, djpaul added strict checking to an in_array()
check in bp_nouveau_prepare_group_potential_invites_for_js()
: https://github.com/buddypress/BuddyPress/commit/e97284f6f8a772e1bb7f899c1fa10ed0db88ca96#diff-1a38eaecaee735b5f109d2057ef15e93
The function: https://github.com/buddypress/BuddyPress/blob/e97284f6f8a772e1bb7f899c1fa10ed0db88ca96/src/bp-templates/bp-nouveau/includes/groups/functions.php#L180
What he didn't know was that the inviter_ids
param as returned by Nouveau was a string value, but the logged-in user id is an integer, so the check failed, meaning that the inviter couldn't attempt to delete an invitation that he or she sent (though it would have worked at that time because the delete action handler was too permissive).
Boone added the "is_admin" permission check here, which doesn't mirror the logic in the can_edit
check in bp_nouveau_prepare_group_potential_invites_for_js()
: https://github.com/buddypress/BuddyPress/commit/82eb8c3d316fc13c4f5ffcc828726374d24b6ce8
And I changed the logic that returns the inviter_ids
to use the new API logic (they are now integers), https://github.com/buddypress/BuddyPress/commit/d38562d6334003984d6382a3145f5d48eefe2f69, which, by making the strict check succeed, exposed the mismatch between the prepare and delete logic.
Ha, so it was a series of small changes that added up to the current state.
Anyway, I'll update my patch to add your suggestions, imath, and also ensure that the can_delete logic is the same in the prepare and delete steps for Nouveau.
Re a unit test, I'm not sure how to write a unit test following our typical conventions for this, since this is an interface + AJAX issue. Any ideas?
#4
in reply to:
↑ 2
;
follow-up:
↓ 5
@
5 years ago
Replying to ekadevau:
As the original bug poster I tried to merge the patch (manually) into my test environment. After merging, the group view /groups/ does not load, it stays in "loading group" state. I tried twice to rule out c/p errors. There is, though, no PHP error (what I'd expect when I break the php file). Can anybody reproduce this? Unfortunately I don't have a git environment here to do proper merges.
It sounds like you're having an error in the AJAX handler. Try replacing the entire file's contents (I mean the file bp-templates/bp-nouveau/includes/groups/ajax.php
) with this version which is working on my test environment:
https://raw.githubusercontent.com/dcavins/buddypress-wp-svn/331cd892f535c403770ebc413feb60dd4c4c23fc/src/bp-templates/bp-nouveau/includes/groups/ajax.php
Thanks!
#5
in reply to:
↑ 4
@
5 years ago
Replying to dcavins:
It sounds like you're having an error in the AJAX handler. Try replacing the entire file's contents (I mean the file
bp-templates/bp-nouveau/includes/groups/ajax.php
) with this version which is working on my test environment:
https://raw.githubusercontent.com/dcavins/buddypress-wp-svn/331cd892f535c403770ebc413feb60dd4c4c23fc/src/bp-templates/bp-nouveau/includes/groups/ajax.php
Thank you, apparently I was struggling with the merge process, which is a bit embarrassing ;-) Your version works like a charm. I'll wait for your update to test and hopefully deploy it to my prod environment.
#6
@
5 years ago
- Milestone changed from Awaiting Review to 5.1.0
Awesome, thanks a lot for your work, investigations and explanations about this @dcavins 💪. I think we should fix it in 5.1.0.
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
5 years ago
@
5 years ago
Use the same logic in the "delete invitations" routine as was used in the "can edit" permissions calculation.
#8
@
5 years ago
I've just attached a simpler patch that uses the same logic in the "delete" routine as was used in the "can_edit" permissions check.
I also fixed what I suspect was a mistake, where the error message suggests that we are checking for membership, but the logic was checking for an outstanding membership request. Neither case is that likely, because both actions (joining a group or a member both requesting membership and being invited to a group) would cause the invitation to be removed, so I'm not sure what problem that check is avoiding. I did change the check to mirror the error message, though.
#9
@
5 years ago
@ekadevau- The updated file is here: https://github.com/dcavins/bp-svn-bporg/blob/64b1c0716264fb2207f85de9a98df54cf6199bf4/src/bp-templates/bp-nouveau/includes/groups/ajax.php
Allows inviter and site admins to delete group invitations.