Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#7192 closed defect (bug) (fixed)

Improve $args included in `bp_current_user_can` and `bp_user_can` filters.

Reported by: dcavins Owned by: dcavins
Milestone: 2.7 Priority: normal
Severity: normal Version: 1.6
Component: Core Keywords: has-patch commit
Cc:

Description

in bp_current_user_can() and bp_user_can(), the originally passed $args array gets overwritten for use with WordPress's functions like this:

$args = array( $blog_id, $capability, $args );
$retval = call_user_func_array( 'current_user_can_for_blog', $args );

Then, when they're included in the final filter, this compounded version is used. So, in bp_current_user_can()`, the filter parameters are actually:
apply_filters( 'bp_current_user_can', $retval, $capability, $blog_id, array( $blog_id, $capability, $args ) )

Similarly, in bp_user_can(), the filter parameters are actually:
apply_filters( 'bp_user_can', $retval, $user_id, $capability, $site_id, array( $user_id, $capability, $args ) )

I'd like pass the original $args array back out for easy manipulation by plugins. (And the other elements in the compounded array are duplicated in the filter parameters, so don't add value.)

My patch removes the modification of the $args variable in the course of each function, so that the original $args variables are passed out as they were passed in.

Attachments (5)

bp-user-can-args.diff (1.0 KB) - added by dcavins 3 years ago.
Avoid changing the $args variable.
7192.02.patch (4.9 KB) - added by r-a-y 3 years ago.
7192.03.patch (5.4 KB) - added by dcavins 3 years ago.
Add note about changed filter arg.
7192.04.backcompat.patch (607 bytes) - added by dcavins 3 years ago.
Fix backcompat for old-style arguments.
7192.get_current_user_id.patch (633 bytes) - added by r-a-y 3 years ago.

Download all attachments as: .zip

Change History (27)

@dcavins
3 years ago

Avoid changing the $args variable.

#1 @boonebgorges
3 years ago

This change seems fine to me, though it should get some documentation in the filter docblock, since it's technically a compatibility break.

#2 @r-a-y
3 years ago

Good catch, dcavins!

Perhaps, we should think about using the new bp_user_can() function in bp_current_user_can(). 02.patch is an attempt at merging this functionality. In an ideal world, we would drop support for the 'bp_current_user_can' filter in place of the 'bp_user_can' filter instead.

The XProfile cap changes are related to #6730.

Last edited 3 years ago by r-a-y (previous) (diff)

@r-a-y
3 years ago

#3 @dcavins
3 years ago

@r-a-y I'm totally in favor of user bp_current_user_can() as a wrapper/helper for bp_user_can(). Your patch looks good to me, and works as expected.

#4 follow-up: @DJPaul
3 years ago

Patch missed removing $blog_id var on line 266 (as i look at it on trac with its line number).

We also need to catch if the user isn't logged in. current_user_can_for_blog took care of this but we aren't using it any more. (line 280 of the patch in the trac view, again). I think an 0 might cause problems in WP current_user_can (it seems like relying on that would be undefined behaviour).

#5 in reply to: ↑ 4 @dcavins
3 years ago

Replying to DJPaul:

Patch missed removing $blog_id var on line 266 (as i look at it on trac with its line number).

@DJPaul I was reading that as backcompat. If they pass an $args['blog_id'], use it for the $args['site_id'] but unset it. Maybe we should throw a deprecation notice there in 2.8?

We also need to catch if the user isn't logged in. current_user_can_for_blog took care of this but we aren't using it any more. (line 280 of the patch in the trac view, again). I think an 0 might cause problems in WP current_user_can (it seems like relying on that would be undefined behaviour).

Are you talking about passing a user_id of 0 to user_can()? Passing a 0 for user_id should result in the same behavior as calling current_user_can() when no one is logged in:
user_can(): https://core.trac.wordpress.org/browser/tags/4.5.3/src/wp-includes/capabilities.php#L505
current_user_can(): https://core.trac.wordpress.org/browser/tags/4.5.3/src/wp-includes/capabilities.php#L427

Huh, why is there no user_can() analog for current_user_can_for_blog(). I was wondering why we're doing multisite work in bp_user_can(). Interesting.

@r-a-y: Let's work this out and get it committed to trunk. I'll attach a version of your patch that adds info to the filter docblock per @boonebgorges' suggestion.

@dcavins
3 years ago

Add note about changed filter arg.

#6 @DJPaul
3 years ago

  • Milestone changed from Awaiting Review to 2.7

#7 @dcavins
3 years ago

  • Keywords dev-feedback added

#8 @dcavins
3 years ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


3 years ago

#10 @r-a-y
3 years ago

  • Keywords commit added

Let's get this in! Thanks @dcavins!

#11 @dcavins
3 years ago

  • Owner set to dcavins
  • Resolution set to fixed
  • Status changed from new to closed

In 11052:

Improve bp_current_user_can().

  • Deprecate $args['blog_id’] in favor of $args['site_id’].
  • Improve the structure of the passed $args array in the

bp_current_user_can filter.

Props r-a-y, dcavins.

Fixes #7192.

#12 @DJPaul
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I am guessing this change is the cause for our test_bp_current_user_can_should_interpret_integer_second_param_as_a_blog_id to fail -- it works on normal WordPress, but fails on multisite.

https://travis-ci.org/buddypress/BuddyPress/jobs/156703464#L509

@dcavins
3 years ago

Fix backcompat for old-style arguments.

#13 @dcavins
3 years ago

I'll commit this fix once I'm back at my SVN repo (unless anyone has any comments about it before then). Thanks for pointing out the Travis failure!

#14 @dcavins
3 years ago

In 11058:

bp_current_user_can(): Fix backward compatibility.

The changes to bp_current_user_can() in r11052 broke backward
compatibility for passing the site id as the $arg instead of an
array. This commit restores the old behavior.

See #7192.

#15 @dcavins
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version set to 1.6

#16 @boonebgorges
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[11052] broke the ability to access wp-admin/edit.php?post_type=bp-email - the bp_current_user_can( 'bp_moderate' ) check during post type registration is returning false. MS, network-activated.

#17 @r-a-y
3 years ago

get_current_user_id.patch fixes the bp_current_user_can( 'bp_moderate' ) issue.

Problem was due to using bp_loggedin_user_id() instead of get_current_user_id(). If we use bp_current_user_can() earlier than when bp_loggedin_user_id() is initialized, the user ID would be 0, which is wrong.

#18 @dcavins
3 years ago

@r-a-y Thanks for chasing that problem down. I was unaware of the limitations of bp_loggedin_user_id().

@boonebgorges Does Ray's patch solve the issue on your system?

#19 @boonebgorges
3 years ago

  • Keywords dev-feedback removed

Yup, patch fixes the issue for me.

Are there any future implications we should be thinking about, knowing that we can't use our filtered bp_loggedin_user_id() in the context of cap checks? I see that this has likely always been an issue (hidden behind current_user_can_for_blog()) but it might be something to flag for future research.

#20 @r-a-y
3 years ago

$bp->loggedin_user->id is initialized on 'bp_setup_globals', which is quite late in the action firing sequence:
https://buddypress.trac.wordpress.org/browser/tags/2.6.2/src/bp-core/classes/class-bp-core.php#L215

We should probably consider initializing $bp->loggedin_user->ID earlier, but for 2.7, let's just leave this alone.

I've refreshed my get_current_user_id.patch to add the 'bp_loggedin_user_id' filter.

Last edited 3 years ago by r-a-y (previous) (diff)

#21 @r-a-y
3 years ago

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

In 11107:

Caps: In bp_current_user_can(), use get_current_user_id() instead of bp_loggedin_user_id().

We're switching to get_current_user_id() because bp_loggedin_user_id()
is initialized on the 'bp_setup_globals' hook, which fires too late in
certain instances. We also apply the 'bp_loggedin_user_id' filter for
devs that are using it.

Fixes #7192.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


3 years ago

Note: See TracTickets for help on using tickets.