297646 - Write helper functions for Bugzilla::Token.pm

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[:glob ✱]

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

Comment 1

[:glob ✱]

Attachment: v1

Comment 2

[Frédéric Buclin]

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.

Comment 3

[:glob ✱]

Attachment: v2

Comment 4

[:glob ✱]

Attachment: v3

sorry, previous version had a bug.

Comment 5

[Frédéric Buclin]

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.

Comment 6

[:glob ✱]

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.

Comment 7

[:glob ✱]

Attachment: v4

fixes nits, including sql_date_format fixes to Cancel and GetTokenData

Comment 8

[Frédéric Buclin]

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.

Comment 9

[:glob ✱]

Attachment: v5

trick_taint --> detaint_natural

Comment 10

[Frédéric Buclin]

Comment on attachment 186781 [details] [diff] [review]
v5

I did some tests. Works fine. r=LpSolit

Comment 11

[Frédéric Buclin]

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

Comment 12

[Dave Miller [:justdave]]

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

Comment 13

[Frédéric Buclin]

(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 ??

Comment 14

[Frédéric Buclin]

glob, you and me need this backport. ;)

Comment 15

[:glob ✱]

Attachment: 2.18 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 16

[Frédéric Buclin]

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 17

[:glob ✱]

Attachment: 2.18 backport v2

Comment 18

[Frédéric Buclin]

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 19

[:glob ✱]

Attachment: 2.18 backport v3

Comment 20

[Frédéric Buclin]

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

r=LpSolit

Comment 21

[:glob ✱]

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

Comment 22

[:glob ✱]

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