call to router during menu access check causes infinite loop

Experiencing problems with this also when setting site front page to be a group node.

Tried to figure out if at current url in access check, but the information isn't really there. Could guess, but would basically need to copy over a lot of logic and couldn't be certain (Perhaps, need to store the router path is then would do some code modaled after menu_get_item down to menu_check_access to get what the current arguments were, compare to arguments given, etc. [Can't directly call menu_get_item due to recursion]).

However, that wouldn't solve my issue, as front page access check is called even during things like cron, drush, which would trigger the redirect and break cron/drush.

My guess would be that it's best to move it out of spaces_menu_access, like before (originally looks like it was in nodeapi). However, looks like other items were changed to prevent that (when moving them out it access denied me, meaning no nodeapi call with $page = 1 to react to).

Summery above: Basically due to above no node can be set to site wide front page that is part of a space that would cause a redirect.

So my desires:
-----

1) Have items with spaces_menu_access redirect like currently
2) Not break drush / cron / etc.
3) Not limit to certain urls (could have node/%node/notifications for example)

Approaches I considered
----

Init -- ruled out, wouldn't help 2

Back to Nodeapi/user (revert that commit) -- wouldn't always work for 1; doesn't work on access denied.

Switch around callback to only operate for the page access check == wouldn't help 2, and damn hard without abusing debug_backtrace among other atrocities; ya don't really know what exactly you're checking access without assumptions.

Wrap page callback like access -- wouldn't work for access denied which is likely

Change current path on drush (just to solve my homepage issue)-- not sustainable

And some other ideas I forgot

Current Approach
----

preprocess_page so know that it is rendering *a* page (ie not drush, cron).

Grab all the objects using code abstracted from functions called via menu_get_item (due to access denied, objects not stored with the item) and run spaces_router.

Ugly, slower, but works and is likely less conflicting causing and a bit more flexiable.

I Hate it, specially the passing the path around using a global, and having to remake the objects, but it works for both 1, 2, and 3.

Small tweek

meh, serializing issues with reusing those functions

I traced an cron issue of an infinite loop to this same sequence. spaces_menu_access() calling spaces_router(). It only occurs when cron is attempting to send a notification for a certain content type.

I received an error with the patch in #4 stating it was malformed.

Here is a re-roll which works for me (and also looks the same as #4)

After applying the patch my cron run worked.

hmm... something funky happened with my re-roll

trying again.

Setting it to needs work because fixing this issue reveals other issues, ie. #960318: Access to node/%node/edit controlled by OG instead of spaces and OG, #955074: Spaces_menu_access tests for current space for links relavent to other spaces.

I think might need a more generalized approach to say "This should be tested if needs to be redirected"

I am having this problem as well. An issue I found with the patch in #8 is that when you visit /node/x you are not properly redirected to foo/node/x and instead get an access denied error. This becomes a big problem when the node is linked from outside a space. Attached is a stab at addressing this, and although it seems to work (and maintains group privacy like expected), the approach feels sketchy.

Tylor, take look at the use of global $spaces_current_path; the patch is using it to get around that issue, so why doesn't it work in your case?

Arg(1) seems risky in this case; the check in that function doesn't care what position the object is in; I've used it in cases like blahblah/blah/blaah/%node and there's no checking whether arg(1) corralates to the current object being processed (think user/1 and node/1).

Here's a quick update using $spaces_current_path instead of arg(1), a little stronger but still sketchy.

The problem for me seems to be that because no space is set when you hit http://foo.com/node/1234 and the variable 'spaces_features' does not contain the feature driving my content ('atrium_book', which probably shouldn't be in this variable anyway), spaces_access_feature() then returns false (from the 'No active space' case) to spaces_menu_access() and brings down an access denied for the page. Once the access denied is triggered, we can't move down to the preprocess and properly route the page.

As I tried indicating, and as the comments in the patch itself mention, that global is purely for getting around the access denied.

Why is your access denied page not calling template_preprocess_page? (see the theme('page', http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ac...). Are you calling die() or such in your access page callback? In that case, add the check at the bottom? Or is there some sort of funky caching going on?

That is why I introduced the weirdo icky global use, so I could find out what the page /would/ have been if access denied had not been called, so preprocess page stuff can determine where would have been. (there's no reliable hooks between the access check and page callback that will work on any generic page, TMK; hook_init is too early due to the drush issues. hook_nodeapi/hook_user is too specific.).

For anyone testing, please try #8 still.

Sorry for the stubbornness and thanks for your patience hefox, still getting a handle on the architecture and how this all connects. You were right that spaces_preprocess_page() was getting called.

I traced the calls from spaces_preprocess_page() and found that my problem is space_type_purl->activate() (called from space_og->activate()). Here, verify_purl() returns false and purl_goto() is called. Since purl_goto is using $_GET['q'], the redirection is from /access-denied to /group/access-denied.

Should verify_purl() return true here or is this where you are expecting the redirection to originate? If not, how is $spaces_current_path injected once you are activating space_og?

One way around this was to reset the path in spaces_preprocess_page() using $_GET['q'] = $spaces_current_path; (adding to patch from #8).

Ya + other stuff made me rethink some of this.

I think, in spaces access checks, if there's not already, need a way to tell if the current item is part of the space and only check access it is, which would also get around the access denied/need for global issue and #955074: Spaces_menu_access tests for current space for links relavent to other spaces.

However, I wonder if there also needs more dynamic way to indicate that redirection check needs to happen outside of the access checks, as from the other issues, it's easy to miss a need for redirection.

(Take in mind, I'm not a maintainer of spaces, just an opinionated patch maker).

Noticed also that this patch can cause calls to user_category_load() without all the mapped arguments when visiting user/x/edit (reload after visiting, this happens late in the page load).

A redirect when called from Drush is a separate issue, since it's not a loop--even a single redirect breaks Drush requests. Moved to #982708: Redirect breaks drush.

Removing the Drush issue should permit a much simpler fix to this bug.

Untested patch attached.

menu_get_object calls menu_get_item, where the access check is done, and the information that menu_get_object would need is only set when a user has access to the menu item. ( #19 == infinite loop any time spaces_menu_access is called from menu_get_item).

The current implementation of spaces_router inside of spaces access check causes various issues and there's various concerns; I don't think any of the patches provided so far (including my own) fully address the issue. I plan to keep my main comments on this issue to this issue as it feels like the solutions to the various issues are likely to be addressed together.

In order to be able to provide a smoothly working website, pages that are in a space should redirect to space; this is the reason why spaces redirects, I think.

What would be best if the links had the current purl to begin with, but as most links are not clicked on, say, listing of nodes, the extra processing that would be needed for that would be unpleasent, I suspect. Purl does not use custom_url_outbound TMK; node/nid would need to translated to what og group it belonged to what purl, which is okay for one link but not tons, imo, and still won't catch all cases.

Those include the basics, node/%node/view and node/%node/edit, but should also likely include more 'complex' paths like comment/reply/%node (so for example a list of nodes posted on comment on a user profile could have a comment link that would correctly link). The current spaces implementation actually works for non-standard placement of the object as it does that foreach and doesn't care what placement the object is in, which is quite nice.

But it doesn't work for views (node/%/something)* unless the view is defined via hook_menu as node/%node/node and access callback overridden later to be spaces_menu_access.

The requirement that spaces_menu_access is used to trigger a redirect seems a bit problamtic; as evidence: spaces_og itself doesn't use spaces_menu_access, so no redirect on node/%node/features* access check (see spaces_og_menu_alter and the issue on that).

I'm getting to the opinion that the system to redirect needs to be plugable in such a way that views and other paths can easily plug in (perhaps via context?), but should be separate of access check, based on it's complicated to decide the current access check is being called for the current page on (I looked into doing that at the beginning) and tends to miss some suitations easily.

Spaces_menu_access is a cool function and should stay, but should restrain it's checking to correct space for the given item. (For example, if in a site wide menu I have a link to node/5, which is a post in a community, I'd either expect it to always show if it's part of a public community, and for spaces_menu_access to check the space it belongs to; same thing for /user).

* With the current implementation, the the access check on the other local tasks cause a redirect, which is ...ugly.

Please, not trying to be mean, but those patching for this issue, please spend time considering the different aspects of it. A simple solution is likely possible (looking directly at $_GET['q'] >.O), but I'm hopeful someone will come up with flexiable one.

@hefox: I thik many of the questions you're raising deserve attention. But as I suggested in #18, most of what's been discussed here seems tangential to the reported bug. Specifically, anything to do with a redirect triggered on the home page should be handled in #982708: Redirect breaks drush--it's a different bug.

The patch in which this bug was introduced changed routing from being limited to the current page request to applying to multiple items (multiple router calls per page request). The fix therefore looks simple: reinstate the limit to the current page request.

In my testing menu_get_object() in this context does not trigger recursion.

However, the attached patch works just as well as that in #19 and on my testing either #19 or this one resolve the bug.

Subscribing.

subscribing so I know when this is part of the standard release #21 worked for me as I was having a hell of time getting a node that replicated other nodes working

Subscribing.

Neither patch 19 or 21 work for me. They both still result in infinite loops, albeit different ones.

One thing that does work for me (although I'm not sure it's right and so won't roll it as a patch) is wrapping the $space->activate() call in spaces_og.inc at line 137 in a (!$space->active) test.

@NigelCunningham: sounds like your error is likely unrelated to this issue but is a version of the problem in #933634: infinite loop on space activation: e.g. spaces_taxonomy redirect loop when editing term (I've just updated the title of that issue because previously it referred only to spaces_taxonomy). Please give that patch a try.

@nedjo: Thanks. Will do.

After giving this a lot more thought, this is my current preferred patch.

This patch moves the reroute into hook_init (depends on the cli patch #982708: Redirect breaks drush) (like the earlier patches, it changes spaces_menu_access to not call spaces_router). The key change is that it adds checking so extra spaces menu access checking is only used when the object is part of the current space, allowing global menu items to work addressing #955074: Spaces_menu_access tests for current space for links relavent to other spaces. Since the access callback returns true for the path that needs redirect, $item['map'] will be set so don't need to do ugly handling to get the object or current path later.

Since the hook_init doesn't check the access callback, also it addresses issues that paths (#960318: Access to node/%node/edit controlled by OG instead of spaces and OG) have not redirecting as they aren't controlled by spaces_menu_access (though ideally the path would be still using spaces_menu_access).

It does have a disadvantage; if the menu had menu items specific to one space, it will show in other spaces. I'm not sure how relevant that use case is. I could see it as desirable for some, but undesirable for others. Hm.

I copy and pasted the changes from an a patched version of spaces to a clean one, so hopefully it works.

subscribe

error: patch failed: spaces.module:693
error: spaces.module: patch does not apply
error: patch failed: spaces_og/spaces_og.module:542
error: spaces_og/spaces_og.module: patch does not apply
error: patch failed: spaces_user/spaces_user.module:102
error: spaces_user/spaces_user.module: patch does not apply
I tried against latest 6.x-3.x curl http://drupal.org/files/issues/955074_spaces_only_test_when_current_spac... | git apply

hmm.. against error messages, it seems, the patch was applied.
I will review soon.

Ok, this is my issue:
Before applied patch, if I put a group node in og_spaces to primary menu, some of my admin menu item turned into idiot, and caused infinite loop in path [space-path]/admin/etc..
The admin admin/build/features/create, and admin/build/menu/item/[item]/edit was these items.

After patch the redirect loop was gone, and now everything works well. I can't change the state rtbc, because the problem is more complex, than my, but I will play soon with taxonomy_spaces, and other deep fr-context-spaces vudu, I will report, if something wrong.
Thanks for this patch.

Not so well.. Now, my og items in primary menu aren't redirected.

I figured it out. In spaces_og_spaces_get_space_from_object($type, $object) {} everywhere was $node-> intead of $object. Fixed it.

Oops, good catch!

3.1 + this one helps me with spaces / organic groups. The patch referred to #26 doesn't apply any more.

Subscribe.

NB: I came looking for this after finding https://community.openatrium.com/issues/node/3016#comment-6863 which solved my problem of being unable to "Create a new Feature" due to the loop.

@ hyrcan are you working on this? If not, i would do the port to 7.x.

Here is the D7 patch. I run a few more tests tomorrow.

Pulled in to 7.x-3.x from webflo's sandbox.