Currently, Views implements a pass-through check of user_access('access all views') at the main menu access callback level. This can be troublesome for a large class of access plugin implementations that may restrict access not as an access-check per se, but to control visibility of menu items for a variety of reasons (e.g. displaying a tab only for certain node types).

This patch moves this access check into the domain of each plugin, allowing a plugin to alternatively omit the 'access all views' permission as a positive condition for access. This is also more consistent with the behavior or views_block() and just a raw $view->access() check.

Patched against DRUPAL-6--2, also works with DRUPAL-6--3.

CommentFileSizeAuthor
#7 673852_views_check_perm_callback.patch3.85 KByhahn
#5 views_check_perm_callback.patch3.16 KBdawehner
views_check_perm_callback.patch3.84 KByhahn

Comments

dawehner’s picture

dawehner
Primary language German
commented 
+++ views.module	4 Jan 2010 15:21:27 -0000
@@ -447,17 +443,29 @@ function views_access() {
+  $account = is_null($account) ? $user : $account;

Why not use isset here? isset is always faster.

Its used also for user_access of core.

Powered by Dreditor.

yhahn’s picture

yhahn commented

No good reason -- I did not know isset is faster. Seems like a fine change to make.

dawehner’s picture

dawehner
Primary language German
commented

In contrast to is_null is isset a part of the language php and not a function. Thats the same case with empty.

+++ views.module	4 Jan 2010 15:21:27 -0000
@@ -447,17 +443,29 @@ function views_access() {
+  return user_access($perm, $account) || user_access('access all views', $account);

See below

+++ views.module	4 Jan 2010 15:21:27 -0000
@@ -447,17 +443,29 @@ function views_access() {
+  return array_intersect(array_filter($rids), $roles) || user_access('access all views', $account);

What about reversing the checks. Then php does not execute array_intersect if its not needed.

Powered by Dreditor.

I will have time tomorrow to test the patch and write simpletests for it.

yhahn’s picture

yhahn commented

I think reversing the checks is fine and your explanation makes sense.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new views_check_perm_callback.patch3.16 KB

So here is a new version.

yhahn’s picture

yhahn commented

Works as expected. Thank you for the change.

yhahn’s picture

yhahn commented
StatusFileSize
new 673852_views_check_perm_callback.patch3.85 KB

This updated patch fixes a bad ternary. Fix by tomhung.

dawehner’s picture

dawehner
Primary language German
commented

Which one?

yhahn’s picture

yhahn commented

I wrote it, don't worry your conscience can be guilt free : )

In views_check_roles():

Evil patch:

+  $account = isset($account) ? $user : $account;

Good patch:

+  $account = isset($account) ? $account : $user;
dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Hehe i thougth this could be my problem.

The difference make sense, but a simpletest would be cool :)
The rest looks fine. I tested functionality before manually

yhahn’s picture

yhahn commented

Yeah... views is a bit too big and large to continue flying without tests right? : )

dawehner’s picture

dawehner
Primary language German
commented

Hehe, there are some tests already written, but it tests perhaps 5% of views :(

So i try to make tests for every new patch, because there it is easy to know what to test. But this could be done later.

merlinofchaos’s picture

merlinofchaos commented
Status: Reviewed & tested by the community » Fixed

This makes me truly loathe Drupal's behavior with UID 1.

Committed to all branches.

Status: Fixed » Closed (fixed)

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