Skip to:
Content

Opened 18 months ago

Closed 16 months ago

Last modified 16 months 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 18 months ago.
Avoid changing the $args variable.
7192.02.patch (4.9 KB) - added by r-a-y 18 months ago.
7192.03.patch (5.4 KB) - added by dcavins 18 months ago.
Add note about changed filter arg.
7192.04.backcompat.patch (607 bytes) - added by dcavins 17 months ago.
Fix backcompat for old-style arguments.
7192.get_current_user_id.patch (633 bytes) - added by r-a-y 16 months ago.

Download all attachments as: .zip

Change History (27)

@dcavins
18 months ago

Avoid changing the $args variable.

#1 @boonebgorges
18 months 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
18 months 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 18 months ago by r-a-y (previous) (diff)

@r-a-y
18 months ago

#3 @dcavins
18 months 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
18 months 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
18 months 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
18 months ago

Add note about changed filter arg.

#6 @DJPaul
18 months ago

  • Milestone changed from Awaiting Review to 2.7

#7 @dcavins
17 months ago

  • Keywords dev-feedback added

#8 @dcavins
17 months ago

  • Keywords has-patch added

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


17 months ago

#10 @r-a-y
17 months ago

  • Keywords commit added

Let's get this in! Thanks @dcavins!

#11 @dcavins
17 months 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
17 months 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
17 months ago

Fix backcompat for old-style arguments.

#13 @dcavins
17 months 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
17 months 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
17 months ago

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

#16 @boonebgorges
16 months 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
16 months 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
16 months 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
16 months 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
16 months 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 16 months ago by r-a-y (previous) (diff)

#21 @r-a-y
16 months 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.


16 months ago

Note: See TracTickets for help on using tickets.