Bugzilla::Token needs some helper functions. IssueSessionToken, GetToken and DeleteToken

Comment on attachment 186197 [details] [diff] [review] v1 >+ my $token_ts = time(); >+ my $issuedate = time2str("%Y-%m-%d %H:%M", $token_ts); I much prefer using NOW() in the INSERT statement than calling time() and time2str() here. I know this is used in IssueEmailChangeToken() but I even don't understand why. So unless we have any good reason to do so, we could simplify the code a bit by omitting time(). Moreover, why do you limit $token_ts to the minute? What if we need seconds somewhere in the code? Using NOW() is a better solution IMO. >+ my $token = GenerateUniqueToken(); >+ >+ # Generate a unique token and insert it into the tokens table. >+ # We have to lock the tokens table before generating the token, >+ # since the database must be queried for token uniqueness. >+ my $dbh = Bugzilla->dbh; >+ $dbh->bz_lock_tables('tokens WRITE'); >+ $dbh->do("INSERT INTO tokens (userid, issuedate, token, tokentype, eventdata) >+ VALUES (?, ?, ?, ?, ?)", undef, ($userid, $issuedate, $token, 'session', $data)); >+ $dbh->bz_unlock_tables(); Such INSERT statements appear several times in Token.pm. I wonder if a private _insert_token($tokentype, $eventdata) routine would not be a better idea than duplicating this code in all routines. If we don't fix it in this patch, I would appreciate to see a bug opened to do this cleanup.

sorry, previous version had a bug.

Comment on attachment 186319 [details] [diff] [review] v3 > sub IssueEmailChangeToken { [...] >+ my $token = _create_token($userid, 'emailold', $old_email . ":" . $new_email); >+ my (undef, $token_ts, undef) = GetTokenData($token); Among the 5 fields the 'tokens' table contains, 3 are provided by the user. The remaining two fields, $token and $token_ts, should be returned by _create_token() itself IMO. As long as we need to check that a token is not too old, we will require this information. So why not returning it automatically when creating a new token? I'm pretty sure you need time information to fix 38862. >+ my $token = _create_token($userid, 'password', $::ENV{'REMOTE_ADDR'}); >+ my (undef, $token_ts, undef) = GetTokenData($token); See, you need it here too. ;) >+sub IssueSessionToken { >+ # Generates a random token, adds it to the tokens table, and returns >+ # the token to the caller. >+ >+ return _create_token(Bugzilla->user->id, 'session', shift); >+} Nit: Could you add a line with my $data = shift ? It would make the code easier to read. Here, we have no idea what 'shift' contains. > $token = &::GenerateRandomPassword(); >- &::SendSQL("SELECT userid FROM tokens WHERE token = " . &::SqlQuote($token)); >- $duplicate = &::FetchSQLData(); >+ trick_taint($token); Question: do we really need to detaint $token here?? I don't think so. Note that it would make sense to move GenerateRandomPassword() into Token.pm (User.pm uses it only once). But that's another bug. >+ return $dbh->selectrow_array("SELECT userid, issuedate, eventdata FROM tokens WHERE token = ?", >+ undef, $token); I'm pretty sure you have to write $dbh->sql_date_format('issuedate'). Moreover, this guaranties that a string is returned, see IssueEmailChangeToken(). >+sub _create_token($$$;$) { What is the 4th (optional) parameter used for??? I cannot find it in this routine. >+ my $token = GenerateUniqueToken(); >+ >+ trick_taint($userid); >+ trick_taint($token); Same comment as above: I don't think you need to detaint $token. >+ $dbh->bz_lock_tables('tokens WRITE'); You have to lock tables *before* you call GenerateUniqueToken(), to guarantee uniqueness. As ThrowCodeError() automatically unlocks tables if being called, you even don't need to lock additional tables.

thanks frédéric, sorted out all the review points except for.. > >+ return $dbh->selectrow_array("SELECT userid, issuedate, eventdata FROM tokens WHERE token = ?", > >+ undef, $token); > > I'm pretty sure you have to write $dbh->sql_date_format('issuedate'). > Moreover, this guaranties that a string is returned, see > IssueEmailChangeToken(). the current code doesn't use sql_date_format also i need the time as unix time, which doesn't appear to be a format that sql_date_format supports. i'm sure that str2time can cope with whatever format a database returns by default.

fixes nits, including sql_date_format fixes to Cancel and GetTokenData

Comment on attachment 186457 [details] [diff] [review] v4 >+sub _create_token($$$) { >+ # Generates a unique token and inserts it into the database >+ # Returns the token and the token timestamp >+ my ($userid, $tokentype, $eventdata) = @_; >+ >+ trick_taint($userid); $userid is numeric. You have to use detaint_natural($userid). >+ if (wantarray) { >+ my (undef, $token_ts, undef) = GetTokenData($token); >+ $token_ts = str2time($token_ts); >+ return ($token, $token_ts); >+ } else { >+ return $token; >+ } What's that??? First, wantarray should be $wantarray, then... where does it come from?? I did not test your patch yet, but except for these two things here, it looks very good.

trick_taint --> detaint_natural

Comment on attachment 186781 [details] [diff] [review] v5 I did some tests. Works fine. r=LpSolit

glob, this patch would be very useful for 2.18 too. Could you backport it?

what's it useful for in 2.18? (is there a security/stability fix it needs that would require these?)

(In reply to comment #12) > what's it useful for in 2.18? (is there a security/stability fix it needs that > would require these?) justdave, look at bugs being blocked by this one ;) Unless you don't want them for 2.18 ??

glob, you and me need this backport. ;)

here's the 2.18 backport. it's a simple port of the new Token.pm; i haven't updated any existing code to make use of the helper functions.

Comment on attachment 187087 [details] [diff] [review] 2.18 backport >+ return $dbh->selectrow_array( >+ "SELECT userid, " . $dbh->sql_date_format('issuedate') . ", eventdata >+ FROM tokens >+ WHERE token = ?", undef, $token); $dbh->sql_date_format does not exist in 2.18. >+ $dbh->bz_lock_tables('tokens WRITE'); >+ $dbh->do("DELETE FROM tokens WHERE token = ?", undef, $token); >+ $dbh->bz_unlock_tables(); $dbh->bz_lock_tables and $dbh->bz_unlock_tables do not exist in 2.18.

Comment on attachment 187375 [details] [diff] [review] 2.18 backport v2 >+sub DeleteToken($) { [...] >+ my $dbh = Bugzilla->dbh; >+ &::SendSQL("LOCK TABLES tokens WRITE"); >+ $dbh->do("DELETE FROM tokens WHERE token = ?", undef, $token); >+ &::SendSQL("UNLOCK TABLES"); >+} Please write $dbh->do("LOCK TABLES tokens WRITE") and $dbh->do("UNLOCK TABLES"). SendSQL() is deprecated. >+sub _create_token($$$) { [...] >+ my $dbh = Bugzilla->dbh; >+ &::SendSQL("LOCK TABLES tokens WRITE"); [...] >+ &::SendSQL("UNLOCK TABLES"); Same here. Moreover, you forgot to convert old code to _create_token() in IssuePasswordToken(). Else the patch works fine.

Comment on attachment 187616 [details] [diff] [review] 2.18 backport v3 r=LpSolit

HEAD: Checking in Bugzilla/Token.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v <-- Token.pm new revision: 1.30; previous revision: 1.29 done

2.18: Checking in Bugzilla/Token.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Token.pm,v <-- Token.pm new revision: 1.22.2.4; previous revision: 1.22.2.3 done