User.login should ThrowUserError when called with incorrect parameters

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
WebService
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

unspecified
Bugzilla 3.0
Bug Flags:
approval +
approval3.2 +
approval3.0 +

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 330175 [details] [diff] [review]
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
Attachment #330175 - Flags: review?(mkanat)

Comment 2

10 years ago
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.
Attachment #330175 - Flags: review?(mkanat) → review-
(Assignee)

Comment 3

10 years ago
(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
(Assignee)

Comment 4

10 years ago
Created attachment 330266 [details] [diff] [review]
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
Assignee: webservice → dkl
Attachment #330175 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #330266 - Flags: review?(mkanat)

Comment 5

10 years ago
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.
Attachment #330266 - Flags: review?(mkanat) → review-
(Assignee)

Comment 6

10 years ago
I will submit a new patch that just checks for defined variables and
not worry about their values.
(Assignee)

Comment 7

10 years ago
Created attachment 330411 [details] [diff] [review]
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
Attachment #330266 - Attachment is obsolete: true
Attachment #330411 - Flags: review?(mkanat)

Comment 8

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

Great.
Attachment #330411 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval3.2+
Flags: approval+
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 4.0
(Assignee)

Comment 9

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 10

10 years ago
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.
Target Milestone: Bugzilla 4.0 → Bugzilla 3.2
(Assignee)

Comment 11

10 years ago
Created attachment 331185 [details] [diff] [review]
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
Attachment #331185 - Flags: review?(LpSolit)
(Assignee)

Comment 12

10 years ago
Created attachment 331188 [details] [diff] [review]
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
Attachment #330411 - Attachment is obsolete: true
Attachment #331188 - Flags: review+
(Assignee)

Comment 13

10 years ago
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
(Assignee)

Updated

10 years ago
Attachment #331185 - Flags: review?(LpSolit) → review?(mkanat)

Updated

10 years ago
Attachment #331185 - Flags: review?(mkanat) → review+

Updated

10 years ago
Flags: approval3.0+
(Assignee)

Updated

10 years ago
Target Milestone: Bugzilla 3.2 → Bugzilla 3.0
(Assignee)

Comment 14

10 years ago
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

Updated

6 years ago
Duplicate of this bug: 409667
You need to log in before you can comment on or make changes to this bug.