Skip to:
Content

BuddyPress.org

Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#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)

8167.1.diff (2.1 KB) - added by dcavins 6 months ago.
Allows inviter and site admins to delete group invitations.
8167.2.diff (1.9 KB) - added by dcavins 6 months ago.
Use the same logic in the "delete invitations" routine as was used in the "can edit" permissions calculation.

Download all attachments as: .zip

Change History (14)

@dcavins
6 months ago

Allows inviter and site admins to delete group invitations.

#1 @imath
6 months 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 ?

Last edited 6 months ago by imath (previous) (diff)

#2 follow-up: @ekadevau
6 months 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 @dcavins
6 months 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: @dcavins
6 months 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 @ekadevau
6 months 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 @imath
6 months 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.


6 months ago

@dcavins
6 months ago

Use the same logic in the "delete invitations" routine as was used in the "can edit" permissions calculation.

#8 @dcavins
6 months 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.

#10 @imath
6 months ago

  • Keywords commit added

Hi @dcavins

Thanks a lot for your work on this. I've tested without and with your patch and I confirm 8167.2.diff is fixing the issue.

Let's fix this ;)

#11 @dcavins
6 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 12501:

Nouveau: Fix group uninivite AJAX handler logic.

Ensure that site admins, group admins and the original inviter can all delete a pending invitation.

Fixes #8167.

#12 @dcavins
6 months ago

In 12502:

Nouveau: Fix group uninivite AJAX handler logic.

Ensure that site admins, group admins and the original inviter can all delete a pending invitation.

Fixes #8167.

Note: See TracTickets for help on using tickets.