83044 - Any page should be able to have ?GoAheadAndLogIn=1 in the URL to force a login.

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P3)

Description

[Dave Miller [:justdave]]

Summary's a little funny-looking, but the real story won't fit there.  But here's 
the scenario:

Steps to reproduce:
1) You receive bugmail for a change to a confidential bug.
2) You click the link in the bugmail from your email client
3) browser opens to the "This bug cannot be viewed unless you first log in" page.
4) click "log in"
5) fill out your username and password, then click Login

Actual results:
- You are deposited on the Query page.

Expexted results:
- You are shown the bug that you originally clicked the link to, or told that 
your account doesn't have permission to view it.

This used to work.  I'm not sure when it broke.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Works for me on both b.m.o and my own installation running the tip.

Comment 2

[Tara Hernandez]

i can't reproduce it either.  closing out.

Comment 3

[Dave Miller [:justdave]]

just managed to duplicate this again...  but here's the catch.

I accidently clicked the "Log in" link in the footer instead of the one in the 
error message.  The Log In link in the footer really should take you back to the 
same page you were already looking at instead of going to the query page...

Comment 4

[Matthew Tuck [:CodeMachine]]

As described by Dave this is not critical ...

Comment 5

[Andreas Franke (gone)]

-> Bugzilla product, User Accounts component, reassigning.

Comment 6

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 7

[datschge]

We at bugs.kde.org are using Bugzilla since about one year. All our database is 
public, so nobody is required to login first before being able to see a particular bug. 
However all our logins are done through query.cgi?GoAheadAndLogIn=1 which still 
leads to the query page instead to the refer link. This behavior is very annoying and 
scares the hell out of anyone who is not able to handle something complex like the 
query page but still needs to login to post something simple like an ordinary comment. I 
really wish someone will pick this bug up after nearly 1 1/2 years of idling time. We 
could try to fix that locally but it's not our intention to create a fork. 
 
Thank you for the much needed attention to this bug report. =) 

Comment 8

[Jacob Steenhagen]

Attachment: Partial Hack

This patch isn't a full fix for this bug as reported, but it does at least
partially solve the problem.  This patch allows any Bugzilla page to respond to
GoAheadAndLogIn (currently, only those with the funky if statement [such as
query.cgi and show_bug.cgi] do).  It also changes the link in the footer for
logging in to point to index.cgi instead of query.cgi (to reduce the
complexity).

Comment 9

[Jacob Steenhagen]

Attachment: Slightly more complete hack

This version does bring the user back to the page they were viewing provided
they weren't viewing it because of a POST (eg, the information required to
bring them back is located in the QUERY_STRING portion of the URL).

Comment 10

[Jacob Steenhagen]

Comment on attachment 121977 [details] [diff] [review]
Partial Hack

I'm not entirely sure if I'm using the new CGI module stuff corretly, but it
does work :)

Comment 11

[Jacob Steenhagen]

I'm not sure how to solve the problems with attachment 121978 [details] [diff] [review] (POSTed form)...
perhaps the easiest way is to simply use attachment 121977 [details] [diff] [review] (the 1st) instead.

Comment 12

[Bradley Baetz (:bbaetz)]

Comment on attachment 121977 [details] [diff] [review]
Partial Hack

>Index: Bugzilla.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v
>retrieving revision 1.6
>diff -u -r1.6 Bugzilla.pm
>--- Bugzilla.pm	22 Mar 2003 04:47:09 -0000	1.6
>+++ Bugzilla.pm	29 Apr 2003 03:30:03 -0000
>@@ -65,6 +65,7 @@
>     }
> 
>     $type = LOGIN_NORMAL unless defined $type;
>+    $type = LOGIN_REQUIRED if Bugzilla->cgi->param('GoAheadAndLogIn');
> 

Move this line above the previous one to save a comparision, and r=bbaetz

The other patch cannot work, because of the POST issue. If you want to do that,
you'd have to:

a) Make sure that all the pages which do modifications only accept POST
responses, and error out for GET. Stuff which does both, like attachment.cgi,
makes this harder. Note that we should do that anyway at some point, anyway

b) Go to index.cgi if the current page was a POST (again, get that from [%
Bugzilla.cgi %] in the template)

Does it matter that our redirection pages such as xml.cgi don't pass this on? I
don't think so (since they never worked before)

Comment 13

[Jacob Steenhagen]

Dave, good enough?

Also, should we leave this bug open for a more complete fix?

Comment 14

[datschge]

Please leave it open since this fix regrettably isn't sufficient in our case. At bugs.kde.org we use 
vanilla Bugzilla scripts but heavily changed templates which seem to cause 
index.cgi?GoAheadAndLogIn=1 to not work for us. I'll look what exactly causes this behavior, but in 
the end it would be really nice in any case to just have query.cgi?GoAheadAndLogIn=1 work as 
expected (ie. let the user return to the page where he started his login procedure). 
 
But first thanks for picking up this old issue that quickly, I really appreciated it. =) 

Comment 15

[Jacob Steenhagen]

Simply using index.cgi as the login page won't work unless you make the code
change (also included in this patch) in Bugzilla.bm for 2.17.4. If you are
trying to use an earlier version (eg, before the Auth module patch), a different
code change would be required. I'm not sure what that change would be exactly
w/out looking at it, but it would involve a modification to the
quietly_check_login() routine.  Without that change, only a select few CGI's are
capable of processing a login.

Comment 16

[Dave Miller [:justdave]]

I like the idea of checking the method used, and if it was the result of a POST,
link to index.cgi instead of the current page.  That sounds like a decent
workaround for the POST problems.

Comment 17

[Bradley Baetz (:bbaetz)]

dave: There are issues, though, with that. I can't think of any off hand, mind
you logins would be, because they can turn a GET into a POST, but thats not an
issue here. Although, what would happen if you logged in, gave an incorrect
username/password, and then clicked on the login link? We'd have to make that
one generic to index.cgi, I guess, but that coul dlead to surprising results.

We'd also need the check I mentioned in (a).

The other issue, however, is that for utf8 forms, we need POST. Will that affect
stuff? What about utf8 aliases?

that fact that we have to login to make any changes, and so this link then won't
show, does make this easier. (although what if you try to login to
relogin.cgi?). I'm just not convinced that its always safe, and won't lead to
'stuff' happening more than once.

Comment 18

[Jacob Steenhagen]

Attachment: More complex hack v2

The login with incorrect password problem is quite interesting, but this patch
doesn't address it at all.  I think my prefered behavior would be to include
the login form again on that page instead of just an error.

This patch also doesn't address the utf8 issues and I'm not even certain if I
understand them. Is it that any form w/an item that uses utf8 characters in
it's name has to be POSTed or any form that submits utf8 information?

The relogin.cgi was also interesting... if you visit
relogin.cgi?GoAheadAndLogIn=1 (w/the code part of this patch applied) it will
prompt you for your username and password. As soon as you supply them, it will
tell you that you were logged out :)

I'm not sure that this is the right method, but here's the patch anyway :)

Comment 19

[Bradley Baetz (:bbaetz)]

Comment on attachment 122146 [details] [diff] [review]
More complex hack v2

The issue is that if you have a login which fails, then that form var will be
present in the cgi variables.

So when you go to log in, the script will see the login info already there, try
to authenticate, and fail.

Now, that doesn't happen because login requests are POST. I know that userprefs
has those form vars to deal with authentication too. Thats POST as well,
though.

Is there anywhere which does that as a GET? Theres nothing stopping someone
from just typing that into the URL field of their browser. I'd feel safer if
you stripped out Bugzilla_login and Bugzilla_password first. You can use
canonicalise_query (from Bugzilla::CGI) to arrange that.

Comment 20

[bdonlan]

I may be missing something obvious, but why not have the login page look at the
Referrer: header? It could stuff in in a hidden field, and do a 302 redirect on
a successful login. If it fails, just keep the hidden field.

Comment 21

[Florencio Queiroz]

Hello friends, Any way to put this patch on version 2.16.3? The query page after
login is too complex for my users.
Thanks in advance.

Comment 22

[Dave Miller [:justdave]]

taking... Jake's Army Reserve unit has been deployed.

Comment 23

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 24

[Jacob Steenhagen]

Attachment: Partial Hack - Updated to tip

I kinda dropped the ball on this one, but here's the aforementioned "Partial
Hack" from attachment 121977 [details] [diff] [review] updated for the new Bugzilla::Auth::Login::WWW
stuff. This also puts the login form on the index page instead of just having a
link to it.

Comment 25

[Jacob Steenhagen]

Morphing this bug entirely so that it matches the patch in attachment 168612 [details] [diff] [review].
Also requesting that this make it into 2.20. Filed new bug #276565 to address
the more complex hack.

Comment 26

[Gervase Markham [:gerv]]

Any chance we can change the parameter name to something that sucks a little
less, like "login" perhaps? We can keep "GoAheadAndLogIn" as a special case
which works on query.cgi...

Gerv

Comment 27

[Jacob Steenhagen]

Gerv, that is a little out of the scope of this bug... this is really about
making any page a potential login page, but if a paramater name was agreed up by
all, it could probably be done here (keeping GoAheadAndLogIn as an alias)...

Though it is about 20 lines worth of code changes touching numerous additional
files.

Comment 28

[Dave Miller [:justdave]]

I'll take this for 2.20 branch, but it's not blocking release.

Comment 29

[Joel Peshkin]

Comment on attachment 168612 [details] [diff] [review]
Partial Hack - Updated to tip

r=joel if you skip the index.cgi change and leave out the login-small file
(filter violation)

Comment 30

[Jacob Steenhagen]

Attachment: Partial Hack - No login box

I'm pulling the part of the patch that puts the login box on the index page out
of this patch. I'll attach it to bug 165075 so the UI can be debated seperate
from this bug.

Comment 31

[Joel Peshkin]

Comment on attachment 177248 [details] [diff] [review]
Partial Hack - No login box

r=joel

Comment 32

[Jacob Steenhagen]

Checking in Bugzilla/Auth/Login/WWW.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Login/WWW.pm,v  <--  WWW.pm
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/index.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/index.html.tmpl,v  <-- 
index.html.tmpl
new revision: 1.20; previous revision: 1.19
done
Checking in template/en/default/sidebar.xul.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/sidebar.xul.tmpl,v  <--
sidebar.xul.tmpl
new revision: 1.16; previous revision: 1.15
done
Checking in template/en/default/account/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/created.html.tmpl,v
 <--  created.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v
 <--  messages.html.tmpl
new revision: 1.26; previous revision: 1.25
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v
 <--  useful-links.html.tmpl
new revision: 1.36; previous revision: 1.35
done