Closed Bug 412725 Opened 16 years ago Closed 16 years ago

WebService functions to get information about a User (User.get)

Categories

(Bugzilla :: WebService, enhancement, P1)

3.1.2
enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 3.4

People

(Reporter: nelhawar, Assigned: nelhawar)

References

Details

(Whiteboard: [wanted-bmo])

Attachments

(2 files, 18 obsolete files)

8.05 KB, patch
mkanat
: review+
Details | Diff | Splinter Review
8.06 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070417 Fedora/2.0.0.3-4.fc7 Firefox/2.0.0.3
Build Identifier: 

In our customized xmlrpc interface at redhat we have 2 functions in the xmlrpc module User.pm, one to convert userid to login name and another one to do the opposite , convert from login name to userid. I rewrote those functions to work with the upstream code. attached is a patch for the WebService module User.pm that has those functions , no detailed documentation is added yet for them. Please review and let me know what you think.

Thanks,
Noura 

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Instead there should be a function that returns all of a User's information, in a hash. It should be User.get. It should take either an array of ids or an array of logins, and return a hash with a single item pointing to an array of hashes.
Attachment #297485 - Attachment is patch: true
Happy to see you didn't forget to check user privs (due to requirelogin and usevisibilitygroup). ;)
(In reply to comment #2)
> Instead there should be a function that returns all of a User's information, in
> a hash. It should be User.get. It should take either an array of ids or an
> array of logins, and return a hash with a single item pointing to an array of
> hashes.

That sounds like a good idea in my opinion is we should keep the amount of methods minimal and just make them very flexible. We are working on cutting down the number of methods we have now in our API while we port over to 3.x (i.e. Bug.update()).

(In reply to comment #4)
> That sounds like a good idea. In my opinion is we should keep the amount of
> methods minimal and just make them very flexible.

  Yes, agreed completely.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Summary: WebService functions to convert userid to login name and the other way around → WebService functions to get information about a User.
Target Milestone: --- → Bugzilla 4.0
Version: unspecified → 3.1.2
OK attached is a new patch that has function WebService::User::get() that can either take a list of user ids or list of login names and returns the user details for them. I wasn't sure how deep in user details we should return . Please review and let me know what you think and if more details need to be returned.

Thanks,
Noura
Attachment #297485 - Attachment is obsolete: true
Comment on attachment 297685 [details] [diff] [review]
patch for WebService module User.pm contains get() function

>+    Bugzilla->user->in_group('editusers')
>+        || ThrowUserError("auth_failure", { group  => "editusers",
>+                                            action => "use",
>+                                            object => "users"});

  Users who are not in editusers should still be able to get the login name, id, and realname of a user. HOWEVER, if they only specified the user's id, they should not be able to get that user's information at all if they cannot see the user (according to $user->can_see_user).

>+    if ( $params->{ids} ){
>+        @user_objs = map new Bugzilla::User($_), @{ $params->{ids} };

  You can use Bugzilla::User->new_from_list there.

>+    else {
>+        my @ids = map Bugzilla::User::login_to_id($_), @{ $params->{logins} };       

  You can use Bugzilla::User->new({ name => $login });
 
  You should allow the user to specify both ids and logins at the same time.

>+    my @users = 
>+        map {{
>+            id => type('int')->value($_->id),
>+            realname => type('string')->value($_->name),

  Make that real_name. We try to be more consistent in the WebService interface than we are in our internal interface.

>+            login => type('string')->value($_->login),

  For future compatibilty, also return "email =>" as a separate item.

>+            disabledtext => type('string')->value($_->disabledtext),
>+            disable_mail => type('int')->value($_->{disable_mail}),

  That's a boolean, not an int.

>+            groups => $_->groups,

  Let's remove "groups" until we decide on the exact format it should have.

>+            bless_groups => $_->bless_groups,

  Same here.


  And this will of course need POD.
Attachment #297685 - Flags: review-
(In reply to comment #7)
> (From update of attachment 297685 [details] [diff] [review])
> >+    Bugzilla->user->in_group('editusers')
> >+        || ThrowUserError("auth_failure", { group  => "editusers",
> >+                                            action => "use",
> >+                                            object => "users"});
> 
>   Users who are not in editusers should still be able to get the login name,
> id, and realname of a user. HOWEVER, if they only specified the user's id, they
> should not be able to get that user's information at all if they cannot see the
> user (according to $user->can_see_user).
>

so basically the design for the above part would be :

if the user is member of editusers and can see otheruser
   then return full otheruser info
else if the user can see otheruser but not a member of editusers 
   then return only login, id , and realname
else
   return error that otheruser is not visible to the user 

 


That sounds exactly right, with one exception:

The visibility check only applies to users specified in "ids =>". Users specified in "login =>" don't need to undergo a visibility check, because if you already know that they exist, it's not a problem. (It's just too easy to enumerate user ids and get a login name out of that.)
Assignee: webservice → nelhawar
Status: NEW → ASSIGNED
Target Milestone: Bugzilla 4.0 → Bugzilla 3.2
I modified the patch according to the review. One thing I was thinking about is that if the user passes in a list of user ids , few of them he can view and few he can not ,, with the ones that he can not view shall we return them in a list pointed to by hash key called for example "non_visible => [ ]",, currently my patch just ignores the ones that are not visible to the user and only returns info for the visible ones. also I haven't done the POD , I will do that when no more modifications will be required for the function.

Thanks,
Noura
ah sorry I forgot to fix the data type of disable_mail from int to boolean in the previous patch ,, resubmitted the patch with the fix
Hey Noura. Do read the contributor guidelines at:

  http://wiki.mozilla.org/Bugzilla:Developers

In particular, make sure to obsolete your old patches, and to request review from the right person.
Comment on attachment 297709 [details] [diff] [review]
same as prev patch just with little modification

>Index: Bugzilla/WebService/User.pm
>+#    $call = $rpc->call( 'User.get', { ids => [1,2,3], logins => ['testusera@redhat.com', 'testuserb@redhat.com'] });

  For future API consistency, we probably want to call logins "names", because that's what everything else will call it...even though it's slightly confusing in this context. What do you think?

>+    my $obj_by_ids = Bugzilla::User->new_from_list($params->{ids}) if $params->{ids};
>+    my @obj_by_login = map Bugzilla::User->new({ name => $_ }), @{ $params->{logins} } if $params->{logins};

  You can use Bugzilla::User->check here to validate the login's existence.

>+    # an array to hold the objects that the user will 
>+    # be able to get information about
>+    my @visible_user_objects;
>+
>+    # all the objects by login are visible to the user  
>+    # so add it to the bigger array
>+    push @visible_user_objects, @obj_by_login;

  You can just do my @visible_user_objects = @obj_by_login;

  It's also not necessary to describe what visible_user_objects is--that's self-documenting.

>+    for my $obj (@$obj_by_ids){
>+        if (Bugzilla->user->can_see_user($obj)){
>+            push @visible_user_objects, $obj;
>+        }

  Actually, I'd like to just throw an error if you ask for a user you can't see.

  Later (maybe in another bug, if somebody needs it), we can add a boolean parameter called "permissive" or something that just returns the valid ones.

>+    else {
>+        @users =
>+            map {{
>+                id => type('int')->value($_->id),
>+                real_name => type('string')->value($_->name),
>+                login => type('string')->value($_->login),

  Also include "email" here separately.
Attachment #297709 - Flags: review-
Attachment #297685 - Attachment is obsolete: true
Attachment #297707 - Attachment is obsolete: true
(In reply to comment #13)
> (From update of attachment 297709 [details] [diff] [review])
> >Index: Bugzilla/WebService/User.pm
> >+#    $call = $rpc->call( 'User.get', { ids => [1,2,3], logins => ['testusera@redhat.com', 'testuserb@redhat.com'] });
> 
>   For future API consistency, we probably want to call logins "names", because
> that's what everything else will call it...even though it's slightly confusing
> in this context. What do you think?
> 

Yes I agree with you :), consistency is important and more confusion will be caused if we call logins different things in different places.

Hi Max,, I have made all the modifications suggested by your review. also I added pod for the function one thing I wasn't sure about was the Error section in the POD i am not sure where to get the error codes from , as I have seen in some other modules documentation all the Error code next to the error description ei: 500 , 301 300 etc 

Thanks,
Noura
Attachment #297709 - Attachment is obsolete: true
Attachment #298214 - Flags: review?
fixed a typo and review requestee.
Attachment #298214 - Attachment is obsolete: true
Attachment #298215 - Flags: review?(mkanat)
Attachment #298214 - Flags: review?
I'm not really happy with this method, for two reasons: first, we should require the user to authenticate in order to ask for such data. If I were a spammer, it would be too easy to collect data this way. All I would have to do is to ask for all IDs, from 1 to 1000000 and collect all the corresponding email addresses. Second, I still think not everybody should have access to such data, even if the user is authenticated. IMO, we should require the user to have editusers privs, as in the first patch. I know it's a weak protection, but knowing the login name is a first step towards forcing the user account, see bug 355283. This method is also incompatible with bug 219021.

Consider this as a r- from me.
What if we just required a valid login using

Bugzilla->login(LOGIN_REQUIRED);

Would this be helpful in discouraging spam farming? We do similar with our
current installation by only showing real names in place of email addresses
for any pages where a user is not logged in.

Requiring a valid login is not too much to ask in my opinion.
It's not that we're going to call logins different things in different places.

It's that in Bugzilla, almost every object has an "id" which is a unique integer that identifies it, and a "name", which is a unique string that identifies it. For a user, their "name" happens to be their login, but that's actually confusing for most API users.
Hey Noura. The way that you get error messages and codes is you trace all of the code that your function calls, and see what error messages can be thrown. Then you look in Bugzilla::WebService::Constants and see if there's already a number assigned to each error. If not, you need to assign a number. 

You'll notice that numbers are assigned in groups--that is, there are sometimes many errors with the same id number. This is because, from the caller's perspective, those errors all represent the same "error" on the caller's part.

So, when assigning numbers to your errors, it requires judgment--you have to decide if you should assign a new number or re-use an old one.
Oh, and in response to LpSolit's comment, ALL webservice functions already require a login unless they are listed in LOGIN_EXEMPT, as I recall.
Oh, but if I'm wrong, of *course* this should be LOGIN_REQUIRED.
(In reply to comment #21)
> Oh, and in response to LpSolit's comment, ALL webservice functions already
> require a login unless they are listed in LOGIN_EXEMPT, as I recall.
> 

If that is the case then disregard my comment #18.

Dave
(In reply to comment #22)
> Oh, but if I'm wrong, of *course* this should be LOGIN_REQUIRED.

You are wrong. :) If 'requirelogin' is turned off, then you are free to use read-only methods as you like. For instance, you can call Bug.get() without being logged in and still get all the data.

Now that we all agree that such a method requires to be logged in, using LOGIN_REQUIRED, we still have to decide which privs are required in order to collect such data. I still don't think that all logged in users should have access to such data.
It is fine to have the information accessible to all logged-in users. The usability of the interface is more important than a nearly-useless security concern. It's already possible to easily get the assignees and QA contacts of every bug in the system (I know because I have a script that does it) and getting every commenter and every CC would only be a tiny bit more work (just several id arguments to the XML format of show_bug). And even logged out users can do that, currently!

So really, I'm not even sure this needs a LOGIN_REQUIRED.
(In reply to comment #25)
> So really, I'm not even sure this needs a LOGIN_REQUIRED.

I still want it, for the reasons mentioned in comment 17. Add LOGIN_REQUIRED and I will be happy with the method.
regarding the authorization discussion ,, basically the logic of the function User.get checks that every user id passed to it is an id of a user that can be seen by the *logged in user* in the following lines:

    for my $obj (@$obj_by_ids){
        if (Bugzilla->user->can_see_user($obj)){


if only one user id can not be seen by the logged in user the function will throw and error and exit. 

in the case of the passed login names ,, the user already knows the login name so no need to check for authorization however the info that they will be returned back is only "id, login, real name and email" it won't go deeper than that. So i am not really sure if LOGIN_REQUIRED is still needed here, as in comment 17 if a user searched for users ids from 1 to 100000 but he can not see those user ids which means he must be logged in then he will not get any info returned. Please let me know what you think?
(In reply to comment #20)
> Hey Noura. The way that you get error messages and codes is you trace all of
> the code that your function calls, and see what error messages can be thrown.
> Then you look in Bugzilla::WebService::Constants and see if there's already a
> number assigned to each error. If not, you need to assign a number. 
> 
> You'll notice that numbers are assigned in groups--that is, there are sometimes
> many errors with the same id number. This is because, from the caller's
> perspective, those errors all represent the same "error" on the caller's part.
> 
> So, when assigning numbers to your errors, it requires judgment--you have to
> decide if you should assign a new number or re-use an old one.
> 

Hi Max,, Thanks for the explanation ,, so looking at the current code of User.get() I would say there are 2 sort of errors that we might get:
1- the passed login name is invalid which i think for this we can have error
   invalid_username => 504,   from WebService/Constants.pm

2- the user can not see the otheruser ,, which I don't think there is any error code that match it so I suggest we add another error code to that under the user errors group so it would be : user_not_visible => 505,

What do you think?

Thanks,
Noura
(In reply to comment #28)
> What do you think?

  Yes, from what you wrote there, those both sound right. :-)
As discussed on IRC ,, I have made the login required by checking Bugzilla->user->id it is not true then only realnames will be returned ,otherwise the previous logic pf the function will be executed. updated the POD with that new change and added error codes for the errors, also updated Bugzilla/WebService/Constants.pm with the extra error code needed for non visible users.

Please review when you can.

Thanks,
Noura
Attachment #298215 - Attachment is obsolete: true
Attachment #298598 - Flags: review?(mkanat)
Attachment #298215 - Flags: review?(mkanat)
Comment on attachment 298598 [details] [diff] [review]
v6 for WebService function User.get() 

>Index: Bugzilla/WebService/Constants.pm

>+    # can not see user
>+    user_not_visible => 505, 

No method throws such an error. user_not_visible is not a valid error tag.
Attachment #298598 - Flags: review?(mkanat) → review-
Hi Frederic, I fixed the error codes in the documentation section and the Constants.pm I hope it looks better now ,, I didn't completely understand how they worked before sorry about that.

Thanks,
Noura
Attachment #298598 - Attachment is obsolete: true
Attachment #299057 - Flags: review?(LpSolit)
Comment on attachment 299057 [details] [diff] [review]
v7 of Bugzilla::WebService::User::get() , fixed error codes

This patch is of no use (and will fail if I have a different version of these files than you). Could you resubmit your patch using:
cvs diff -3 -puN
Attachment #299057 - Flags: review?(LpSolit) → review-
OK regenerated the patch with the right options cvs diff options: cvs diff -3 -puN
Attachment #299057 - Attachment is obsolete: true
Attachment #300543 - Flags: review?(LpSolit)
Isn't WebService/Constants.pm missing in this last patch?
Frederic, aah yes you are right sorry about that, attaching new patch with missing file generated with right cvs diff options.

Thanks,
Noura
Attachment #300543 - Attachment is obsolete: true
Attachment #300629 - Flags: review?(LpSolit)
Attachment #300543 - Flags: review?(LpSolit)
Comment on attachment 300629 [details] [diff] [review]
v9 of Bugzilla::WebService::User::get() with the missing WebService/Constants.pm file 

>+    if (!Bugzilla->user->id){
>+        push @visible_user_objects, @$obj_by_ids;


>+        if (Bugzilla->user->can_see_user($obj)){
>+            push @visible_user_objects, $obj;


This looks wrong to me. If you are logged out, you get real names of users you cannot see. If you are logged in, an error is thrown.
(In reply to comment #37)
> (From update of attachment 300629 [details] [diff] [review])
> >+    if (!Bugzilla->user->id){
> >+        push @visible_user_objects, @$obj_by_ids;
> 
> 
> >+        if (Bugzilla->user->can_see_user($obj)){
> >+            push @visible_user_objects, $obj;
> 
> 
> This looks wrong to me. If you are logged out, you get real names of users you
> cannot see. If you are logged in, an error is thrown.
> 

yeah I agree,, it doesn't make sense that a logged in user get an error and nothing returned and a non logged in user would get realnames returned. So what would be the right behavior here ?

shall we just not return anything for a non logged in user?
or 
shall we for non logged in user and for logged in users who can not see other users we always return minimum information which is realnames?


Thanks,
Noura

(In reply to comment #38)
> shall we just not return anything for a non logged in user?
> or 
> shall we for non logged in user and for logged in users who can not see other
> users we always return minimum information which is realnames?

If you cannot see another user, don't return any information about him if you passed the user ID, and only the realname if you passed a valid login name. Both when you are logged in or out.
did some modifications to the patch I hope the logic looks better.
Attachment #300629 - Attachment is obsolete: true
Attachment #300871 - Flags: review?(LpSolit)
Attachment #300629 - Flags: review?(LpSolit)
modified patch according to Frederic's review.
Attachment #300871 - Attachment is obsolete: true
Attachment #301273 - Flags: review?(LpSolit)
Attachment #300871 - Flags: review?(LpSolit)
Comment on attachment 301273 [details] [diff] [review]
Bugzilla::WebService::User::get()

>Index: Bugzilla/WebService/User.pm

>+# function to return user information by passing either user ids or login names or both together:
>+#    $call = $rpc->call( 'User.get', { ids => [1,2,3], names => ['testusera@redhat.com', 'testuserb@redhat.com'] });

These lines are too long. Please limit them to 80 characters per line.


>+    my @obj_by_login = map { Bugzilla::User->check({ name => $_ }) } @{ $params->{names} } if $params->{names};

Move |if $params->{names}| on its own line, else the line is too long.


>+    if ( !Bugzilla->user->id ){
>+        @users = map {{ real_name => type('string')->value($_->name), }} @obj_by_login;

The hash must include the login name, else you have no chance to know which real name goes with which login name.


>+    for my $obj ( @$obj_by_ids ){

Please write "foreach" instead of "for", for consistency.


>+=head2 List Accounts

POD doesn't pass test:

*** ERROR: =over on line 269 without closing =back (at head2) at line 365 in file Bugzilla/WebService/User.pm
not ok 121 - Bugzilla/WebService/User.pm has incorrect POD syntax --ERROR


While testing your patch, I got the following error if the real name uses non-ASCII characters:

not well-formed (invalid token) at line 1, column 869, byte 869 at /usr/lib/perl5/vendor_perl/5.8.8/i386-linux/XML/Parser.pm line 187

It chokes e.g. when the real name is "Frédéric". Any idea how to fix this problem?
Attachment #301273 - Flags: review?(LpSolit) → review-
applied modification to the patch according to Frederic's review.

UTF-8 issue is still unsolved as it will require more time to find good solution and it affects other WebServices functions.

Noura
Attachment #301273 - Attachment is obsolete: true
Attachment #301521 - Flags: review?(LpSolit)
Comment on attachment 301521 [details] [diff] [review]
Bugzilla::WebService::User::get()

Works fine, thanks. r=LpSolit
Attachment #301521 - Flags: review?(LpSolit) → review+
ID of users you cannot see are silently ignored, which is fine for me. a=me
Flags: approval+
Keywords: relnote
IDs of users you cannot see should throw an error. The WebService should never silently ignore anything--it's an API. It's painful for the client to figure out which IDs were successfully returned and which weren't, if they have to do that.
Flags: approval+
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
Not a reason to postpone it to 4.0. This patch doesn't affect core code.
Target Milestone: Bugzilla 4.0 → Bugzilla 3.2
Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
Flags: blocking3.2?
Yes, this patch is so close that there's no reason to wait for 4.0.
Flags: blocking3.2? → blocking3.2+
Target Milestone: Bugzilla 4.0 → Bugzilla 3.2
Whiteboard: [wanted-bmo]
fixed the patch to return an error if logged in user passes an id for users they can not see. update Constants.pm accordingly with the error codes and the POD.

Noura
Attachment #301521 - Attachment is obsolete: true
Attachment #301653 - Flags: review?(LpSolit)
Comment on attachment 301653 [details] [diff] [review]
return auth failure error is user passed id for users they can not see to Bugzilla::WebService::User::get()

>+    my @visible_user_objects = @obj_by_login;

>+            push ( @visible_user_objects, $obj );

Note that there may be duplicated entries in the array, e.g if you pass ids => 4, names => foo@bar.com, and both matches the same user account. Storing them in a hash of the form { $user_id => $user_object } would fix the problem. Noura, could you resubmit a new patch doing this?


>+            ThrowUserError('auth_failure', {reason => "not_visible",
>+                                            action => "access",
>+                                            object => "user"});

Nit: note that this error doesn't specify which user account cannot be accessed. But that's not specific to this bug, this error tag already exists. It would probably be fine to pass the login name of the user which cannot be accessed (on the other hand, this would give you the login despite you are not allowed to see the user; annoying) or at least the user ID.
Attachment #301653 - Flags: review?(LpSolit) → review+
Comment on attachment 301653 [details] [diff] [review]
return auth failure error is user passed id for users they can not see to Bugzilla::WebService::User::get()

>+    my @obj_by_login = map { Bugzilla::User->check({ name => $_ }) } @{ $params->{names} 

  You don't have to pass a hashref, just $_. check() defaults to checking "name".

>+    # if user not logged in then only return real names for 
>+    # valid passed login names, don't return anything for 
>+    # passed user ids.

  WebService functions must never silently fail. If they are not logged in and they pass user ids, you have to throw an error.

>+    if ( !Bugzilla->user->id ){
>+        @users = map {{ 
>+                     real_name => type('string')->value($_->name),
>+                     login => type('string')->value($_->login),

  Okay, I'm thinking about this more, and I'm realizing that for consistency, we should call this "name" instead of "login". Even though that's confusing (because this is a user), it's consistent with what Products, Components, and everything else do.

>+    foreach my $obj ( @$obj_by_ids ){

  Nit: You don't need the extra spaces inside those parentheses.

>+            push ( @visible_user_objects, $obj );

  Nit: Or here. (Or anywhere else.)

>+        }
>+        else {
>+            ThrowUserError('auth_failure', {reason => "not_visible",
>+                                            action => "access",
>+                                            object => "user"});

  I agree with LpSolit that after this bug is filed, we MUST modify this error to say exactly which IDs failed. Otherwise this could be very confusing for the user of the API.

>+                is_disabled => type('boolean')->value($_->is_disabled),
>+                email_disabled => type('boolean')->value($_->email_disabled),

  Why shouldn't we show these to normal users? Is there any security problem with showing this information to people?

> __END__
>+=item B<Params>
>+
>+A hash containing either C<ids>, that is an array of users ids, 
>+or C<names>, that is an array of users login names, or both items. 

  This is not the correct way to write this. Look at how User.login's parameters are documented.

>+=item email
>+
>+C<string> The email address of the user.

  Note here that currently this is the same as "login" (or "name" as we're going to call it) but that in the future they may be different.

>+Note: If the logged in user is not in the editusers group then the 
>+returned hash will only have the following items: C<id>, C<real_name>, 
>+C<email>, and C<login>. If they user is not logged in but they passed valid
>+login names then the returned hash will only have the item: C<real_name>,
>+and C<login> for the valid login names.

  This note is not enough. You have to explain the two situations under separate headings: Logged-In Users and Non-Logged-In Users. Then, for non-logged-in users, you have to explain what happens if you pass ids.

>+=item 51 (Invalid Object)
>+
>+A non existing login name was passed to the function, as a result no 
>+user object existed for that login name.

  That's a really complicated way to say that. Why not just say, "You passed an invalid login name in the "names" array."

>+=item 304 (Authorization Required)
>+
>+The logged in user is not authorized to see the user that they wanted 
>+to get information about.

  Replace "the user that" with "one of the users"
Attachment #301653 - Flags: review-
Also, duplicated entries are absolutely fine. I want the data returned in the exact order of the input parameters as much as possible.
(In reply to comment #52)
>   Okay, I'm thinking about this more, and I'm realizing that for consistency,
> we should call this "name" instead of "login".

"name" is really too confusing. This field actually returns the login of the user, not his name (as in "real name"). I'm sure you are going to confuse more people with it.


> >+                is_disabled => type('boolean')->value($_->is_disabled),
> >+                email_disabled => type('boolean')->value($_->email_disabled),
> 
>   Why shouldn't we show these to normal users? Is there any security problem
> with showing this information to people?

Why should the user be able to collect data using WebService which you don't normally have access to? It's like saying giving access to editusers.cgi to everybody (as read-only).


> >+=item email
> >+
> >+C<string> The email address of the user.
> 
>   Note here that currently this is the same as "login" (or "name" as we're
> going to call it) but that in the future they may be different.

No, they are currently not the same, due to emailsuffix not being empty on some installations.
(In reply to comment #53)
> Also, duplicated entries are absolutely fine. I want the data returned in the
> exact order of the input parameters as much as possible.

No, this doesn't work. User->new_from_list() runs a single SQL query with bla IN (...) ORDER by login_name, so the list is sorted, independently of the order of the input data.
(In reply to comment #54)
> "name" is really too confusing. This field actually returns the login of the
> user, not his name (as in "real name"). I'm sure you are going to confuse more
> people with it.

  Yes, but consistency is more important here.

> Why should the user be able to collect data using WebService which you don't
> normally have access to? It's like saying giving access to editusers.cgi to
> everybody (as read-only).

  Yes, but why not, if it makes the code simpler to write and the API simpler to understand? The only reason we haven't exposed that in the normal Bugzilla UI is that there wasn't any reason--that is, nobody needed to know it. But here, it makes things simpler and gives the API more power, so there's no reason to exclude that information.

> No, they are currently not the same, due to emailsuffix not being empty on some
> installations.

  Oh, good point!

(In reply to comment #55)
> No, this doesn't work. User->new_from_list() runs a single SQL query with bla
> IN (...) ORDER by login_name, so the list is sorted, independently of the order
> of the input data.

  Ah, you're right. Okay, so yeah, then let's make the returned entries unique. :-)
(In reply to comment #56)
>   Yes, but consistency is more important here.

Consistently confusing, you mean? :) Point me to a place where a user login/name is returned elsewhere in the Webservice? There is none. The only place where "name" can be found is in Product.get_products where the "name" in question is the one of products, which is really a name, not a login. So we are really not inconsistent by using "login" here, and would avoid all confusion.


>   Yes, but why not, if it makes the code simpler to write and the API simpler
> to understand? The only reason we haven't exposed that in the normal Bugzilla
> UI is that there wasn't any reason--that is, nobody needed to know it.

Exactly, because nobody needed to know about it. That's the point. I still think that these 2 attributes are part of the user administration and that unprivileged users don't have any valid reason to get this data (returning everything for everybody makes me think of a big brother, security-related or not).
If a bug were filed reporting that you could access this data from the UI, we would immediately put it in the security group, but on the other hand you would freely let anyone get this information using webservice. I disagree.
(In reply to comment #57)
> (In reply to comment #56)
> >   Yes, but consistency is more important here.
> 
> Consistently confusing, you mean? :) Point me to a place where a user
> login/name is returned elsewhere in the Webservice? 

  The consistency is that all objects should return their hash values as "id" and "name".

> If a bug were filed reporting that you could access this data from the UI, we
> would immediately put it in the security group, but on the other hand you would
> freely let anyone get this information using webservice. I disagree.

  Not necessarily. I don't see any security argument for being able to get this data here. You'd have to show me an actual situation where this would be a security issue.
Hi Max, I have done the modifications that you suggested and discussed with Frederic. I hope this patch looks better. I made some modification to the user-error template to enable the passing of the userid that the logged/non-logged in users can not access.

Please review when you can.

Thanks,
Noura
Attachment #301653 - Attachment is obsolete: true
Attachment #301907 - Flags: review?(mkanat)
Comment on attachment 301907 [details] [diff] [review]
Bugzilla::WebService::User::get() 

>Index: template/en/default/global/user-error.html.tmpl

>     [% ELSIF object == "user" %]
>-      the user you specified
>+      the user '[% userid FILTER html %]' you specified

editusers.cgi calls auth_failure with object => 'user' twice, and so must be fixed. Either that or write [% IF userid %] with ID '[% userid FILTER html %]'[% END %] so that the sentence still makes sense when no userid is passed.
Comment on attachment 301907 [details] [diff] [review]
Bugzilla::WebService::User::get() 

>+    my $obj_by_ids = Bugzilla::User->new_from_list($params->{ids}) if $params->{ids};
>+
>+    my @obj_by_login = map { Bugzilla::User->check($_) } @{ $params->{names} } 
>+                       if $params->{names};

  The way you wrote these two lines, the variable is only declared if the value is true, which is a problem. First declare the variable, and then assign to it with an if. (I'm pretty sure that this will throw an error or warning in 5.10.)

>+    # all the objects by login are visible to the user  
>+    # so add it to the bigger array
>+    my @visible_user_objects = @obj_by_login;

  There's no reason to have those be separate arrays, is there?

>+    # if user not logged return an error is the passed any
>+    # user ids, otherwise return real names and names for 
>+    # valid passed login names.
>+    if (!Bugzilla->user->id){

  There's no reason to even create the obj_by_ids array before this point.

>+        if ($params->{ids}){
>+            ThrowUserError('auth_failure', {action => "access",
>+                                            object => "user",
>+                                            userid => $params->{ids}->[0]});
>+
>+        }

  That is not an accurate enough error. You need to throw a specific error that explains that logged-out users cannot use the "ids" argument to this function.

>+    foreach my $user (@obj_by_login){
>+        if (!$unique_users{$user->id}){
>+            $unique_users{$user->id} = $user;
>+        }

  You could just do this with a map { $user->id => $user }, couldn't you?

>+Returns list of information about user accounts passed to it.

  "Gets information about user accounts in Bugzilla."

>+=item B<Params>
>+
>+=over
>+
>+=item C<ids> (array) B<Optional> - An array of users ids.

  An array of integers, representing user ids.

>+=item C<names> (array) B<Optional> - An array of users login names

  An array of login names (strings).

>+=item name
>+
>+C<string> The login name of the user

  "Note that in some situations this is different than their email."

>+=item is_disabled
>+
>+C<boolean> A boolean value to indicate if the user's account is disabled 
>+or enabled. 

  Actually, let's call this can_login. (So, reverse the sense of it and fix its description.)

>+=item email_disabled

  And let's call this email_enabled. (So reverse the sense of it.)

>+C<boolean> A boolean value to indicate if the bug-related mail will be sent
>+to the user or not.

  Remove "the" in "the bug-related". The word "the" means, "that one, specifically", which doesn't make sense there.

>+=item B<Logged-In Users>

  Okay. Here's what you should do. As part of the description of the C<ids> parameter, say:

  Logged-out users cannot pass this parameter to this function. If they try, they will get an error. Logged-in users will get an error if they specify the id of a user they cannot see.

  Then, in the Returns section, all you have to say is:

----
B<Note>: If you are not logged in to Bugzilla when you call this function, you will only be returned the C<name> and C<real_name> items.
----

  That should go below the list, not in an =item.

>+=item 51 (Invalid Object)

  Don't call it Invalid Object, just call it Bad Login Name.

>Index: template/en/default/global/user-error.html.tmpl
>     [% ELSIF object == "user" %]
>-      the user you specified
>+      the user '[% userid FILTER html %]' you specified

  This error is used other places in Bugzilla. That's why it already exists. Those other places do not use the userid argument, and so this will be very confusing if thrown in those places. You have to think about things like this when you are making modifications to Bugzilla.
Attachment #301907 - Flags: review?(mkanat) → review-
Hey Max, another patch version , please review when you can. 

Thanks,
Noura
Attachment #301907 - Attachment is obsolete: true
Attachment #302048 - Flags: review?(mkanat)
Added disbaled_text to the returned info about the user, to make the API consistent between User.get and User.update

Noura
Attachment #302048 - Attachment is obsolete: true
Attachment #305441 - Flags: review?(mkanat)
Attachment #302048 - Flags: review?(mkanat)
Comment on attachment 305441 [details] [diff] [review]
Bugzilla::WebService::User::get()

>Index: Bugzilla/WebService/User.pm
>+    # if user not logged return an error is the passed any
>+    # user ids, otherwise return real names and names for 
>+    # valid passed login names.

  "If the user is not logged in: Return an error if they passed any user ids. Otherwise, return a limited amount of information based on login names."

>+    if (!Bugzilla->user->id){
>+        @users = map {{ 
>+                     real_name => type('string')->value($_->name), 
>+                     name => type('string')->value($_->login),

  Include the user id. It couldn't hurt, and could possibly be useful.

>+    my $obj_by_ids;
>+    $obj_by_ids = Bugzilla::User->new_from_list($params->{ids}) if $params->{ids};

  You know, for these sorts of things you can also do:

  $params->{ids} || []

  And then you wouldn't have to write that on two lines.

>+    # store the current user objects by login into
>+    # a hash with the ids as the keys to use it later
>+    # for checking dupliated user objects.
>+    my %unique_users;
>+    map { $unique_users{$_->id} = $_ } @user_objects;

  You could write that on one line, like this:

  my %unique_users = map { $_->id => $_ } @user_objects;

  At least, I *think* that will work.

>+    foreach my $obj (@$obj_by_ids){

>+        if (Bugzilla->user->can_see_user($obj)){
>+            push (@user_objects, $obj) if !$unique_users{$obj->id};

  That doesn't prevent duplicate ids, because you don't add anything to unique_users in this loop.

>+    @users =
>+        map {{
>+            id => type('int')->value($_->id),
>+            real_name => type('string')->value($_->name),
>+            name => type('string')->value($_->login),
>+            email => type('string')->value($_->email),
>+            can_login => type('boolean')->value($_->is_disabled),
>+            email_enabled => type('boolean')->value($_->email_disabled),
>+            disabled_text => type('string')->value($_->disabledtext),

  Hm, let's call that login_denied_text instead of disabled_text. I think that's clearer, and it might be more like what we'll be calling it in the user interface at some point.

>+=head2 List Accounts

  "User Info"

>+=item B<Params>

  Right under here, add:

  "At least one of the following two parameters must be specified:"

  (Then you can remove the "Optional" on each one.)


>+=item id
>+
>+C<int> The id of the user.

  "The unique integer ID that Bugzilla uses to represent this user. Even if the user's login name changes, this will not change."

>+=item real_name
>+
>+C<string> The actual name of the user.

  Add: "May be blank."

>+=item disabled_text
>+
>+C<string> A text field that holds the reason for disabling a user from logging
>+into bugzilla, if empty then the user account is enabled otherwise it is 
>+disabled/closed.

  Add a period after "enabled".

>Index: Bugzilla/WebService/Constants.pm
>+    user_info_access_denied      => 305,

  I think that should be in the 500 range instead, since it's specific to User.

>Index: template/en/default/global/user-error.html.tmpl
> [snip]
>-    [% ELSIF object == "user" %]
>-      the user you specified
>+    [% ELSIF object == "user" %]  
>+      the user [% IF userid %] with ID '[% userid FILTER html %]'[% END %]
>+      you specified

  Don't include "you specified" if there's a userid.

>@@ -1595,6 +1596,11 @@
>+  [% ELSIF error == "user_info_access_denied" %]
>+    [% title = "User Info Access Denied" %]
>+    Logged-out users cannot use the "ids" argument to this function
>+    to access any user information.

  Let's call that user_access_by_id_denied instead.
Attachment #305441 - Flags: review?(mkanat) → review-
Hey Max,, Attached is a modified patch according to your last review.

Thanks,
Noura
Attachment #305441 - Attachment is obsolete: true
Attachment #307385 - Flags: review?(mkanat)
Comment on attachment 307385 [details] [diff] [review]
new WebService function User.get() + POD 

>+            email_enabled => type('boolean')->value($_->email_disabled),

The logic is wrong. If bugmail is disabled, email_enabled returns true!


(In reply to comment #58)
>   Not necessarily. I don't see any security argument for being able to get this
> data here. You'd have to show me an actual situation where this would be a
> security issue.

Simply because it makes the code simpler and because it's not necessarily security-related doesn't mean everybody should have access to all the data available about a user account. Simply because "that's easy to do" doesn't mean we should do it. I think I made such comments elsewhere already. I still object to make no distinction between power users and powerless users. Making the API more powerful is a wrong argument.
Attachment #307385 - Flags: review?(mkanat) → review-
Keywords: relnote
I agree with LpSolit here, the level of information you're able to get via this method should be relative to the amount of privileges you have.  A good chunk of user data (particularly their preference information and anything that's not already displayed in the UI, for example) should only be available to people with editusers privs or better.
(In reply to comment #66)
> (From update of attachment 307385 [details] [diff] [review])
> >+            email_enabled => type('boolean')->value($_->email_disabled),
> 
> The logic is wrong. If bugmail is disabled, email_enabled returns true!
> 

Yeah True .. I will call $_->email_anabled instead.

And I also vote for returning email_enabled and disabled_text only for users who are in the editusers group only. So what would be the final decision?

Thanks,
Noura
Max, as there is an obvious disagreement about how User.get() should work and what it should return, I suggest we skip this method for 3.2. Else we will probably be in a hurry if we change our mind again later before 3.2 is released.
Hi, I was really hoping that this function could get into 3.2
One point that maybe can solve the disagreement on the function is: from the web ui normal users can not really view iformation about users related to the disabled text or the disabled email , only users in editusers group can do this from the webUI so why don't we just go by that in the WebService function , why should normal users get extra info about users using the WebService interface? also in our company for security reasons we wouldn't want to reveal reasons for disabling other users to non privileged users.
I am attaching a new patch for User.get() , again I really hope it can get into 3.2 but ofcourse the final decision is yours :)

Thanks,
Noura
Attachment #307385 - Attachment is obsolete: true
Attachment #309435 - Flags: review?(mkanat)
Okay, sounds good to me.
Flags: blocking3.2+ → blocking3.2-
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
Blocks: 432914
Blocks: 432916
Comment on attachment 309435 [details] [diff] [review]
Bugzilla::WebService::User::get()

>Index: Bugzilla/WebService/User.pm
>+=item email_enabled
>+
>+C<boolean> A boolean value to indicate if bug-related mail will be sent
>+to the user or not.

  You should note that this is only passed to users in the editusers group.

>+Logged-in users are not authorized to see one of the users they wanted 
>+to get information about by user id.

  Instead, say:

  You are logged in, but you are not authorized to see one of the users you wanted to get information about by user id.

  Otherwise this all looks great!
Attachment #309435 - Flags: review?(mkanat) → review+
Flags: approval+
(In reply to comment #72)
>   You should note that this is only passed to users in the editusers group.

  Oh, nevermind that comment, by the way.
Thanks for the review Max, updated the patch with the little fix for the pod.

Noura
tip:
Checking in Bugzilla/WebService/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/User.pm,v  <--  User.pm
new revision: 1.7; previous revision: 1.6
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.17; previous revision: 1.16
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.250; previous revision: 1.249
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Keywords: relnote
Target Milestone: Bugzilla 4.0 → Bugzilla 3.4
Flags: testcase?
Summary: WebService functions to get information about a User. → WebService functions to get information about a User (User.get)
Added to the release notes for Bugzilla 3.4 in bug 494037.
Keywords: relnote
Flags: testcase? → testcase+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: