Closed
Bug 350232
Opened 18 years ago
Closed 18 years ago
Allow the Webservice interface to create User Accounts
Categories
(Bugzilla :: WebService, enhancement)
Tracking
()
RESOLVED
FIXED
Bugzilla 3.0
People
(Reporter: mkanat, Assigned: mkanat)
References
Details
Attachments
(1 file, 1 obsolete file)
5.46 KB,
patch
|
Wurblzap
:
review+
|
Details | Diff | Splinter Review |
Now that we have Bugzilla::User->create, it should be trivial to add a stable API for creating users.
Assignee | ||
Comment 1•18 years ago
|
||
Okay, here it is. I haven't tested it, but it compiles and the perldoc looks right in text format.
Assignee: user-accounts → mkanat
Status: NEW → ASSIGNED
Attachment #235580 -
Flags: review?(wurblzap)
Comment 2•18 years ago
|
||
Create method creates a security problem IMO (anyone with some scripting knowledge could create an account and provide the password). I want offer_account_by_email to have the arguments that create has. And create should go.
Assignee | ||
Comment 3•18 years ago
|
||
(In reply to comment #2) > Create method creates a security problem IMO (anyone with some scripting > knowledge could create an account and provide the password). That's true now, also, basically. I don't see what the security problem is. The script would have to be more complicated, presently, but it's the same thing. Remember that User->create does all the validations necessary.
Comment 4•18 years ago
|
||
From the discussion from IRC: create should check for editusers; otherwise there never is any verification that the person adding the emailaddress+password is really the owner of that email address. Adding an (optional!) password option to offer_account_by_email requires backend changes and is for another bug.
Comment 5•18 years ago
|
||
Comment on attachment 235580 [details] [diff] [review] v1 >+sub create { >+ my $user = Bugzilla::User->create({ >+ login_name => $email, >+ realname => $realname, >+ password => $password >+ }); I see nowhere a place where the email is checked against 'createemailregexp'. This is a huge security hole IMO. Cancel my review if I missed something (and explain why I'm wrong).
Attachment #235580 -
Flags: review-
Assignee | ||
Comment 6•18 years ago
|
||
Comment on attachment 235580 [details] [diff] [review] v1 (In reply to comment #5) > I see nowhere a place where the email is checked against 'createemailregexp'. > This is a huge security hole IMO. > > Cancel my review if I missed something (and explain why I'm wrong). It's checked inside of create().
Attachment #235580 -
Flags: review-
Comment 7•18 years ago
|
||
I agree that create() must check for editusers so that we don't rip open again what Frédéric mended in bug 87795.
Comment 8•18 years ago
|
||
Comment on attachment 235580 [details] [diff] [review] v1 >Index: Bugzilla/WebService/User.pm >+sub offer_account_by_email { >+ my $self = shift; >+ my ($params) = @_; >+ my $email = trim($params->{email}) >+ || ThrowCodeError('param_required', { param => 'email' }); You'll need to use Bugzilla/Error.pm for ThrowCodeError() to work. >+ $email = Bugzilla::User::check_login_name_for_creation($email); You're calling a method, so the first param must be a classref. So offer_account_by_email doesn't get beyond this point. >+sub create { Per comment 7, this needs to check for editusers memberships first. >+ my $email = trim($params->{email}) >+ || ThrowCodeError('param_required', { param => 'email' }); Same as above. >+ my $user = Bugzilla::User->create({ >+ login_name => $email, >+ realname => $realname, >+ password => $password >+ }); Not bitrotten, but contextrotten (param names have changed in the meantime). >+__END__ Didn't check POD.
Attachment #235580 -
Flags: review?(wurblzap) → review-
Assignee | ||
Comment 9•18 years ago
|
||
Okay, I fixed all your comments, and I updated the error codes stuff.
Attachment #235580 -
Attachment is obsolete: true
Attachment #241224 -
Flags: review?(wurblzap)
Comment 10•18 years ago
|
||
Comment on attachment 241224 [details] [diff] [review] v2 Perhaps you can consider fixing these two nits on checkin? >+sub create { >+ my $self = shift; >+ my ($params) = @_; >+ >+ Bugzilla->user->in_group('editusers') Nit: maybe you'll want to place a Bugzilla->login(); before Bugzilla->user->in_group. >+ my $realname = trim($params->{full_name}); Nit: I don't think it's easily explained that this param is called full_name while Bugzilla's table has realname here, so I think the parameter should be realname as well.
Attachment #241224 -
Flags: review?(wurblzap) → review+
Assignee | ||
Comment 11•18 years ago
|
||
It actually happens quite frequently that the WebService parameters are different than what we call them in Bugzilla. As a WebService user, I picture somebody who knows nothing about the internal structure of Bugzilla. Ideally, people won't have to know much about the internal structure of Bugzilla anymore, thanks to this XML-RPC interface. As far as the Bugzilla->login, I think that should always be done by anybody calling the webservice interface, from xmlrpc.cgi, to enforce requirelogin.
Flags: approval?
Comment 12•18 years ago
|
||
(In reply to comment #11) > It actually happens quite frequently that the WebService parameters are > different than what we call them in Bugzilla. We don't have a lot of them yet, do we? Neither do we have a lot of users. This is the time to make the choice, I think, since we still *can* choose. So unless you have a very compelling reason not to, my nit asks to choose to make life easier for API maintainers (or at least as close to each other as password/cryptpassword). What I'm saying is overridden by any decision we make to move to some convention about field names -- if we decide to move to a convention to call table column profiles.realname by the variable full_name everywhere, then I'm happy again because I consider API maintainers to be happy as well. > As a WebService user, I picture somebody who knows nothing about the internal > structure of Bugzilla. Ideally, people won't have to know much about the > internal structure of Bugzilla anymore, thanks to this XML-RPC interface. True. But maintainers need to wrap their minds about both. > As far as the Bugzilla->login, I think that should always be done by anybody > calling the webservice interface, from xmlrpc.cgi, to enforce requirelogin. That's not a good idea because this would make anonymous get_bug requests impossible. The login nit here is not too bad because you don't get any further because the in_group check catches that. I brought up the nit (and it really is just a nit in this place here, no security doubts or anything) because the error message might potentially be a little confusing to some application programmer who thinks his program has logged in but hasn't.
Comment 13•18 years ago
|
||
ok, for 2.23.3
Updated•18 years ago
|
Flags: approval? → approval+
Assignee | ||
Comment 14•18 years ago
|
||
I kept the patch as it is, for now. Checking in Bugzilla/WebService/Constants.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v <-- Constants.pm new revision: 1.3; previous revision: 1.2 done Checking in Bugzilla/WebService/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/User.pm,v <-- User.pm new revision: 1.2; previous revision: 1.1 done
Assignee | ||
Updated•18 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Component: User Accounts → WebService
Comment 16•16 years ago
|
||
test scripts: webservice_user_create.t and webservice_user_offer_account_by_email.t
Flags: testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•