views_embed_view() does not check access

access has to be called with the display, otherwise it always return FALSE;

    $displays = (array)$displays;
    foreach ($displays as $display_id) {
      if (!empty($this->display[$display_id]->handler)) {
        if ($this->display[$display_id]->handler->access($account)) {
          return TRUE;
        }
      }
    }

    return FALSE;

Question. Do we still need the fix that was committed in #484222: Access / Permission IGNORED in Attachment Displays? Is that solving a different problem than this?

As far as i understand, thats definitive two different issues.

I'm wondering whether the call to the method access should have the $display_id, i will do some more testing for it, but thats not related to this issue.

In general i think access should be called here. If you would have to call access yourself, you need much much more code then just calling views_embed_view.

I'm not convinced, so I'm not going to commit this without Earl's input. views_embed_view() is a (relatively) low-level API. You might need to embed a view in another context where the view's native access checking doesn't make sense. Somehow, you have to be able to just say "render the view, let me worry about who can see it"...

Furthermore, views_embed_view() specifically does *not* set the page title based on what the view says, specifically because you just want the view output and you're putting it somewhere else that the view normally wouldn't appear based on its own configuration. Access checking seems similar...

The PHP doc even says:

It's meant to provide the simplest solution and doesn't really offer a lot of options, but breaking the function apart is pretty easy, and this provides a worthwhile guide to doing so.

If you want to load the view and call $view->access() yourself, more power to you. That's what the developer docs tell you to do if you want anything more fancy than "give me the output of the given view, display, and arguments".

All that said, I can see how people could be confused by this and accidentally expose data they don't mean to via views_embed_view(). :( So, perhaps the 2nd argument (currently the optional display ID) should become an $options array, something like:

$output = views_embed_view($view_name, array('display_id' => 'default', 'check_access' => TRUE), $arg1, $arg1);

If the 2nd arg is a string, not an array, views_embed_view() would treat it as a display_id so that we don't break the API for dozens (hundreds?) of call sites already using it. I'd even be willing to consider making 'check_access' default to TRUE, but I'd have to think about it.

Anyway, I'm -1 on the patches here as they currently stand. But, I'll let Earl weigh in with his vastly more profound wisdom on such matters... Maybe the right answer is "if you really want the view regardless of access, load it and call preview() yourself". ;)

Cheers,
-Derek

The more I think about this, I've changed my mind. It's better for access() to be checked by default, and if callers really want to subvert that, they get to go a bit out of their way to load the view and call preview() themselves. I'd rather we were secure by default. $dww_vote += 2;... ;)

I'd still like Earl to comment here before I'd commit it, and I haven't actually tested this myself yet. However, my time is vanishing rapidly, so he's probably going to have to commit this if it's going to land before 9/8 or so (when I'm back from being offline for 2 weeks).

After some thought I agree, by default views_embed_view() should be access controlled and if you don't want access control it is easy enough to go around.

Committed!