150153 - ConnectToDatabase/quietly_check_login issues.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P1)

Description

[Matthew Tuck [:CodeMachine]]

I did an audit of CGIs with regard to ConnectToDatabase and quietly_check_login.
 What I found was not good, and needs to be fixed for rc3/final.

confirm_login is ok on it's own because it calls ConnectToDatabase itself.

No ConnectToDatabase before quietly_check_login
-----------------------------------------------

Results in the CGI dying in certain situations.

xml.cgi: No CTD.

colchange.cgi: CTD is done *after* qcl.

No quietly_check_login
----------------------

Results in a logged out footer.

quips.cgi

token.cgi: Possibly not a bug, are there any situations in which it should?
createaccount.cgi: Probably not a bug, given this shouldn't occur when you're
logged in anyway.

Stuff between ConnectToDatabase & quietly_check_login
-----------------------------------------------------

Results in a logged out footer in some situations.

buglist.cgi: Definitely a bug, try buglist.cgi?format=nonexistantformat.

describecomponents.cgi, duplicates.cgi, sidebar.cgi: Call GetVersionTable in
between, maybe not a bug at the moment, but could become one later if
GetVersionTable calls Throw*Error or the like.

queryhelp.cgi: Similar to above, called SendSQL, probably not a bug at the moment.

Comment 1

[Matthew Tuck [:CodeMachine]]

Also, the template initialisation code in globals.pl can fail, and throw a
template error.  This needs to qcl too.

Comment 2

[Bradley Baetz (:bbaetz)]

OK, I looked at these independantly, and seem to have come up with the same list
as mattyt. (I actually have one more; move.pl had confirm_login a bit too late
for one error message)

Re comment #1, we don't actually need to call quietly_check_login() in that
case. What we need to do is make our code which is called when we can't create a
template object not try to use templates. ThrowTemplateError is closer to what
we want, but theres no 'second error' here. File a new bug on this issue, please.

Patch coming soonish, which should be applied to 2.16 + trunk. I'm helping to
'test' this in conjunction to a second patch, which I don't know if we want on
the branch. The second patch removes ConnectToDatabase calls from everywhere
else in the code.

I've mainly moved code, and have also moved GetVersionTable calls below login
ones when appropriate, just for when that code starts having errors handled by
templates.

Comment 3

[Bradley Baetz (:bbaetz)]

Attachment: v1, pt1

OK, try this. I've done basic testing, but others should look and test, too.

Comment 4

[Bradley Baetz (:bbaetz)]

Attachment: pt2, v1

I'm not sure if this part should go into 2.16. Its unlikly this would break
anything, but....

Comment 5

[Jouni Heikniemi]

Comment on attachment 86929 [details] [diff] [review]
v1, pt1

Nit: You're not being consistent with whether or not there is an empty line
between CTD and qcl/c_l calls. Please fix this either way. I prefer CTD &
qcl/c_l being close together and separated from the rest of the code by empty
lines, but I'm just as fine with something else too. r=jouni otherwise :-)

I think we should push part 2 to be a separate 2.18 bug, because it's somewhat
different and potentially more risky. I'm ready to get part one into 2.16, but
I'm not certain about part 2. It would need more testing time, and I don't
think we're going to have it before release.

Comment 6

[Bradley Baetz (:bbaetz)]

Attachment: pt1, v2

OK, whitespace nits fixed.

You can r= the second part, even if we're not taking it for 2.16.

Comment 7

[Jouni Heikniemi]

>You can r= the second part, even if we're not taking it for 2.16.

I had a quick look at it but didn't have the time to check it out thoroughly
then. The same applies now: it still looks basically good but I don't think I'll
have the time to test it properly until Monday. If someone else gets there
before me, I'm grateful :-)

Comment 8

[Jouni Heikniemi]

Comment on attachment 87766 [details] [diff] [review]
pt1, v2

r=jouni

Comment 9

[Jouni Heikniemi]

Comment on attachment 86930 [details] [diff] [review]
pt2, v1

r=jouni, seems to work and code is clean. But still hesitant about the branch
checkin. :-)

Comment 10

[Bradley Baetz (:bbaetz)]

Yeah, I won't check patch 2 in on the branch.

Comment 11

[Bradley Baetz (:bbaetz)]

The first patch has been checked into the trunk and the branch; moving to 2.18
for the second patch, which still needs another review.

Comment 12

[Jouni Heikniemi]

Comment on attachment 86930 [details] [diff] [review]
pt2, v1

Feh. Get this 2xr=jouni and shove it in. We'll see if it breaks, then. :-)

Comment 13

[Bradley Baetz (:bbaetz)]

Part 2 checked into trunk.