Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#6520 closed defect (bug) (fixed)

xprofile_check_is_required_field should also check for capability when in admin

Reported by: thomaslhotta Owned by: djpaul
Milestone: 2.4 Priority: normal
Severity: normal Version: 2.3.2
Component: Extended Profile Keywords: has-patch 2nd-opinion needs-testing
Cc:

Description

Hi,

I am trying to implement a more granular user management as I already stated in a previous ticket (#5932).

Currently the xprofile_check_is_required_field function checks for capabilities only on the front end due to this line:

	// Super admins can skip required check
	if ( bp_current_user_can( 'bp_moderate' ) && ! is_admin() ) {

With this in place I cannot edit fields in the WordPress admin interface, even if I have the bp_moderate capability. I can't think of any reason why this check should only be performed on the front end side. So maybe the ! is_admin() part should be removed.

Attachments (1)

6520.diff (3.1 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (7)

#1 @boonebgorges
4 years ago

  • Milestone changed from Awaiting Review to 2.4

Introduced in [5165]. See #3593. My intepretation is that the is_admin() check was put there to hide the "Required" label in the admin, but that the xprofile_check_is_required_field() function is the wrong place to put the is_admin() check - that hiding should happen when the markup is built.

@boonebgorges
4 years ago

#2 @boonebgorges
4 years ago

  • Keywords has-patch 2nd-opinion added

6520.diff is a suggested solution. The idea here is that the field_is_required() functions should return the same thing for everyone, as it's a property of the field only (not the field-user combo). Then, in the relevant places where these functions are used in BP - namely, during form submission - bp_moderate users are able to bypass the requirements. I've chosen to leave the "(required)" label in place even for 'bp_moderate' users, because IMO it's useful information, even if you choose to bypass it.

Note that 6520.diff will break backward compatibility in cases where the field_is_required() functions are called directly, and the plugin expects the function to return true for 'bp_moderate' users. I doubt anyone's doing this, and if they are, they're subject to the same bug being raised in this ticket, so I don't mind the change in behavior.

Anyone have thoughts about this approach?

#3 @DJPaul
4 years ago

  • Keywords needs-testing added

#4 @DJPaul
4 years ago

Everything sounds and looks great.

#5 @djpaul
4 years ago

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

In 10232:

xprofile: remove is_admin() logic from required field check

The function should return the same thing for everyone, as it’s a
property of the field only (not the field-user combo). In the relevant
places where these functions are used in BP - namely, during form
submission - bp_moderate users are able to bypass the requirement.

This change breaks backwards compatibility in cases where the
field_is_required() functions are called directly, and the plugin
expects the function to return true for bp_moderate users. This is
unlikely, and given that if there were, they’d run into this same bug,
let’s fix it.

Fixes #6520

Props boonebgorges

#6 @locomo
4 years ago

I'm using this patch and it's working cleanly for me. Nice fix, thanks!

Note: See TracTickets for help on using tickets.