Can 'change' visibility on registration form even for fields marked "Enforce field visibility"

[maccast]

This is a follow-up to #7298.

I think the fix referenced above may have taken things too far. I am not seeing fields on my registration form displaying the "Change" links and options even when they are marked as "Enforce field visibility" in the admin. In fact the primary name field is also allowing non-logged in users to override it's visibility during registration which I don't think is desired behavior?

[maccast]

Screenshot of bug

[slaFFik]

The seems to be like this: registration screen should not display the visibility switcher (Change link) for fields with enforced visibility setting.

[hnla]

It's confirmed but for clarity the changed field on the reg screen isn't actually carried through to the user profile, there you'll find the admin set level is still in force so for user can't change this is the case and user can't change the visibility setting in their account.

So this feels like ?simply a display issue where we're not correctly checking the vis level in the reg screen to disable the choices dropdown.

[hnla]

The general check is applied to allow changing vis level using:

bp_current_user_can('bp_xprofile_change_field_visibility')

This however isn't actually checking whether:

'disabled' == bp_xprofile_get_meta( $field_id, 'field', 'allow_custom_visibility' )

In bp-xprofile-caps.php we run a function:

bp_xprofile_map_meta_caps()

This sets up our bp_current_user_can() check correctly when logged in on profile edit screen.

For logged out users we run another function to:
Grant the 'bp_xprofile_change_field_visibility' cap to logged-out users.

Likely in bp_user_can() we're not correctly checking the $capability var.

For logged out purposes it seems to me as though a more basic checki would work by doing either:

1/ Wrap the html into template, write a function to load it, and in that function check 'disabled' == bp_xprofile_get_meta( $field_id, 'field', 'allow_custom_visibility' )

2/ Create a simple template tag to return $bp_allow_visibility_change = ( 'disabled' == bp_xprofile_get_meta( $field_id, 'field', 'allow_custom_visibility' ) )? true : false;

or something along those lines, I don't see that we really need to be running a cap check and are creating work for ourselves in trying to pass our caps over to WP for logged out users when for the registration screen this simpler template tag check suffices?

[hnla]

A test patch taking the new function check and template update approach.

Creates a new function bp_allow_profile_visibility_change() & updates template check to replace bp_current_user_can( 'bp_xprofile_change_field_visibility' ) check.

This isn't necessarily the best option & have just considered a problem in changing templates to effect this approach will leave unmodified templates with the current problem *sigh*

We'll have to tackle the bp_xprofile_grant_bp_xprofile_change_field_visibility_for_logged_out_users()
and fix that up to also check the vis change.

[hnla]

Creates a new function check for default vis level.

[hnla]

My last comment on this for the moment:

In bp-xprofile-caps.php we run:
bp_xprofile_grant_bp_xprofile_change_field_visibility_for_logged_out_users()

This is the check we use to show or not the profile change radios when logged out.

My issue with this is that it's not a check on the actual users caps as a logged out user strictly doesn't have caps to check also it's not a check on whether a profile field can be changed, changing a profile field is not a user capability it's a default setting that is user agnostic (unless I guess you're admin, but we're logged out!)

The naming of the function does not suggest a capability check really so I propose we simply re-write this function, remove the filter to bp_user_can and just run a true vis field id check.

[hnla]

One last test patch - test first as the preferable-ish approach:

Updates the bp_xprofile_grant_bp_xprofile_change_field_visibility_for_logged_out_users() function to include a vis field default value check.

This approach doesn't require template editing, preserving the function. This seems to test out ok.

[boonebgorges]

Thanks for your patches and thoughts here, @hnla.

I know that it seems cumbersome to move these sorts of checks into capability checks instead of just using template functions. But using proper cap checks becomes increasingly important as we move toward REST API endpoints, because we need consistent ways of checking permissions regardless of client (theme templates, REST request, etc). So the approach in 7391-update-xprofile-caps.patch​ seems preferable to me. 7391.diff​ is a slightly cleaned up version.

The fact that we have a special case for non-logged-in users in this case is a result of a stupid quirk in the way that BP interacts with WP. In the long run, we may need a better solution. See ​https://buddypress.trac.wordpress.org/ticket/7298#comment:5 and follow-up conversation.

Looking at this, I see that there are some other bugs in the way this capability is mapped. First, we should adopt a policy of always passing the field_id to the bp_current_user_can() check, and only use the bp_get_the_profile_field_id() value as a fallback. This helps to prevent security-related bugs. However, this isn't a simple change to make: At some point in the past, we started requiring $args (as in bp_current_user_can( 'cap_name', $args )) to be an associative array. (I think this had something to do with multisite support - I haven't looked through the logs.) So the checks should look like this:

if ( bp_current_user_can( 'bp_xprofile_change_field_visibility', array( 'field_id' => $field_id ) ) ) { 

But the way we check $args in bp_xprofile_map_meta_caps() is not compatible with this: we look for a numerically-indexed $args array. This problem is not something we necessarily need to fix for 2.7.x (I don't think it's new to 2.7) but we should address it for 2.8.

@hnla Could you have a glance at 7391.diff​ to see if my updates look OK to you? If so, please go ahead and commit, and I can open a new ticket to address the other bugs.

[hnla]

@boonebgorges yep my concern was not not getting caps checks per sé but that what we were/are checking is not really a user capability but simply a check to see whether something said yes or no, however the more I looked and thought about the caps functions made sense but don't find them easy to fathom at a glance.

of always passing the field_id to the bp_current_user_can() check, and only use the bp_get_the_profile_field_id() value as a fallback

Yes I too looked first for a means to use $field_id but failed so used bp_get_the_profile_field_id() but it felt wrong.

I'll run your patch tomorrow - Your patch makes sense reading it through so sure this will be fine to address this ticket and regression.

[hnla]

In 11301:

Xprofile: profile field visibility levels, 'enforce field visibility'

Corrects an issue where logged out users are allowed to change profile field visibility level for registration despite admin disabling setting, introduced in 2.7.1 (r: 11221) which fixed inability to change profile visibility by user but omitted check for admin enforced levels.

Add checks for $field->allow_custom_visibility to logged out user caps check in bp-xprofile-caps.php

Fixes #7391

Props maccast, boonebgorges, slaFFik, hnla

[hnla]

In 11302:

Xprofile: profile field visibility levels, 'enforce field visibility'

Corrects an issue where logged out users are allowed to change profile field visibility level for registration despite admin disabling setting, introduced in 2.7.1 (r: 11221) which fixed inability to change profile visibility by user but omitted check for admin enforced levels.

Add checks for $field->allow_custom_visibility to logged out user caps check in bp-xprofile-caps.php

Fixes #7391 ( for 2.7 branch )

Props maccast, boonebgorges, slaFFik, hnla

[hnla]

@maccast Thanks for spotting and raising this. The fix will be applied to 2.7.4 which should be released soon.

[DJPaul]

I'm posting for feedback before I (maybe) change that last commit:

I consider conditional assignment -- ($field_id && $field = xprofile_get_field( $field_id )) -- to be hard to read, and easy to miss. WordPress' PHP standards have a "clever code" section that I argue this kinda falls under.

PSR-2 (not a standard we follow) does cover it explicitly: ​https://github.com/php-fig-rectified/fig-rectified-standards/blob/master/PSR-2-R-coding-style-guide-additions.md#avoid-conditional-assigment

[hnla]

It does the same thing, and yes it's easier to read, make the change, I hazard Boone won't object?

[boonebgorges]

Replying to DJPaul:

I'm posting for feedback before I (maybe) change that last commit:

I consider conditional assignment -- ($field_id && $field = xprofile_get_field( $field_id )) -- to be hard to read, and easy to miss. WordPress' PHP standards have a "clever code" section that I argue this kinda falls under.

PSR-2 (not a standard we follow) does cover it explicitly: ​https://github.com/php-fig-rectified/fig-rectified-standards/blob/master/PSR-2-R-coding-style-guide-additions.md#avoid-conditional-assigment

I specifically did this to avoid another level of if indentation, probably because I read Paul chiding such indentation on a separate ticket just the other day. If one of you feels strongly enough about "readabilty" (or our strict adherence to PSR-2 (that's a joke)) to make the change, be my guest.

[hnla]

Ok @all please leave this as stands we all have enough to do, paid and free, three cooks on this is just wasting resources, the fix is good if we want to make it pretty lets do this when we're twiddling our thumbs looking for something to do :)

& if we're to follow that guide strictly we need to remove all keyword constructs and replace with curly brackets, nice job for someone!

[maccast]

Replying to hnla:

@maccast Thanks for spotting and raising this. The fix will be applied to 2.7.4 which should be released soon.

You're welcome. Glad to have helped.

[DJPaul]

This fixed got moved to BP 2.7.5, which we'll release in the new year.

[djpaul]

In 11343:

Revert r11317 which was a revert. This fixes #7391 for the 2.7 branch.

[boonebgorges]

2.7.5 didn't happen, so let's call this fixed for 2.8.