| Project: | Panels |
| Version: | 7.x-3.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | major |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
Access Denied Errors
Panels' ajax_router uses empty($cache) as an access check; ipe_unlock and ajax_save_form calls clear the cache in order to release the IPE lock. This causes AJAX Access Denied errors unless the page is refreshed (which reestablishes the cache most of the time, except in a race condition due to an AJAX ipe_unlock call associated with an onunload event).
The race condition occurs because "unload" events are called after the new page has been requested. This causes the cache to be built by the new page request followed by the ipe_unlock call, unsetting the cache and preventing any IPE AJAX calls from succeeding.
Layout Regression
If a user changes the layout via the IPE, the display change is saved to the database but the ctools_object_cache is not updated until the page is reloaded. If a "Customize Page" IPE request comes in before the cache is updated, it retrieves the stale layout from the cache. Once the customization is saved, it effectively overwrites the previous layout change.
Steps to Reproduce
Access Denied:
Customize Page
-> Cancel
Customize Page (or Change Layout)
Results in AJAX 403
Change Layout
-> Cancel
Customize Page (or Change Layout)
Results in AJAX 403
Layout Regression:
Change Layout
-> Choose a Different Layout
-> Save Layout
Customize Page
-> Save
Layout reverts to previous layout
Comments
#1
Jay.Dansand and I have created a patch that seems to solve the issues outlined above.
In panels/panels_ipe/js/panels_ipe.js we bound the ipe.cancelLock() call to "onbeforeunload" rather than "onunload."
This causes the lock to be cleared before the new page is requested, allowing the new page request to be sent after the unlock_ipe AJAX call.
In panels/panels_ipe/plugins/display_renderers/panels_renderer_ipe.class.php we:
#2
Probably ajax_set_layout() is not the right place to put the panels_edit_cache_set() call, but it works (at least in our tests).
More likely it should be called from panels_edit_cache_save(), which sounds like it should be saving the object cache, though currently it does not (since it just falls through to panels_save_display() which does not touch the object cache - should it? It might make sense to invalidate/update the cache when the display is saved). Or, maybe the IPE module should implement pseudohook_panels_cache_save($cache) and call panels_edit_cache_set() there, but that'd require adding some $module info to the $cache_key.
#3
How do I apply this patch? Where do I put it?
#4
This should guide you in the right direction: http://drupal.org/patch/apply
#5
After reviewing the issue priority descriptions, the priority "major" seems to be more fitting. We're hoping to get more feedback on this patch and hear from anyone who is running into a similar problem.
#6
patch doesn't apply
git apply panels_ipe_fix_ajax.patch
panels_ipe_fix_ajax.patch:10: trailing whitespace.
$(window).bind('beforeunload', function() {
panels_ipe_fix_ajax.patch:27: trailing whitespace.
$this->cache->ipe_locked = NULL;
panels_ipe_fix_ajax.patch:28: trailing whitespace.
panels_edit_cache_set($this->cache);
panels_ipe_fix_ajax.patch:41: trailing whitespace.
'lockPath' => base_path() . $this->get_url('unlock_ipe'),
panels_ipe_fix_ajax.patch:55: trailing whitespace.
// Cancelled. Release the lock.
error: patch failed: panels_ipe/js/panels_ipe.js:82
error: panels_ipe/js/panels_ipe.js: patch does not apply
error: patch failed: panels_ipe/plugins/display_renderers/panels_renderer_ipe.class.php:207
error: panels_ipe/plugins/display_renderers/panels_renderer_ipe.class.php: patch does not apply
#7
I have rerolled the patch against the latest dev version of Panels (7.x-3.x-dev). Please let me know if it is still not patching.
#8
I can't reproduce the error presented in the initial post no matter how many times I customize + cancel or customize + save + cancel. Are you sure this is still an issue in the latest 3.x-dev?
#9
Thank you for looking into this issue!
I am still experiencing some of the same issues, even in the latest 3.x-dev. I am able to reliably replicate the issue by taking the following steps:
I checked the latest 7.x-3.x-dev branch, and it still seems like the cache is getting cleared in the IPE unlock calls, which in turn revokes authorization on future calls (see the access callback). I have rolled our patch against the latest 7.x-3.x-dev version. When applied, I no longer experience authorization issues.
It should be noted that the problem is twofold; there is also an AJAX race condition on unload as described in the original post, which is fixed with this patch. The race condition may not be experienced without our included lockPath fix, because the onunload callback 404s on pages with hierarchical paths.
If there is any further information I can provide, please let me know.
Again, thanks for taking a look at this!
#10
This patch is definitely not right. Setting a locked flag in the cache is definitely wrong.
#11
The IPE does this already; we are not adding this behavior. The ipe_locked flag is undefined in the cache until the lock is obtained (when it's set to TRUE); our patch just changes unlocking behavior to set the lock flag to NULL since clearing the cache entirely isn't viable at all (as it breaks the AJAX access callback).
We settled on NULL since all the lock checks test
empty($this->cache->ipe_locked), which should treat NULL the same as when it is undefined (in the case where there was no lock obtained).There's definitely a better way out there, but clearing the cache isn't it (or the access callback needs to be rewritten):
<?php// From panels_ajax_router() in panels.module, lines 1341-1344:
$cache = panels_edit_cache_get($cache_key);
if (empty($cache)) {
return MENU_ACCESS_DENIED;
}
?>
As you can see, the current setup in panels_ajax_router() causes unlocking the IPE via clearing the cache to be functionally equivalent to de-authorizing all future AJAX calls until the client reloads the page (which regenerates the cache).
Were you able to duplicate the error as we were? This was done on a vanilla instance, so it's probably pretty common. A quick glance at the issue queue shows a few mentions of AJAX 403 errors, and I have to wonder how many are really the same as this post.
#12
I Have exactly the same problem. Patch solved it.
Is it accepted by the Panels Dev Team in order to solve the problem in next version?
#13
The thing is, when you first visit the node, there shouldn't be anything in the cache. Removing the cache should put it back to exactly that state. Leaving the cache with an unlocked flag is leaving stale data around, which is why I believe the solution is incorrect. You should absolutely be able to click customize without anything in the cache, so the problem is there. I will see if I can reproduce this issue again, though I'm dubious since last time I wasn't. However, the latest reproduction steps indicate this happened on a panel node, which I didn't test on (because panel nodes are a historical legacy and for the most part should not be used).
#14
Okay, good news: This IS reproducible using panel nodes. So the bug is specific to panel nodes.
#15
I see the problem now. With panel nodes, it's relying completely on the default caching mechanism. Unfortunately, the default caching mechanism does not actually contain enough information to reconstruct the cache, which is why this bug is occurring.
panel nodes need to implement panel_node_panels_cache_get/set and NOT use the default mechanism in order to fix this. I'll work on a patch when I get a chance.
#16
Okay, I committed this patch: http://drupalcode.org/project/panels.git/commit/eac315873403712fdc1709ca...
It resolves the issue (and makes panel nodes respect locking a little better).
That said, you should consider transitioning to panelizer from panel nodes.
#17
Automatically closed -- issue fixed for 2 weeks with no activity.