Problem/Motivation

Admins may mark a profile field as private. However, doing so has no effect.

Steps to reproduce:

  • Create a new profile type, test. Add two fields, fullname and bio. For bio, select "This is a private field".
  • Create two users. For both, fill in both profile fields.
  • Assign the User module permission "view user information" (access user profiles) and the Profile module permission "test: View any profile" (view any test profile) to authenticated users.
  • Log in as one of the two users you created.
  • Visit the profile page of the other user you created.

Expected result: you see the value of the fullname field but not that of the (private) bio field.
Actual result: you see the values of both fields.

The relevant test incorrectly passes. The issue is that the test user is assigned a permission that doesn't allow access to the test profile type. The "other" user doesn't see the private field only because they can't seen the profile in the first place.

The minimum needed to for the test fail, demonstrating the bug, is in #2. The current patch includes an expanded test to cover the case where a user has view access to a profile but can't view a private field.

Proposed resolution

  • Implement hook_entity_field_access().
  • Improve tests by correcting the permission assigned to the "other" test user and testing for both public and private field access.

Remaining tasks

The new hook implementation will require a cache clear to show up. This should be noted in release notes.

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nedjo created an issue. See original summary.

nedjo’s picture

Status: Active » Needs review
FileSize
618 bytes

The tests in ProfileFieldAccessTest currently pass. Apparently, this test passes only because the user doesn't have access to the profile at all:

    // Verify that the private field value does not appear for other users.
    ...
    $this->assertNoText($secret);

The test setup assigns the permission "view own [profile type] profile". Instead, it should assign the permission "view any [profile type] profile", so that the other user can access the user profile page.

Attached patch fixes this.

A better approach to testing might be to have two fields, one private and one public, and test both for all users.

Anyway, posting this to see what the testbot makes of it.

[Edited to remove superfluous/incorrect details.]

Status: Needs review » Needs work

The last submitted patch, 2: profile-private-test-only.patch, failed testing.

nedjo’s picture

Status: Needs work » Needs review
FileSize
2.1 KB

Here's a patch including a fix to the bug. Tests still need work.

Status: Needs review » Needs work

The last submitted patch, 4: profile-private-4-2854336.patch, failed testing.

nedjo’s picture

Status: Needs work » Needs review
FileSize
5.82 KB
6.18 KB
7.68 KB

Work in progress.

Expanding the test to introduce a second field so we can distinguish between the case where a profile page can't be accessed and that where only a single field can't be accessed.

Not yet passing for me locally.

The last submitted patch, 6: profile-private-2854336-6-test-only.patch, failed testing.

The last submitted patch, 6: profile-private-2854336-6-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: profile-private-2854336-6.patch, failed testing.

nedjo’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.29 KB
6.64 KB
8.74 KB

Okay, this is now passing for me locally.

Changes to the patch:

  • Added handling for case where user is viewing own profile and should see private fields.
  • Renamed field properties in test to be more descriptive.

Also filling in steps to reproduce in issue summary.

The last submitted patch, 10: profile-private-2854336-10-test-only.patch, failed testing.

The last submitted patch, 10: profile-private-2854336-10-test-only.patch, failed testing.

nedjo’s picture

Issue summary: View changes

The new hook implementation will require a cache clear to show up. This should be noted in release notes.

nedjo’s picture

Fix some typos in code comments.

Fill in details on resolution in issue summary.

The last submitted patch, 14: profile-private-2854336-14-test-only.patch, failed testing.

The last submitted patch, 14: profile-private-2854336-14-test-only.patch, failed testing.

nedjo’s picture

Issue summary: View changes
mglaman’s picture

mglaman’s picture

Assigned: Unassigned » mglaman
Issue tags: +MidCamp2017
mglaman’s picture

Here's a reroll against changes with permissions. The bypass permission was removed since it provided same functionality as administer.

Status: Needs review » Needs work

The last submitted patch, 20: private_fields_are-2854336-20.patch, failed testing.

mglaman’s picture

Status: Needs work » Needs review
mglaman’s picture

Re-roll to add second field (like original patch) which isn't private.

  • mglaman committed daa0243 on 8.x-1.x authored by nedjo
    Issue #2854336 by nedjo, mglaman: Private fields are visible to any user...
mglaman’s picture

Status: Needs review » Fixed

Thanks, nedjo!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.