Page MenuHomePhabricator

Redirect to Special:UserLogin when logging is in required to proceed, instead of showing an error message
Open, LowPublicFeature

Description

Author: filbranden

Description:
Patch against SVN

Hello,

I have a private Wiki, in which access to any page requires authentication. I did that by using this configuration (as suggested in http://www.mediawiki.org/wiki/Manual:Preventing_access):

$wgGroupPermissions['*']['read'] = false;
$wgGroupPermissions['*']['edit'] = false;
$wgGroupPermissions['*']['createaccount'] = false;

I was annoyed by the fact that every time I accessed while not logged in I would get a "You must log in to view other pages" message, and then I would have to click on "log in" to go to the log in dialog, and then after the log in I would get a "You are now logged in" message and with a link to the page I originally wanted to see, and then I would have to click on this link again.

So I decided to patch MediaWiki to transform those into redirects instead of pages with messages. While doing it, I decided to do it in a generic and configurable fashion so that, if you agree that this might be useful to others, you may incorporate it on the main code.

This are the variables that I added to DefaultSettings.php:

/**

  • Set the $wgRedirectMustLogin flag to skip the "You must log in to
  • view other pages." notice when you do not have enough rights to view
  • the page. Set the $wgRedirectLoggedIn flag to skip the "You are now
  • logged in to ..." notice after you log in successfully.
  • These are convenient in a private Wiki, if you also set
  • $wgGroupPermissions['*']['read'], ['edit'] and
  • ['createaccount'] to false.

*/
$wgRedirectMustLogin = false;
$wgRedirectLoggedIn = false;

If you set the first of them to true on your LocalSettings.php, it will skip the first page. If you set the second one of them to true, it will skip the page that comes after a successful login, and it will jump to the page you originally requested when you got the login dialog.

I am attaching the patch. I created the patch with a "svn diff" on the trunk of phase3. I tested it against MediaWiki 1.13, it applies cleanly and it works.

I hope you find it useful and incorporate it to MediaWiki!

Thanks,
Filipe


Version: 1.14.x
Severity: enhancement
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=10868

attachment loginredirect.patch ignored as obsolete

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:21 PM
bzimport set Reference to bz15484.

ayg wrote:

There are some problems with this patch:

  1. There's no notice given on the login page that you need to log in to take the action you requested. You're just dumped on a login screen instead of what you requested, with no explanation. This is a nitpick, I guess it's pretty obvious from context, but it would be nice if this could be fixed.
  1. returnto= isn't always reliable, because it will eat query parameters. For instance, try going to http://path.to/yourwiki/index.php?title=Special:ListUsers&username=&group=sysop&limit=50 or some other special page with query parameters. With your patch, you get redirected to the login page. Then try logging in -- and you get redirected to http://path.to/yourwiki/index.php?title=Special:ListUsers, which is not what you were at before. This is a regression, because you can't click the "back" button to get back to the correct URL. returnto needs to be made more reliable before this part can be implemented by default, IMO. See bug 13.
  1. Your $wgRedirectLoggedIn change is much more general than its title suggests, in a bad way. It will change *any* use of OutputPage::returnToMain() into a hard redirect. This is definitely wrong. It breaks tons of stuff. For instance, if you try clicking a "view source" link for a page you can't edit, you get redirected to the page itself. returnToMain() is called in a lot of places where you have significant output, and then also a convenience link to return you to the previous page. In all these cases you're preventing the user from seeing the significant output. You almost certainly want to implement this option by changing something in SpecialUserlogin.php, like changing the returnToMain() call in LoginForm::successfulLogin() to OutputPage::redirect().

The $wgRedirectMustLogin functionality is probably pretty difficult to get working properly due to point (2). The $wgRedirectLoggedIn thing should be easy to do, though, it just has to be done in the correct place and not where you did it.

filbranden wrote:

Patch against revision 40611

attachment loginredirect40611.patch ignored as obsolete

filbranden wrote:

Hello,

I submitted a new patch that addresses the issues.

(In reply to comment #1)

  1. There's no notice given on the login page that you need to log in to take

the action you requested. You're just dumped on a login screen instead of what
you requested, with no explanation. This is a nitpick, I guess it's pretty
obvious from context, but it would be nice if this could be fixed.

I did not address this one, as I would see this as a feature mostly useful for private Wikis only. However, if you have a suggestion on how to fix this, let me know.

  1. returnto= isn't always reliable, because it will eat query parameters. For

instance, try going to
http://path.to/yourwiki/index.php?title=Special:ListUsers&username=&group=sysop&limit=50
or some other special page with query parameters. With your patch, you get
redirected to the login page. Then try logging in -- and you get redirected to
http://path.to/yourwiki/index.php?title=Special:ListUsers, which is not what
you were at before. This is a regression, because you can't click the "back"
button to get back to the correct URL. returnto needs to be made more reliable
before this part can be implemented by default, IMO. See bug 13.

Ok, I agree with the problem, however I fail to see why you think this is that much worse than without the patch.

For instance, do you think always redirecting to the main page would be more useful than redirecting to a page that might be broken <1% of the time?

In any case, I implemented the ability to configure this behaviour on the patch. I added yet another variable, $wgRedirectMustLoginReturnTo. Now the returnto= parameter will only be included if $wgRedirectMustLoginReturnTo is true. Its default (in DefaultSettings.php) is false, so by default you will be redirected to the main page after login. Do you think this (having a configurable way to turn the "broken" behaviour on) is an acceptable compromise?

  1. Your $wgRedirectLoggedIn change is much more general than its title

suggests, in a bad way. It will change *any* use of OutputPage::returnToMain()
into a hard redirect. This is definitely wrong. It breaks tons of stuff. For
instance, if you try clicking a "view source" link for a page you can't edit,
you get redirected to the page itself. returnToMain() is called in a lot of
places where you have significant output, and then also a convenience link to
return you to the previous page. In all these cases you're preventing the user
from seeing the significant output. You almost certainly want to implement
this option by changing something in SpecialUserlogin.php, like changing the
returnToMain() call in LoginForm::successfulLogin() to OutputPage::redirect().

Ok, I fixed this, by creating a new "redirectToMain()" function that accepts a parameter to redirect. "returnToMain()" is now defined in terms of "redirectToMain()". I changed "successfulLogin()" to call "redirectToMain()" instead of "returnToMain()". This was the easier way I could implement this without duplicating code and without fixing all the calls to "returnToMain()" to use the first parameter reliably, but maybe you might find a better way to do this.

The $wgRedirectMustLogin functionality is probably pretty difficult to get
working properly due to point (2). The $wgRedirectLoggedIn thing should be
easy to do, though, it just has to be done in the correct place and not where
you did it.

Let me know if you think this still stands.

Thanks!
Filipe

ayg wrote:

(In reply to comment #3)

I did not address this one, as I would see this as a feature mostly useful for
private Wikis only. However, if you have a suggestion on how to fix this, let
me know.

It shouldn't be hard. I'll think about it. Anyway, it's okay if it doesn't tell you this at first.

Ok, I agree with the problem, however I fail to see why you think this is that
much worse than without the patch.

For instance, do you think always redirecting to the main page would be more
useful than redirecting to a page that might be broken <1% of the time?

No, I'm saying that not redirecting to *anything* is best in this case, if we can't reliably redirect back. Again, say you go to this URL:

http://path.to/yourwiki/index.php?title=Special:ListUsers&username=&group=sysop&limit=50

You get an error page. Then let's say you have to click to get to the login page, and are *not* redirected. You log in, and hit back a couple of times, then hit refresh, and the correct page loads, with the exact same URL as before. On the other hand, if you *were* redirected, try hitting back a couple of times. You'll find that the browser (at least Firefox) won't let you get back to the URL you were at before. The page won't be in the history, because it was a redirect. So you have to re-enter it. This applies to all pages with query strings. You save one click if you get redirected, *but* you have to manually re-enter the URL, or find the link you clicked on the page before it and click it again, to get the right page. So the redirect is usually faster and more convenient, but in the case of a page with a query string, the redirect is worse if you want to get back to the exact same page.

In any case, I implemented the ability to configure this behaviour on the
patch. I added yet another variable, $wgRedirectMustLoginReturnTo. Now the
returnto= parameter will only be included if $wgRedirectMustLoginReturnTo is
true. Its default (in DefaultSettings.php) is false, so by default you will be
redirected to the main page after login. Do you think this (having a
configurable way to turn the "broken" behaviour on) is an acceptable
compromise?

No, of course that's worse than the current way and your way. See above.

Ok, I fixed this, by creating a new "redirectToMain()" function that accepts a
parameter to redirect. "returnToMain()" is now defined in terms of
"redirectToMain()". I changed "successfulLogin()" to call "redirectToMain()"
instead of "returnToMain()". This was the easier way I could implement this
without duplicating code and without fixing all the calls to "returnToMain()"
to use the first parameter reliably, but maybe you might find a better way to
do this.

This doesn't duplicate code at all, really. I've implemented it in r40621, taking a different approach from your patch. One thing that came up that I didn't foresee is that successfulLogin() is also used for the "welcome" message on new account creation, and that shouldn't be suppressed.

The $wgRedirectMustLogin functionality is probably pretty difficult to get
working properly due to point (2). The $wgRedirectLoggedIn thing should be
easy to do, though, it just has to be done in the correct place and not where
you did it.

Let me know if you think this still stands.

Well, $wgRedirectLoggedIn should now be obsolete (unless my commit is reverted). I still think that the $wgRedirectMustLogin functionality is blocked by bug 13. You could still do it without fixing that, but only if there's no query string in the current page's URL. Issue (1) from above is still something I'd like to see fixed, too, before I'd be willing to commit the patch. You could add an extra query parameter to the redirect to SpecialUserlogin specifying that an error message should be displayed.

Also, don't use a configuration variable for this. It should be enabled unconditionally. It's better to fix the behavior so that everyone will want it, instead of implementing it with flaws and allowing people to opt out to avoid the flaws.

filbranden wrote:

(In reply to comment #4)

No, I'm saying that not redirecting to *anything* is best in this case, if we
can't reliably redirect back.

Ok, I understand.

Do you think including the full URL in returnto= (URL-encoded) would fix this specific problem? In that case, any query strings would be preserverd.

I see a potential issue with it being possible to use that to create a redirect to anything, not only something inside the Wiki, but I don't think this would be a security issue as it's only a redirect.

By using the full URL, we would be able to always redirect back (am I 100% right here?), so it would be possible to use the redirect to the login page without any issues regarding not being able to reliably redirect to the original target page.

If you think that's OK, let me know so that I can provide a patch for that.

Thanks,
Filipe

ayg wrote:

There's discussion about fixing up returnto= in bug 13, so if you have any thoughts on that, please read the discussion there and comment there. I'm not sure what the best way is to approach that.

filbranden wrote:

I was thinking about these two issues:

  1. There's no notice given on the login page that you need to log in to take

the action you requested. You're just dumped on a login screen instead of what
you requested, with no explanation. [...]

And:

  1. [...] This is a regression, because you can't click the "back"

button to get back to the correct URL.

One way to solve both problems at once would be actually presenting the login form at the URL that triggered the login, in that case the form could be presented just *after* the message indicating why you got there, and as there is no redirect, the original full URL would be kept in the browser's history, in that case pressing the "back" button would take you to where you originally wanted to go. Do you think this would be a reasonable option?

However, this goes way beyond my programming skills... I could do if you would give me some hints on what to do. Anyway, let me know what you think of this idea.

There's discussion about fixing up returnto= in bug 13, so if you have any
thoughts on that, please read the discussion there and comment there.

That bug is open since 2004 and still no solution for it?! Is this really as hard as that?

Cheers,
Filipe

ayg wrote:

(In reply to comment #7)

One way to solve both problems at once would be actually presenting the login
form at the URL that triggered the login, in that case the form could be
presented just *after* the message indicating why you got there, and as there
is no redirect, the original full URL would be kept in the browser's history,
in that case pressing the "back" button would take you to where you originally
wanted to go. Do you think this would be a reasonable option?

Yes, it seems like a good idea. Unfortunately it would be trickier to do. It would be nice if we had a generic way to just dump a login form anywhere: then we could put it on top of the edit page if you're not logged in, and anywhere else where it might be handy. Currently I don't think we have the code available for this: it's all in the code for Special:Userlogin. Could be split off (and preferably improved while we're at it: AJAX create account?).

That bug is open since 2004 and still no solution for it?! Is this really as
hard as that?

No, just nobody's gone ahead and done it.

*Bulk BZ Change: Patch to open bugs with patches attached that are missing the keyword*

sumanah wrote:

Filipe Brandenburger, I'm adding the "need-review" keyword here to ensure that you get any additional review and response to your patch and your general approach that you need. Thanks.

sumanah wrote:

Comment on attachment 5304
Patch against revision 40611

Patch is obsolete and no longer applies cleanly against trunk, per http://lists.wikimedia.org/pipermail/wikitech-l/2011-November/056340.html automated testing.

sumanah wrote:

Filipe, I'm sorry to say that the codebase has changed enough, since you wrote this patch, that your code no longer cleanly merges with MediaWiki as it is in Subversion. If you have time and interest, could you take a look at the current trunk and revise your patch? And we're always in IRC if you want to discuss your approach: https://www.mediawiki.org/wiki/MediaWiki_on_IRC Thanks.

filbranden wrote:

(In reply to comment #12)

If you have time and interest, could you take a look at the
current trunk and revise your patch?

No time and no interest in reviewing a patch just to leave it to rot for another 3 years until it no longer patches cleanly anymore. I'm closing the issue as I don't have any interest in the feature anymore.

Thanks,
Filipe

Reopening. This seams like a valid feature to add to MW, even though the original poster isn't interested anymore we should leave this open in case someone else comes by and feels like implementing it.

  • Bug 58637 has been marked as a duplicate of this bug. ***

This is the correct behaviour. I will personally 2 any fix for this problem if you personally mail me with the gerrit patch.

In mobile we read the return to parameters and we show a message at the top of the login form to tell the user why they are not on the page they expected.
https://en.m.wikipedia.org/w/index.php?title=Special:UserLogin&returnto=watchlist

would be great to upstream this code to core as part of this so that a user doesn't get cobfused about what is happening.

Change 146515 had a related patch set uploaded by Parent5446:
Make UserNotLoggedIn redirect to login page

https://gerrit.wikimedia.org/r/146515

Change 146515 merged by jenkins-bot:
Make UserNotLoggedIn redirect to login page

https://gerrit.wikimedia.org/r/146515

Change 146643 had a related patch set uploaded by Bartosz Dziewoński:
Revert "Make UserNotLoggedIn redirect to login page"

https://gerrit.wikimedia.org/r/146643

Leaving PATCH_TO_REVIEW as there is some code to review/resubmit. Sounds feasible to correct, setting milestone for next release.

Change 146643 merged by jenkins-bot:
Revert "Make UserNotLoggedIn redirect to login page"

https://gerrit.wikimedia.org/r/146643

The patch needs a little bit more work.

  • Bug 10868 has been marked as a duplicate of this bug. ***

Change 148144 had a related patch set uploaded by Parent5446:
Make UserNotLoggedIn redirect to login page

https://gerrit.wikimedia.org/r/148144

Change 148144 merged by jenkins-bot:
Make UserNotLoggedIn redirect to login page

https://gerrit.wikimedia.org/r/148144

Sadly bug 70855 broke this. This is a sad day (personally I think this bug was a bigger deal than not being able to visit the login form).

My mistake. Seems I uncovered a mobile bug.
http://en.wikipedia.beta.wmflabs.org/wiki/Special:Watchlist still redirects as expected.

Bugreporter subscribed.

The patch introduced in T72855: Login as another user no longer works did reopen this task (per description), You may confirm it here: https://checkuser.wikimedia.org/wiki/test

That is not the fault of the patch for bug 70855 / T72855.

It looks like we originally resolved a slightly different issue than described – viewing restricted pages on a public wiki (e.g. Special:Watchlist) redirects as expected, but viewing pages on a private wiki doesn't.

I don't think this changed since the patch was merged; I actually thought there was a separate task about that second part, but I can't find one.

When this bug is just closed, viewing pages on a private wiki did redirect to login page.

I must agree with @matmarex: The code hasn't change since 3801a3cd288f01694e9953bd5fdc070e561078f7, as far as I can see, so this is unlikely to have worked before (in a recent version of MediaWiki). However, I think this is a valid request, and it's reasonable to minimize the needed amount of work/clicks for the user to get what he/she wants. And if we show the "You need to log in to view this page" is visible as a warning on the login page, there shouldn't be a big problem.

Change 376050 had a related patch set uploaded (by Florianschmidtwelzow; owner: Florianschmidtwelzow):
[mediawiki/core@master] Redirect to Special:UserLogin on private wikis when user is not logged in

https://gerrit.wikimedia.org/r/376050

Change 538716 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] exception: Add missing early return for UserNotLoggedIn error page

https://gerrit.wikimedia.org/r/538716

Change 538716 merged by jenkins-bot:
[mediawiki/core@master] exception: Add missing early return for UserNotLoggedIn error page

https://gerrit.wikimedia.org/r/538716

Aklapper subscribed.

Removing task assignee due to inactivity, as this open task has been assigned to the same person for more than two years (see the emails sent to the task assignee on Oct27 and Nov23). Please assign this task to yourself again if you still realistically [plan to] work on this task - it would be welcome.
(See https://www.mediawiki.org/wiki/Bug_management/Assignee_cleanup for tips how to best manage your individual work in Phabricator.)

Aklapper changed the subtype of this task from "Task" to "Feature Request".Feb 4 2022, 11:01 AM

Change 858679 had a related patch set uploaded (by RAN1; author: RAN1):

[mediawiki/core@master] Permissions: Redirect login message page to Special:UserLogin

https://gerrit.wikimedia.org/r/858679

Pppery subscribed.

Removing good first task as code review comments and the litter of past attempts show this is neither non-controversial nor easy enough.