Created attachment 291690 [details]
patch for Bugzilla/WebService/User.pm 

the patch contains 2 functions one to get login name from userid and one to get
userid from login name, basically those 2 functions are similar to our xmlrpc
functions in rh_bugzilla_2_18  nameToId and IdToName

one point here we can maybe discuss with upstream is to either pass single user
id or a list of user ids.

Created attachment 291953 [details]
patch for WebService module that includes function id_to_login

I created a bug for that in the upstream :

https://bugzilla.mozilla.org/show_bug.cgi?id=412725

Attached is another version for the previous patch I changed it to accept a
list of user ids instead of a single user id. "please note that the patch
also includes function for converting name to id"

Created attachment 292108 [details]
xmlrpc function User.get()

according to the upstream ,, 
https://bugzilla.mozilla.org/show_bug.cgi?id=412725

The discussion is leading to replacing the functions the return any user
informations with single function in the WebService::User modules called get().


attached new patch that contains an initial User.get() function.

(In reply to comment #3)
> I created a bug for that in the upstream :
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=412725

you're rockin'. Can we add this bugzilla id to the "External Bugzilla 
References" below? ;)

*** Bug 427889 has been marked as a duplicate of this bug. ***

Inputs: hasref with ids/loginnames = 1
Outputs: authentication errors , input validation errors, logging messages,
         hashref with users details = 4
Inquiries: user data , user groups = 2
Logical files: noneero
External files = Zero

FP total count(simple weighting factor)= (3*1)+(4*4)+(3*2)+(7*0)+(5*0)= 25

FP = 25 * 1.11 = 28

LOC = 28 * 60 = 1680


For Unit testing:

100 LOC for one xmlrpc testcase

total LOC = 1680+100 - 1780 LOC

fixed error codes in the POD section and in Bugzilla/WebService/Bug.pm

got the xmlrpc function code reviewed by upstream several times, and did
modifications on it several times.

Created attachment 295913 [details]
WebService function User.get()

Attached is patch for new WebService function User.get() please review when you
can so we can add it to milestone 2.

Thanks,
Noura

Comment on attachment 295913 [details]
WebService function User.get()

>+sub get {
>+    my ($self, $params) = @_;
>+
>+    my @user_objects;
>+    @user_objects = map { Bugzilla::User->check($_) } @{ $params->{names} }
>+                    if $params->{names};

This may break if the user passes in $params->{names} or $params->{ids} as a
single value if the functions we call are expecting a list.

      my @name_list = ref($params->{names}) ? @{$params->{names}} :
($params->{names};
      my @user_objects;
      @user_objects = map { Bugzilla::User->check($_) } @name_list
		    if @name_list;

>+    
>+    my @users;
>+
>+    # 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 user is not logged in, return an error if they passed any
      # user ids, otherwise return real names and login names for
      # valid passed in login names.

>+    if (!Bugzilla->user->id){
>+        if ($params->{ids}){
>+            ThrowUserError("user_info_access_denied");
>+        }
>+        @users = map {{ 
>+                     real_name => type('string')->value($_->name), 
>+                     name => type('string')->value($_->login),
>+                 }} @user_objects;
>+
>+        return { users => \@users };
>+    }
>+
>+    my $obj_by_ids;
>+    $obj_by_ids = Bugzilla::User->new_from_list($params->{ids}) if $params->{ids};
>+

Same as comment above about checking to make sure $params->{ids} is indeed a
list or single value;

>+    # 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.

      # for checking duplicated user objects

>+    my %unique_users;
>+    map { $unique_users{$_->id} = $_ } @user_objects;
>+
>+    # obj_by_ids are only visible to the user if he can see 
>+    # the otheruser, for non visible otheruser throw an error
>+    foreach my $obj (@$obj_by_ids){
>+        if (Bugzilla->user->can_see_user($obj)){
>+            push (@user_objects, $obj) if !$unique_users{$obj->id};
>+        }
>+        else {
>+            ThrowUserError('auth_failure', {reason => "not_visible",
>+                                            action => "access",
>+                                            object => "user",
>+                                            userid => $obj->id});
>+        }
>+    }
>+
>+    @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),
>+        }} @user_objects;
>+
>+    return { users => \@users };
>+}

I would be inclined not send back the disabled_text value unless the calling
user is in the editusers group. Reason being for Red Hat at least this value
can contain sensitive information about a users departure. We could instead
just pass back disabled_for_login => 0|1
or something similar if they are not in the editusers group.

Thoughts?

Otherwise the rest looks good to me.

Dave

> This may break if the user passes in $params->{names} or $params->{ids} as a
> single value if the functions we call are expecting a list.
> 
>       my @name_list = ref($params->{names}) ? @{$params->{names}} :
> ($params->{names};
>       my @user_objects;
>       @user_objects = map { Bugzilla::User->check($_) } @name_list
> 		    if @name_list;
> 
Yeah true good idea, I will add this to a new patch. shall we suggest this to
the upstream or just keep it for us as our redhat users are used to the single
values? also shall we add to the POD that they can pass single value or
array/list of values?, in the upstream everything is basically lists and
allowing single values means that few changes will have to be made to other
WebService functions as they are very concerned with the API consistency.

> Same as comment above about checking to make sure $params->{ids} is indeed a
> list or single value;
> 
 will also add this to the new patch.

> 
> I would be inclined not send back the disabled_text value unless the calling
> user is in the editusers group. Reason being for Red Hat at least this value
> can contain sensitive information about a users departure. We could instead
> just pass back disabled_for_login => 0|1
> or something similar if they are not in the editusers group.
> 
> Thoughts?
> 
yeah I think that disabled_text is not really necessary to be returned as we
return value called can_login which will tell us if the user is disabled or not.
so I will delete it.


> Otherwise the rest looks good to me.
> 
> Dave
> 

Created attachment 296196 [details]
Bugzlla::WebService::User::get()

new patch for User.get() with Dave's suggestions

Created attachment 296210 [details]
Bugzilla::WebService::User::get()

done a little modification instaled of checking if @ids or if @name_list it
checks for the actual passed param if $params->{ids} and if params->{names} so
we can allow they they can be both passed or either one of them otherwise both
will have to be passed or we will get an error.

(In reply to comment #12)
> Yeah true good idea, I will add this to a new patch. shall we suggest this to
> the upstream or just keep it for us as our redhat users are used to the single
> values? also shall we add to the POD that they can pass single value or
> array/list of values?, in the upstream everything is basically lists and
> allowing single values means that few changes will have to be made to other
> WebService functions as they are very concerned with the API consistency.

I would definitely suggest it to upstream and argue that it doesn't hurt their
API in any way and that we are just providing one more level of flexibility to
the end user.

Comment on attachment 296210 [details]
Bugzilla::WebService::User::get()

>+sub get {
>+    my ($self, $params) = @_;
>+
>+    my @name_list = ref($params->{names}) ? @{$params->{names}} :
>+                    ($params->{names});
>+
>+    my @user_objects;
>+    @user_objects = map { Bugzilla::User->check($_) } @name_list
>+		    if $params->{names};
>+    
>+    my @users;

Nit-Pick: Usually best to put variable declarations near the top of the
subroutine. Not a show-stopper though.

>+
>+    # 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){
>+        if ($params->{ids}){
>+            ThrowUserError("user_info_access_denied");
>+        }
>+        @users = map {{ 
>+                     real_name => type('string')->value($_->name), 
>+                     name => type('string')->value($_->login),
>+                 }} @user_objects;
>+
>+        return { users => \@users };
>+    }
>+
>+    my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});

Keep the variable names consistent. So maybe change to my @id_list or change
my @name_list to my @names?

>+
>+    my $obj_by_ids;
>+    $obj_by_ids = Bugzilla::User->new_from_list(\@ids) if $params->{ids};
>+
>+    # 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;
>+
>+    # obj_by_ids are only visible to the user if he can see 
>+    # the otheruser, for non visible otheruser throw an error
>+    foreach my $obj (@$obj_by_ids){
>+        if (Bugzilla->user->can_see_user($obj)){
>+            push (@user_objects, $obj) if !$unique_users{$obj->id};
>+        }
>+        else {
>+            ThrowUserError('auth_failure', {reason => "not_visible",
>+                                            action => "access",
>+                                            object => "user",
>+                                            userid => $obj->id});
>+        }
>+    }

Move the 

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

to here instead. Where it is located now I could pass in { ids => [6], names =>
['dkl'] } and get my user data returned twice if I am allowed to see
user 6 since you are only filtering unique values after the names.

(In reply to comment #16)
> >+    foreach my $obj (@$obj_by_ids){
> >+        if (Bugzilla->user->can_see_user($obj)){
> >+            push (@user_objects, $obj) if !$unique_users{$obj->id};
> >+        }
> >+        else {
> >+            ThrowUserError('auth_failure', {reason => "not_visible",
> >+                                            action => "access",
> >+                                            object => "user",
> >+                                            userid => $obj->id});
> >+        }
> >+    }
> 
> Move the 
> 
> map { $unique_users{$_->id} = $_ } @user_objects; 
> 
> to here instead. Where it is located now I could pass in { ids => [6], names =>
> ['dkl'] } and get my user data returned twice if I am allowed to see
> user 6 since you are only filtering unique values after the names.
> 

Ok, my suggestion here was premature. I mistakenly missed that you were checking
for !$unique_users{$obj->id} in the foreach loop. But there is still a problem
here. If I pass in { ids => [6, 6] } I will get that account twice. So change to
 the following:

foreach my $obj (@$obj_by_ids){
    if (Bugzilla->user->can_see_user($obj)){
        if ( !$unique_user{$obj->id} ) {
            push (@user_objects, $obj);
            $unique_user{$obj->id} = $obj;
    }
    else {
        ThrowUserError('auth_failure', {reason => "not_visible",
                                        action => "access",
                                        object => "user",
                                        userid => $obj->id});
    }
}

Fix that and the variable names, position and r=dkl.

(In reply to comment #17)
> >+    my @users;
> 
> Nit-Pick: Usually best to put variable declarations near the top of the
> subroutine. Not a show-stopper though.

I've told Noura in the past to put the variable declarations as close to where 
they are used as possible. On the premise that when you're looking at code for 
the first time you don't have to search around to find out where it is 
declared. Further rationale on p241-243 of Code Complete

(In reply to comment #18)
> (In reply to comment #17)
> > >+    my @users;
> > 
> > Nit-Pick: Usually best to put variable declarations near the top of the
> > subroutine. Not a show-stopper though.
> 
> I've told Noura in the past to put the variable declarations as close to where 
> they are used as possible. On the premise that when you're looking at code for 
> the first time you don't have to search around to find out where it is 
> declared. Further rationale on p241-243 of Code Complete

Cool. Like I said definitely not a show-stopper with review. I know I have read
that was a good practice somewhere but can't recall right off. But after
re-visiting the Code Complete, it is probably better choice to leave them.

Created attachment 296303 [details]
Bugzilla::WebService::User::get()

patch with Dave's suggestions applied, also I moved the filtering of user
objects up to the top before returning the the userinfo for non logged in users
so this also can get filtered out.

patch committed to cvs now

This feature is included now in Milestone 2. Any bugs found with this feature
should be file as a new report and set to block the 3.2 final release tracker.