On Fri, 24 Oct 2008 12:00:33 -0400, Angela Byron <drupal-devel@webchick.net> wrote:
Gerhard brought this up on the security team list, but it seems like it's worth broader discussion:
---
Hi there,
as more an more people use Drupal to provide non-traditional Webpages (e.g. providing services using Ajax, Flex, ...) our traditional access permission checks in hook_menu are less than ideal.
For example, you can use drupal_execute to conveniently create content or anything else. However, no check for access permissions is done since this only happens in the menu hook for node/add/whatever.
I therefore propose to push for D7 that the #access parameter on forms be made mandatory.
Opinions?
---
My initial thought on the downside is that this has implications for people who are using drupal_execute() to perform programmatic tasks (node/block/etc. creation, etc.); they would no longer work unless the script switched to user with the proper credentials (so we should probably get a nice user switching API function in core). It is also something in the upgrade steps that, if missed, will cause forms to completely disappear which is bound to result in support requests.
On the other hand, it would provide extra security and would be akin to the way we force menu callbacks to provide an access control or they don't appear for anyone. It might also help us clean up some nasty places in core (node form, I am looking at you) where we have if (user_access(...)) hard-coded.
So, I echo: opinions? :)
-Angie
I have long argued that any part of a form that should only appear for some users should *always* be added, and then controlled with #access, and the validate and submit handlers must be designed to handle either case regardless of permission settings. That makes forms much easier to form_alter in useful ways. I've been bitten by modules not doing that, or not being able to handle a form value that is #access = FALSE, way too many times, especially by OG. We may need to tweak the internals of FAPI a little so that it handles #access = FALSE in a consistent manner to make the submit hooks easier. Of course, having to specify #access everywhere would be a nightmare. So I'd propose: - *All* FAPI/drupal_render() elements must support #access. A validate/submit callback that does not function properly with #access set true or false is by definition buggy. - All elements define whether #access defaults to TRUE or FALSE if not specified. - For the main form itself, #access defaults to FALSE. For everything else, it defaults to TRUE. - Establish a standard to handle conditional form adds via #access, not via an if statement. Using an if(user_access()) is treated as a bug. If that causes bugs anywhere, then the bugs get fixed per the first item. - Document the hell out of the above so that people know the Power of the #Access side. Does that sound like a reasonable balance? In the degenerate case for a typical form, it's one extra line to add an #access => TRUE or #access => user_access() to each form. Of course, one alternative is to have an #access_callback and #access_arguments property pair a la menu API, but I don't know if that's overkill. :-) --Larry Garfield