xprofile fied user visibility settings regression

[hnla]

Previously if custom xprofile field created and user control allowed for them to be changed for visibility i.e 'me', 'my freinds', all members' etc if these fields were added to the base group on the registration page the new user could opt to change the selection visibility of a field.

In 2.7 we have lost this ability with only a default visibility message shown.

Tested by reverting to a 2.6 tag where it worked as expected.

(Reported in ​https://buddypress.org/support/topic/problems-with-xprofile-field-visibilty-on-2-7-0/)

[hnla]

In addition:
If admin creates a field sets that to e.g 'Me Only' but does NOT enforce visibility i.e lets user change on registration and login that field does not respect the initial default setting but displays field to all general members.

These issues are introduced with 2.7, having progressed through the tags checking each.

[DJPaul]

Confirmed. Introduced by r11052.

To clarify, this report relates to the "Change" link after the "This field can seen by: Everyone" -- it doesn't show.

[DJPaul]

Two issues. Our code relied on something in WordPress that I don't think should have worked, so here's a WordPress patch just for reference and reporting upstream if someone wants to do that: ​https://gist.github.com/paulgibbs/1f0c9b60374c45c4f0f1ed27bcd9f952

wp_get_current_user returns a blank WP_User object when you are not logged in, but the empty check here inadequate to follow the intention of this code block (to bail out early). As a consequence, a later call to WP_User->has_cap was being made, which invokes map_meta_cap which calls the filter of the same name, eventually letting our bp_xprofile_map_meta_caps, which allows anonymous users to have the bp_xprofile_change_field_visibility capability, which is directly related to the bug @hnla found.

We changed how bp_current_user_can and friends work in 2.7, which removed the call to current_user_can_for_blog (which is where that WP patch applies).

I am still looking how best to fix this in BuddyPress.

[DJPaul]

My thoughts on different ways to address this:

1) Revert the bp_current_user_can change and rely on the "broken" behaviour with current_user_can_for_blog... which will last until if/when WordPress fixes that function. Just punting the problem down the calendar.

2) Add special edge-case handling to bp_user_can specifically for bp_xprofile_change_field_visibility. Probably calling it directly. This is a bit of a poor long term solution but would fix the problem for now.

3) Do something like 2, but accommodate anonymous user capabilities properly. I don't know if bbPress has anything smart for this, I know it uses capabilities for logged-out users a lot more than WordPress does (WordPress doesn't, really; there is that odd read capability for a few things).

4) Decide that the profile field visibility on the registration form isn't something we want to support, and call this a feature.

[hnla]

1) Sounds viable if we knew or know when / if WP would attend to some solution, if they intend to then roll with this option ( does this have other ramifications for us though in the interim? ).

4) Sort of discount this as the 'feature' is out there and has been for a while and the sudden lack of noticed - however in many ways think maybe it is slightly odd to have on a reg form, more a feature to be accessed once registered and logged in, but then again I can see cases where it's nice to allow users this ability on registration so no to four :)

3) Is probably the route to follow however if we're to fix as a 2.7.1 is this not too long winded to pull together in a shortish space of time?

[DJPaul]

(Reported in ​https://buddypress.org/support/topic/problems-with-xprofile-field-visibilty-on-2-7-0/)
Copying this to the opening comment ~hnla

[boonebgorges]

Thanks for the research on this so far.

I agree that something like 2 or 3 is probably the best route for the time being. 7298.diff​ is a somewhat less horrendous technique than 2 (a special case hardcoded into bp_user_can()) while avoiding a huge amount of infrastructure to handle logged-out user caps. At the moment, the cases where we need to add caps for logged-out users are quite limited, and using a separate callback function for each makes it very explicit.

It seems to me that WP will not easily be able to eliminate the inconsistency noted by DJPaul, for back compat reasons. BP demonstrates why cap checks for user 0 need to go through the same capability mapping filters as other cap checks.

A side note: if we go with something like 7298.diff​, we might want to consider adding a dynamic filter bp_user_can_{$capability}. This will make the callbacks cleaner, and will reduce greatly reduce overhead ('bp_user_can' can be called hundreds of times on a pageload).

[DJPaul]

@boonebgorges I don't really agree that a quick function, not doing any significant work, even being called hundreds of time, is not by itself any significant overheard. I also don't feel I can offer much further technical wisdom regarding best ways to fix this; my best effort in this area was #7176 -- which I was hoping would start moving that ball rolling more than it has -- so more than happy to go with whatever you think a good fix is.

[boonebgorges]

I don't really agree that a quick function, not doing any significant work, even being called hundreds of time, is not by itself any significant overheard.

@DJPaul Calling a function vs not calling it does add overhead, though whether it's "significant" is, I guess, subject to debate. ​https://gist.github.com/boonebgorges/a4f40af56d3e9263575daee5cd6d4c3c

I also don't feel I can offer much further technical wisdom regarding best ways to fix this; my best effort in this area was #7176 -- which I was hoping would start moving that ball rolling more than it has -- so more than happy to go with whatever you think a good fix is.

Like you, I spent many hours looking at #7176, so please don't think I'm ignoring the efforts there. As far as I can see, that ticket wouldn't directly address this one anyway - the assumption there is that we'll use 'map_meta_cap' to implement BP caps, while the inconsistent application of the 'map_meta_cap' filter for non-logged-in users is the source of the difficulty here.

If others have strong feelings about a general strategy for anonymous user caps, please offer them up. Otherwise I guess I'll go with something like 7298.diff​ for 2.7.1.

[dcavins]

Replying to boonebgorges:

If others have strong feelings about a general strategy for anonymous user caps, please offer them up. Otherwise I guess I'll go with something like 7298.diff​ for 2.7.1.

I'm sorry that I caused this mess with the changes to the _can() functions.

Should the check only return true when bp_is_register_page() is true? There are other bits in place to prevent anonymous users from visiting profile edit screens, so maybe it's not important. (But when I see users with id 0 can do something, my impulse is to be overly specific about when it's allowed.)

[djpaul]

In 11220:

Core: grant the 'bp_xprofile_change_field_visibility' cap to logged-out users.

Fixes problem introduced in v2.7 where profile field visibilty options were not appearing on the registration form.
WordPress does not well handle capabilty checks for anonymous users, and it's likely we'll iterate on this technique in the future as we begin to fully adopt user capabilities.

Fixes #7298

[djpaul]

In 11221:

Core: grant the 'bp_xprofile_change_field_visibility' cap to logged-out users.

Fixes problem introduced in v2.7 where profile field visibilty options were not appearing on the registration form.
WordPress does not well handle capabilty checks for anonymous users, and it's likely we'll iterate on this technique in the future as we begin to fully adopt user capabilities.

Fixes #7298 for 2.7 branch.

[DJPaul]

I am very sorry everyone, I forgot to write the props section in the commit. :( Need more tea to wake up.