Last modified: 2014-09-23 23:07:40 UTC
Created attachment 5290 [details] 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
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. 2) 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. 3) 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.
Created attachment 5304 [details] Patch against revision 40611
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. > 2) 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? > 3) 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
(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.
(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
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.
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: > 2) [...] 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
(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*
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.
Comment on attachment 5304 [details] 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.
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.
(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.
