Updated: Comment #N

Problem/Motivation

After #2107137: Fix the DX for declaring custom access checkers we will have the ability to add one off custom access checks. These do not even have to implement accessInterface etc... So it makes sense to enforce that what is returned from access checkers in general is one of the AccessInterface constants.

Proposed resolution

Throw an exception in Access Manager if the return value is not one of the allowed constants. To also help with this, we may change the constants from bool/null values to strings. This will now allow people to just return TRUE, or any other value.

Remaining tasks

Patch (from related issue below), tests, fix other tests.

User interface changes

None

API changes

You have to use interface constants (which should be the case anyway).

#2107137: Fix the DX for declaring custom access checkers

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

damiankloip’s picture

Status: Active » Needs review
FileSize
4.22 KB

Here's the initial patch (yoinked from related issue).

Status: Needs review » Needs work

The last submitted patch, 2108829.patch, failed testing.

damiankloip’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.28 KB
2.25 KB

OK, rerolled for addition of AccountInterface for access checks and fix the actual test. The should pass now.

dawehner’s picture

Is there a reason why we haven't changed the actual values of these constants? Only that change ensures that we do use the proper values everywhere.

damiankloip’s picture

FileSize
5.54 KB
1.55 KB

OK, I was going to have a follow up for that, but let's do it here. So how this gets on first.

I will update the tests for TRUE, FALSE in the next patch.

dawehner’s picture

I will update the tests for TRUE, FALSE in the next patch.

Do you talk about the default access checker here? Maybe we could do that in another issue.

Status: Needs review » Needs work

The last submitted patch, 5: 2108829-5.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
7.73 KB
2.18 KB

Let's see if this fixes the tests.

Status: Needs review » Needs work

The last submitted patch, 8: 2108829-8.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
10.58 KB
3.58 KB

Oh.. Let's try this. That Node revision check needs to return too, so let's return FALSE instead of NULL like it was doing before. Also fixed a couple of other unit tests.

Status: Needs review » Needs work

The last submitted patch, 10: 2108829-10.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
587 bytes
11.15 KB

This should fix the test failures.

damiankloip’s picture

The last one, great. I knew there would be one more access checker not playing by the rules. Shows that this patch is worth it IMO.

damiankloip’s picture

12: access-2108829.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 12: access-2108829.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

12: access-2108829.patch queued for re-testing.

damiankloip’s picture

FileSize
11.16 KB

Rerolled.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +undefined

Adding some tags.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2108829-17.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

17: 2108829-17.patch queued for re-testing.

dawehner’s picture

Component: base system » routing system
Status: Needs review » Reviewed & tested by the community
Issue tags: -undefined +WSCCI

re-rtbc

Crell’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -261,6 +261,11 @@ protected function checkAll(array $checks, Route $route, Request $request, Accou
+      if (!in_array($service_access, array(AccessInterface::ALLOW, AccessInterface::DENY, AccessInterface::KILL), TRUE)) {
+        throw new AccessException('Access services can only return AccessInterface::ALLOW, AccessInterface::DENY, or AccessInterface::KILL constants.');
+      }

Which access checker failed? If we're going to throw an exception with the expectation that it gets displayed, it should be helpful to the developer and say which access checker/method is Doing It Wrong(tm). That way it's easier to fix.

I like the idea of this patch on "fail usefully" grounds, but we should go all the way to failing very usefully.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
11.21 KB
1.64 KB

Ok, that IS a fair point I guess :)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good point!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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