Currently, entity access controllers get separate methods for each $operation (view, delete, update, create). We originally did that in order to give developers a way to easily split up the code as some entities currently have a horrible amount of if/else and switches in their access callbacks due to the entire thing being forced into a single function callback. Now that we got entity access controllers we got that separation naturally but for some reason decided to force developers to use these namespaced methods via the interface (viewAccess, deleteAccess, etc.). Not only is that somewhat weird from an interface perspective and also considering the way it is currently invoked (we mostly map plain ->access() methods to ->{$operation}Access()).
Now, after converting a few of the core entity access callbacks to those new controllers it becomes clear that this step was not very well thought through and that the interface should probably just come with a plain ->access() method itself. We got the code separation already anyways due to us using controllers for that now. If a controller is complex enough it can very well still use separate methods, but we should not enforce it.
This issue should:
- Simplify EntityAccessControllerInterface to only require a plain ->access() method with the $operation as an argument.
- Convert all existing implementations to that new pattern and simplify wherever possible.
Comment | File | Size | Author |
---|---|---|---|
#37 | drupal-accesscontrollerdx-1947892-37.patch | 37.45 KB | Berdir |
#37 | drupal-accesscontrollerdx-1947892-37-interdiff.txt | 968 bytes | Berdir |
#26 | drupal-accesscontrollerdx-1947892-26.patch | 36.51 KB | Berdir |
#26 | drupal-accesscontrollerdx-1947892-26-interdiff.txt | 11.52 KB | Berdir |
#22 | drupal-accesscontrollerdx-1947892-22.patch | 32.38 KB | Berdir |
Comments
Comment #1
theduke CreditAttribution: theduke commentedAfter looking at the AccessControllerInterface implementations,
I fully agree that one access() method is sufficient.
Attaching a patch.
I might have missed some dynamic invocations of *Acces(), but should have found most.
We'll see if some tests fail.
Also, I am not sure if it's possible that some other operation than view, update, create or delete are checked.
I decided to return false for unknown operations. Let me know if this is unnecessary.
I checked all affected files with drupalcs, and it reports no errors (well, none affecting my edits).
Comment #3
theduke CreditAttribution: theduke commentedWhoops, missed one access() call that was causing infinite recursion...
Comment #5
theduke CreditAttribution: theduke commentedWhy is $GLOBALS['user'] not a instance of the User class but stdClass??
Anyway, this should fix the failures.
Also made UserAccessController::viewAccess protected.
Comment #6
Berdirglobal user is more like a session object not a user entity, we need to fix that (not by making it an entity but something else).
Thanks for working on this!
Comment #7
BerdirCareful with fixing unrelated things like this.
The problem is that there might be another patch that fixes this as well (either only does or another patch that fixes it too) and then those patches will unnecessarily conflict.
checkAccess() maybe?
I prefer early returns over such variables. Then it would just be if view return TRUE, elseif ... return user access else return FALSE.
So either that or at least rename the variable to $access, I think that would make more sense.
While we are at it, we should probably rename this to check/computeAccess because then we get the static cache functionality from the base class for free.
Not sure if we can keep this, the reason for this is the fallback at the end, which allows access if it's public by default. So we need to either keep method as access() or move the logic at the bottom to somewhere else.
We have now at least three different ways to implement the check access method.
- Ignore what parent does (custom block)
- Fallback to parent if not defined specifically (block)
- Use parent implementation (the access hook) if something is returned. (node)
I think we should unify all implementations to they way node works, the hook isn't very useful and works differently otherwise.
We could maybe even move the hook invocation into the default access method() and only call checkAccess() if nobody overrides it with the hook?
Comment #9
theduke CreditAttribution: theduke commentedI also noticed another problem:
For view operations, the passed in $account is not taken into account, but always the global user.
BlockInterface::access() does not allow an account to be passed in.
I will open a ticket for extending the interface to allow just that.See #1951386: Extend BlockPluginInterface::access to allow passing in an account
Comment #10
theduke CreditAttribution: theduke commentedComment #11
theduke CreditAttribution: theduke commentedI addressed most of your feedback.
All extending controllers now use the caching of EntityAccessController.
The base access() calls the entity_access hook, and then $this->checkAccess if no result is returned.
So you just need to override checkAccess with the specific checking.
NodeAccessController does implement access() too though, to ensure that "bypass node access" permission always takes precendence.
I also (hopefully) fixed the logic and order in NodeAccessController.
And I don't really think this is a trivial issue, so removing the novice tag.
-----------
This probable is not the right place to discuss this, but:
#1862750: Implement entity access API for nodes has some discussion on hook_entity_access() and hook_entitytype_access().
Right now, hook_entitytype_access() is invoked.
Should we add hook_entity_access() too?
Comment #12
BerdirThanks, I really like where this is going.
I thought about making checkAccess() abstract but that's a bit tricky because then you can't use it directly anymore. But that doesn't really make sense because if nobody implements the hook then it doesn't return anything.
About the hook, I'm not sure, is there a use case to grant access to all entity types and not a specfic one?
The problem with that is often that we need to decide which one is called first but this should not be a problem here, we could just call both, merge $access together and then check. But I think we should discuss that in a separate issue.
You're skipping the cache set here.
We also need to introduce constants instead of TRUE/FALSE as we have for Node right now and then use those there but that should be a separate issue I think, to keep this one reviewable.
CheckS was correct here.
typo in "acces". Also, I'd suggest to start with the second sentence on the same line and use as much as possible of the 80 characters. It is easier to read IMHO.
The parent does nothing, so this is kinda pointless, I'd just remove the else and only return something if you know what to do.
This one should also be updated to either return immediatly (I *think* we usually do that but don't have strong preference) or use $access as the variable.
I'm working on a patch to clean up the User type hint mess :)
Comment #14
BerdirFixed the test fails and the stuff pointed out in #12.
Comment #16
BerdirMessed up the re-roll. Interdiff is good.
Comment #18
Berdir#16: drupal-accesscontrollerdx-1947892-16.patch queued for re-testing.
Comment #19
andypostMethods vs switch() - looks like switch() requires a less code to write
entity_load('user', ...)
Please clarify why uid=2
Comment #20
BerdirDoesn't matter if it's user_load() or entity_load(), both loads an entity that is defined in entity.module. Using entity_load() just paints it in a different color :) #1825332: Introduce an AccountInterface to represent the current user is the place to solve this.
It's uid 2 because uid 1 has no permission restructions. This is copied from another test method, it's just required now because the structure is now different and we do a load for all calls by default.
Comment #21
fubhy CreditAttribution: fubhy commentedNormally I'd say "let's please use entity_load() in favor of entity-specific wrappers" but this is really just temporary and won't be necessary once we can rely on the $account argument.
Other than that this looks good already.
Here are a few minor things that I found. Mostly nitpicks.
That last 'else' statement is redundant in both places.
These 'breaks' are redundant given the return statemens above them.
This could also be simplified by moving the return statements right into the switch statement instead of first populating $account and then returning it at the end of the method.
Comment #22
BerdirRe-roll, simplified those conditions a bit.
Also trying to remove the default arguments of those protected helper functions, allows to remove some user_loads() but might be a problem if one of those user_load()'s returns NULL, but that shouldn't be happening...
Comment #23
andypostIn case of different operations the result would NULL so better return predictable TRUE/FALSE if no condition for operation provided
suppose this require return FALSE after condition
Comment #24
BerdirNo. FALSE/NULL are different. NULL means I have nothing to say if access should be granted or not and I'll let others decide. FALSE means access is denied, no matter what others say.
While that isn't important in some cases as this is the last check, node for example is different as the node access controller falls back to node grants if this method returns FALSE.
Comment #25
Berdir#22: drupal-accesscontrollerdx-1947892-22.patch queued for re-testing.
Comment #26
BerdirRe-rolled, used @inheritdoc, made that method protected everywhere. Converted the new comment access controller, removed the interface there as it's no longer of any use.
Comment #27
andypostPatch looks nice improvement just minor:
drop this "break"
Comment #28
BerdirYes, I wasn't sure about that, do we have any coding standards for that? @fubhy said the same, @dww in IRC said that he personally likes them, so I'm not sure :)
Comment #29
tim.plunkettThe coding standard implies but does not specify that there should always be a break;
I always have break; in case I refactor out the return statements later. I'd say leave them in.
I like this patch a lot, EntityAccessControllerInterface was very confusing when I first read it.
Comment #30
theduke CreditAttribution: theduke commented@Berdir: thanks for continuing this, sadly I was too busy to finish things up.
Comment #31
fubhy CreditAttribution: fubhy commentedYah, it's not that bad I guess. I can live with the 'break's. So RTBC+1 from me too.
Comment #32
andypost#26: drupal-accesscontrollerdx-1947892-26.patch queued for re-testing.
Comment #34
Berdir#26: drupal-accesscontrollerdx-1947892-26.patch queued for re-testing.
Comment #36
andypostpatch needs re-roll
Comment #37
BerdirThe patch still applied but we got a new access controller that needed to be updated.
Comment #38
andypostBack to RTBC
Comment #39
Berdir#37: drupal-accesscontrollerdx-1947892-37.patch queued for re-testing.
Comment #40
alexpottCommitted 47308d2 and pushed to 8.x. Thanks!
I guess we need to update https://drupal.org/node/1862420
Comment #41
BerdirOk, updated https://drupal.org/node/1862420, made quite some changes, see https://drupal.org/node/1862420/revisions/view/2628104/2665924.
Comment #42
alexpottLooks good
Comment #43
andypostAdded example of usage for $operation - I think this Change Notice requires it