#1696660: Add an entity access API for single entity access got commited, but now we need to actually implement the API for all entity types.

  • Implement an entity access controller for users
  • Convert access checks to use $entity->access().
Files: 
CommentFileSizeAuthor
#44 user-access-1862752.44.interdiff.txt1 KBlarowlan
#44 user-access-1862752.44.patch10.6 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 50,690 pass(es).
[ View ]
#41 user-access-1862752.40.interdiff.txt680 byteslarowlan
#40 user-access-1862752.40.interdiff.txt0 byteslarowlan
#40 user-access-1862752.40.patch9.6 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 50,696 pass(es), 96 fail(s), and 122 exception(s).
[ View ]
#36 user-access-1862752.36.interdiff.txt3.8 KBlarowlan
#36 user-access-1862752.36.patch9.6 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#28 user-access-1862752.28.interdiff.txt669 byteslarowlan
#28 user-access-1862752.28.patch8.02 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 50,730 pass(es).
[ View ]
#25 user-access-1862752.25.patch8.09 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#25 user-access-1862752.25.interdiff.txt383 byteslarowlan
#23 user-access-1862752.23.interdiff.txt4.68 KBlarowlan
#23 user-access-1862752.23.patch8.29 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.module.
[ View ]
#18 user-access-1862752.18.interdiff.txt669 byteslarowlan
#18 user-access-1862752.18.patch5.13 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 50,613 pass(es).
[ View ]
#13 user-access-1862752.13.patch5.2 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#13 user-access-1862752.13.interdiff.txt1.98 KBlarowlan
#8 user-access-1862752.8.patch5.12 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 49,280 pass(es).
[ View ]
#8 user-access-1862752.8.interdiff.txt392 byteslarowlan
#6 user-access-1862752.6.patch5.12 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#2 user-access-1862752.2.patch104.53 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Comments

Assigned:Unassigned» larowlan

I'll tackle this one.

Status:Active» Needs review
StatusFileSize
new104.53 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

First crack

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.2.patch, failed testing.

Status:Needs work» Needs review

#2: user-access-1862752.2.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new5.12 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

needed a merge against 8.x

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.6.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new392 bytes
new5.12 KB
PASSED: [[SimpleTest]]: [MySQL] 49,280 pass(es).
[ View ]

credit for jthorson for setting me straight on this

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.8.patch, failed testing.

these two tests pass locally, kicking off again.

Status:Needs work» Needs review

#8: user-access-1862752.8.patch queued for re-testing.

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -0,0 +1,74 @@
+      if ($account->uid == $uid || user_access('administer users')) {
+        return TRUE;
+      }
+      elseif (user_access('access user profiles')) {
+        // Only allow view access if the account is active.
+        return $entity->status;

$account should be passed to these user_access() calls. Same for a few more below. Some do it correctly.

StatusFileSize
new1.98 KB
new5.2 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Fixes issues identified by @Berdir and also an incorrect use of $account->uid > 0 which should have been $entity->uid.

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.13.patch, failed testing.

Status:Needs work» Needs review

#13: user-access-1862752.13.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.13.patch, failed testing.

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -19,6 +19,8 @@
+ *   render_controller_class = "Drupal\user\UserRenderController",
+ *   access_controller_class = "Drupal\user\UserAccessController",

There is no custom render controller for users anymore, I guess that's why this failed.

Status:Needs work» Needs review
StatusFileSize
new5.13 KB
PASSED: [[SimpleTest]]: [MySQL] 50,613 pass(es).
[ View ]
new669 bytes

@Berdir, thanks you're a legend!

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.18.patch, failed testing.

entity_test_mul: Two entities loaded by uid without caring about property translatability. Other EntityTranslationTest.php 196 Drupal\system\Tests\Entity\EntityTranslationTest->testMultilingualProperties()

Looks like a random test failure. I've seen a few of those already related to translation tests. Something strange is going on there...

Status:Needs work» Needs review

#18: user-access-1862752.18.patch queued for re-testing.

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -0,0 +1,74 @@
+ * Contains Drupal\user\UserAccessController.

Should be \Drupal\...

+++ b/core/modules/user/user.moduleundefined
@@ -840,30 +840,20 @@ function user_register_access() {
function user_view_access($account) {
-  $uid = is_object($account) ? $account->uid : (int) $account;
-
-  // Never allow access to view the anonymous user account.
-  if ($uid) {
-    // Admins can view all, users can view own profiles at all times.
-    if ($GLOBALS['user']->uid == $uid || user_access('administer users')) {
-      return TRUE;
-    }
-    elseif (user_access('access user profiles')) {
-      // At this point, load the complete account object.
-      if (!is_object($account)) {
-        $account = user_load($uid);
-      }
-      return (is_object($account) && $account->status);
-    }
+  if (is_numeric($account)) {
+    $account = user_load($account);
   }
-  return FALSE;
+  return $account->access('view');

I haven't found a call in core that was still calling that with a user id.

I think we can safely drop that "feature" and type hint $account here accordingly.

Same for the other functions below.

And I would suggest to only keep using this functions for access callbacks and change direct calls to ->access('view').

StatusFileSize
new8.29 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.module.
[ View ]
new4.68 KB

Thanks, attached fixes issues at #22 as well as fixing some doc blocks now that $account is type-hinted.

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new383 bytes
new8.09 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

meh two use Drupal\user\Plugin\Core\Entity\User;

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -19,6 +19,8 @@
+ *   render_controller_class = "Drupal\user\UserRenderController",
+ *   access_controller_class = "Drupal\user\UserAccessController",

Magic re-appearance of the render controller class? :)

+++ b/core/modules/user/user.moduleundefined
@@ -2679,7 +2678,7 @@ function user_rdf_mapping() {
function user_file_download_access($field, EntityInterface $entity, File $file) {
   if ($entity->entityType() == 'user') {
-    return user_view_access($entity);
+    return $entity->access('view');
   }

We can replace this with a entity-generic implementation in system or entity module as soon as all entities are converted. We might even be able to kill this hook completely and just keep the alter. I've already started with a patch for that in the issue about taxonomy terms not implementing this.

It's a nice example of what the unified access API allows us to do :)

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 50,730 pass(es).
[ View ]
new669 bytes

Sun morning fail.
Definitely committed to my local branch now.

Status:Needs review» Reviewed & tested by the community

Ok, I think this is ready to go. It has the same problems with User vs. anoymous user vs. $GLOBALS['user'] which are all different things but it's an existing problem and not blocking us here.

Issue tags:+Needs change record

This will need a change notice as the new preferred approach is eg $entity->access('view') over user_view_access() except for access callbacks

I guess we can update the existing access change notice, add all the conversion issues and a list of removed/deprecated functions. Speaking of that...

+++ b/core/modules/user/user.moduleundefined
@@ -837,34 +837,27 @@ function user_register_access() {
+function user_view_access(User $account) {
+  return $account->access('view');

Wondering if we want to introduce a entity_page_access(Entity $entity, $operation = 'view') to unify those three access callbacks here.

Makes sense to me. Want a follow up?

Either that or update the patch here, I'll try to RTBC either asap :)

Status:Reviewed & tested by the community» Needs work

Where abouts?
entity.module?

Let's add it to entity.inc for now, where everything else is. We haven't yet decided what exactly should happen with entity.module vs. entity.inc. Right now, the module is just the owner for the display entity type.

Status:Needs work» Needs review
StatusFileSize
new9.6 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
new3.8 KB

Let's see how this goes.

Def needs a change notice now we've dropped user_view_access, user_edit_access and user_cancel_access

Yes, let's update http://drupal.org/node/1862420 with entity_page_access() (make sure to state that it should only be used for access callbacks) and the list of removed functions (including the operation that should be used for them now)

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.36.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.6 KB
FAILED: [[SimpleTest]]: [MySQL] 50,696 pass(es), 96 fail(s), and 122 exception(s).
[ View ]
new0 bytes

ha, 'edit' != 'update'

StatusFileSize
new680 bytes

Wrong interdiff

Status:Needs review» Needs work

The last submitted patch, user-access-1862752.40.patch, failed testing.

hmm these fails look valid, menu data cached that references the old access callbacks for the upgrade tests and open id issues with same removed functions

Status:Needs work» Needs review
StatusFileSize
new10.6 KB
PASSED: [[SimpleTest]]: [MySQL] 50,690 pass(es).
[ View ]
new1 KB

Missed some refs in openid.module

Status:Needs review» Reviewed & tested by the community

This is good to go if it passes the tests.

+++ b/core/modules/tracker/tracker.moduleundefined
@@ -185,7 +185,7 @@ function _tracker_myrecent_access($account) {
function _tracker_user_access($account) {
-  return user_view_access($account) && user_access('access content');
+  return $account->access('view') && user_access('access content');
}

I think this should be 'hasAccess()' instead of 'access()'.

+++ b/core/modules/user/lib/Drupal/user/UserAccessController.phpundefined
@@ -0,0 +1,74 @@
+  public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {

I think it would be better to name this 'hasDeleteAccess()' instead of 'deleteAccess()'. For most people, 'deleteAccess()' means we'll delete access.

I understand none of these two things are introduced in this patch, but I wanted to share my thoughts nonetheless. Maybe we can create another issue for this, if not already? Small changes like this can avoid many WTFs. :-)

Status:Reviewed & tested by the community» Fixed

Other than these API suggestions, I think this looks good. Committed to 8.x. Thanks.

hasAccess() sounds weird, like we're checking if the entity itself has access to something. Then again I can't really think of a better alternative besides leaving it 'access'.

Status:Fixed» Closed (fixed)

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

Priority:Normal» Major
Status:Closed (fixed)» Active

This was marked for a change notice update but I don't see the change notice referenced in the summary. Can we make sure the change notice got updated and add this issue to its issue list? Thanks!

Priority:Major» Normal
Status:Active» Fixed
Issue tags:-Needs change record

Added this to https://drupal.org/node/1862420, but this was just a conversion issue, none of the others were added either.

Assigned:larowlan» Unassigned

Status:Fixed» Closed (fixed)

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