Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
5.48 KB

See attached.

Crell’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.php
@@ -0,0 +1,53 @@
+   *   _access_entity: 'node:update'

The code uses a ., but the example in the docblock users a :. Pick one. :-) (Probably the .)

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.php
@@ -0,0 +1,53 @@
+  public function access(Route $route, Request $request) {

This looks like we could (gasp!) make a unit test for it rather than a functional test.

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.php
@@ -0,0 +1,53 @@
+    return FALSE;

Should this be a return FALSE, or a return null? (False overrides all other access checkers unconditionally; null allows others to grant access. I'm inclined toward null.)

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue tags: +WSCCI, +FormInterface
FileSize
703 bytes
5.46 KB

Yeah, I don't know how to write a unit test for routes, \Drupal\system\Tests\Routing\RouterTest was a functional test and I just mimicked that.

Fixed the other two things.

Status: Needs review » Needs work
Issue tags: -WSCCI, -FormInterface

The last submitted patch, entity-access-check-1947432-3.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +FormInterface

#3: entity-access-check-1947432-3.patch queued for re-testing.

tim.plunkett’s picture

Issue tags: +WSCCI novice

@msonnabaum gave me this, can someone turn it into a test?

https://gist.github.com/msonnabaum/5207807

msonnabaum’s picture

Bro, the test is there at the bottom. You just need to do the other method.

tim.plunkett’s picture

s/turn it into a test/turn the gist into a patch

There. :)

mtift’s picture

Assigned: Unassigned » mtift
Status: Needs review » Needs work

I'll try.

mtift’s picture

I've got a start on this, but I have a couple of questions.

1. Like the other classes that extend UnitTestCase, is it OK to put this one in core/tests/Drupal/Tests/Core (away from all of the other Entity tests)?
2. Is the getInfo() method in this test desirable, so that this test shows up on admin/config/development/testing?
3. Is my testAccess() method anywhere close?

I'm getting a fatal error on:

$operation = $access_check->access($route, $request);

jibran’s picture

Status: Needs work » Needs review

For test

Status: Needs review » Needs work
Issue tags: -WSCCI, -WSCCI novice, -FormInterface

The last submitted patch, entity-access-check-1947432-10.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI, +WSCCI novice, +FormInterface
tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.phpundefined
@@ -0,0 +1,52 @@
+    return array_key_exists('_access_entity', $route->getRequirements());

Let's rename this _entity_access

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.phpundefined
@@ -0,0 +1,52 @@
+    $requirements = $route->getRequirements();
...
+    list($slug, $operation) = explode('.', $requirements['_access_entity']);

Turns out there is a getRequirement() method, so we should use that instead.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.phpundefined
@@ -0,0 +1,72 @@
+ * @file
+ * Contains Drupal\Tests\Core\Entity\EntityAccessCheckTest.

Contains \Drupal\...

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.phpundefined
@@ -0,0 +1,72 @@
+   * Test the method for determining if the access check applies to a specific
+   * route or not
...
+   * Test the method for checking access to routes.

Start with "Tests" and it needs to be under 80 chars.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.phpundefined
@@ -0,0 +1,72 @@
+                  ->disableOriginalConstructor()
+                  ->getMock();
+    $route->expects($this->any())
+          ->method('getRequirements')
+          ->will($this->returnValue(array('_access_entity' => '')));
+    $res = $applies_check->applies($route);
+    $this->assertEquals(TRUE, $res);
+
+    $route = $this->getMockBuilder('Symfony\Component\Routing\Route')
+                  ->disableOriginalConstructor()
+                  ->getMock();
+    $route->expects($this->any())
+          ->method('getRequirements')
+          ->will($this->returnValue(array()));

This doesn't follow our coding standard, each of the chained calls should be indented two spaces, regardless of how long the top one is.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityRouteAccessTest.php
@@ -0,0 +1,52 @@
+class EntityRouteAccessTest extends WebTestBase {

This class can be removed, as we don't need the functional test if we have the unit test.

+++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Controller/EntityTestController.php
@@ -0,0 +1,25 @@
+class EntityTestController {

Ibid.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.php
@@ -0,0 +1,72 @@
+    $this->assertEquals('node.update', $operation);

That seems wrong... access() should be returning a boolean, not a string.

Berdir’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.phpundefined
@@ -0,0 +1,52 @@
+    list($slug, $operation) = explode('.', $requirements['_access_entity']);

I don't have a lot of experience with this yet, so mostly a few questions/remarks.

Is there a reason it was combined instead of _access_entity + _access_entity_operation or something like that?

Something that we've been fighting with in the access callback function is create access, there are still changes for that pending in #1862758: [Followup] Implement entity access API for terms and vocabularies. Basically, we need to create an entity including the correct bundle to be able to do that. Would be unfortunate if we'd need a custom access check for all entity types for that.

->access() isn't entity specific, it exists for all typed data, you could e.g. also use it on a field. Sounds complicated, all I'm suggesting to do is use the specific interface (Accessible) instead of EntityInterface and a different variable name.

Would it be possible to write a generic implementation that could check access for multiple arguments? ("node:view, user:view")

mtift’s picture

Status: Needs work » Needs review
FileSize
6.8 KB
5.33 KB

Here is an updated patch that covers most of #14 and #15. I have not been able to get the testAccess() method to work correctly.

Status: Needs review » Needs work

The last submitted patch, entity-access-check-1947432-17.patch, failed testing.

tim.plunkett’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.phpundefined
@@ -0,0 +1,71 @@
+      ->will($this->returnValue(array('_access_entity' => '')));
...
+    $args = array('/foo', array(), array('_access_entity' => 'node.update'));

These would also need to be switched to _entity_access.

mtift’s picture

The attached patch changes all instances of _access_entity to _entity_access. Next, I need to dig into the PHPUnit Manual and figure out the correct way to use setConstructorArgs().

mtift’s picture

Assigned: mtift » Unassigned
FileSize
1.44 KB
5.16 KB

I still haven't figured out testAccess()

mtift’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, entity-access-check-1947432-21.patch, failed testing.

Crell’s picture

+++ b/core/modules/system/tests/modules/entity_test/entity_test.routing.yml
@@ -0,0 +1,6 @@
+entity_test_access:
+  pattern: '/entity_test/{entity_test}/delete'
+  defaults:
+    _content: 'Drupal\entity_test\Controller\EntityTestController::deletePage'
+  requirements:

I don't think we need this test route at all since we're just doing unit tests.

Berdir’s picture

Add all the tags.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.phpundefined
@@ -0,0 +1,67 @@
+    $route = new Route('/foo', array(), array('_entity_access: node.update'));
+    $request = new Request;
+    $request->attributes = 'node';

What you could use is /entity_test/{entity_test}/delete as path and directly set the entity_test object into the route defaults. We test the EntityAccessChecker not the param converter.

In general you would probably have to write a DrupalUnitTest for access checking at the moment, because the entity->access() method relies on drupal_container() which is not available in PHPUnit

Crell’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityAccessCheckTest.php
@@ -0,0 +1,67 @@
+    $request->attributes = 'node';

This is wrong. $request->attributes is a ParameterBag. Assigning a string to it is likely to break all sorts of things in exciting ways.

It should actually be set to a new mock object of type EntityInterface, and we can then mock what the access() method does.

Xano’s picture

Title: Add a generic EntityAccessCheck to replace entity_page_access() » Replace entity_page_access() and others with a typed data access checker
Assigned: Unassigned » Xano

I'm converting this to an check access for any typed data object that implements AccessibleInterface, as per Berdir's original suggestion in #16. Need to run now, but I'll post a patch later tonight.

Crell’s picture

Title: Replace entity_page_access() and others with a typed data access checker » Add a generic EntityAccessCheck to replace entity_page_access()

Ugh. I really have to stop this somewhere. Replacing every object in Drupal with typed data, a system that several people still don't understand the purpose of in the first place, is not going to fly. For Dries' sake, all we need here is to finish the tests. This issue is holding up many others at this point. Do NOT complicate it further.

tim.plunkett’s picture

Priority: Normal » Major
Issue tags: -WSCCI, -WSCCI novice, -FormInterface

I second Crell's sentiment and request.

Also, since this blocks a half dozen conversions, it's major.

tim.plunkett’s picture

Issue tags: +WSCCI, +WSCCI novice, +FormInterface

Didn't touch the tags.

Berdir’s picture

All I'm suggesting is to use the proper interface. Accessible, which defines access(), not EntityInterface, which extends from Accessible :)

msonnabaum’s picture

Just because EntityInterface extends AccessibleInterface doesn't mean we should always default to the lowest common denominator.

andypost’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.phpundefined
@@ -0,0 +1,53 @@
+    if ($request->attributes->has($slug)) {
+      $entity = $request->attributes->get($slug);
+      if ($entity instanceof EntityInterface) {
+        return $entity->access($operation);
+      }
+    }

Is it ok to return NULL? Suppose better $result = FALSE; return $result

Xano’s picture

Maybe throw an exception when the object does not implement EntityInterface to help debugging?

Xano’s picture

Assigned: Xano » Unassigned

@larowlan expressed an interest in finishing the patch this weekend.

larowlan’s picture

Assigned: Unassigned » Xano
Status: Needs work » Needs review
FileSize
4.8 KB
3.34 KB

1) Credit to @rootatwc who pointed me to an awesome reference on php mock units (this is my first time, be gentle)
2) Uses AccessibleInterface instead of EntityInterface, yes this is the lowest denominator but in this case it is the relevant chunk, the access method comes from that interface, not EntityInterface.
3) Fixes the unit tests by mocking a node.
4) Removes the test route (we don't need it with unit tests)
5) Adds a comment regarding #34 to clarify that NULL *is* a valid return, and that it indicates that this access check has no opinion.
Go bot go!

larowlan’s picture

Assigned: Xano » Unassigned
larowlan’s picture

@Crell in terms of timing - would this entity access check fire before the param converter had validated the entity?
ie would we ever hit the chunk of code that checks the $slug parameter implements AccessibleInterface without a valid option (other than when the routing configuration was broken).
IE has the param converter already verified that the slug is actually an entity by this stage of the processing?

klausi’s picture

Title: Add a generic EntityAccessCheck to replace entity_page_access() » Add a generic ObjectAccessCheck to replace entity_page_access()
Status: Needs review » Needs work

Why do we ever return NULL here? If we cannot retrieve the entity then the route configuration must be broken?

And why is this entity specific? The only thing we rely on here is AccessibleInterface, not entities? Not sure about a good name instead of _entity_access, maybe _object_access and ObjectAccessCheck?

Also: silently eating errors if the AccessibleInterface is not implemented by the object does not help developers. Either throw exceptions or just remove the check, a fatal error indication is fine that the route is somehow b0rken :-)

(I'm assuming here that the routing system already handled non existing entities at the point when the access checkers are invoked, correct me if I'm wrong. So we can assume that the object is just there.)

tim.plunkett’s picture

Title: Add a generic ObjectAccessCheck to replace entity_page_access() » Add a generic EntityAccessCheck to replace entity_page_access()

No, that's ridiculous. We have a callback to check access for entities, and we need an OO version.

We have no more need for ObectAccessCheck than we do StringAccessCheck.

I also agree with msonnabaum that we should use the interface we care about, which is EntityInterface.

larowlan’s picture

Let's just switch back to entity interface and get it in.
Simple name change from here.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.83 KB
4.66 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

We're done now. Let's unblock things.

larowlan’s picture

+1 Thanks @tim.plunkett

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Entity/EntityAccessCheck.phpundefined
@@ -0,0 +1,55 @@
+  /**
+   * Implements \Drupal\Core\Access\AccessCheckInterface::access().
+   *
+   * The value of the '_entity_access' key must be in the pattern
+   * 'slug.operation.' The slug corresponds to the named param {slug}.
+   * This will check a node for 'update' access:
+   * @code
+   * pattern: '/foo/{node}/bar'
+   * requirements:
+   *   _entity_access: 'node.update'
+   * @endcode
+   */
+  public function access(Route $route, Request $request) {
+    // Split the slug and the operation.
+    $requirement = $route->getRequirement('_entity_access');
+    list($slug, $operation) = explode('.', $requirement);
+    // If this slug is in the request, and it is an entity, check its access.
+    if ($request->attributes->has($slug)) {
+      $entity = $request->attributes->get($slug);
+      if ($entity instanceof EntityInterface) {
+        return $entity->access($operation);
+      }
+    }
+    // No opinion, so other access checks should decide if access should be
+    // allowed or not.
+    return NULL;

I have absolutely no idea what a "slug" is (assume that's some Symfony thing) but in this case we know that $slug actually means $entity, so it would make the code clearer if we just called it that.

Additionally, we need to document somewhere in here what the valid operations are, and that the first part of that key corresponds to the machine name of the entity.

And finally, I'd love to see at least one conversion in this patch too, so we can verify that this is working properly in the "real world."

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Slug is the term for {foo} placeholders in Symfony routes. It comes from http://en.wikipedia.org/wiki/Slug_(publishing)

I can revisit those docs and add in a conversion. I'll just pick whichever issue this blocks that is closest.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
21.23 KB

Okay, here it is combined with #1945564: Convert confirm_form() in shortcut to the new form interface and convert route which is a pretty representative conversion.

tim.plunkett’s picture

rootawc reminded me that now we can inject services.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks good now, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, entity-access-1947432-49.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.98 KB
22.64 KB

I typehinted the factory not the class, and the unit test needed some constants. Back to RTBC per #50.

andypost’s picture

Status: Reviewed & tested by the community » Needs work

patch needs re-roll

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Core/Entity/Shortcut.phpundefined
@@ -20,6 +20,7 @@
+ *   access_controller_class = "Drupal\shortcut\ShortcutAccessController",

after #1807042: Reorganizie entity storage/list/form/render controller annotation keys

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.phpundefined
@@ -0,0 +1,30 @@
+class ShortcutAccessController extends EntityAccessController {
...
+  public function deleteAccess(EntityInterface $entity, $langcode = LANGUAGE_DEFAULT, User $account = NULL) {

could change after #1947892: Improve DX with EntityAccessControllerInterface

andypost’s picture

Status: Needs work » Needs review
FileSize
22.27 KB
557 bytes

Also no reason to mark access callback as deprecated because it's used to grant access to edit not delete

  $items['admin/config/user-interface/shortcut/link/%menu_link'] = array(
    'title' => 'Edit shortcut',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('shortcut_link_edit', 5),
    'access callback' => 'shortcut_link_access',
    'access arguments' => array(5),
    'file' => 'shortcut.admin.inc',
  );
andypost’s picture

with patch extension

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This was RTBC before the reroll, and I guess I was a little overzealous with that @deprecated, removing it is fine. Thanks @andypost!

webchick’s picture

Title: Add a generic EntityAccessCheck to replace entity_page_access() » Change notice: Add a generic EntityAccessCheck to replace entity_page_access()
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. Thanks!

Will need a change notice to tell people what to do with former calls to entity_page_access() probably.

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: -Needs change record
Berdir’s picture

Looks good, but not sure about mentioning entity_page_access(), it's a function that we only added a few weeks ago, doesn't have a change notice (No reason to, we already knew when we added it that it's only temporary and will be replaced with this).

I'd just say something like "Route access depending on an entity access operation" (possibly linked to the entity access api change notice).

Berdir’s picture

Title: Change notice: Add a generic EntityAccessCheck to replace entity_page_access() » Add a generic EntityAccessCheck to replace entity_page_access()
Priority: Critical » Major
Status: Needs review » Fixed

Discussed in IRC with @timplunkett, changed a bit: http://drupal.org/node/1982078/revisions/view/2667842/2671300.

Let's close this now :)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.