350232 - Allow the Webservice interface to create User Accounts

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Max Kanat-Alexander]

Now that we have Bugzilla::User->create, it should be trivial to add a stable API for creating users.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here it is. I haven't tested it, but it compiles and the perldoc looks right in text format.

Comment 2

[Olav Vitters]

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.

Comment 3

[Max Kanat-Alexander]

(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

[Olav Vitters]

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

[Frédéric Buclin]

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).

Comment 6

[Max Kanat-Alexander]

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().

Comment 7

[Marc Schumann [:Wurblzap]]

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

[Marc Schumann [:Wurblzap]]

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.

Comment 9

[Max Kanat-Alexander]

Attachment: v2

Okay, I fixed all your comments, and I updated the error codes stuff.

Comment 10

[Marc Schumann [:Wurblzap]]

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.

Comment 11

[Max Kanat-Alexander]

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.

Comment 12

[Marc Schumann [:Wurblzap]]

(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

[Frédéric Buclin]

ok, for 2.23.3

Comment 14

[Max Kanat-Alexander]

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

Comment 16

[Frédéric Buclin]

test scripts: webservice_user_create.t and webservice_user_offer_account_by_email.t