445885 - User.login should ThrowUserError when called with incorrect parameters

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[David Lawrence [:dkl]]

Currently when User.login() is called without parameters a userid of 0 is returned. Instead User.login() should throw an error stating the parameters that are required.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to throw error when proper params are not passed to User.login (v1)

Patch adding ThrowUserError to User.login that is thrown with either username or password is missing from the params.

Please review.
Dave

Comment 2

[Max Kanat-Alexander]

Comment on attachment 330175 [details] [diff] [review]
Patch to throw error when proper params are not passed to User.login (v1)

Hmm, we should be more consistent and throw the normal param_missing (or whatever it's called) error that we throw from other functions.

Comment 3

[David Lawrence [:dkl]]

(In reply to comment #2)
> (From update of attachment 330175 [details] [diff] [review])
> Hmm, we should be more consistent and throw the normal param_missing (or
> whatever it's called) error that we throw from other functions.

I thought about that as well first, but then realized that if someone did not pass either username and password, the param_missing error only handles a single param at a time. 

I could alter the param_missing error to accept multiple params as a list and then update the other ThrowUserError instances to use the new structure. Sound okay?

Dave

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to throw error when proper params are not passed to User.login (v2)

New patch that reuses the 'param_required' error. It will fail when either login or password are missing or empty.

Please review.
Dave

Comment 5

[Max Kanat-Alexander]

Comment on attachment 330266 [details] [diff] [review]
Patch to throw error when proper params are not passed to User.login (v2)

>+        defined $params->{$param} && $params->{$param}

  Somebody could theoretically have a username of "0".

  Otherwise this looks fine.

Comment 6

[David Lawrence [:dkl]]

I will submit a new patch that just checks for defined variables and
not worry about their values.

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch to throw error when proper params are not passed to User.login (v3)

New patch. In this one I am just checking for defined params and not whether they have actual values.

Please review
Dave

Comment 8

[Max Kanat-Alexander]

Comment on attachment 330411 [details] [diff] [review]
Patch to throw error when proper params are not passed to User.login (v3)

Great.

Comment 9

[David Lawrence [:dkl]]

tip:
Checking in Bugzilla/WebService/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/User.pm,v  <--  User.pm
new revision: 1.11; previous revision: 1.10
done

3.2:
Checking in Bugzilla/WebService/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/User.pm,v  <--  User.pm
new revision: 1.6.2.1; previous revision: 1.6
done

Comment 10

[Frédéric Buclin]

Note that the patch v3 (which wouldn't pass tests anyway) is not the one which has been checked in (fortunately). First, ThrowCodeError() could never be reached, and secondly it should have been ThrowUserError(). But the patch which has been checked in is fine, so all is fine. :) Fixing the TM as the patch also landed on 3.2.

Comment 11

[David Lawrence [:dkl]]

Attachment: Patch to throw error when proper params are not passed to User.login for 3.0.5 (v1)

Patch for Bugzilla 3.0.5. Needed to add line to Bugzilla/WebService/Constants.pm for param_required => 50.

Please review
Dave

Comment 12

[David Lawrence [:dkl]]

Attachment: Patch to throw error when proper params are not passed to User.login (v4)

Apologize that the actual code checked in was different that the last patch that was reviewed. I did fix the error before checking in so the tree was not broken. I am attaching the actual patch used here.

Dave

Comment 13

[David Lawrence [:dkl]]

Oops, backported the wrong patch to 3.0.5. Changing the review request to mkanat. Feel free to deny this request if you do not feel we need this for 3.0.5.

Dave

Comment 14

[David Lawrence [:dkl]]

3.0.5:
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.6.2.5; previous revision: 1.6.2.4
done
Checking in Bugzilla/WebService/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/User.pm,v  <--  User.pm
new revision: 1.4.2.2; previous revision: 1.4.2.1
done