Hello

Profile ProfilePermissionProvider extends EntityPermissionProvider adding some extra permissions "View any {profile_type}" & "View own {profile_type}".

However, those permissions are not being checked in ProfileAccessControlHandler, given that EntityApiAccessControlHandler::checkEntityOwnerPermissions is not being extended to support these granular fields.

As a result, I'm having issues with a user having "view own personal profile" permission but not the generic "view profile" one.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plopesc created an issue. See original summary.

plopesc’s picture

Here is a first patch. Let's see how it behaves.

plopesc’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: permissions-improvement-2926019-2.patch, failed testing. View results

plopesc’s picture

Sorry, wrong patch

plopesc’s picture

Status: Needs work » Needs review
plopesc’s picture

Uploading a new version of the patch.

Investigating Entity module, discovered that UncacheableEntityPermissionProvider provides the extra permissions that Profile module is creating by himself, and UncacheableEntityAccessControlHandler checks them. So no need to addextra logic in our classes.

The use of this classes adds a new permission "View own profiles" and change the machine name of the "View profile entities" one, so added a hook_update_N implementation to make this change in existing sites.

The only drawback is that this requires beta1 version of entity module, that requires Drupal 8.3.x. Is that a big problem?

Attaching also an empty patch file to demonstrate that permissions related tests are broken with current code.

Thank you

The last submitted patch, 7: permissions-improvement-2926019-7-empty.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

SocialNicheGuru’s picture

Status: Needs review » Needs work

reroll on 1.x-1.0-rc

drupal-8.4.3/html/modules/contrib/profile$ git apply permissions-improvement-2926019-7.patch
error: patch failed: profile.info.yml:4
error: profile.info.yml: patch does not apply

drupal-8.4.3/html/modules/contrib/profile$ patch -p1 < permissions-improvement-2926019-7.patch
patching file composer.json
patching file profile.info.yml
Hunk #1 FAILED at 4.
1 out of 1 hunk FAILED -- saving rejects to file profile.info.yml.rej
patching file profile.install
patching file src/Entity/Profile.php
patching file src/ProfileAccessControlHandler.php
patching file src/ProfilePermissionProvider.php
aegir@insitehost:~/platforms/drupal/8/servers/1-dev/sng-contrib/dev-social-8.x-1.8-drupal-8.4.3/html/modules/contrib/profile$ more profile.info.yml.rej
--- profile.info.yml
+++ profile.info.yml
@@ -4,8 +4,7 @@
core: 8.x
configure: entity.profile_type.collection
dependencies:
- - user
- - entity
- - field
- - views
- - system (>=8.1.0)
+ - drupal:field
+ - drupal:user
+ - drupal:views
+ - entity:entity (>=8.x-1.0-beta1)

plopesc’s picture

Version: 8.x-1.0-rc1 » 8.x-1.x-dev
Status: Needs work » Needs review

Patches have to apply against -dev version.

Drupal packaging system adds automatic changes to .info.yml files, so that's probably the reason why patch is not applying cleanly in that file.

Also, as #7 demonstrates, the patch apply properly because testbot worked as expected.

Could you try to apply it against -dev and confirm that works?

Thank you.

zrpnr’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #7 applies cleanly to 8.x-1.x-dev with git or composer.
This adds a lot to the changes to using entity for permissions started in https://www.drupal.org/project/profile/issues/2844908

and fixes the main issues I was having with "view own" and "edit own".

Code looks good! Thanks, marking RTBC.

mglaman’s picture

I'm concerned the tests were not finding bugs. Really we need tests to prove a fix.

mglaman’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

We need test coverage. Also, do we want it to be "view any"? I feel like the default would be "own"

mglaman’s picture

Issue tags: -Needs tests

We don't need tests. We have them. That's why this failed in the one issue.

  • mglaman committed 2c667bc on 8.x-1.x authored by plopesc
    Issue #2926019 by plopesc, zrpnr: Need to add...
mglaman’s picture

Status: Needs work » Fixed

Committed, sorry for initial setback to needs work. Appreciated plopesc!

Status: Fixed » Closed (fixed)

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

anpel’s picture

Reroll on 8.x-1.x

Thats what happens when you are in a rush. Can't delete this so please ignore.