Bug 427893 - XMLRPC functions idToName and nameToId replaced by User.get() function
XMLRPC functions idToName and nameToId replaced by User.get() function
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: WebService (Show other bugs)
3.2
All Linux
high Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
:
: 427889 (view as bug list)
Depends On:
Blocks: RHBZ30UpgradeTracker 406131 427053
  Show dependency treegraph
 
Reported: 2008-01-07 21:02 EST by Noura El hawary
Modified: 2013-06-24 00:19 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-18 16:35:52 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
patch for Bugzilla/WebService/User.pm (1.40 KB, text/plain)
2008-01-15 01:59 EST, Noura El hawary
no flags Details
patch for WebService module that includes function id_to_login (1.76 KB, text/plain)
2008-01-16 23:53 EST, Noura El hawary
no flags Details
xmlrpc function User.get() (1.76 KB, patch)
2008-01-17 23:50 EST, Noura El hawary
no flags Details | Diff
WebService function User.get() (6.88 KB, patch)
2008-02-26 09:13 EST, Noura El hawary
no flags Details | Diff
Bugzlla::WebService::User::get() (6.94 KB, patch)
2008-02-28 07:58 EST, Noura El hawary
no flags Details | Diff
Bugzilla::WebService::User::get() (6.96 KB, patch)
2008-02-28 09:22 EST, Noura El hawary
dkl: review-
Details | Diff
Bugzilla::WebService::User::get() (5.94 KB, patch)
2008-02-29 00:03 EST, Noura El hawary
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Mozilla Foundation 412725 None None None Never

  None (edit)
Description Noura El hawary 2008-01-07 21:02:28 EST
write xmlrpc function idToName that is to be contributed to the upstream.
Please refer to the following url for details on Bugzilla::RPC 2.18 functions mapped to upstream Bugzilla::WebService and for 2.18 xmlrpc functions sorted by percentage of calls: https://engineering.redhat.com/trac/bugzilla-3.0-rh/wiki/XmlrpcCrossComparison#a2.18xmlrpcfunctionssortedbypercentageofcalls
Comment 1 Noura El hawary 2008-01-15 01:59:54 EST
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
Comment 2 Noura El hawary 2008-01-15 02:10:00 EST
one point here we can maybe discuss with upstream is to either pass single user
id or a list of user ids.
Comment 3 Noura El hawary 2008-01-16 23:53:23 EST
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"
Comment 4 Noura El hawary 2008-01-17 23:50:35 EST
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.
Comment 5 Kevin Baker 2008-01-18 11:53:18 EST
(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? ;)
Comment 6 Noura El hawary 2008-01-21 20:05:35 EST
*** Bug 427889 has been marked as a duplicate of this bug. ***
Comment 7 Noura El hawary 2008-01-21 20:18:09 EST
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
Comment 8 Noura El hawary 2008-01-24 19:17:36 EST
fixed error codes in the POD section and in Bugzilla/WebService/Bug.pm
Comment 9 Noura El hawary 2008-02-08 01:52:24 EST
got the xmlrpc function code reviewed by upstream several times, and did
modifications on it several times.
Comment 10 Noura El hawary 2008-02-26 09:13:27 EST
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 11 David Lawrence 2008-02-26 16:01:07 EST
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
Comment 12 Noura El hawary 2008-02-28 07:54:31 EST
> 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
> 

Comment 13 Noura El hawary 2008-02-28 07:58:40 EST
Created attachment 296196 [details]
Bugzlla::WebService::User::get()

new patch for User.get() with Dave's suggestions
Comment 14 Noura El hawary 2008-02-28 09:22:13 EST
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.
Comment 15 David Lawrence 2008-02-28 10:48:29 EST
(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 16 David Lawrence 2008-02-28 11:27:28 EST
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@redhat.com'] } 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.
Comment 17 David Lawrence 2008-02-28 11:50:46 EST
(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@redhat.com'] } 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.
Comment 18 Kevin Baker 2008-02-28 13:52:54 EST
(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
Comment 19 David Lawrence 2008-02-28 14:32:31 EST
(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.
Comment 20 Noura El hawary 2008-02-29 00:03:26 EST
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
Comment 21 David Lawrence 2008-03-18 16:33:52 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.