profiles.cryptpassword has changed to 34 characters. logincookies.cryptpassword should do the same since they both store the same kinds of values.

This should take 5 minutes, I'll see what I can do.

correcting version field lost in product move

After speaking to justdave on IRC, lets just remove this field, and clear out logincookies whenever the password changes, for any reason (token, editusers, etc) checksetup will have to remove these invalid cookies when updating, of course

It turns out that both values are the same, from at least the crypt password checkin, according to bonsai. I only noticed this after changing everything else, though, so here you go. Theres nothing too complicated - someone testing the conversion code on more than my one installation would be helpful; I can't seem to get to landfill now.

OK, tested on landfill: mysql> use bugs; Reading table information for completion of table and column names You can turn off this feature to get a quicker startup with -A Database changed mysql> select count(*) from logincookies, profiles where logincookies.cryptpassword != profiles.cryptpassword and logincookies.userid=profiles.userid; +----------+ | count(*) | +----------+ | 60 | +----------+ 1 row in set (0.01 sec) mysql> select count(*) from logincookies; +----------+ | count(*) | +----------+ | 167 | +----------+ 1 row in set (0.00 sec) mysql> use bugs_bbaetz; Reading table information for completion of table and column names You can turn off this feature to get a quicker startup with -A Database changed mysql> select count(*) from logincookies; +----------+ | count(*) | +----------+ | 107 | +----------+ 1 row in set (0.00 sec) 167-60==107, so thats correct.

Oh, and this is on http://landfill.tequilarista.org/bbaetz/ ITs a schema change to, so if you test this on your own installation remember to use a different db.

Comment on attachment 65406 [details] [diff] [review] patch >Index: globals.pl ... >+sub invalidateLogins { Nit: These kinds of functions in globals.pl tend to have a capitalized first character. >Index: relogin.cgi ... >+if ($::userid) { >+ SendSQL("DELETE FROM logincookies WHERE cookie = " . >+ SqlQuote($::COOKIE{"Bugzilla_logincookie"}) . >+ "AND userid = $::userid"); Isn't "AND userid = $::userid" unnecessary here since the login cookie is guaranteed to belong to the current user? Haven't tested it yet. Still getting TT 2.06 set up on my b.m.o. test installation.

Tested it, and it works. Waiting for response from bbaetz regarding potentially unnecessary code in relogin.cgi before granting review.

Call it a sanity check. Its certainly correct, and since we don't lock tables arround this its technically possible for two of these to run in parallel, with the login cookie being reused for someone else after the first delete. (OK, getting that to actually happen in real life is probably impossible, and I'd have a beter chance of winning the lottery 100 times in a row. Still, it doesn't hurt) Do you want me to change the sub's name?

>Call it a sanity check. Its certainly correct, and since we don't lock tables >arround this its technically possible for two of these to run in parallel, with >the login cookie being reused for someone else after the first delete. (OK, >getting that to actually happen in real life is probably impossible, and I'd >have a beter chance of winning the lottery 100 times in a row. Still, it >doesn't hurt) Ok, I'll buy it, but add a comment with this explanation so future generations can buy it too. >Do you want me to change the sub's name? Yes.

OK, this takes myk's changes into account

Comment on attachment 67570 [details] [diff] [review] v2 looks good, works. r=myk

Comment on attachment 67570 [details] [diff] [review] v2 >+sub InvalidateLogins { >+ my ($userid, $keep) = @_; >+ >+ my $remove = "DELETE FROM logincookies WHERE userid = $userid"; >+ if (defined $keep) { >+ $remove .= " AND cookie != " . SqlQuote($keep); >+ } >+ SendSQL($remove); >+} >+ [...] >+if ($::userid) { >+ # Even though we know the userid must match, we still check it in the >+ # SQL as a sanity check, since there is no locking here, and if >+ # the user logged out from two machines simulataniously, while someone >+ # else logged in and got the same cookie, we could be logging the >+ # other user out here. Yes, this is very very very unlikely, but why >+ # take chances? - bbaetz >+ SendSQL("DELETE FROM logincookies WHERE cookie = " . >+ SqlQuote($::COOKIE{"Bugzilla_logincookie"}) . >+ "AND userid = $::userid"); >+} Just one question: why not wrap this up into InvalidateLogins as well? It would be the opposite of keep, and would make the code more SQL-free. If you feel I'm polluting the API, ignore this, and r=kiko.

Because I just want to remove one cookie here, and its the only place which requires this.

Comment on attachment 67570 [details] [diff] [review] v2 makring r=kiko - see previous comment

Comment on attachment 67570 [details] [diff] [review] v2 makring r=kiko - see previous comment

...and checked in (sorry for the double post before)