Hi, this is my setup:
Flag module (7.36)
Authcache
Authcache Ajax
Authcache Flag

I have created two flags:

  1. Subscribe user - users are not allowed to subscribe themselfes
  2. Like content - users are allowed to like content of themselfes and others

Thanks to Authcache Ajax flags are loaded via ajax.
When user visits his own content, both of the flags are not displayed, yet he should be able to likes his content.

I was able to find the reason of this action. The flags go to frontcontroller in certain order (this is important). The bug happens, when the first flag is Subscribe user. And I found the bug in the code.

First, frontcontroller gets handler and run handle method (file authcache/modules/authcache_p13n/includes/AuthcacheP13nDefaultRequestHandler.inc). Handle method executes build method and this invokes another build method - from contentBuilder, and this is in AuthcacheP13nFragmentAssemblyBuilder.inc
This is the code:

  public function build($req, $context) {
    global $user;

    $result = array();

    foreach ($req['a'] as $paramname => $params) {
      // Skip when there is no handler present.
      if (empty($this->partials[$paramname]['renderer'])) {
        continue;
      }

      $partial = $this->partials[$paramname];

      // Run loader.
      if (!empty($partial['loader'])) {
        $params = $partial['loader']->load($params, $context);
      }

      foreach ($params as $key => $subject) {
        // Run access check.
        if (!empty($partial['access']) && !$partial['access']->check($user, $key, $subject, $context)) {
          throw new AuthcacheP13nRequestAccessDenied();
        }
        else {
          $result[$paramname][$key] = $partial['renderer']->render($key, $subject, $context);
        }
      }
    }

    return $result;
  }

There is foreach loop, which throws AccessDenied error with first flag that user is forbidden to use. But then it has impact on next flags as well - loop is interrupted.

Even with access denied for some of the flags, the result still shoud be returned. I believe this code:

  // Run access check.
        if (!empty($partial['access']) && !$partial['access']->check($user, $key, $subject, $context)) {
          throw new AuthcacheP13nRequestAccessDenied();
        }
        else {
          $result[$paramname][$key] = $partial['renderer']->render($key, $subject, $context);
        }

should be changed to this:

  // Run access check.
        if (empty($partial['access']) || $partial['access']->check($user, $key, $subject, $context)) {
          $result[$paramname][$key] = $partial['renderer']->render($key, $subject, $context);
        }

with this code everything works.

Sadly i have no idea if this change has any bad impact on other functionality, but i provide a little patch if you find it useful.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matysek145 created an issue. See original summary.

matysek145’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

The last submitted patch, 2: multiple-flags-403-2711505-2.patch, failed testing.

znerol’s picture

Looking into this now. I understand the problem and I agree on the solution. I am fixing tests now. One problem I see is that we are introducing an inconsistency here. I.e., the assembly builder still might short-circuit with a 404 if only one fragment fails to load. So I guess we also should protect against that case (and simply return empty fragments for unloadable content).

znerol’s picture

znerol’s picture

Modify test case such that it now mirrors exactly the situation reported in this issue.

Status: Needs review » Needs work

The last submitted patch, 14: multiple_flags_403_for-2711505-14.patch, failed testing.

znerol’s picture

Status: Needs work » Needs review

  • znerol committed cb9f111 on 7.x-2.x
    Issue #2711505 by znerol, matysek145: Multiple flags 403 for all
    
znerol’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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