Diff and Revisioning modules argue over who has URL permissions [#1550856]

We already had revisioning.module enabled on a build, and added Diff. At this point we started seeing 403 Access Denied for URLs that show an unpublished revision. These are of the form:

node/194/revisions/3011/view

When we switched off diff, they were fine again.

A workaround is to re-weight revisioning at the database level in the system table, to be greater than diff:

mysql> UPDATE system SET weight = 5 WHERE name = 'revisioning';

You have to clear the menu cache when you do this. To see what was actually happening in the menu registry, we used the following code to dump out the handler for that URL, along with its access handler:

var_dump(menu_get_item('node/194/revisions/3011/view'));

When the two weights are the same (= 0), we get:

array (
  'path' => 'node/%/revisions/%/view',
  'load_functions' => 
  array (
    1 => 'node_load',
    3 => NULL,
  ),
  'to_arg_functions' => '',
  'access_callback' => '0',
  'access_arguments' => 'a:0:{}',
  'page_callback' => 'diff_diffs_overview',
  'page_arguments' => 'a:1:{i:0;i:1;}',
  'fit' => '21',
  'number_parts' => '5',
  'tab_parent' => '',
  'tab_root' => 'node/%/revisions/%/view',
  'title' => '',
  'title_callback' => 't',
  'title_arguments' => '',
  'type' => '6',
  'block_callback' => '',
  'description' => '',
  'position' => '',
  'weight' => '0',
  'file' => '/diff.pages.inc',
  'href' => 'node/194/revisions/3011/view',
  'options' => 
  array (
  ),
  'access' => false,
)

You can see that Diff is handling this URL, but the access handler seems to be set to '0'.

However, when Revisioning is weighted heavier (revisioning=5, diff=0), we get:

array (
  'path' => 'node/%/revisions/%/view',
  'load_functions' => 
  array (
    1 => 'node_load',
    3 => NULL,
  ),
  'to_arg_functions' => 'a:1:{i:3;s:10:"vid_to_arg";}',
  'access_callback' => '_revisioning_node_revision_access',
  'access_arguments' => 'a:2:{i:0;s:14:"view revisions";i:1;i:1;}',
  'page_callback' => '_revisioning_view_revision',
  'page_arguments' => 
  array (
    0 => 
    stdClass::__set_state(array(
       'nid' => '194',
       /* ... */
       'vid' => '3011',
       /* ... */
    )),
  ),
  'fit' => '21',
  'number_parts' => '5',
  'tab_parent' => 'node/%/revisions',
  'tab_root' => 'node/%',
  'title' => 'View',
  'title_callback' => 't',
  'title_arguments' => '',
  'type' => '128',
  'block_callback' => '',
  'description' => '',
  'position' => '',
  'weight' => '-10',
  'file' => '',
  'href' => 'node/194/revisions/3011/view',
  'options' => 
  array (
  ),
  'access' => true,
  'localized_options' => 
  array (
  ),
  'map' => 
  array (
    0 => 'node',
    1 => stdClass::__set_state(array(
       'nid' => '194',
       /* ... */
       'vid' => '3011',
       /* ... */
    )),
    2 => 'revisions',
    3 => '3011',
    4 => 'view',
  ),
)

Revisioning is now correctly handling the URL, and the access handler is an existing function too.

So I think the weight fix works by letting revisioning have "the last word" on what should handle that URL. The fix works, but it's a bit of a hack. Surely Diff shouldn't be responding on that URL when there's only one revision argument; that seems to be what's causing this to fail (Diff thinks it's being asked to compare two revisions, one of which is NULL and hence cannot be accessed.)

Alternative solutions could include: change the URL to have explicit /%/% at the end of it for two node arguments; or change the URL to be entirely different from the revisioning URLs.

Any thoughts?

Comments

Comment #1

labboy0276 CreditAttribution: labboy0276 commented 23 November 2013 at 00:28

I can confirm this works on D7 as well.

I was having a problem with one of our university clients who was using workbench, diff and the revisioning module. The were all trying to get access to the node on revert and we were ending up with 403 errors.

I checked the module weights of all the modules above and ran this:

UPDATE system SET weight ='15' WHERE name = 'revisioning';

Viola all is well again, thanks again for this.

Comment #2

Alan D. CreditAttribution: Alan D. commented 23 November 2013 at 01:49

Project:	Diff	» Revisioning
Version:	6.x-2.3	» 6.x-3.x-dev
Component:	User interface	» Code
Issue summary:	View changes

Year, Diff menu callbacks are strange. Minor changes can trigger the entire system to collapse, but the workaround is sound and can not be handled directly by Diff, Revisioning {system}.weight

Switching queues to see if that maintainer is interested in implementing this.

Comment #3

RdeBoer

Melbourne

CreditAttribution: RdeBoer commented 23 November 2013 at 03:08

Special case of #2142429: 403 when trying to revert node, diff and workbench installed with identical fix.
Marking as duplicate.

Comment #4

RdeBoer

Melbourne

CreditAttribution: RdeBoer commented 23 November 2013 at 03:08

Assigned:	Unassigned	» RdeBoer
Status:	Active	» Closed (duplicate)

Thanks so much for bringing this to our attention.
Rik

Comment #5

Alan D. CreditAttribution: Alan D. commented 23 November 2013 at 05:01

Sorry Rik for the duplicate :(

Comment #6

RdeBoer

Melbourne

CreditAttribution: RdeBoer commented 23 November 2013 at 06:01

Hi Alan,
That's ok -- just a side-effect of the issue being re-assigned from Diff to Revisioning queue.
Rik

Comment #7

jstoller

he/him

CreditAttribution: jstoller commented 1 December 2013 at 00:07

Priority:	Normal	» Major
Status:	Closed (duplicate)	» Active
Related issues:		+#2142429: 403 when trying to revert node, diff and workbench installed, +#482126: Triggered publish of pending revision fails if node saved and workflow state changed at once

I see this was fixed for D7 in #2142429: 403 when trying to revert node, diff and workbench installed, but the issue still exists in D6, which of course doesn't have any Workbench involvement.

As previously suggested, I was able to fix the conflict by changing the weight of Revisioning to 1 in the system table, so it executes after Diff. However, I see Revisioning's weight had been set to 1 until the 6.x-3.13 release, when it was changed due to a conflict with the Workflow module (see #482126: Triggered publish of pending revision fails if node saved and workflow state changed at once). So, can this be fixed in D6, or would the Workflow problem still be an issue. And if not, then what do we do about the ongoing conflict with Diff? Right now I get an access denied error anytime I try to view a revision if both Diff and Revisioning are installed.

I'm bumping the priority to Major, since Diff is a popular module who's use is encouraged on the Revisioning project page, and this bug renders Revisioning useless.

Comment #8

RdeBoer

Melbourne

CreditAttribution: RdeBoer commented 1 December 2013 at 00:27

@jstoller
If two modules have the same weight, than they're ordered alphabetically, if I remember correctly.
So if the weights of Diff, Revisioning and Workflow are all the same, e.g., zero, all should be good.
If changing the weight of Revisioning from 0 to 1 makes a difference, then my suspicion is that on your system the weight of Diff equals 1. Try setting the weight of Diff to 0 or even -1.
Rik

Comment #9

jstoller

he/him

CreditAttribution: jstoller commented 1 December 2013 at 08:29

Well, this is interesting. Equally weighted modules are ordered alphabetically by their path. Not by the module name. In my case, we had started moving modules into a "contrib" directory. Revisioning had been moved, but Diff had not. Since sites/all/contrib comes before sites/all/diff, Revisioning was executing before Diff. Once I moved Diff into the same contrib directory, the errors went away.

This kind of works, I guess, but it's a little tenuous. For instance, if Revisioning were included in an installation profile, but Diff was added in the sites directory, then you would have the same problem, since "profiles" comes before "sites". Likewise if you have a multi-site setup with Revisioning in sites/all/ and Diff in sites/mydomain.com/. Or if you patch Diff and move it to sites/all/patched/, which would then execute after anything in sites/all/contrib/. All of these are potential real-world use cases. The only way to ensure that Revisioning executes after Diff is to adjust its weight.

Comment #10

RdeBoer

Melbourne

CreditAttribution: RdeBoer commented 1 December 2013 at 10:07

@jstoller,
Yes that is interesting. I didn't know that my rule of thumb of "alphabetical module weight" was in fact a special case of what you've just learned.
Thanks for sharing with me and everyone who stumbles across your post.

Just goes to show how fickle the module weighting system is, when you can change your site's behaviour dramatically simply by moving a file into a different directory.

I disagree with this: "The only way to ensure that Revisioning executes after Diff is to adjust its weight."

I repeat my suggestion from #8: rather than Revisioning increasing its weight (which conflicts with its ties with Workflow), Diff could decrease its weight. A weight of -1 (minus one) is not conventional, but certainly not illegal. I'm not sure of the implications of Diff being the first ahead of the "core & contrib pack" but it's something worth trying, don't you think?

I can guarantee nothing will explode and there will be no casualties...
The worst that can happen is you cursing Diff, Revisioning and Drupal in general...
;-)

Rik

Comment #11

Alan D. CreditAttribution: Alan D. commented 1 December 2013 at 23:38

"alphabetical module weight" - maybe if all modules are registered (not installed, just detected) for the very first time. After this, it is the db order, meaning that you install any of these modules by itself, this one should always peculate to the top of the db query ordered by weight (and of the same weight). But that is on MySQL, maybe other DB do different things....

Maybe there are individual queries that do order, but I find that the order is random if same weight on module_implements() as usually the modules in question were all installed at separate times.