| Project: | Flag |
| Version: | 6.x-2.x-dev |
| Component: | Miscellaneous |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Modules are allowed to vote either by explicitly allowing or disallowing access. Every subsequent check overwrites previous so order of check is critical. Example of wtf I found:
flag_flag access methods check user_access() on flags. Then any module can overwrite that check, giving access even while user shouldn't have access. Ok, I thought, in some rare cases this could be useful. So modules have last say in the process. But then flag_user's access methods are not consistent with this: they first take parent access check results (which already contain modules decisions) and overwrite them again. For example, a module can explicitly allow some user to flag himself even while globally that flag is not configured to do that, but it won't work because flag_user overwrites modules decision with it's own check.
So my question is do we have official policy on order of checks, and if we don't, should we make one ? Currently it looks like a mess. My points:
- If we allow modules to grant access even against Flag's own decisions, we should do it consistently, meaning every child class of flag_flag implements own access methods with own checks going first and module checks in the end.
- If some settings are not overridable by modules, it should be stated officially, which ones are overridable and which are not. It should also be clear why a setting is not overridable.
Comments
#1
Generally ALL access check in Drupal are done in this same way. If your module wants to disallow access, you return FALSE. If your module doesn't care, return NULL. You should NEVER return TRUE unless you have a very explicit reason to do so. If all modules return NULL, access is granted.
#2
I understand that concept. But it doesn't work now. Examples of breakage:
Look at this (access method of flag_user with my comments):
<?php
function access($content_id, $action = NULL, $account = NULL) {
// Let's find out, what Flag module itself and other modules think!
$access = parent::access($content_id, $action, $account);
$account = isset($account) ? $account : $GLOBALS['user'];
// Prevent users from flagging themselves.
if ($this->access_uid == 'others' && $content_id == $account->uid) {
$access = FALSE; // Ooops! Sorry, modules! No vote for you
}
return $access;
}
?>
What is it ? Inconsistency. I simply don't understand why modules are allowed to overwrite $flag->user_access() check results, but are not allowed to overwrite $flag->access_uid check.
I hoped that if we could have such policy we would fix such inconsistencies and it would become clear what is the best practice and how subclasses of flag_flag should behave.
#3
So basically we just need to move the $this->access_uid check above
$access = parent::access($content_id, $action, $account);right? I'm fine with this or any other rearrangement we need to increase consistency. Could you post a patch? Code changes speak more clearly than explanations.#4
If we simply move $access = parent::access($content_id, $action, $account) to the end in each subclass of flag_flag, then we lose proper order of checks too: flag_flag original (is it called abstract ?) implementation first checks $this->applies_to_content_id($content_id) and terminates early if Flag is not applicable. Then if we move $access = parent::access($content_id, $action, $account) to the end of $flag_user->access (and _multiple) we will perform checks on content where Flag is not even applicable.
Ideas I have so far:
I think 3rd solution is the best: it will still allow to have everything overridable while keeping same order of checks. In rare cases flag implementations will still be able to implement own $flag->access_modules and $flag->access.
Let's discuss, and agree on some solution first before making patches cause proposed changes are rather serious.
#5
If it's too late to refactor access methods given that 2.x already in beta than simplest solution would be to require all Flag implementation to not use parent::access() and parent::access_multiple() but rather repeat all checks in it's own method. I gave it second look and it's not that much work to copy the code. That's what I meant in my 1st idea above.
#6
Attaching patch that fixes it. Also added comment of flag::access that states best practice for implementations.
#7
I think the proper fix here is what you recommended as your third option:
This makes it so that Flag can adjust the global access() functions without other modules needing to worry about the changes. It also makes it so that the module implementations of the flag class don't need to worry about duplicated logic that every implementation will need. Though like you say, if it's absolutely necessary to have full control, you can override the access() method entirely.
I named these child access classes type_access() and type_access_multiple(), because they are type-specific (node, user, comment, term, etc.) methods, though I'm not super-happy with the name, I think it's accurate.
#8
I got a little over-zealous in my code removal in the user implementation of type_access_multiple(). This version keeps the same behaviors.
#9
I haven't tested the patch, but generally it looks good.
This also means existing Flag implementations in contrib modules may need to update their code in case they already implemented access() and access_multiple() methods.
#10
Related to order and access, but a bit outside the current discussion, but figured since the area were similar.
Bear in mind it's getting late and I'm working off very limited sleep.
<?php
// Allow modules to disallow (or allow) access to flagging.
$access_array = module_invoke_all('flag_access', $this, $content_id, $action, $account);
foreach ($access_array as $set_access) {
if (isset($set_access)) {
$access = $set_access;
}
}
?>
access function of flag class
So whatever the last access check to run wins, whether it was true or false. That seems a bit potentially dangerous; people would have to watch their weights. It's also unexpected, for those familiar with hook_field_access and other checks where returning false from any disables.
The same thing happens in flag_access_multiple.
However, I wonder, should flag_access_multiple really exist? I understand the performance issue, but I'm wondering if it instead could be addressed another way.
Well, let's go first for another desire; flag_access be passed an optional last paramabeter of $object, so, for example, node_load doesn't need to be called. While node_load is cached, if it's called during hook_nodeapi insert/update, which both flag-core have on node/add edit form and actions can call, the node_load is called before the node is finished saving, so caches (both static and cache_set) various stuff (try having flag run before cache and insert a node + flag via add form, /ouch/). Since the node is often present, it seems reasonable that it could be passed as $object param like content_access (for hook_field_access) does (d6).
Now how that relates to removing flag_access_multiple, is well, use edit link from views as an example. It creates a semi loaded node (which is could cause issues with custom hook_access implementations) to call node_access('update', $node). flag_access could also be done in that manner; make a semi-loaded with uid/type (for nodes, other info for other objects) etc. in the views, else fallback to null like content_access. As long as it's documented, it should be fine. I believe views also use semi loaded nodes for access checks. This would also be quite nice for checking access on node add form (as can store additional info, like node->type in the object!)
Needing to replicate logic in both flag_access and flag_access_multiple seems a bit problematic for module developers (confusing, pitefully, etc.); like if a person doesn't know to implement flag_access_multiple (it's easy to miss; I've looked at flag code in a quick read fashion and missed it), or updates one but not the other check, etc. Just having one centerilized way to handle access checking seems better.
Anyhow, tangent or whatever done.
So back to this patch, from what I can see it adds a function that sets the 'default' value for the access, for which each hook_flag_access overrides with whatever returns a result and whatever returns a set value last takes control. Hm, I don't understand why not have all the access checks in hook_flag_access instead, other then to override them? (if that's the case, it feels like it'd more consistent for the module to disable the setting so it won't return false anyhow (hook_flag_alter).
Anyhow, conclusion of what I'd think would be most ideal:
1) Remove hook_flag_access_multiple related code, and pass in psuedo nodes (if loaded nodes not provideable), instead
2) Move type related checks into flag_flag_access (or node_flag_access user_flag_access, comment_flag_access for code separation).
3) Change the access check to return false if any access check function does so.
I didn't look at the code too deeply today, so I was likely missing why some things were done as they were done.
#11
Year later, can we have this committed ? :P
#12
I think flag_access_multiple() is just as necessary as the *_multiple() loaders in Drupal 7 (node_load_multiple) and the multiple access checks that have existed in Drupal since access systems were first introduced into nodes. Yes you have to duplicate some logic, but I don't think this is really up for debate.
Well, I was hoping for a review at some point. Last I heard, "I haven't tested the patch, but generally it looks good." If you've tested it and it looks good, please mark RTBC.
#13
Since this is an important component of the module, it really should require some tests as well.
#14
@Lars Toomre
it would be great if you did it.
@quicksketch
Unfortunately I don't have good usecase to test this now. Year ago it was relevant :)
#15
We currently don't have any tests on access checks, though we've already got #616524: Add tests for Flag access for tracking.
#16
I do not have a local D6 simpletest environment set up. Hence, I am unable to verify any tests I might write.
@quicksketch would you be interested in an untested D6 test patch to help move the tests issue forward?
#17
@Lars Toomre: Yes anything you can put together would at least be a start. Let's move that discussion over to the dedicated testing issue.
#18
I've manually tested out this patch against both 2.x branches by setting up a node flag and a user flag and tested the following:
- User cannot flag themselves
- User can only flag content of others
- User can only flag content they own
All work as expected. I'm still very, very interested in getting SimpleTests in place, let's continue that discussion in #616524: Add tests for Flag access. Right now testing this stuff is rather labor intensive.
#19
#20
Automatically closed -- issue fixed for 2 weeks with no activity.