1170722 - Authentication Delegation should add an App ID column to associate api keys with specific callbacks

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Dylan Hardison [:dylan] (he/him)]

Add an app_id column for api keys. The column is nullable, and NULL indicates a manually created API key. Non-NULL should be the callback URI from authentication delegation.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1170722_1.patch

This ended up being far simpler than the earlier designs. Perhaps it's too simple?

Comment 2

[Frédéric Buclin]

Comment on attachment 8616247 [details] [diff] [review]
1170722_1.patch

>+++ b/Bugzilla/DB/Schema.pm

>+            app_id        => {TYPE => 'VARCHAR(64)'},

For consistency with other occurences in this file, write 'varchar' lowercase. Please also fix the other two occurences in the user_api_keys table.


>+            user_api_keys_user_id_app_id_idx  => {FIELDS => ['user_id', 'app_id'], TYPE => 'UNIQUE'},


>+++ b/Bugzilla/Install/DB.pm

>+    $dbh->bz_add_column('user_api_keys', 'app_id',
>+                        {TYPE => 'VARCHAR(64)'});

Same here: write 'varchar' lowercase.


>+    $dbh->bz_add_index('user_api_keys', "api_keys_user_id_app_id_idx",

This index name doesn't match the one defined above. It must start with user_.

Comment 3

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1170722_2.patch

Thanks for the review, Frédéric! I have not changed the VARCHAR in the Schema.pm, as the case used in that paragraph is VARCHAR (elsewhere in the file it is a mix of both.) We should make all the sql types consistent in the future, but not today.

As for the index name -- that was me failing at yanking the current word (yw).

dkl: fresh patch.

Comment 4

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #3)
> Thanks for the review, Frédéric! I have not changed the VARCHAR in the
> Schema.pm, as the case used in that paragraph is VARCHAR (elsewhere in the
> file it is a mix of both.) We should make all the sql types consistent in
> the future, but not today.

That's not true. Everywhere else, it's 'varchar' lowercase. The only place where it's VARCHAR uppercase is in the user_api_keys table because sgreen didn't follow the convention to write it lowercase in bug 726696, and dkl let it go through when reviewing it. It's silly to propagate inconsistency.

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 8616332 [details] [diff] [review]
1170722_2.patch

Review of attachment 8616332 [details] [diff] [review]:
-----------------------------------------------------------------

Index failure when using UI to revoke all keys from a given description and app_id as it tries to insert dupe values the next time you do the delegation.

DBD::mysql::db do failed: Duplicate entry '786-fc724ed1bec87fbb40e07110f9bf5c8669eed41d96d87bf237236262d3f3' for key 'user_api_keys_user_id_app_id_idx' [for Statement "INSERT INTO user_api_keys (app_id, user_id, description, api_key) VALUES (?,?,?,?)"] at Bugzilla/Object.pm line 724.
	Bugzilla::Object::insert_create_data('Bugzilla::User::APIKey', 'HASH(0x13afa18)') called at Bugzilla/Object.pm line 634
	Bugzilla::Object::create('Bugzilla::User::APIKey', 'HASH(0x33dc890)') called at /home/bugzilla/devel/htdocs/1170722/auth.cgi line 77

::: Bugzilla/DB/Schema.pm
@@ +1784,4 @@
>              revoked       => {TYPE => 'BOOLEAN', NOTNULL => 1,
>                                DEFAULT => 'FALSE'},
>              last_used     => {TYPE => 'DATETIME'},
> +            app_id        => {TYPE => 'VARCHAR(64)'},

s/VARCHAR/varchar/

Go ahead to and do the others while we are here such as api_keyand description. Will put us back to consistency.

::: auth.cgi
@@ +73,5 @@
> +    if (@$keys) {
> +        $api_key = $keys->[0];
> +    }
> +    else {
> +        my $new_key = Bugzilla::User::APIKey->create({

nit: slightly cleaner just to do do: 

$api_key = Bugzilla::User::APIKey->create({

then no need for 

$api_key = $new_key;

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1170722_3.patch

Fixed up. it's pretty weird that "varchar" is lower case and most of the other types are not. :-)

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 8617056 [details] [diff] [review]
1170722_3.patch

Review of attachment 8617056 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 8

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   7186b64..a8675e3  master -> master