Bug 438946

Summary: Need to audit all code that enables privacy in Bugzilla to assure proper operation
Product: [Community] Bugzilla Reporter: David Lawrence <dkl>
Component: User AccountsAssignee: Tony Fu <tfu>
Status: CLOSED NEXTRELEASE QA Contact:
Severity: low Docs Contact:
Priority: low    
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-05-28 03:48:59 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: 406471    
Bug Blocks: 406071    
Attachments:
Description Flags
OR based group checking in get_selectable_products() and get_enterable_products() dkl: review+

Description David Lawrence 2008-03-26 03:46:19 UTC 
We need to look at all functions in Bugzilla that are used to block users from
seeing data that they are not allowed to assure that the code is performing
properly.

The following functions are used to determine permissions to view data:

Bugzilla::Search - wrapper around search SQL that blocks bugs based on permissions.

Bugzilla::User::can_see_user - Determines if user can see info about other user.

Bugzilla::User::can_edit_product - Determine whether user can edit bugs in a
specific product.

Bugzilla::User::can_see_bug - Determines whether user can see a particular bug
report.

Bugzilla::User::can_see_product - Determines whether user can query for bugs in
a specific product. Calls Bugzilla::User::get_selectable_products.

Bugzilla::User::get_selectable_products - Returns a list of products that a user
has permission to query against.

Bugzilla::User::can_enter_product - Determines whether a user can enter a bug
against a specific product. Calls Bugzilla::User::get_enterable_products().

Bugzilla::User::get_enterable_products - Returns list of products that a user
can enter bugs against.

Bugzilla::User::get_accessible_products - get_selectable_products and
get_enterable_products

Bugzilla::User::can_admin_product - Determines if user can edit attributes of a
product

Bugzilla::User::can_set_flag - Can the user set a given flag (+ -)

Bugzilla::User::can_request_flag - Can the user request a given flag (?)

Bugzilla::User::can_see_flag - Can the user even see the flag

I am sure there are more. Please add more to the list if you see any that were
missed.
Comment 1 David Lawrence 2008-04-01 14:47:48 UTC 
Tony, now that fix for bug 437969 has been committed, can you take a look at
some of these functions to make sure that they are checking for proper group
permissions in the same fashion?

Thanks
Dave
Comment 2 Kevin Baker 2008-04-15 02:56:37 UTC 
Tony, please review the sql for OR group checking.
Comment 3 Kevin Baker 2008-04-15 02:57:04 UTC 
needed for Milestone 3.
Comment 4 Tony Fu 2008-04-17 06:39:09 UTC 
Dave,

I have reviewed all sql queries related to user, bug, product privacy.  All
functions look good, except the or base group check queries in
get_select_products and get_enterable_products.

I have attached a patch to fix them.  Please review the patch.


Thanks,
Tony
Comment 5 Tony Fu 2008-04-17 06:41:22 UTC 
Created attachment 302713 [details]
OR based group checking in get_selectable_products() and get_enterable_products()
Comment 6 David Lawrence 2008-04-17 22:11:58 UTC 
Comment on attachment 302713 [details]
OR based group checking in get_selectable_products() and get_enterable_products()

>+            $query .= "  WHERE group_id IN (" . $self->groups_as_string . ")" .
>+                      "     OR group_control_map.group_id IS NULL";

I think we need to put parens ( ) around the OR parts like this:

	    $query .= "  WHERE (group_id IN (" . $self->groups_as_string . ")"
.
		      " 	OR group_control_map.group_id IS NULL) ";

cause we are looking for either one to be true for that one part of the SQL. If
this seems
right and still works with your testing, then feel free to review=dkl+ and
check it in.

Thanks
Dave
Comment 7 Tony Fu 2008-04-24 06:31:04 UTC 
(In reply to comment #6)
> (From update of attachment 302713 [details] [edit])
> >+            $query .= "  WHERE group_id IN (" . $self->groups_as_string . ")" .
> >+                      "     OR group_control_map.group_id IS NULL";
> 
> I think we need to put parens ( ) around the OR parts like this:
> 
> 	    $query .= "  WHERE (group_id IN (" . $self->groups_as_string . ")"
> .
> 		      " 	OR group_control_map.group_id IS NULL) ";
> 
> cause we are looking for either one to be true for that one part of the SQL. If
> this seems
> right and still works with your testing, then feel free to review=dkl+ and
> check it in.
> 
> Thanks
> Dave
> 

Dave,

I have added extra parents according to your comments and committed it into cvs
repo.


Thanks,
Tony