OpenID login fails when in maintenance mode

Just ran into this and it's persistent in HEAD.

There might be a better way around this....

functionality test, and coder review'ed one line patch :)

Hm, ick. :\ I really don't want to put in conditionals for optional core modules in menu.inc. Plus, if OpenID has this problem, others such as LDAP integration module probably do too, for example.

Let's ask chx/pwolanin if they can come up with something clever to work around this.

Sure they can.

If someone wants to write a test: implement test module with a single login_paths hook with filter/tips ('cos that has an access callback TRUE), drupalPost (admin / maintenancepage ) , logout, check the page you just defined it's accessible, check another page say 'node' to see it's naccessible, done!

Mmm, this feels a little cumbersome. Need to ponder about it a bit.

I can't think of a better solution.

We're still missing tests, regardless of the final solution that gets accepted.

Tagging.

My site have the same problem on version 6.12.

Still needs tests in 7.

We can't add another hook in Drupal 6, so this would have to be something like #2 for 6.

#5: hook_login_paths.patch queued for re-testing.

This should be critical as it means people who depend on logging into their website via OpenID can't log into their site if it's in maintenance.

Here is a reroll of the patch, so it applies. Looking into writing tests. Agreed adding hooks in Drupal 6 does not sound like a good idea.

Ok, looking at the existing test case, I'm not sure we can get better coverage by adding more. Since the patch moves the existing user and user/login paths to the hook, testing that this hook works will already be required for existing tests to pass. This is what we have:

/**
 * Tests site maintenance functionality.
 */
class SiteMaintenanceTestCase extends DrupalWebTestCase {
  protected $admin_user;

  public static function getInfo() {
    return array(
      'name' => 'Site maintenance mode functionality',
      'description' => 'Test access to site while in maintenance mode.',
      'group' => 'System',
    );
  }

  function setUp() {
    parent::setUp();

    // Create a user allowed to access site in maintenance mode.
    $this->user = $this->drupalCreateUser(array('access site in maintenance mode'));
    // Create an administrative user.
    $this->admin_user = $this->drupalCreateUser(array('administer site configuration', 'access site in maintenance mode'));
    $this->drupalLogin($this->admin_user);
  }

  /**
   * Verify site maintenance mode functionality.
   */
  function testSiteMaintenance() {
    // Turn on maintenance mode.
    $edit = array(
      'maintenance_mode' => 1,
    );
    $this->drupalPost('admin/config/development/maintenance', $edit, t('Save configuration'));

    $admin_message = t('Operating in maintenance mode. <a href="@url">Go online.</a>', array('@url' => url('admin/config/development/maintenance')));
    $user_message = t('Operating in maintenance mode.');
    $offline_message = t('@site is currently under maintenance. We should be back shortly. Thank you for your patience.', array('@site' => variable_get('site_name', 'Drupal')));

    $this->drupalGet('');
    $this->assertRaw($admin_message, t('Found the site maintenance mode message.'));

    // Logout and verify that offline message is displayed.
    $this->drupalLogout();
    $this->drupalGet('');
    $this->assertText($offline_message);
    $this->drupalGet('node');
    $this->assertText($offline_message);
    $this->drupalGet('user/register');
    $this->assertText($offline_message);
    $this->drupalGet('user/password');
    $this->assertText($offline_message);

    // Verify that user is able to log in.
    $this->drupalGet('user');
    $this->assertNoText($offline_message);
    $this->drupalGet('user/login');
    $this->assertNoText($offline_message);

    // Log in user and verify that maintenance mode message is displayed
    // directly after login.
    $edit = array(
      'name' => $this->user->name,
      'pass' => $this->user->pass_raw,
    );
    $this->drupalPost(NULL, $edit, t('Log in'));
    $this->assertText($user_message);

    // Log in administrative user and configure a custom site offline message.
    $this->drupalLogout();
    $this->drupalLogin($this->admin_user);
    $this->drupalGet('admin/config/development/maintenance');
    $this->assertNoRaw($admin_message, t('Site maintenance mode message not displayed.'));

    $offline_message = 'Sorry, not online.';
    $edit = array(
      'maintenance_mode_message' => $offline_message,
    );
    $this->drupalPost(NULL, $edit, t('Save configuration'));

    // Logout and verify that custom site offline message is displayed.
    $this->drupalLogout();
    $this->drupalGet('');
    $this->assertRaw($offline_message, t('Found the site offline message.'));
  }
}

Now we could add another module with another path and check for that other path, but we already check for two paths which are now defined via the hook in the user module. So I think we cannot get better test coverage on this by adding a custom module.

What are your thoughts on testing the hook through menu_test.module?

@Rob Loach: that path has access control which will 403 it for anonymous users. Therefore @chx's suggestion to look at the filter/tips path instead which is access callback TRUE. Or you can just create one such path in menu_test.module to make the tests more robust.

Updated patch including my suggestions.

I reviewed the code and it looks good. I will rtbc once bot is happy.

Here's a version that I think will pass the testbot. There was one small error in hook_menu(), and I drupalLogout() didn't seem to make sense.

I have to admit to changing drupalLogout(), but I think the new version is actually more correct, as an actual user would hit /user/logout and then hit /user.

Now, given that, could I piggyback #733784: Friendly response to logged-in user landing on user/login or user/register into this one? If that's acceptable it could be rolled into here pretty easily. Instead of using a variable there, it would just change its behavior based on hook_login_paths().

Alternately, perhaps that one could follow this one in.

Great job, Randy.

I think that piggybacking is a good idea -- but needs to be done before commit because that needs a hook which returns login path => redirect (i dont want to hardwire 'user' redirect path in) and also an alter would be just nice.

Per #26, this patch extends hook_login_paths() so that it can be used in more circumstances.

1. It changes the use of the hook so it returns an associative array.
2. Uses the array to handle the case where a logged-in user accesses user/login (and other paths) with a friendly response instead of a 403.
3. Adds handling for the case where an anon user hits a protected URL and sends them to /user.
4. Adds tests for these cases.

The issues around handling these URLs with more kindness to our users is discussed in #733784: Friendly response to logged-in user landing on user/login or user/register, especially #1.

The difference between this patch and #24 is also attached.

This implements the hook as described in #26. #27 not just went over the board with the hook but misses the alter. I have created a utility function which has the alter too. I have totally removed the "anonymous user should be presented with 'user' instead of 403" that's absolutely unwanted. It's entirely possible that you want to communicate some information (premium content!) about why a page is denied. You are free to make your 403 page a redirect but adding this to core is not what we want -- a separate issue if you want but I doubt it'll get in.

I don't feel like I have much standing to comment on this issue, as I'm just trying to get in the friendly handling of auth users innocently landing on user/login and friends So I'm probably fine with any result that preserves the original issue and (hopefully) lands some friendliness to poor naive users. In general this one would work.

#28 makes the fundamental assumption that the paths which should be handled in a friendly way for logged in users are exactly the same as the paths which should be allowed to anon users during maintenance mode. I don't think this is necessarily the case. I really don't like the path openid/authenticate returning a redirect path of 'user'. Sure, it would never happen, but it's misleading and probably inappropriate. And that's the basic problem with consolidating the two different ideas together (friendly handling for logged-in users + allowed paths for anon users when in maintenance mode). Very high WTF factor.

I'm not convinced that the friendly handling of auth users shouldn't just be done with an array of paths from a Drupal variable unless we can increase the expressiveness of hook_login_paths() (and it's up to those of you who have invested in this issue whether that should happen).

I like the drupal_alter() for login_paths, and I imagine chx expected that I'd put that in, but I missed it.

We have #24 as RTBC if we just don't want to go down this route.

I ran the tests on this and it fails for some fairly trivial reasons that are easily fixed, and this approach can work with just a little fixup if it's the way we want to go.

I've attached a diff of #27 to #28 if it's useful.

+++ includes/common.inc	2010-03-14 01:38:13 +0000
@@ -2361,6 +2361,14 @@ function drupal_deliver_html_page($page_
+          drupal_set_message('The page you requested is only allowed for anonymous users, so you are redirected to @path', array('@path' => $authenticated_redirect));

Needs a t()

+++ modules/system/system.api.php	2010-03-14 01:35:14 +0000
@@ -3218,6 +3218,32 @@ function hook_countries_alter(&$countrie
+ *   An array of arrays keyed by Drupal paths. Each value is a string defining
+ *   a redirect path. If you only want to allow access during maintenance but
+ *   no redirect then specify the same path as value.

Comments not relevant to this version.

+++ modules/openid/openid.module	2010-03-14 01:19:02 +0000
@@ -39,6 +39,13 @@ function openid_menu() {
+function openid_login_paths() {
+  return array('openid/authenticate' => 'user');
+}

The WTF factor on this one is too high. Perhaps 'openid/authenticate' => TRUE? This is the whole problem with consolidating two different semantics into one simple data structure.

This one also reverted the change to drupalLogout() in drupal_web_test_case.php (logging out, then hitting /user, instead of just hitting /logout?destination=user) which improved the test in #24. I think that will have to go back in.

142 critical left. Go review some!

Yes drupal_set_message needs a t that's why a tests failed. Anyways, check the API as I wrote it , you can simply specify redirection to the same path to not redirect. I am fine changing it to TRUE if you want -- it's a good idea.

Here's another roll based on chx's comments and patch.

I had to drop 'user/register' out of the paths, because it's something that should not be allowed during maintenance mode. That's where the conflict in semantics comes in.

Other than that, I think it's basically the way @chx sees it, and it does get us a friendly response to a user landing on user/login, which is the most significant case.

Ooops. Left the test in for user/register even after the code was gone.

I did some work on this and i must realize that there are paths like user/register which are not accessible during maintenance but still require friendly redirect. Oh well! #27 it is but a) add alter b) remove the force-redirect to user on 403.

OK, here's #27 rerolled with chx's alter (and the anon-user redirect removed).

So to sum up, we have two kinds of paths

1. A path which needs to be accessed during maintenance mode.
2. A path which is accessible only to anonymous but needs a redirect when a logged in user access visits it.

These paths often have two of these. Any page that has a login form on it, like user/login or any other site-specific login form is both. So we thought it's best to solve the two together.

The bugfix to drupalLogout() (two-step logout, first hitting user/logout, then hitting user) is also in #721086: Create tests for system actions, clean up token replacement, clean up triggers, so whichever one goes in second will need a reroll.

Critical bump.

Fails testing, re-rolling.

Removed "needs tests" because the patch includes tests.

Rerolled patch from #36

patch works as promised on todays drupal dev

Like so many issues, it's hard to figure out how we got from the start to the finish on this one. Here's an attempt at an issue summary.

• OpenID has a magic path that gets used for logins.
• However, in Maintenance mode, the checking for what paths are "login paths", meaning paths that should be allowed when the site is in maintenance mode, was done in common.inc or somewhere that it was not extensible.
• Thus, it's impossible for an admin to log in using openid when in maintenance mode, which is why this is a critical issue.

This patch provides a new hook_login_paths() to deal with this problem in a generic, extensible way. It was obvious that since we had this hidden problem with OpenID, we could have the exact same problem with many other situations, including contrib modules providing alternate authentication, etc. So now user.module and openid.module implement hook_login_paths() to declare paths that should be available when in maintenance mode.

While doing this, it became obvious that there's another case where it's important to know when a path is a "login path". Authenticated users who stumble upon a "login path" have traditionally in Drupal been given an incomprehensible "access denied" instead of a friendly redirection. So we allowed hook_login_paths() to provide information about that situation as well, specifying how "login paths" should be redirected if an authenticated user lands on them.

The bottom line: This is critical because it takes core openid out of service at a terribly important time. It's done as a hook so that problems like openid's can be handled easily by the module instead of requiring a core patch.

Marked #551880: Make "Request new password" available to anonymous users when site is offline a duplicate (or at least a dependency) of this one.

I'd like to propose one minor change to this patch which would support wildcards in hook_login_paths().

<?php
function drupal_get_login_paths($path = NULL) {
  $login_paths = module_invoke_all('login_paths');
  drupal_alter('login_paths', $login_paths);
  if (isset($path)) {
    foreach ($login_paths as $login_path => $settings) {
      if (drupal_match_path($path, $login_path)) {
        return $settings;
      }
    }
    return FALSE;
  }
  return $login_paths;
}
?>

I think that makes perfect sense and would be an improvement. It would require additional tests, though.

I'm hesitant to muck with this one as it hasn't gotten committed even as is. But your improvement is excellent.

What does anybody else say? Should we muck with it?

Also, since #551880: Make "Request new password" available to anonymous users when site is offline has been marked as dup, should we use user/password and user/reset/* as login paths? The use case: Site is in maintenance mode, admin user doesn't have password so has to use the user/password to access the site. This must have happened to Shawn DeArmond before.

Exactly... well, not to me personally, but to my clients who were supposed to be adding content to an "under construction" site. I didn't grant them MySQL access (for obvious reasons) and I was inaccessible to them to reset their password in an appropriate amount of time.

I already wrote a little contrib module that takes advantage of this hook and solves #551880: Make "Request new password" available to anonymous users when site is offline in a very flexible way. If this patch is committed, I'll create the project node and contribute the module. No point in doing so until this patch is in core, though.

@Shawn, I think we should go for it. Please roll a patch. If you can, please add a test for the user/password + user/reset path.

Sure. I'll do that tonight or tomorrow. I'm going to have to also add the hook implementation into common-test.module. Think that's the best place? hook_login_paths() is invoked in common.inc, so I would imagine that's where it should go.

@Shawn, I was assuming you'd add the /user/password and /user/reset/* right into user.module's hook_login_paths(). I think it makes sense.

@rfay, oh. That's different. Okay, I can do that.

Okay, here's a patch, re-rolled from HEAD, which includes my change from #46, and includes user/password and user/reset/* in user_login_paths(). Also, I added tests to the maintenance mode tests in system.test which verifies that the password reset functionality works when the site is in maintenance mode.

@Shawn: It looks like you lost the existing tests (see #42). Please put them back in.

This brings back the tests from #42, although there is a little bit of overlap.

It also updates the hook documentation to mention that we can now handle path patterns.

The issue summary is in #44.

If somebody would RTBC this minor change maybe we can get it in this week.

Once again, a fine issue summary is in #44.

#55: drupal.openid_maint_363580_55.patch queued for re-testing.

Still applies…

#55: drupal.openid_maint_363580_55.patch queued for re-testing.

Verified patch. First verified that issue occurs without patch. Applied patch drupal.openid_maint_363580_55.patch and user can now login in using OpenId when site is in maintenance mode. Tests passed. #ladrupal #lacodecamp

This looks like a fine patch but I wonder if we can simplify this. Instead of adding another hook, can we do something based upon convention (i.e. say anything below /user/login/%)? It seems like that could simplify things drastically ... Thoughts?

On the other hand, this is consistent with hook_admin_paths() ...

+++ includes/common.inc
@@ -2371,6 +2371,15 @@ function drupal_deliver_html_page($page_callback_result) {
+        if (!empty($login_path['authenticated_redirect']) && $_GET['q'] != $login_path['authenticated_redirect']) {

1) isset() seems to be sufficient here instead of !empty().

2) Could use type-agnostic comparison, i.e., !==.

+++ includes/common.inc
@@ -2371,6 +2371,15 @@ function drupal_deliver_html_page($page_callback_result) {
+          drupal_set_message(t('The page you requested is for anonymous users, but you are already logged in, so you have been redirected to this page.'));

Do we really need this message? I'd prefer to drop it.

+++ includes/common.inc
@@ -2466,6 +2475,32 @@ function drupal_exit($destination = NULL) {
+ /**
+ * Collect and optionally check a login path.

Wrong indentation on first line.

+++ includes/common.inc
@@ -2466,6 +2475,32 @@ function drupal_exit($destination = NULL) {
+ * @param $path
+ *   An optional Drupal path.

Do we still call this "Drupal path"? I thought it's a "system path" in the meantime, or similar without "Drupal" in it...?

+++ includes/common.inc
@@ -2466,6 +2475,32 @@ function drupal_exit($destination = NULL) {
+function drupal_get_login_paths($path = NULL) {
+  $login_paths = module_invoke_all('login_paths');
+  drupal_alter('login_paths', $login_paths);
+  if (isset($path)) {
+    foreach ($login_paths as $login_path => $settings) {
+      if (drupal_match_path($path, $login_path)) {
+        return $settings;

hm, this loop is quite slow from a performance perspective.

+++ includes/menu.inc
@@ -3364,9 +3364,13 @@ function _menu_site_is_offline($check_only = FALSE) {
+      // Return TRUE if this is an anonymous user unless they are at a login
+      // path, in which case return FALSE to allow them to visit the page.

Reads a bit weird; read it 3 times, but still don't understand...

"Anonymous users have to be able to access certain pages in maintenance mode."

+++ includes/menu.inc
@@ -3364,9 +3364,13 @@ function _menu_site_is_offline($check_only = FALSE) {
+      // if drupal_get_login_paths()[['maintenance_mode_allow_access'] is TRUE,
+      // return FALSE from this function, meaning to allow access.

1) Should start with a uppercase letter.

2) Strange parenthesis here.

3) It also looks like this entire sentence could be moved down, above the actual line that invokes drupal_get_login_paths(), so as to maintain context.

4) However, with simplification of the previous sentence, this second one might as well be dropped entirely.

+++ modules/openid/openid.module
@@ -39,6 +39,13 @@ function openid_menu() {
+function openid_login_paths() {
+  return array('openid/authenticate' => array('maintenance_mode_allow_access' => TRUE));

+++ modules/simpletest/tests/menu_test.module
@@ -329,3 +335,18 @@ function menu_test_static_variable($value = NULL) {
+function menu_test_login_paths() {
+  // Allow access to ?q=menu_login_callback even if in maintenance mode.
+  return array('menu_login_callback' => array('maintenance_mode_allow_access' => TRUE));

Can we maintain a consistent coding style for this hook implementation and use a multi-line array syntax, please?

+++ modules/simpletest/drupal_web_test_case.php
@@ -1083,10 +1083,11 @@ class DrupalWebTestCase extends DrupalTestCase {
   protected function drupalLogout() {
-    // Make a request to the logout page, and redirect to the user page, the
+    // Make a request to the logout page, and then to the user page, the
     // idea being if you were properly logged out you should be seeing a login
     // screen.
-    $this->drupalGet('user/logout', array('query' => array('destination' => 'user')));
+    $this->drupalGet('user/logout');
+    $this->drupalGet('user');

Why is this changed? I'm not sure in which way the user/logout behavior is related to this issue/patch...?

+++ modules/system/system.api.php
@@ -3888,6 +3888,64 @@ function hook_countries_alter(&$countries) {
+ * This hook is called to determine whether special action is to be taken
+ * when a path is a login path. For example, if the path should be available
+ * to anonymous users even though the site is in maintenance mode, or if
+ * a 403 error (such as with user/login for an authenticated user) should be
+ * handled with a redirect.

"special action"...? I think this entire Doxygen paragraph can be improved and reworded a bit.

+++ modules/system/system.api.php
@@ -3888,6 +3888,64 @@ function hook_countries_alter(&$countries) {
+ *   An array keyed by Drupal path patterns. Each key is a path pattern
+ *   which will be matched by drupal_match_path() to determine whether the

We cannot and must not allow fully fledged path patterns here (which support to define multiple paths in a single key) -- I wonder whether drupal_match_path() is the proper lookup/identification in the first place... why aren't these router paths?

Also, speaking of menu router paths... isn't this becoming a parallel registry to hook_menu()? Can't we declare, register, and store those properties via hook_menu()?

+++ modules/system/system.api.php
@@ -3888,6 +3888,64 @@ function hook_countries_alter(&$countries) {
+ *   Each value is an associative array keyed by the type of behavior to
+ *   prescribe.
+ *   'authenticated_redirect' means that an authenticated user should be
+ *   redirected to the named path instead of being given a 403.
+ *   'maintenance_mode_allow_access' means that an anonymous user should be
+ *   allowed to access this path even if the site is in maintenance mode.

See http://drupal.org/node/1354 for proper list formatting in PHPDoc.

+++ modules/system/system.api.php
@@ -3888,6 +3888,64 @@ function hook_countries_alter(&$countries) {
+function hook_login_paths() {

Should be hook_user_login_paths() and therefore located in user.api.php.

+++ modules/system/system.api.php
@@ -3888,6 +3888,64 @@ function hook_countries_alter(&$countries) {
+      'authenticated_redirect' => 'mymodule_logged_in',
+      'maintenance_mode_allow_access' => TRUE,

1) Let's replace underscores with spaces in these properties.

2) We can improve these property names to make them more precise:

"redirect authenticated"
"maintenance mode access"

+++ modules/system/system.test
@@ -742,6 +740,23 @@ class SiteMaintenanceTestCase extends DrupalWebTestCase {
+    ¶

Trailing white-space.

+++ modules/system/system.test
@@ -742,6 +740,23 @@ class SiteMaintenanceTestCase extends DrupalWebTestCase {
+    $path = substr($mails[0]['body'], $start, 66 + strlen($this->user->uid));

What's the devilish "66" here?

+++ modules/user/user.module
@@ -1698,6 +1698,34 @@ function user_menu() {
+    'user' => array(
+      'maintenance_mode_allow_access' => TRUE,
+    ),

Interestingly - shouldn't "user" redirect to "user/login" for anonymous users? (whereas "redirect anonymous" is not yet handled in this patch)

Overall, however, this entire logic is bound to menu router paths, which have access handling already. It feels a bit odd to first negotiate a request, attempt to return an access denied, and while doing so, hack in some special access cases - which should have been covered during regular access handling already - to figure out that we actually want to revert our decision and grant access, or redirect to another system path... this back and forth already sounds icky...

But hold on... why do we build a registry of user login paths here in the first place? Why don't we skip that needless registry and instead, build a quick + small $context to pass around to modules after figuring out a basic positive/negative result in http://api.drupal.org/api/function/menu_execute_active_handler/7 - i.e.

1) Figure out a basic accessible/offline/404/403 result in menu_execute_active_handler().
2) put that into a small $context array
3) drupal_alter('menu_execute_active_handler', $context)
4) proceed as usual, but based on $context

Of course, not a full-blown context à la Butler, but possibly a very first tiny step towards that.

Powered by Dreditor.

Most of sun's thorough comments are relatively easy to deal with.

However, both Dries (#62) and sun (end of #64) suggested vastly different alternate approaches for this issue as well.

The existing approach has been reviewed and RTBC on and off for months. It's important to come to a conclusion here whether we're going to abandon this and start over. IMO, the most likely and fastest way to get this "done" is to stay with the current approach.

I agree with staying with current approach. If someone can patch for sun's concerns, i will review until rtbc.

Uhm.

I started a re-roll of this patch and then pretty much got lost. I don't get the point of this hook as it is now:

- first, it is call hook_login_paths(), but is not limited to that (that hook can be used to allow access to /whatever/you/want to anon users in maintenance mode for example)
- It is used to address two technically unrelated special cases (forwarding authenticated users on specific pages and allow access to anon users during maintenance mode) which only have in common that they are related to user login paths. So, the hook is named after what we are using it for? That doesn't make any sense to me.
- As of the above, I guess it is obvious that I don't agree with renaming this to hook_user_login_paths(), and why should it be in user.api.php if we're using it in the menu system?
- Saying that, I don't know a good name for the hook either (hook_authenticed_redirect_and_allow_anon_maintenance_access_paths() :p)

I actually had a similiar idea as sun in #64, adding a hook to which we pass some information and let the hook decide if he wants to do something special instead of collecting a list of paths to handle two specific cases.

I really wanted to not delay this any further (the opposite, actually), sorry :)

Very good points, Berdir, thanks! I must confess I didn't read the issue prior to reviewing the latest patch. If we both had the same or similar idea, then I can only guess it must be the proper resolution for this issue.

I'll try to write a patch.

We already have "drupal_alter('menu_active_handler', $router_item, $path);", I'll try to move stuff around a bit so that we can re-use that.

@berdir, @sun, the issue summary is in #44. It's important that if we're going to start over the history is understood. Note that the patch as it stands has significant support and has been reviewed and RTBC for some time.

Update: One way of dealing with this is hook_menu_active_handler_alter(). However, @chx is deperately opposed to the future life of that hook, and has filed #838408: Remove hook_menu_active_handler_alter.

Here's a patch with an extended hook_menu_active_handler_alter().

- Not yet finished, I just want to see what the tests will do with my patch.
- The patch changes menu_execute_active_handler() to also load the menu router item and execute the alter hook when the site is in maintance mode. page_callback_result can be altered by hook implementations to disable (and enable, if they'd want to do that) offline mode for that request (
- moved all user specific stuff in _menu_site_is_offline() to user.module
- the hook implementation in user.module is a mess currently ;)
- I've just added the context as a third argument to the hook, if we are allowed to break the api of that hook, then we can merge path into $context. Especially because $path is actually *not* so useful as it is not set to the actual path on a normal page request.

Note that chx did not like (already existing) hook_menu_active_handler_alter() at all :) If that hook is removed, then this approach won't work anymore. But I'd say the forwarding could simply be put in a hook_init() or something like that.

While testing this, I've noticed #838404: drupal_access_denied() in page callback displays access denied page twice.

Ok, let's see if the tests pass and feel free to vote this down if you don't like it. I'm *not* interested in bikeshedding.

Oh yeah we are adding context to a hook which should not exist in the first place. This whole context idea screams Drupal 8 to me.

Hm.

Note that I did add "context to a hook which should not exist in the first place" because the hook still existed at the time I wrote the patch and I wanted to re-use something existing as it was already almost there. (And it doesn't apply because the hook got removed in the meantime ;))

- We can also make a simple hook which we call directly in _menu_site_is_offline() and allow modules to disable offline mode without giving them access to the router_item.

- We can rename $context to something different if you don't like the name ;). Also, $context is not a D8 pattern. There is for example http://api.drupal.org/api/function/t/7, which has a context key in the $options and drupal_alter() itself uses context: http://api.drupal.org/api/function/drupal_alter/7. And there are some fields hooks which use $context and probably many more that just don't use the name context but it is one :)

$ grep -R "function hook_" modules/ | grep \$context
modules/field/field.api.php:function hook_field_attach_preprocess_alter(&$variables, $context) {
modules/field/field.api.php:function hook_field_attach_view_alter(&$output, $context) {
modules/field/field.api.php:function hook_field_attach_prepare_translation_alter(&$entity, $context) {
modules/field/field.api.php:function hook_field_language_alter(&$display_language, $context) {
modules/field/field.api.php:function hook_field_available_languages_alter(&$languages, $context) {
modules/field/field.api.php:function hook_field_display_alter(&$display, $context) {
modules/field/field.api.php:function hook_field_display_ENTITY_TYPE_alter(&$display, $context) {
modules/field/field.api.php:function hook_field_extra_fields_display_alter(&$displays, $context) {

- Why not simply moving these goto()'s to hook_init() or another place in user.module that's called when this happens? There must be something we can re-use :) (custom access callback could work too, but probably a bit hacky (the current solution is hacky too, so...)).

- In fact, all I suggest is switching from a passive hook (hook implementations provide information about specific cases and core checks if these cases are met) to an active one (modules decide if they want to enable or disable offline mode). So modules like update.module or backup.module could temporarly set the site to offline mode (while uploading new files, for example). Sure, they can currently do this by setting variable_set() but that is persisting and would for example end up in your backup or will not be automatically re-enabled if the backup/upload dies with a fatal error.

The actual code could look as simple as this:
<?php
$is_offline = variable_get('maintenance_mode', 0);
drupal_alter('menu_site_offline', $is_offline);
return $is_offline;

@berdir, I'm still thinking that since we had strong consensus for the earlier approach, and we can just fix sun's style issues and questions and go forward. However, I don't want to cross wires with you on this. I'm in IRC; chx and I are at the code sprint in Denver. We could conceivably get somewhere on this today. If you think you have a viable solution (especially one that handles the whole situation) I'm fine with you running with it. Otherwise, you or I can fix up the original approach with sun's comments.

I had at one point a hope that the two issues here (login paths and authenticated redirects on 403) can be merged. user/register smashed that hope. We have dived into a bottomless rabbithole trying to solve this.

Don't.

Two hooks. One to give a list of login paths to solve this critical. One to give redirects on 403 to solve the usability critical. Two simple hooks. Do it, move on.

I began addressing sun's points in #64 but really keyed on:

speaking of menu router paths... isn't this becoming a parallel registry to hook_menu()? Can't we declare, register, and store those properties via hook_menu()?

The alternate, "mini" context option in this thread seems too great of an endeavor at this late point, but the hook_menu() idea that sun brings up does match the context option in providing a method that is not built around special cases.

So, back to the hook_menu() idea, what would that take? Would we have to alter the menu_router schema and would that kind of change even get in?

I guess I'm fine with #78. Just wondering is there is a better name than hook_login_paths. Something that better explains what it is used for (allow anon users access to paths in offline mode).

I just don't get why everyone seems to stumble over the word context. It's a 100% typical alter hook. We pass an array with some information to a hook and let other modules change it. With the new site offline hook idea from #76, it's not even a array anymore, just a single boolean (It would probably be cleaner to pass the actual path (not the $path argument which is useless in most cases) to that hook as a .... context ;))

Extending hook_menu() and have a new column in the schema to store something that ~5 of possibly hundreds of router items need sounds a much bigger change to me...

Berdir, I fully agree with you. I don't see why we need two hooks, if we can solve this elegantly by rolling back the removal of hook_menu_active_handler_alter() and slightly improving that to account for our real requirements here.

People wanted use-cases for that hook, here are two. Counter-proposal to introduce two new hooks for the very same. Confusion on a innocent word "context", just because some butlers talked too much instead of serving drinks. Yes, we are trying to fix the last criticals. But can we maintain at least some sanity and open-minded process to fix the system properly? Don't panic.

So if we can forget about variable names and release date panic for a moment, are there actually any real reasons to not go after the hook_menu_active_handler_alter() approach?

The consideration about hook_menu() was related to the last hook_login_paths() patch only. As that would return a similar router_path => properties array only. Merging these additional properties into hook_menu() would require additional dedicated storage columns for router paths. This would add to an already convoluted list of menu router and menu link properties, which we need to entirely revamp in D8.

Contrary to that, hook_menu_active_handler_alter() would allow for a quite flexible reaction to the current request. We do not build yet another registry, and simply allow modules to intercept the request, right after we figured out what we "would" want to do, but before we are actually doing it. We can't use/enhance hook_init(), menu system information is not yet available at that point.

Ok.

chx and I agreed on the following, patch should hopefully come soon, either by me or by chx (I need to finish my exams first)

- We introduce a new hook in menu_execute that can set the $page_callback_result (will be renamed to something more intuitive I guess). This will happen before we call menu_get_item(), maybe even before we call _menu_site_is_offline. That was the last idea, but I'm not sure how to easily force not to use offlne mode. Anyway, that hook can set that variable to MENU_SITE_OFFLINE, MENU_ACCESS_DENIED, MENU_NOT_FOUND or NULL. If the value is not NULL, menu_get_item() is not even called and we directly deliver the page according to the status.

- As obvious from the above description, the hook does *not* have access to the $router_item array. The only use case that has been found in #838408: Remove hook_menu_active_handler_alter is to globally deny access for a specific request, this can be done by setting $page_callback_result to MENU_ACCESS_DENIED.

- The drupal_goto()'s in case of an 403 can be moved directly into the access callbacks, as this does not need access to "the menu system" but simply $_GET['q']. Or into the new hook explained above. Or hook_init(). Or wherever they make sense.

Subscribe.

In #838408: Remove hook_menu_active_handler_alter, sun and Moshe were the two developers most lamenting the loss of hook_menu_active_handler_alter(), so I asked them to use this issue to continue their lobbying efforts for the restoration of that complete hook, rather than the more limited variant suggested in #82.

As obvious from the above description, the hook does *not* have access to the $router_item array. The only use case that has been found in #838408: Remove hook_menu_active_handler_alter is to globally deny access for a specific request, this can be done by setting $page_callback_result to MENU_ACCESS_DENIED.

Not exactly. Moshe's use-case in #838408-11: Remove hook_menu_active_handler_alter wasn't just to deny access to IP addresses in a certain country, but to do so with a custom page per country. One can also extend the use-case to do the same thing, but without a 403 header (perhaps not letting the people know they're being denied access, but instead giving them an alternate page with a 200 header). What I argued for in that issue is that per-country 403 pages would be best achieved with the 'site_403' variable set to a Drupal path whose page callback contains the necessary dispatching logic. And that per-country alternate non-403 pages would be best achieved with hook_url_inbound_alter(). But I'm not sure that sun or Moshe agree with those claims yet.

Agreed on hook_url_inbound_alter(). (This could imho also work for "The drupal_goto()'s in case of an 403 can be moved directly into the access callbacks, as this does not need access to "the menu system" but simply $_GET['q']. Or into the new hook explained above. Or hook_init(). Or wherever they make sense.")

Not sure how access to $router_item would help with the custom 403 page since drupal only allows to configure a single 403 page. So, instead, a module that would define a 'denied/%' router item would probably be needed anyway, and we need to route the request to that page (again, could even happen through the url_inbound_alter() hook).

how will hook_url_inbound_alter solve the offline issue? we need a utility function that is called instead of _menu_site_is_offline from menu_execute_active_handler. This utility function will call _menu_site_is_offline and then the new alter.

It will not. We're already discussing three different things here. The third is moshe's use case for keeping the old hook, and I think that his use case can be solved either by using drupal_goto() anywhere or by using hook_url_inbound_alter().

This is a first try, let's see what the tests say.

No helper function yet.

I also did not move the drupal_goto() to the access callbacks (yet), because user/login uses user_is_anonymous() directly and user/register indirectly and that function has that $_GLOBAL['menu_admin'] check.

Oh, and this will conflict (again) with the clean up patch in the remove hook issue, I guess.

The other issue was commited in the meantime... again :)

Git was able to auto-merge the changes, though.

The approach is solid. I have rerolled and amended the documentation.

That's better :)

There's a trailing whitespace in the hook documentation but I wan't to see some green first (or at least an actually executed test run), I guess there will be another re-roll or two anyway where we can fix that.

Fixed the failing tests. We're not testing against a router path anymore, so it needs drupal_match_path() for user/reset/...

+        default:
+        if (drupal_match_path($path, 'user/reset/*')) {

one, the if needs two more spaces of indent, two, ugh, what's wrong with a simple substr?

Fixed indentation, switched to a strpos() check. Untested...

Why forcibly logout users during maint mode? Once maint mode ends, they'll have to re-enter passwords which they potentially haven't used in months.

"the current router item is retrieve and executed"

missing a 'd' at end of retrieve

We do that since Drupal 6. I have no idea why we do nor about the consequences of changing that now. I just moved the code around (into the hook).

http://api.drupal.org/api/function/_menu_site_is_offline

Ah, OK. If anyone knows why we do that, I'm interested.

Meanwhile, I think we can send this one for commit.

+++ modules/system/system.api.php
@@ -3959,5 +3959,29 @@ function hook_filetransfer_backends() {
+ * the current router item is retrieve and executed by

retrieved instead of retrieve?

+++ modules/user/user.module
@@ -1741,6 +1741,49 @@ function user_menu() {
+          // Forward anonymous user to login page.
+          drupal_goto('user/login');
+          return;

That 'return' is not necessary because drupal_goto() will call drupal_exit().

+++ modules/user/user.module
@@ -1741,6 +1741,49 @@ function user_menu() {
+          // Disable offline mode.
+          $menu_site_status = NULL;
+          break;
+        default:
+          if (strpos($path, 'user/reset') === 0) {
+            // Disable offline mode.
+            $menu_site_status = NULL;

Should we introduce a more explicit variable instead of NULL? Like MENU_SITE_ONLINE?

37 critical left. Go review some!

Yeah, I was wondering that too. MENU_SITE_ONLINE is much easier to grok. Looking at the top of menu.inc, it looks like 0 is unclaimed as a status, although 0 for "it's working" is a little odd.

Fixed the broken system.api.php example, added a slash after user/reset, introduce MENU_SITE_ONLINE and addressed the other two concerns of Dries.

Oh the openid hunk was using == NULL too. Too bad!

All that happened was the test menu alter used NULL still.

Patch looks good to me, I agree with adding MENU_SITE_ONLINE.

@moshe, sun: Can you confirm that this hook is a sufficient replacement for hook_menu_active_handler_alter? As said before, the differences are that you don't have access to the router_item and you can only change access/offline/online/not found or forward to another page. Also, the url can be changed in hook_url_inbound_alter(), so you can either redirect to denied/some-country with drupal_goto() or internally change the path to denied/some-country.

This is a good patch and fixes a critical. I don't think this substitutes for the active_handler hook but thats OK. We can deal with that in the other issue or a new issue or whatever. Lets get this one in.

A definite +1 from me. This resolves all the issues for which I was involved in this issue. I tested the results of hitting /user/login and /user/register as auth user, and tested the login process (including getting a one-time login link) when in maintenance mode and using openid. It all seems to work fine.

The one anomaly: The "Create new account" link to user/register is visible when you visit /user and the site is in maintenance mode. The link just gets a "Site under maintenance" page, so I don't think this is worthy of worry.

Please write an updated issue summary and describe the new hook and its possible other use cases.

Thanks for the good work,
-Randy

In #108 I mentioned the purported anomaly that the "Create new account" link is visible in offline mode, even though the link is not accessible.

That was *not* introduced by this patch. I tested and it's the same without the patch.

I like this much better than previous approaches, thanks. The comments are good, and expanded tests are a great feature, as well (although, humourously enough, not one for this actual bug. ;) Is that easy to add?).

The only thing I don't quite understand is this:

+  // Allow other modules to change the site status but not the path. Code
+  // later will not use the $read_only_path variable.
+  $read_only_path = !empty($path) ? $path : $_GET['q'];
+  drupal_alter('menu_site_status', $page_callback_result, $read_only_path);

Why are we doing this belly-dancing? Why not give them access to change the path? I looked for guidance over in #838408: Remove hook_menu_active_handler_alter but only see reference to it being "made for abuse".

So if there's an actual reason, which I assume there is, let's document it here.

Other than that, this looks good to go from me.

That was a rather spontanous decision but it is quite simple (and has nothing to do with the linked issue):

Changing $path there does not change it globally but only what we pass to menu_get_item(). $_GET['q'], arg() and all other functions would still use/contain the old path. Instead, you should use hook_url_inbound_alter() when you want to change the path. I added that information to the hook_menu_site_status_alter() hook documentation but not when calling it. If the following is clear enough, I can add something similiar to that place:

+ * @param $path
+ *   Contains the system path that is going to be loaded. This is read only,
+ *   use hook_url_inbound_alter() to change the path.

Updated the documentation and added a test for openid login when the site is offline. Let's see if this passes...

Awesome, thanks!

Committed to HEAD!

+++ includes/menu.inc
@@ -447,10 +452,17 @@ function menu_get_item($path = NULL, $router_item = NULL) {
+  // Allow other modules to change the site status but not the path because that
+  // would not change the global variable. hook_url_inbound_alter() can be used

Which "global variable" ?

+++ includes/menu.inc
@@ -447,10 +452,17 @@ function menu_get_item($path = NULL, $router_item = NULL) {
+  // Only continue if the site status is not set.
+  if ($page_callback_result == MENU_SITE_ONLINE) {

The comment totally does not match the condition on the following line. Something must be wrong here?

+++ modules/system/system.api.php
@@ -3961,5 +3961,30 @@ function hook_filetransfer_backends() {
+ * The hook is called after checking whether the site is offline but before ¶

Trailing white-space.

+++ modules/system/system.api.php
@@ -3961,5 +3961,30 @@ function hook_filetransfer_backends() {
+ *   MENU_NOT_FOUND and MENU_SITE_ONLINE. Any other value than
+ *   MENU_SITE_ONLINE will skip the default menu handling system and be passed
+ *   for delivery to drupal_deliver_page() with a NULL
+ *   $default_delivery_callback.

Weird sentence... not sure I understand.

"Any other value than MENU_SITE_ONLINE skips the default menu handling and will be passed to drupal_deliver_page() [with no default callback?]."

Needs improvement.

+++ modules/user/user.module
@@ -1741,6 +1741,48 @@ function user_menu() {
+          drupal_goto('user/login');
+        case 'user/login':
...
+          break;
+        default:

1) drupal_goto() ends the request, but we should still append a break; and newline after this switch case.

2) Separate switch cases with newlines, unless it's a fall-through logic.

40 critical left. Go review some!

Just a note that we still need a backport for D6. The D7 solution seems pretty cumbersome for D6, but the original patch in #2 works fine for me as a D6 solution.

Would be nice to fix the coding style and documentation issues outlined #114.

No idea about the openid aspect - but this is still a problem for core in D6 - going to user/login returns access denied. Previous versions of this are marked as duplicates, I've been going round in circles trying to find where to post this issue.

so have posted a new issue
http://drupal.org/node/1112824

Any plans to backport this to D6?

FYI:
I did not understand the reasoning behind the change to drupalLogout(). I have created a new issue about reverting this change: #1545930: drupalLogout() should only do one request, not two

Thanks for taking care of many "Access denied" situations. I just found an outlier I think, about opening Drush generated one-time login links while logged in, and have created #3316655: Friendly response to logged-in user landing on user/reset. Sharing it here, since it's related.