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

this should be configurable via the params. That way it's a simple admin 
choice. And it will be more palatable to the upstream.

+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

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

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

(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

Created attachment 293982 [details]
add "or_groups" parameter

Created attachment 293983 [details]
add "or_groups" parameter

Created attachment 293984 [details]
OR based group checking in can_see_bug function

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)));
>}

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 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."

(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

Created attachment 294073 [details]
Add OR based group checking function in Bugzilla/Search.pm file

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

Created attachment 294075 [details]
add more description of or_group parameter in groupsecurity.html template file

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.

update "hours worked" information

(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.

(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

(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

(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
> 
> 
> 

Created attachment 294190 [details]
OR based group checking in Bugzilla/Search.pm file

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

(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

Created attachment 294306 [details]
OR based group checking in get_selectable_products() and get_enterable_products()

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

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

(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

Created attachment 294619 [details]
OR based group checking in can_edit_products function

Update template file to display proper description of group restricts, according
to the "or_group" param.

Please see attached patch for details.

Created attachment 294629 [details]
updated template files

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

Dave,

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


Thanks,
Tony

Created attachment 294714 [details]
updated template files

Comment on attachment 294714 [details]
updated template files

Looks good Tony.

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

(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

Created attachment 294863 [details]
Added an update of template/en/default/admin/products/groupcontrol/edit.html.tmpl

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

(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

> Fix the whitespace then check this in.
> 
Deleted whitespaces and attached a new patch here.

Will commit all changes to cvs repository.

Tony
> 
> 

Created attachment 294968 [details]
OR based group checking patch

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

Created attachment 298956 [details]
Changes to User.pm to update can_see_product and get_selectable_products (dkl) (v1)

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

Committed to CVS. Adding 1 hour.

clear old review request, for obsolete attachment

Not closing yet until audit #438946 is finished. There are other places that 
do similar style OR checking.