1001462 - Bug.search causes error when using simple token auth and specifying 'token' instead of 'Bugzilla_token'

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

XMLRPC and JSONRPC allow users to pass 'token' instead of 'Bugzilla_token' as a short-cut to webservice calls. But the shorter 'token' causes Bug.search to generate an error as it considers 'token' to be an invalid param since it uses Bugzilla::Bug->match. Trunk does not have this issue as it uses Bugzilla::Search and 'token' is ignored.

For 4.4, we need to update Bugzilla/Auth/Login/Cookie.pm to use 'token' if present and then delete it from the $params before Bug.search is executed. This will solve the error. As a workaround users can use 'Bugzilla_token' as it is properly removed when it is read.

dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: 1001462_1.patch

Applies to both trunk and 4.4.

dkl

Comment 2

[David Lawrence [:dkl]]

Attachment: Patch for 4.4 v1

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch for trunk v1

Comment 4

[:glob ✱]

Comment on attachment 8418957 [details] [diff] [review]
Patch for 4.4 v1

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

r=glob

Comment 5

[:glob ✱]

Comment on attachment 8418958 [details] [diff] [review]
Patch for trunk v1

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

with this patch, trunk throws the follow error when calling User.login:
> The function requires a login argument, and that argument was not set.

i'm providing the login argument, and the same call works without the patch:

Comment 6

[David Lawrence [:dkl]]

Attachment: Patch for trunk v2

weird :) this one should work much better for XMLRPC.

Comment 7

[:glob ✱]

Comment on attachment 8420208 [details] [diff] [review]
Patch for trunk v2

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

r=glob

Comment 8

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   8243604..70e8ab7  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   7f74756..2234f68  4.4 -> 4.4

Comment 10

[Frédéric Buclin]

Comment on attachment 8418957 [details] [diff] [review]
Patch for 4.4 v1

>=== modified file 'Bugzilla/WebService/User.pm'

>     foreach my $param ("login", "password") {
>-        defined $params->{$param} 
>+        (!defined $params->{$param} && !defined $params->{'Bugzilla_' . $param})
>             || ThrowCodeError('param_required', { param => $param });

This code is wrong. It must be:

  defined $params->{$param} || defined $params->{'Bugzilla_' . $param}
     || ThrowCodeError('param_required', { param => $param });


This makes several QA tests to fail.

Comment 11

[Frédéric Buclin]

Comment on attachment 8420208 [details] [diff] [review]
Patch for trunk v2

Same problem here.

Comment 12

[Frédéric Buclin]

The fix I suggested in comment 10 is not enough. Now QA tests throw:

#   Failed test 'JSON-RPC: User.login returned successfully'
#   at lib/QA/RPC.pm line 84.
# The function requires a login argument, and that argument was not set.
Can't use an undefined value as a HASH reference at lib/QA/RPC.pm line 50.


I think the patch should be backed out as there is something else which is still broken.

Comment 13

[David Lawrence [:dkl]]

Backed out:

To ssh://[e-mail]/bugzilla/bugzilla.git
   d694e68..52b113c  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   1d6e06b..ad99e9d  4.4 -> 4.4

Comment 16

[David Lawrence [:dkl]]

Attachment: Trunk 1001462_3.patch

Comment 17

[David Lawrence [:dkl]]

Attachment: 4.4 1001462_2.patch

Comment 18

[:glob ✱]

Comment on attachment 8516462 [details] [diff] [review]
4.4 1001462_2.patch

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

#   Failed test 'Bugzilla/WebService/User.pm has 1 error(s):
# user error tag 'invalid_login_or_password' is used at line(s) (74) but not defined for language(s): any'
#   at t/012throwables.t line 199.

::: Bugzilla/WebService/User.pm
@@ +70,5 @@
> +    if ($user->id) {
> +        return $self->_login_to_hash($user);
> +    }
> +    else {
> +        ThrowUserError('invalid_login_or_password');

this ThrowUserError is never reached -- Bugzilla->login() throws an error if the user/pass is invalid, and the param_required checks ensures we can't get an anon user back from ->login.

@@ +75,1 @@
>      }

after more investigate, most of this code isn't actually reached.

Bugzilla::WebService::Server::XMLRPC::handle_login handles all the authentication requests where login/password parameters are provided, including calls to User.login (!).

the only time we get past the "already logged in" check is if there are missing parameters.

Comment 19

[David Lawrence [:dkl]]

Attachment: 4.4 1001462_3.patch

Comment 20

[David Lawrence [:dkl]]

Attachment: Trunk 1001462_4.patch

Comment 21

[:glob ✱]

Comment on attachment 8520025 [details] [diff] [review]
Trunk 1001462_4.patch

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

r=glob

Comment 22

[:glob ✱]

Comment on attachment 8520019 [details] [diff] [review]
4.4 1001462_3.patch

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

r=glob

Comment 23

[David Lawrence [:dkl]]

To ssh://[e-mail]/bugzilla/bugzilla.git
   ded5d29..c0156f2  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   4b4ddaf..0b19b10  5.0 -> 5.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   16fa3bf..cbc11fd  4.4 -> 4.4