User login under maintenance mode [#50901]

The system currently allows an authenticated user to login under maintenance mode, but then never shows them anything other than the default maintenance message. I think that a user without proper authorization (administer site configuration) should be denied login for two reasons:

1. the user is not truly logged in and the system should reflect that. A simple example is that if an admin is testing with a user account, the session gets updated with the uid and no way to "logout". Similarly, any reporting done (watchdog, etc) should reflect that only properly credentialed users are active.

2. from an end-user perspective, they have no way of knowing whether their login was accepted or rejected, or more importantly why. I can imagine that less experienced users might then attempt to reset their password because they "couldn't get on". With the system under maintenance mode, they should really receive a message on the login form that site login is not possible right now.


// $Id: user.module,v 1.587 2006/02/21 18:46:54 dries Exp $

//==== line901
      if (!$user->uid) {
        form_set_error('login', t('Sorry. Unrecognized username or password.') .' '. l(t('Have you forgotten your password?'), 'user/password'));
        watchdog('user', t('Login attempt failed for %user.', array('%user' => theme('placeholder', $form_values['name']))));
      }
+      if (variable_get('site_offline', 0)) {
+        if (!user_access('administer site configuration', $user)) {
+          form_set_error('login', t('Sorry. Site login is not possible at the moment.'));
+          unset($GLOBALS['user']);
+        }
+      }

Comment	File	Size	Author
#24	maint_logout.patch	2.92 KB	chx
#24
#22	maint_logout.patch	2.91 KB	chx
#22
#20	maint_logout.patch	2.29 KB	chx
#20
#19	maint_logout.patch	2.28 KB	chx
#19
#12	maint_logout.patch	3.24 KB	chx
#12
#11	maint_logout.patch	2.11 KB	chx
#11
#10	maint_logout.patch	1.67 KB	chx
#10
#9	maint_logout.patch	1.7 KB	chx
#9
#7	offline.patch	995 bytes	Jody Lynn
#7
#4	user.module_46.patch	919 bytes	ebruts
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

john.money CreditAttribution: john.money commented 23 February 2006 at 10:28

Status:

Active

» Needs review

Status => code review

Comment #2

john.money CreditAttribution: john.money commented 24 February 2006 at 01:37

Hmmm... the fix stopped working today. Here is a correction.

// $Id: user.module,v 1.587 2006/02/21 18:46:54 dries Exp $
//==== line901
      if (!$user->uid) {
        form_set_error('login', t('Sorry. Unrecognized username or password.') .' '. l(t('Have you forgotten your password?'), 'user/password'));
        watchdog('user', t('Login attempt failed for %user.', array('%user' => theme('placeholder', $form_values['name']))));
      }
+    if (variable_get('site_offline', 0)) {
+       if (!user_access('administer site configuration', $user)) {
+          form_set_error('login', t('Sorry. Site login is not possible at the moment.'));
-          unset($GLOBALS['user']);
+          session_destroy();
+          $user = user_load(array('uid' => 0));
+       }
+    }

Comment #3

ebruts CreditAttribution: ebruts commented 24 February 2006 at 11:12

Status:

Needs review

» Active

Indeed this is a good point. Please provide a real patch.

Comment #4

ebruts CreditAttribution: ebruts commented 25 February 2006 at 12:26

Component:	user system	» user.module
Status:	Active	» Needs review

File	Size
user.module_46.patch	919 bytes

I have submitted your patch (replaced if with else if) so this can be reviewed.

Comment #5

john.money CreditAttribution: john.money commented 25 February 2006 at 17:45

Thank you eberts... wasn't that I was ignoring the request to make a diff, just didn't have time to get CVS setup on this machine. Thanks again.

Comment #6

Jaza CreditAttribution: Jaza commented 25 January 2007 at 04:01

Version:	x.y.z	» 6.x-dev
Category:	feature	» bug
Status:	Needs review	» Needs work

This is still a problem in Drupal 5. The fact that non-admin users can log in under maintenance mode, but cannot log out, and are given no indication of their login/logout status, IMHO makes this clearly a bug. This should be fixed for Drupal 6, and backported to Drupal 5.

Comment #7

Jody Lynn

she/her

English

CreditAttribution: Jody Lynn commented 21 December 2007 at 21:46

Status:

Needs work

» Reviewed & tested by the community

File	Size
offline.patch	995 bytes

I tested the patch and rerolled it for head. I also changed the error message to be the same as the usual offline text so that it won't be a post-string-freeze issue. This is definitely a bug worth fixing because currently if you login as an authenticated user you will not even be able to logout in order to login again as an administrator.

Comment #8

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 22 December 2007 at 21:10

Status:

Reviewed & tested by the community

» Needs work

I don't think this message is right, we don't say 'Drupal' in a public facing message, and people suggested we even remove the default site name as Drupal, remove all Drupal path instances and replace them with other references of the concept, etc. IMHO what we can basically do is to not allow login for the user and redirect him to the frontpage, where an appropriate message will show. (Not that that message is a site setting and is themeable even, so it can be anything). We cannot make assumptions here about what message to display to the user.

Comment #9

chx CreditAttribution: chx commented 23 December 2007 at 01:45

Status:

Needs work

» Needs review

File	Size
maint_logout.patch	1.7 KB

Here we come. Note that despite I changed the return values of _menu_site_is_offline it's really not expected that anyone else would see the new logoutvalue (as the user is logged out immediately) but even if it does unless there is a check for a literal TRUE nothing changes. I simplified the code a tiny bit while there and written copious amounts of comments and doxygen. Note that even before my patch, the doxygen was incorrect.

Note: the patch was written from Gabor's instructions and have nothing to do with earlier attempts.

Comment #10

chx CreditAttribution: chx commented 23 December 2007 at 01:50

File	Size
maint_logout.patch	1.67 KB

Um. Why do we run drupal_get_normal_path in here when menu.inc is only included in a full bootstrap, way past the path bootstrap phase which does it anyways?

Comment #11

chx CreditAttribution: chx commented 23 December 2007 at 02:00

File	Size
maint_logout.patch	2.11 KB

I can't stop fixing this function, it has so many bugs... this version will allow user/login not just user. Also code underwent further simplification.

Comment #12

chx CreditAttribution: chx commented 23 December 2007 at 02:08

File	Size
maint_logout.patch	3.24 KB

The nonrepeating message breaks encapsulation and bloats the code, because the same in drupal_set_message is shorter and as a side effect its reusable in a BC way. Poor _menu_site_is_offline, not much remained :) I believe the speed impact is nil with just a plus if ran for every message call, which are quite rare anyways (debating this is microoptimization anyways).

Comment #13

JirkaRybka CreditAttribution: JirkaRybka commented 23 December 2007 at 12:44

This seems like kicking out the already-logged-in users too, which is NICE :) Unsure about the drupal_set_message() change this late, though.

Comment #14

chx CreditAttribution: chx commented 23 December 2007 at 17:18

The DSM changes are, deliberately, backwards compatible, as already noted. It'd make more sense to change the logic --ie make nonrepeated default-- but that'd be a too big change this late. And yes, I am kicking already logged in users too.

Comment #15

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 25 December 2007 at 19:19

Priority:

Normal

» Critical

Jaza wrote in #6:

The fact that non-admin users can log in under maintenance mode, but cannot log out, and are given no indication of their login/logout status, IMHO makes this clearly a bug.

This makes this a critical bug which clearly has to be backported.

Comment #16

dmitrig01 CreditAttribution: dmitrig01 commented 25 December 2007 at 20:49

@Pancho - this bug doesn't exist in Drupal 5, because the menu system is totally different

Comment #17

Pancho

UTC+2 🇪🇺 EU

CreditAttribution: Pancho commented 25 December 2007 at 21:35

@dmitrig01: Oops, okay... ;) Missed that. But still it's critical for D6.

Comment #18

Dries CreditAttribution: Dries commented 26 December 2007 at 07:53

Status:

Needs review

» Needs work

1. I don't like the additional parameter to drupal_set_message(). Can we remove that again? Why would we ever want to repeat a message?

2. if ($return = _menu_site_is_offline()) { -- $return is a bad variable name. How about $offline instead?

3. I don't like that fact that _menu_site_is_offline() returns TRUE, FALSE or 'logout'. The function should return TRUE or FALSE -- anything else is a bit of a hack.

While this patch fixes a bug, it degrades the quality of the code.

Comment #19

chx CreditAttribution: chx commented 27 December 2007 at 12:33

Status:

Needs work

» Needs review

File	Size
maint_logout.patch	2.28 KB

Why we would want to repeat a message? I have no idea, I was just keeping current behaviour. If changing that is fine, I am happier. The new patch does an external redirect instead of an internal getting rid of the 'logout' return value.

Comment #20

chx CreditAttribution: chx commented 27 December 2007 at 16:02

File	Size
maint_logout.patch	2.29 KB

This actually works here.

Comment #21

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 31 December 2007 at 15:15

Status:

Needs review

» Needs work

Dries: one reason I can imagine to display the same message multiple times is for debugging purposes. When you have PHP errors, it is possible that the same error message pops up for the same line (same byte sequence == same message text) but you have an error with a different data set (ie. you run a loop though the same code with five non-nodes, and you get errors, you should get them five times). Also, developer modules have drupal set message helpers, which set debugging info as messages. If you have different drupal set message codes and not all of them output your message (because they are the same object dumped for example), you might be a bit misdirected there.

I cannot think of any purpose for user facing duplicate messages though. Maybe it is worth moving developer messages to a 'debug' message type, instead of reusing the 'warning' core message type for example, and duplication should be possible there. This last code version rules out all possible duplication with messages.

chx: What does a drupal_goto('logout'); do in offline mode, when there is no access to that path for unprivileged users anyway? This looks like an infinite HTTP redirection recursion to me. Or do I miss something here?

Comment #22

chx CreditAttribution: chx commented 2 January 2008 at 07:43

Status:

Needs work

» Needs review

File	Size
maint_logout.patch	2.91 KB

Next attempt. Back to repeatable messages. New trick: we call user_logout directly.

Comment #23

catch

he/him

English

CreditAttribution: catch commented 2 January 2008 at 10:34

Status:

Needs review

» Needs work

After saving the maintenance mode form:

    * notice: Undefined variable: messages in drupal6/includes/bootstrap.inc on line 768.
    * warning: in_array() [function.in-array]: Wrong datatype for second argument in drupal6/includes/bootstrap.inc on line 768.

Comment #24

chx CreditAttribution: chx commented 2 January 2008 at 12:40

Status:

Needs work

» Needs review

File	Size
maint_logout.patch	2.92 KB

Comment #25

catch

he/him

English

CreditAttribution: catch commented 2 January 2008 at 14:13

Status:

Needs review

» Reviewed & tested by the community

OK so I when I try to log in as a user with no privileges, it chucks me back to the "maintenance mode" index and logs me out again. Looks like the dsm was taken out on purpose in previous iterations, that's fine with me - we're in maintenance mode, it says so right here, no need for an extra message.

user/1 and a user with administer site configuration get in as expected.

Logs say "session opened", "session closed" right next to each other for the user who's been chucked out -again, that's what's actually happening so seems fine to me.

so on that basis, rtbc.

Comment #26

Gábor Hojtsy

he/him

Hungarian

Hungary

CreditAttribution: Gábor Hojtsy commented 2 January 2008 at 14:31

Status:

Reviewed & tested by the community

» Fixed

I think we should not change message recording behavior now (ie. always disallow repeating messages), so this little API extension looks better. It also helps fix bugs elsewhere (http://drupal.org/node/194010), so committed this one. Thanks.