276565 - "Log In" link in footer should return to same page after logging in.

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Jacob Steenhagen]

Bug 83044 will make it so that any .cgi page can have ?GoAheadAndLogIn=1 added
to the URL and become a login page. It would be nice if the Login link in the
footer could then logically point to the correct page so the user is
automatically returned to the page their viewing after logging in. This does
have some issues that were orignally raised in bug 83044:

> Bug 83044 comment 12 (Bradley Baetz)
> 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)

Bug 83044 comment 16 (Dave Miller)
> 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.

Bug 83044 comment 17 (Bradley Baetz)
> 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.

Bug 83044 comment 18 (Jacob Steenhagen)
> 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.

> 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 :)

Bug 83044 comment 19 (Bradley Baetz)
> 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 1

[Jacob Steenhagen]

BTW, "the other patch" in bug 83044 comment 12 is referring to attachment
121978 [details] [diff] [review]. It's follow up, attachment 122146 [details] [diff] [review] also has some issues as mentioned in
comment 0.

Comment 2

[Reed Loden [:reed]]

I have a working patch for this on https://landfill.bugzilla.org/prodpatches/.

Comment 3

[Reed Loden [:reed]]

Attachment: patch - v1

Implement requested feature. This works for me on prodpatches. I don't know who the proper reviewer of this would be, so I'm picking somebody. :)

Comment 4

[Olav Vitters]

Comment on attachment 249613 [details] [diff] [review]
patch - v1

>Index: template/en/default/global/common-links.html.tmpl
>-      <li><span class="separator">| </span><a href="index.cgi?GoAheadAndLogIn=1">Log&nbsp;In</a></li>
>+      [% current_url = (cgi.request_method == "POST" OR cgi.script_name.match("relogin")) ? "index.cgi" : cgi.script_name %]
>+      <li><span class="separator">| </span><a href="[% current_url FILTER html %]?[% cgi.query_string FILTER html %][% "&amp;" IF cgi.query_string %]GoAheadAndLogIn=1">Log&nbsp;In</a></li>

As LpSolit said, do not use cgi.script_name. See bug 325079 for details. Suggest to use the same logic as cgi.url(Relative => 1) can return an empty string.

Further, if cgi.request_method is POST, cgi.query_string will still contain the posted params (so not just everything after the ?). If POST was used for some reason, we should not make those params visible again via GET.

Comment 5

[Reed Loden [:reed]]

Attachment: patch - v2

This is what is currently on production bmo.

Comment 6

[Reed Loden [:reed]]

Checking in template/en/default/global/common-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/common-links.html.tmpl,v  <--  common-links.html.tmpl
new revision: 1.7; previous revision: 1.6
done