Bug 427893
Summary: | XMLRPC functions idToName and nameToId replaced by User.get() function | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Community] Bugzilla | Reporter: | Noura El hawary <nelhawar> | ||||||||||||||||
Component: | WebService | Assignee: | Noura El hawary <nelhawar> | ||||||||||||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | |||||||||||||||||
Severity: | medium | Docs Contact: | |||||||||||||||||
Priority: | high | ||||||||||||||||||
Version: | 3.2 | ||||||||||||||||||
Target Milestone: | --- | ||||||||||||||||||
Target Release: | --- | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Whiteboard: | |||||||||||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||||||||||
Doc Text: | Story Points: | --- | |||||||||||||||||
Clone Of: | Environment: | ||||||||||||||||||
Last Closed: | 2008-03-18 20:35:52 UTC | Type: | --- | ||||||||||||||||
Regression: | --- | Mount Type: | --- | ||||||||||||||||
Documentation: | --- | CRM: | |||||||||||||||||
Verified Versions: | Category: | --- | |||||||||||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||||||||||
Embargoed: | |||||||||||||||||||
Bug Depends On: | |||||||||||||||||||
Bug Blocks: | 406071, 406131, 427053 | ||||||||||||||||||
Attachments: |
|
Description
Noura El hawary
2008-01-08 02:02:28 UTC
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. |