Bug 406471 - 3.37: OR based group checking for bugs and product entry
3.37: OR based group checking for bugs and product entry
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: Bugzilla General (Show other bugs)
3.2
All Linux
high Severity medium (vote)
: ---
: ---
Assigned To: Tony Fu
: Reopened
Depends On:
Blocks: RHBZ30UpgradeTracker 427051 438946
  Show dependency treegraph
 
Reported: 2007-11-30 11:59 EST by David Lawrence
Modified: 2013-07-26 00:56 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-27 23:49:02 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)
add "or_groups" parameter (2.98 KB, patch)
2008-02-05 02:41 EST, Tony Fu
no flags Details | Diff
add "or_groups" parameter (2.73 KB, patch)
2008-02-05 02:41 EST, Tony Fu
no flags Details | Diff
OR based group checking in can_see_bug function (74.38 KB, patch)
2008-02-05 02:42 EST, Tony Fu
no flags Details | Diff
One way we can do this in can_see_bug() (4.55 KB, patch)
2008-02-05 17:42 EST, David Lawrence
no flags Details | Diff
Add OR based group checking function in Bugzilla/Search.pm file (1.54 KB, patch)
2008-02-06 01:03 EST, Tony Fu
no flags Details | Diff
add more description of or_group parameter in groupsecurity.html template file (3.30 KB, text/plain)
2008-02-06 01:10 EST, Tony Fu
no flags Details
the other possible way we can do OR based group checking in User.pm file (2.49 KB, patch)
2008-02-06 01:51 EST, Tony Fu
no flags Details | Diff
OR based group checking in Bugzilla/Search.pm file (75.96 KB, patch)
2008-02-07 01:53 EST, Tony Fu
no flags Details | Diff
OR based group checking in get_selectable_products() and get_enterable_products() (2.96 KB, patch)
2008-02-08 02:39 EST, Tony Fu
no flags Details | Diff
Patch containing all changes in one so far (9.71 KB, patch)
2008-02-09 18:37 EST, David Lawrence
dkl: review? (tfu)
Details | Diff
OR based group checking in can_edit_products function (1.63 KB, patch)
2008-02-11 21:38 EST, Tony Fu
no flags Details | Diff
updated template files (1.70 KB, patch)
2008-02-12 01:18 EST, Tony Fu
no flags Details | Diff
updated template files (1.51 KB, patch)
2008-02-12 19:19 EST, Tony Fu
dkl: review+
Details | Diff
Patch containing all changes in one so far (v2) (13.71 KB, patch)
2008-02-12 19:43 EST, David Lawrence
dkl: review? (tfu)
Details | Diff
Added an update of template/en/default/admin/products/groupcontrol/edit.html.tmpl (15.07 KB, patch)
2008-02-13 21:39 EST, Tony Fu
dkl: review+
Details | Diff
OR based group checking patch (15.16 KB, patch)
2008-02-14 23:15 EST, Tony Fu
dkl: review+
Details | Diff
Changes to User.pm to update can_see_product and get_selectable_products (dkl) (v1) (3.41 KB, patch)
2008-03-24 17:49 EDT, David Lawrence
dkl: review? (tfu)
Details | Diff


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

  None (edit)
Description David Lawrence 2007-11-30 11:59:13 EST
Description:
An account needs to be one or more of the groups assigned to a bug or marked for entry to a product in order to perform the operation. Stock Bugzilla behaviour is to require an account to be in all groups checked.

Function Requirements:
Bugzilla/User.pm
Comment 1 Tony Fu 2007-12-20 00:52:53 EST
Size estimation:

It needs a function which will includes a simple input, a simple output and a
simple internal logical file for the logic.  The unadjusted-function-point-total
will be 
unadjusted-function-point-total=1*3+1*4+1*7=14
total-function-point-count=14*1.11=15.1
LOC=15.1*60=906

Test unit:
The testing data will includes a simple input, a simple output.  The
unadjusted-function-point-total will be 
unadjusted-function-point-total=1*3+1*4=7
total-function-point-count=7*1.11=7.7
LOC=60*7.7=462

Total LOC=906+462=1368
Comment 2 Kevin Baker 2007-12-29 14:30:50 EST
this should be configurable via the params. That way it's a simple admin 
choice. And it will be more palatable to the upstream.
Comment 3 David Lawrence 2007-12-30 01:07:10 EST
+1

FWIW, current upstream code still hard codes AND style group checking so this is
not fixed yet. For an example of this check can_see_bug() in Bugzilla/User.pm in
rh_bugzilla_3

There is a bug filed upstream for just this feature that was slated for 3.0 but
was removed.

https://bugzilla.mozilla.org/show_bug.cgi?id=172772
Comment 4 Tony Fu 2008-02-04 01:54:13 EST
Dave,

I have started working on this ticket.  So far, I have figured out how AND style
group checking works in upstream code and I can add a piece of code to implement
OR style group checking.  If we need to enable OR style group checking via a
params, where is the best place to set this params? 


Thanks,
Tony

Comment 5 David Lawrence 2008-02-04 14:37:43 EST
Tony, see if you can isolate the smallest code difference between the two
methods and then form code similar to this:

$query = "SELECT foo FROM bar ... ";
if ( Bugzilla->params->{'or_groups'} ) { 
    # Append SQL that does group checks based on OR method
    $query .= " ... ";
}
else {
    # Else append SQL that does group checks using the standard AND method.
    $query .= " ... ";
}

Then you will just need to create a boolean style param in the params related
files to support setting or_groups through the UI.

The files to touch to add the param are:
Bugzilla/Config/GroupSecurity.pm
template/en/default/admin/params/groupsecurity.html.tmpl

Dave
Comment 6 Tony Fu 2008-02-05 02:38:15 EST
(In reply to comment #5)
> Then you will just need to create a boolean style param in the params related
> files to support setting or_groups through the UI.
> 
> The files to touch to add the param are:
> Bugzilla/Config/GroupSecurity.pm
> template/en/default/admin/params/groupsecurity.html.tmpl
> 
Dave,

Thank you very much for your help.  I have updated Bugzilla/User.pm file to
implement OR-based group checking (please see the code in # REDHAT ENTENSION
START and # REDHAT ENTENSION END) and Bugzilla/config/GroupSecurity.pm and
template/en/default/admin/params/groupsecurity.html.tmpl files to add or_groups
params.

Because the Bugzilla installed on my laptop has some problems, I haven't fully
tested the OR-based group checking code I added into Bugzilla/User.pm file.  I
will do the test later.


Thanks,
Tony

Comment 7 Tony Fu 2008-02-05 02:41:13 EST
Created attachment 293982 [details]
add "or_groups" parameter
Comment 8 Tony Fu 2008-02-05 02:41:43 EST
Created attachment 293983 [details]
add "or_groups" parameter
Comment 9 Tony Fu 2008-02-05 02:42:30 EST
Created attachment 293984 [details]
OR based group checking in can_see_bug function
Comment 10 David Lawrence 2008-02-05 15:51:27 EST
Comment on attachment 293984 [details]
OR based group checking in can_see_bug function

>    if (Bugzilla->params->{'or_groups'}) {
>        $sth = $dbh->prepare("SELECT COUNT(*) FROM bug_group_map
>                              LEFT JOIN user_group_map
>                                ON bug_group_map.group_id = user_group_map.group_id
>                              WHERE bug_group_map.bug_id = ?
>                                AND user_group_map.user_id = $userid");
>        $sth->execute($bugid);
>        my $or_group_test = $sth->fetchrow_array();
>        $missinggroup = $missinggroup & !$or_group_test;

$sth->fetchrow_array() will return a list and you are assigning to a scalar. So
you would
do:

	 my ($or_group_test) = $sth->fetchrow_array();

Also can you explain more about what is happening in the line:

	 $missinggroup = $missinggroup & !$or_group_test;

I am trying to wrap my mind around it.

>    };
>    # REDHAT EXTENSION END
>    return ($ready
>            && ((($reporter == $userid) && $reporter_access)
>                || (Bugzilla->params->{'useqacontact'} 
>                    && $qacontact && ($qacontact == $userid))
>                || ($owner == $userid)
>                || ($isoncclist && $cclist_access)
>                || (!$missinggroup)));
>}
Comment 11 David Lawrence 2008-02-05 17:42:07 EST
Created attachment 294053 [details]
One way we can do this in can_see_bug()

Tony, preferrably in can_see_bug() it should be done in a single SQL query
instead of using two. I have attached a sample patch that utilizes the method
of CanSeeBug() in 2.18 globals.pl. I have also added a conditional after the
results are returned that determine if the user is in enough groups based on
AND or OR group checking. Let me know what you think.

Also we will need to do something something similar in Bugzilla/Search.pm
inside lines 776 to 822. The SQL generated should limit the results returned
based on visibility.
Comment 12 David Lawrence 2008-02-05 17:47:51 EST
Comment on attachment 293982 [details]
add "or_groups" parameter

># REDHAT ENTENSION START #406471
>  or_groups => "Allow OR-based groups checking. ",
># REDHAT ENTENSION END #406471

I think we need more description here. Maybe something like

"If set to on, use OR based group checking. If set to off then use AND based
group checking.
With OR group checking, a user would need to be in one or more groups a bug is
marked private to.
With AND group checking, a user would need to be in all groups that a bug is
marked private to."
Comment 13 Tony Fu 2008-02-05 20:33:21 EST
(In reply to comment #11)
> Created an attachment (id=294053) [edit]
> One way we can do this in can_see_bug()
> 
> Tony, preferrably in can_see_bug() it should be done in a single SQL query
> instead of using two. I have attached a sample patch that utilizes the method
> of CanSeeBug() in 2.18 globals.pl. I have also added a conditional after the
> results are returned that determine if the user is in enough groups based on
> AND or OR group checking. Let me know what you think.
> 
Dave,

I think your way is much more straitforward than the way used in upstream code.
     The join condition used by upstream code to do AND based group checking is
clever but very hard to understand.  The only possible advantage of using it is
that it only needs to join two tables (bugs and bug_group_map) instead of three
tables (bugs, bug_group_map and user_group_map) if we do AND based group checking.

Tony
Comment 14 Tony Fu 2008-02-06 01:03:08 EST
Created attachment 294073 [details]
Add OR based group checking function in Bugzilla/Search.pm file
Comment 15 Tony Fu 2008-02-06 01:04:37 EST
> Also we will need to do something something similar in Bugzilla/Search.pm
> inside lines 776 to 822. The SQL generated should limit the results returned
> based on visibility.

I have attached a patch of Bugzilla/Search.pm file to implement OR based group
checking.
Comment 16 Tony Fu 2008-02-06 01:10:05 EST
Created attachment 294075 [details]
add more description of or_group parameter in groupsecurity.html template file
Comment 17 Tony Fu 2008-02-06 01:51:18 EST
Created attachment 294076 [details]
the other possible way we can do OR based group checking in User.pm file

In this way, we don't need to change upstream's query if we don't turn
"or_groups" setting on.
Comment 18 Tony Fu 2008-02-06 01:52:42 EST
update "hours worked" information
Comment 19 David Lawrence 2008-02-06 15:35:55 EST
(In reply to comment #16)
> Created an attachment (id=294075) [edit]
> add more description of or_group parameter in groupsecurity.html template file
> 

Looks good.
Comment 20 David Lawrence 2008-02-06 15:43:07 EST
(In reply to comment #17)
> Created an attachment (id=294076) [edit]
> the other possible way we can do OR based group checking in User.pm file
> 
> In this way, we don't need to change upstream's query if we don't turn
> "or_groups" setting on.

Tony, your solution looks good as well. The only problem I have is that I prefer
not to write the entire SQL statement twice in the if/else blocks. Can you do it
where you only have the common SQL once and then just rewrite the small sections
that need to be based on groups type?

Dave
Comment 21 David Lawrence 2008-02-06 15:45:35 EST
(In reply to comment #15)
> > Also we will need to do something something similar in Bugzilla/Search.pm
> > inside lines 776 to 822. The SQL generated should limit the results returned
> > based on visibility.
> 
> I have attached a patch of Bugzilla/Search.pm file to implement OR based group
> checking.
> 

Tony, can you explain how this would work if the bug is public and there are no
groups checked? Would it allow the bug to be viewed if user_group_map.groups IS
NULL in the WHERE clause?

Dave


Comment 22 Tony Fu 2008-02-07 01:48:52 EST
(In reply to comment #21)
> (In reply to comment #15)
> > > Also we will need to do something something similar in Bugzilla/Search.pm
> > > inside lines 776 to 822. The SQL generated should limit the results returned
> > > based on visibility.
> > 
> > I have attached a patch of Bugzilla/Search.pm file to implement OR based group
> > checking.
> > 
> 
> Tony, can you explain how this would work if the bug is public and there are no
> groups checked? Would it allow the bug to be viewed if user_group_map.groups IS
> NULL in the WHERE clause?
> 

Dave,

I didn't think about the public bug before, so my sql didn't work against public
 bugs.  I should use ((user_group_map.groups IS NOT NULL) OR
(bug_group_map.bug_id IS NULL)) instead in the WHERE clause (please see new
patch for details).  In this case, if a bug is a public bug, the bug's bug_id
shouldn't be in bug_group_map table, so "bug_group_map.bug_id IS NULL" will be true.

Tony
> 
> 
> 

Comment 23 Tony Fu 2008-02-07 01:53:06 EST
Created attachment 294190 [details]
OR based group checking in Bugzilla/Search.pm file
Comment 24 David Lawrence 2008-02-07 18:34:31 EST
Looks good Tony. Also have a look at Bugzilla/User.pm as well. I think we need
to utilize this change for get_selectable_products() as well. That method looks
at the group_control_map table and only allows users to see products currently
that are either not private or are members of all groups that the product is
marked private for entry. We would like that be either AND or OR as well.

Dave
Comment 25 Tony Fu 2008-02-08 02:37:59 EST
(In reply to comment #24)
> Looks good Tony. Also have a look at Bugzilla/User.pm as well. I think we need
> to utilize this change for get_selectable_products() as well. That method looks
> at the group_control_map table and only allows users to see products currently
> that are either not private or are members of all groups that the product is
> marked private for entry. We would like that be either AND or OR as well.
> 
> Dave
> 

I have add OR based group checking in get_selectable_products().  Also I add it
in  get_enterable_products() as well.  Please see attached patch for details.

For can_see_bug(), I think the patch you provided before is fine and we may use it.

Tony
Comment 26 Tony Fu 2008-02-08 02:39:24 EST
Created attachment 294306 [details]
OR based group checking in get_selectable_products() and get_enterable_products()
Comment 27 David Lawrence 2008-02-09 18:37:12 EST
Created attachment 294480 [details]
Patch containing all changes in one so far

Tony, I am attaching a patch that includes all work done so far in one patch.
Please take a look to make sure it looks right. So far it seems to work as
expected on http://bugdev.devel.redhat.com/bugzilla.

Thanks
Dave
Comment 28 David Lawrence 2008-02-11 13:20:48 EST
Tony,
    I found another place where we need to put this division in place as well.

Bugzilla::User::can_edit_product()

This is currently checking for AND groups if there are groups checked that can
only edit a product. We need to put the if/else with or_groups param there as
well please.

Can you take a look at that?

Thanks
Dave
Comment 29 Tony Fu 2008-02-11 21:36:41 EST
(In reply to comment #28)
> Tony,
>     I found another place where we need to put this division in place as well.
> 
> Bugzilla::User::can_edit_product()
> 
> This is currently checking for AND groups if there are groups checked that can
> only edit a product. We need to put the if/else with or_groups param there as
> well please.
> 
> Can you take a look at that?
> 
> Thanks
> Dave


Dave,

I have added OR based group checking into Bugzilla::User::can_edit_product(). 
Please review attached patch.

Thanks,
Tony
Comment 30 Tony Fu 2008-02-11 21:38:06 EST
Created attachment 294619 [details]
OR based group checking in can_edit_products function
Comment 31 Tony Fu 2008-02-12 01:16:36 EST
Update template file to display proper description of group restricts, according
to the "or_group" param.

Please see attached patch for details.
Comment 32 Tony Fu 2008-02-12 01:18:09 EST
Created attachment 294629 [details]
updated template files
Comment 33 David Lawrence 2008-02-12 12:31:46 EST
Comment on attachment 294629 [details]
updated template files

>+              [% IF Param('or_groups') %]
>+                <b>Only users in any of the selected groups can view this [% terms.bug %]:</b>
>+              [% ELSE %]
>                 <b>Only users in all of the selected groups can view this [% terms.bug %]:</b>
>+              [% END %]

Hi Tony, good work. But we should always try to keep out patch as small as
possible with repeating too much information. So for example this would just
be:

<b>Only users in [% IF Param('or_groups') %]any[% ELSE %]all[% END %] of the
selected groups can view this [% terms.bug %]:</b>

>+       [% IF Param('or_groups') %]
>+        <strong>
>+          Only users in any of the selected groups can view this [% terms.bug %]:
>+        </strong>
>+        [% ELSE %]
>         <strong>
>           Only users in all of the selected groups can view this [% terms.bug %]:
>         </strong>
>+        [% END %]

Same comment above for this section as well. Move the common HTML and other
words outside of the IF ELSE END section and just change the one word (any/all)
based on the param setting.

Thanks
Dave
Comment 34 Tony Fu 2008-02-12 19:18:27 EST
Dave,

Thank you for your comment.  I have changed these two template files.  Please
see attached patch.


Thanks,
Tony

Comment 35 Tony Fu 2008-02-12 19:19:49 EST
Created attachment 294714 [details]
updated template files
Comment 36 David Lawrence 2008-02-12 19:32:23 EST
Comment on attachment 294714 [details]
updated template files

Looks good Tony.
Comment 37 David Lawrence 2008-02-12 19:43:04 EST
Created attachment 294715 [details]
Patch containing all changes in one so far (v2)

Here is a new patch that has all the latest changes combined. Tony please take
a look to make sure it still looks ok. After some testing we can get this
checked in.

Dave
Comment 38 Tony Fu 2008-02-13 21:37:56 EST
(In reply to comment #37)
> Created an attachment (id=294715) [edit]
> Patch containing all changes in one so far (v2)
> 
> Here is a new patch that has all the latest changes combined. Tony please take
> a look to make sure it still looks ok. After some testing we can get this
> checked in.
> 
> Dave

Hey Dave,

The patch looks good.  I just found another template file
(template/en/default/admin/products/groupcontrol/edit.html.tmpl) needed to be
modified.  I have added it into your new patch.  Please take a look.


Thanks,
Tony
Comment 39 Tony Fu 2008-02-13 21:39:45 EST
Created attachment 294863 [details]
Added an update of template/en/default/admin/products/groupcontrol/edit.html.tmpl
Comment 40 David Lawrence 2008-02-14 15:35:51 EST
Comment on attachment 294863 [details]
Added an update of template/en/default/admin/products/groupcontrol/edit.html.tmpl

>Index: groupcontrol/edit.html.tmpl
> If any group has <b>Entry</b> selected, then this product will
>-restrict [% terms.bug %] entry to only those users who are members of all the
>+restrict [% terms.bug %] entry to only those users who are members of [% IF Param('or_groups') %] any [% ELSE %]all[% END %] the
> groups with entry selected.

Looks good Tony. I have a one small change request. Missing the word 'of'
before 'the groups'. Also minor cosmetic change  , remove the extra whitespace
around 'any' in the conditional.

Otherwise with those changes, I thinks this ready to checkin so feel free to
commit when you get ready.

Dave
Comment 41 David Lawrence 2008-02-14 15:51:13 EST
(In reply to comment #40)
> (From update of attachment 294863 [details] [edit])
> >Index: groupcontrol/edit.html.tmpl
> > If any group has <b>Entry</b> selected, then this product will
> >-restrict [% terms.bug %] entry to only those users who are members of all the
> >+restrict [% terms.bug %] entry to only those users who are members of [% IF
Param('or_groups') %] any [% ELSE %]all[% END %] the
> > groups with entry selected.
> 
> Looks good Tony. I have a one small change request. Missing the word 'of'
> before 'the groups'. Also minor cosmetic change  , remove the extra whitespace
> around 'any' in the conditional.
> 

Ack, my eyes are not working. Scratch that request. the 'of' is in fact there
already.

Fix the whitespace then check this in.

Dave

Comment 42 Tony Fu 2008-02-14 23:13:44 EST
> Fix the whitespace then check this in.
> 
Deleted whitespaces and attached a new patch here.

Will commit all changes to cvs repository.

Tony
> 
> 

Comment 43 Tony Fu 2008-02-14 23:15:07 EST
Created attachment 294968 [details]
OR based group checking patch
Comment 44 David Lawrence 2008-03-24 17:45:03 EDT
Sorry to keep bringing this issue back up. Since we are getting some good
testing down now by us and others, I am finding some problems with out current
implementation of the OR group changes. Mostly this is my fault as I reviewed
the code and looked good to me at the time. These problems are always going to
materialize during actual use so this is not abnormal at all. 

Doing some queries I noticed that a non-logged in user was still able to see
private products in the query.cgi product list. Something is not correct in our
SQL that filters the products based on whether they are public or not and if not
whether the user is in the necessary groups to see the product. I have created a
patch that seems to work for Bugzilla::User::get_selectable_products() and
Bugzilla::User::can_edit_product() that seems to work in my simple testing.

Tony, can you take a moment to look at these and test them out to see if they
look right to you? Also if you see a way to do it better than what I am trying,
please let me know.

Thanks and sorry for the trouble.
Dave
Comment 45 David Lawrence 2008-03-24 17:49:18 EDT
Created attachment 298956 [details]
Changes to User.pm to update can_see_product and get_selectable_products (dkl) (v1)
Comment 46 Tony Fu 2008-03-25 01:02:49 EDT
Dave,

The patch looks good for me.

Also I added a patch to fix another reported bug that is related to OR based
group checking.  Please review it from bug #437969.


Thanks,
Tony
Comment 47 David Lawrence 2008-03-25 17:10:53 EDT
Committed to CVS. Adding 1 hour.
Comment 48 Noura El hawary 2008-04-10 08:49:44 EDT
clear old review request, for obsolete attachment
Comment 49 Kevin Baker 2008-04-14 22:30:27 EDT
Not closing yet until audit #438946 is finished. There are other places that 
do similar style OR checking. 

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