328108 - Unable to login on a fresh Bugzilla install using the login form on the home page

Status: VERIFIED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Vlad Dascalu]

Regression from bug 325079:

The security patch modified the login form to use 'urlbase' for the login form.
The checksetup.pl message, right after performing a clean install, says:
[e-mail] is now set up as an administrator account.
Now that you have installed Bugzilla, you should visit the
'Parameters' page (linked in the footer of the Administrator
account) to ensure it is set up as you wish - this includes
setting the 'urlbase' option to the correct url.

So I try to login with the admin account in order to set urlbase, but I'm redirected to http://you-havent-visited-editparams.cgi-yet/

Comment 1

[Vlad Dascalu]

Bug 325079 has several patches, some of them marked as obsolete. Nevertheless, they contain good ideas; probably we should select one of them as an alternative to using urlbase so early, in a pre-login stage.

This impacts the 2.20 branch, so probably we'll want to release 2.20 again after fixing this one.

Comment 2

[Vlad Dascalu]

LpSolit noted that you can still login via the "Log In" link, but most people will probably try to login directly from the homepage, see that it's broken, and give up.

Comment 3

[victory <[e-mail]>]

i met this, too.
not-override-upgrade though.
i went editparam.cgi and it worked.

Comment 4

[Frédéric Buclin]

The right fix is to have an empty urlbase by default. This way, you at least have relative URLs till urlbase is fixed and so would still work, even with the login form on the homepage (I haven't tested though, but I'm pretty sure this would work).

As it only affects the very first time you log in and that you can still either use the "Log In" link in the footer or go to editparams.cgi directly by entering it in the address bar, I don't think we need an emergency release. But I agree with vladd that we should release in the very near future, e.g. together with 2.22rc2/final.

Our QA tests couldn't detect that because we don't do clean installs every time we run a test.

Comment 5

[Frédéric Buclin]

Note that there is actually a check for urlbase which doesn't let you clear the field. So probably we should hack this check.

Comment 6

[Frédéric Buclin]

Attachment: patch for 2.22 and tip, v1

Rather than using an invalid URL by default, better is to use no URL at all.

Comment 7

[Frédéric Buclin]

Attachment: backport for 2.20.1, v1

Comment 8

[:glob ✱]

Comment on attachment 212664 [details] [diff] [review]
patch for 2.22 and tip, v1


>-    if ($url !~ m:^http.*/$:) {
>+    if ($url && $url !~ m:^http.*/$:) {

urlbase should be mandatory; it's used in the emails as well.

Comment 9

[Frédéric Buclin]

(In reply to comment #8)
> urlbase should be mandatory; it's used in the emails as well.
> 

I don't think random URLs are better than no URL at all. At least, this doesn't break URLs which use Param('urlbase'), including emails.

Comment 10

[:glob ✱]

> I don't think random URLs are better than no URL at all. At least, this doesn't
> break URLs which use Param('urlbase'), including emails.

oh, i agree that urlbase should be empty by default, however i feel that once you're editing the parameters leaving urlbase empty should throw an error.

note that fixing bug 263340 would also help :)

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> oh, i agree that urlbase should be empty by default, however i feel that once
> you're editing the parameters leaving urlbase empty should throw an error.

I thought about that too... but checking the "Reset" checkbox would throw this error because it would try to clear it.

Comment 12

[Frédéric Buclin]

Comment on attachment 212664 [details] [diff] [review]
patch for 2.22 and tip, v1

vladd isn't around today, requestion wicked too...

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> vladd isn't around today, requestion wicked too...

I created a new word: requestion! requesting + question = requestion. :-D

Comment 14

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

Comment on attachment 212664 [details] [diff] [review]
patch for 2.22 and tip, v1

This is adequate to fix the regression, but we should really ask the admin to provide a urlbase in checksetup.pl if it's required.  Letting them load a page with many settings but then not letting them submit any change unless they make a specific change is bad, frustrating UI.

Comment 15

[Frédéric Buclin]

tip:

Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/Config/Core.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Core.pm,v  <--  Core.pm
new revision: 1.3; previous revision: 1.2
done


2.22rc1:

Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.2.2.1; previous revision: 1.2
done
Checking in Bugzilla/Config/Core.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Core.pm,v  <--  Core.pm
new revision: 1.1.2.1; previous revision: 1.1
done


2.20.1:

Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/Attic/defparams.pl,v  <--  defparams.pl
new revision: 1.160.2.5; previous revision: 1.160.2.4
done

Comment 16

[Dave Miller [:justdave]]

What are we doing about emails?

The phoney URL we used on this originally was specifically because people ignore the directions, and it makes it obvious what they need to do to fix it.  Now they'll get emails with unusable URLs in them and have no clue what to do about it.

And you do realize that by committing this we've just reintroduced the security bug under the conditions that the admin hasn't set the param yet?

I haven't backed this out yet, but it should be strongly considered.

Comment 17

[Dave Miller [:justdave]]

What we really need to do instead of backing this out is just make the admin enter the urlbase during the checksetup.pl script.  I suppose that's another bug (we already have one on that, right?) but it'll be a priority now since without it we still have our security bug...

I really hate to have to add stuff to the answers hash for scripted setup on branches though...  stable branch means we don't change APIs, and that's a packager API...

Comment 18

[Dave Miller [:justdave]]

Ya know what, let's leave this.  We should be able to expect the admin to do setup properly before telling people they should log in.

Comment 19

[Frédéric Buclin]

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

Comment 20

[Max Kanat-Alexander]

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

Comment 21

[Max Kanat-Alexander]

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

Comment 22

[Frédéric Buclin]

Testopia testcase available at:
http://landfill.bugzilla.org/bugzillaqa/tr_show_case.cgi?case_id=29