Closed Bug 452525 Opened 17 years ago Closed 12 years ago

Allow the option of "OR" groups ("any of the groups" instead of "all of the groups")

Categories

(Bugzilla :: Bugzilla-General, enhancement, P1)

Product:
Bugzilla
Component:
Bugzilla-General
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 5.0

People

(Reporter: mkanat, Assigned: mail)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

v4 patch
19.83 KB, patch
gerv
: review+
Details | Diff | Splinter Review
Bugzilla's group system is ridiculously confusing to people. The primary source of this confusion is that you have to be in *all* the groups listed on a bug, not just *any* of the groups listed. We should switch Bugzilla to "OR" groups, meaning that you have to belong to *any* of the groups listed on the bug. It's entirely possible to upgrade installations automatically into this system, by creating "junction groups"--that is, for any bug which is in more than one group (very rare on almost every Bugzilla installation in the world), we create a group that has the names of both groups appended to each other, and which has as members only the intersection of the two groups.
That would be fantastic. I seem to remember we looked at this not long after the groups system was switched on, but were baffled by the migration prospects. We would need to publicise this change loudly both before and after it happens to prevent embarrassing accidents. But let's do it. Gerv
FWIW and as you all probably know already, we have been doing OR groups for years. We had to update 3.2 to do the same as well so would welcome this official method for doing it instead of using our customizations. We would be more than willing to share our changes also as hackish as they will seem. Dave
Wouldn't it be wise to support the existing 'AND' behaviour and offer the 'OR' via a params switch?
(In reply to comment #3) > Wouldn't it be wise to support the existing 'AND' behaviour and offer the 'OR' > via a params switch? No. We had a long discussion about that (see bug 172772). There are no security situations that require the AND behavior, and the majority of security situations actually map much better on the OR behavior.
Whiteboard: [3.6 Focus]
Max, Joel, any idea of how much work this change would require? I guess that all SQL queries related to groups/permissions would have to be revisited (can_see_bug(), can_edit_product(), Search.pm, ...).
That is correct and the changes should be a lot better localized than it was when we considered bug 172772, but I can see it breaking a lot of customizations. The bigger problem is how to make migration work in a way that doesn't force an administrator to do a lot of re-definition work immediately after the upgrade just to keep things working. I think a new compound group would have to be created for every combination of groups that are in use anywhere and those compound groups would have a heck of a time staying up to date. For example, if I had a group "foo" and a group "bar" and there was one bug or product that used "foo and bar", I would need to have a new group "foo_and_bar" created just to manage the transition. If foo had a regexp of "@mycompany\.com$" and was mandatory for product "fooey" and bar was mycompany.com's security team, then I can't see how I would keep the group up-to-date after the upgrade. In that example, what we really want to do is to change "fooey" from foo: entry, mandatory/mandatory bar: optional/optional to.... foo: optional bar: mandatory If, in the other hand, we didn't know that bar is and always will be a subset of foo, we need a compound group.... foo: optional bar_and_foo: mandatory and we need the "bar_and_foo" group to contain only those members of bar who continue to satisfy the regexp for "foo" That involves revisiting the rationale behind every group configuration combination in use and mapping it to its new rationale in the new system. If not automated, that can be a real pain and I don't have a clear view of how to automate it.
There are two issues here: the difficulty of changing Bugzilla to work in the new way, and the difficulty of migrating. It might be useful to try and keep them separate. In terms of migration, it may be that we have to close off Bugzilla to all but admins post-upgrade, and then run the admin through some sort of wizard: "You have bugs that used to require both groups foo and bar to see. [ Details of group foo] [ Details of group bar] Would you like to: (o) Create a new foo_and_bar group combining both ( ) Put all the bugs in a new group, defined as follows: [ creating new group UI ] ( ) Put all the bugs in an existing group or groups: [ UI for multi-selecting groups ] Also, we could have a simpler UI to or better explanation of the function of "find all bugs with group combination X" and "change all these bugs to have group combination Y". Gerv
The wizard seems problematic when you have hundreds of products with tens or hundreds of groups. The admin would have to manually go through them all and select one of "rename/keep/..." for each product. I think the automatic migration should take place instead. The admin can then rename groups if they want to. But there are still some problems: merging group names would break saved searches using group names in them. Also, how do you merge group descriptions, and less importantly, group icons? (I didn't think about group inheritance yet, nor about blessing privs.)
The work you do in the wizard is not proportional to the number of bugs or products or groups, but to the number of different multi-group combinations you have. So if you have a million bugs, a thousand products and a hundred groups, but all of the bugs are only in 0 or 1 group, there would be nothing to do at all. Because GROUP_A || GROUP_A is the same as GROUP_A && GROUP_A ;-) Max asserts above that currently, having bugs in multiple groups is very rare. I have no idea whether he's right about that, but if anyone knows, he will. Gerv
On a generally public Bugzilla, I wouldn't expect bugs to be in multiple groups often. On a private Bugzilla, all bugs will be in at least one group and I would expect bugs in two groups to be as commonplace as bugs in single groups on a public Bugzilla.
All you have to do is set up a new compound group for any set of bugs that is in multiple groups, and then give it to the product with the highest level of Mandatory-ness as a union of all the groups. This can be done automatedly by checksetup, and I can write it. FWIW, I have *never* seen a bug in multiple groups in any Bugzilla that I've worked with (and that's a lot).
(In reply to comment #11) > FWIW, I have *never* seen a bug in multiple groups in any Bugzilla that I've > worked with (and that's a lot). We use multiple groups quite extensively here at Red Hat FWIW. For example we have Red Hat internal groups such as devel, qa, support, etc. And then we have partner groups such as Dell, Intel, IBM, etc. So for Red Hat developers to work on private bugs reported by say Dell, then the bug is marked as devel and dell. That way the RH developer doesn't also have to be in the dell group. But we have been using OR based groups since the beginning so this is not a big issue for us. Dave
(In reply to comment #12) > But we have been using OR based groups since the beginning so this is not a big issue > for us. Right. I'm saying that nobody uses multiple groups with AND groups, because it doesn't make a lot of sense and it's a very rare use case.
Unfortunately, while it may be rare, it is not nonexistent. In case exactly analogous to DKL's example above, the RH developer would be inherited into the dell group and the dell group would be mandatory for the product. I can confirm that those systems exist and even that they are the reason that group inheritance is there.
(In reply to comment #14) > In case exactly > analogous to DKL's example above, the RH developer would be inherited into the > dell group and the dell group would be mandatory for the product. Which would be one group. Group inheritance isn't going to change.
(In reply to comment #13) > Right. I'm saying that nobody uses multiple groups with AND groups, because > it doesn't make a lot of sense and it's a very rare use case. Hmm, we're using AND groups extensively. On the one hand, groups separate projects (and their teams) from each other. On the other hand, groups separate internals from (various groups of) externals. AND groups happens whenever somebody decides that a bug in a certain product (in a mandatory product group) is to be visible to internals only. I'm not saying we shouldn't move to OR groups, quite the contrary. I'm just saying that AND groups *are* being used, and that we need to take care. My use case, doesn't seem all that rare to me, so there may be other non-rare cases we haven't thought of yet either.
(In reply to comment #16) That's the exact case I was referring to and had implemented. The same logic does apply. OR groups would be better and it would even have been worth it to me to switch. I just have a hard time seeing how it wouldn't need a wizard at the least. With regard to the wizard, I wouldn't do it mid-upgrade. During checksetup, before any other updates, checksetup should check for anything beyond the simple (automatic) cases. If it detects a more complex case, it should look for a config file (the output of the wizard) to tell it what to do with each complex case. If that file is missing, it should just warn and not proceed. If Max is correct, very few sites should even see a hiccup during upgrade. Then, we can consider the help we provide for the (relatively few) sites that do need to remap their groups.
Hmm. I think there are no cases not handled by my automatic upgrade description. If you know of one, please describe it.
Attached patch Patch v.1 (obsolete) — — Splinter Review
Could this be it? I've looked at where bug_group_map is used, and these do seem to be the only places where this decision is made. On my test install, if a bug is in two groups, a user in only one of those groups can view it, view an attachment attached to it, and find it in a search. What might I have missed? I realise the migration code is going to be more work than this :-) Gerv
Assignee: general → gerv
Status: NEW → ASSIGNED
Comment on attachment 405498 [details] [diff] [review] Patch v.1 Requesting review to see if this is on the right track. There's no point working on migration if this part isn't right. Gerv
Attachment #405498 - Flags: review?(mkanat)
I think that based on what a serious change this will be, it should wait for the beginning of the Bugzilla 3.8 feature development cycle instead of coming at the very end of the Bugzilla 3.6 feature development cycle.
Target Milestone: Bugzilla 4.0 → Bugzilla 3.8
Comment on attachment 405498 [details] [diff] [review] Patch v.1 >Index: Bugzilla/User.pm >- my $has_external_groups = >+ my $in_at_least_one_group = > $dbh->selectrow_array('SELECT 1 > FROM group_control_map > WHERE product_id = ? > AND canedit != 0 >- AND group_id NOT IN(' . $self->groups_as_string . ')', >+ AND group_id IN(' . $self->groups_as_string . ')', This doesn't work in the case when the product is not in any canedit groups (which is the most common case). >@@ -591,30 +591,30 @@ sub can_see_bug { What version of Bugzilla is this against? This code hasn't been in can_see_bug for a while. >- AND bug_group_map.group_ID NOT IN(" . >+ AND bug_group_map.group_ID IN(" . I think this will have to be moved from the JOIN to the WHERE (to handle the case where the bug isn't in any groups). >Index: Bugzilla/Search.pm >=================================================================== > [snip] > if (%{$user->groups}) { >- $query .= " AND bug_group_map.group_id NOT IN (" . join(',', values(%{$user->groups})) . ") "; >+ $query .= " AND bug_group_map.group_id IN (" . join(',', values(%{$user->groups})) . ") "; > } I think that exposes all bugs to anybody who isn't in a group at all.
Attachment #405498 - Flags: review?(mkanat) → review-
Here is what I propose... 1) bug_group_map grants permission to groups to see a bug. But, if no groups have an entry, the bug is public. 2) In the process of migration, the system identifies what composite groups are potentially needed, then identifies which composite groups are redundant because we have groups like "x_AND_y" and it turns out that members of "x" are already inherited into group "y" so "x" can be used whenever we would have used "x_AND_y" 3) In the group controls, CANEDIT and EDITBUGS get replaced by a new control, EDITRIGHTS, with settings... * READONLY (no-write access granted) * CANCOMMENT * CANEDIT This control would dictate the degree to which membership in the group grants authority to make changes to a bug the user is permitted to see. Products that had one group in CANEDIT or EDITBUGS would have EDITRIGHTS set appropriately for each group, using composite groups if needed. EDITCOMPONENTS group permissions would remain just as they are today. 4) Each combination of group with Membercontrol/Othercontrol would be mapped to an appropriate new combinatons of Always, Default, Optional, and None. For example, if we have MemberControl values of... a) No restrictions PUBLIC: Always b) One group plop_access: Mandatory/Mandatory becomes plop_access: Always c) One Optional Group splat_access: Shown/[shown|default] Becomes... splat_access: Always PUBLIC: Default **** But, if non--members of splat_access are not supposed to be able to remove the PUBLIC permission or, if the reporter or CClist member, be able to grant public permission, we don't have a way to express this. d) 2 groups fizz_access: Mandatory fizz_developers: Default without counting inheritance, that would be mapped to... fizz_developers_and_fizz_access: Always fizz_access: Optional but, assuming that fizz_developers are inherited into fizz_access, this becomes.... fizz_developers: Always fizz_access: Optional **** Like the case above, the question of who is allowed to make a group permission change within the realm of group permissions allowed is complicated. To try out any proposed solutions, consider a moderated product... widget_support_group: Default/Mandatory widget_users: ENTRY,Mandatory/Mandotory So, any widget user can enter a bug, but it defaults to being visible only to the reported and the support_group. Once the support group makes sure that there is no customer confidential information in the report, they can drop the restriction to the support group. How an we express this with OR groups?
(In reply to comment #23) > Here is what I propose... > [snip] Well, that looks like a very comprehensive plan, though I didn't read all of it. I think that any details beyond the scope of this bug, if they should be discussed, should probably go to the developers list, and then we can file bugs as required. For now, changes to the Group Controls system would be beyond the scope of this bug--we'd be using the same controls and UI, we'd just have them work on an OR instead of an AND.
(In reply to comment #24) > discussed, should probably go to the developers list No, the developers list is not the right place to discuss this. People should be pointed to this bug instead. We can keep track of bugs and bug comments. But it's too easy to loose track of comments made in a mailing-list (and I personally delete emails pretty frequently). > as required. For now, changes to the Group Controls system would be beyond the > scope of this bug--we'd be using the same controls and UI, we'd just have them > work on an OR instead of an AND. No, moving from AND to OR involves some UI modifications to make the UI understandable. I discussed this with joel on IRC. Not everything mentioned by joel needs to be fixed in this bug, I agree, but at least a part of it should be done at the same time, i.e. in this bug.
(In reply to comment #25) > No, the developers list is not the right place to discuss this. People should > be pointed to this bug instead. We can keep track of bugs and bug comments. But > it's too easy to loose track of comments made in a mailing-list (and I > personally delete emails pretty frequently). Well, technical debates or discussions should go on the developers list, and the results of those discussions should go here. > No, moving from AND to OR involves some UI modifications to make the UI > understandable. I discussed this with joel on IRC. Not everything mentioned by > joel needs to be fixed in this bug, I agree, but at least a part of it should > be done at the same time, i.e. in this bug. Well, I don't think that there are UI modifications that are absolutely *necessary* for the *functionality* of this bug (except changing some text on show_bug and enter_bug), but we could possibly have some other bugs that we work on simultaneously to improve UIs with this. (joel and I talked about that too.)
Target Milestone: Bugzilla 4.0 → Bugzilla 5.0
Priority: -- → P1
I'd still like to see this done, but having it assigned to me is misleading, as I'm not working on it. Gerv
Assignee: gerv → general
We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved. I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.
Target Milestone: Bugzilla 4.4 → ---
Status: ASSIGNED → NEW
Assignee: general → sgreen
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 5.0
Version: 3.3 → 4.4
Whiteboard: [3.6 Focus]
Attached patch v2 patch (obsolete) — — Splinter Review
This is taken from our live code, so any issue probably also affect us too.
Attachment #405498 - Attachment is obsolete: true
Attachment #781488 - Flags: review?(justdave)
Summary: Switch to "OR" groups ("any of the groups" instead of "all of the groups") → Allow the option of "OR" groups ("any of the groups" instead of "all of the groups")
Attached patch v3 patch (obsolete) — — Splinter Review
Fixes: * Remove some redhat customisations in visible_bugs_or * Indenting of SQL * Formatting of an if statement * OR groups will be enabled for new installations by default
Attachment #781488 - Attachment is obsolete: true
Attachment #781488 - Flags: review?(justdave)
Attachment #781498 - Flags: review?(justdave)
Comment on attachment 781498 [details] [diff] [review] v3 patch Review of attachment 781498 [details] [diff] [review]: ----------------------------------------------------------------- Hi Simon, I grepped the source code for the two tables involved in this, bug_group_map and group_control_map, and found some places where it's possible that you also need to update the SQL. Could you review these sections (line numbers are for the start of a section, not the exact line) and either add to the patch or confirm no change is necessary in that place? bug_group_map: buglist.cgi line 819 Bugzilla/Search.pm line 2823, 2899 Bugzilla/Bug.pm line 3615 request.cgi line 121 group_control_map: buglist.cgi line 819 Bugzilla/Product.pm line 762 Bugzilla/Bug.pm line 3615 Bugzilla/User.pm line 831 Do we have any plans to write migration code so that the visibility of bugs does not change when switching from AND to OR? The other option is a scary warning like the one I write below, perhaps combined with a migration guide. It would perhaps be useful to have a place where users could see how many bugs they had in their installation which are in multiple groups (and therefore whose visibility would be affected). What do you think? ::: Bugzilla/Search.pm @@ +1318,5 @@ > + $security_term .= " (security_map.group_id IS NULL OR security_map.group_id IN (" . $user->groups_as_string . "))"; > + } > + else { > + $security_term = 'security_map.group_id IS NULL'; > + } It seems like this extra SQL here "matches" the and-only SQL in the higher-up change to this file. Can you explain why this can't be switched-in in the same place that the other SQL is switched-out? Can we add comments linking the two? ::: Bugzilla/User.pm @@ +880,5 @@ > + if (Bugzilla->params->{'or_groups'}) { > + my $groups = $self->groups_as_string; > + my ($cnt_can_edit, $cnt_group_member) = $dbh->selectrow_array( > + "SELECT SUM(foo.cnt_can_edit), > + SUM(foo.cnt_group_member) Please don't use "foo" as an identifier in production code. :-) @@ +882,5 @@ > + my ($cnt_can_edit, $cnt_group_member) = $dbh->selectrow_array( > + "SELECT SUM(foo.cnt_can_edit), > + SUM(foo.cnt_group_member) > + FROM (SELECT CASE WHEN canedit = 1 THEN 1 ELSE 0 END AS cnt_can_edit, > + CASE WHEN canedit = 1 AND group_id IN ($groups) THEN 1 ELSE 0 END AS cnt_group_member Are we sure this SQL works fine across all our DBs? Same Q for all the other SQL you've added. I'm not sure what our coding standards are here. @@ +885,5 @@ > + FROM (SELECT CASE WHEN canedit = 1 THEN 1 ELSE 0 END AS cnt_can_edit, > + CASE WHEN canedit = 1 AND group_id IN ($groups) THEN 1 ELSE 0 END AS cnt_group_member > + FROM group_control_map > + WHERE product_id = $prod_id) AS foo"); > + return ($cnt_can_edit == 0 or $cnt_group_member > 0); This took a bit of puzzling out; a comment might elucidate: # Check that either a) product is not using group-based canedit, or b) user is in one of the necessary groups. @@ +886,5 @@ > + CASE WHEN canedit = 1 AND group_id IN ($groups) THEN 1 ELSE 0 END AS cnt_group_member > + FROM group_control_map > + WHERE product_id = $prod_id) AS foo"); > + return ($cnt_can_edit == 0 or $cnt_group_member > 0); > + } Can we please make this an if/else rather than an if/return continue/return, to make it more clear that one is the or_groups case and the other is the and_groups case? @@ +912,5 @@ > + : $self->_visible_bugs_and(@_); > +} > + > +sub _visible_bugs_or { > + my ($self, $bugs) = @_; This shares too much code with _visible_bugs_and to be an entirely separate function. There are only two real differences - the SQL executed, and the decision code for deciding visibility. There's a lot of duplicated boilerplate. Can you make the split more fine-grained? @@ +943,5 @@ > + $sth ||= $dbh->prepare( > + # This checks for groups that the bug is in that the user > + # *isn't* in. Then, in the Perl code below, we check if > + # the user can otherwise access the bug (for example, by being > + # the assignee or QA Contact). Is this cut-and-pasted comment still valid for the OR case? I think it might well not be. @@ +2586,5 @@ > +=item C<visible_bugs($bugs)> > + > +Description: Determines if bugs are visible to the user. Calls _visible_bugs_or > + or _visible_bugs_and to do the work depending on the value of the > + or_groups parameter. If you change how this is done above, this text will need to change also. ::: template/en/default/admin/params/groupsecurity.html.tmpl @@ +44,5 @@ > > + or_groups => "To view a bug, a user only needs to be a member of one " _ > + "group the bug is restricted too. If off, a user needs to be " _ > + "a member of all groups to view a bug. Exceptions apply to " _ > + "users that have a role in a bug in either case." Better text: <p>Define the visibility of a bug which is in multiple groups. If this is on (recommended), a user only needs to be a member of one of the bug's groups in order to view it. If it is off, a user needs to be a member of all the bug's groups. Note that in either case, if the user has a role on the bug (e.g. reporter) that may also affect their permissions.</p> <p><b>Warning</b>: Changing this parameter on an existing installation may seriously affect who can view which bugs. Make sure you understand the consequences.</p> ::: template/en/default/admin/products/groupcontrol/edit.html.tmpl @@ +129,5 @@ > </p> > <p> > If any group has <b>Entry</b> selected, then this product will > +restrict [% terms.bug %] entry to only those users who are members of > +[% IF Param('or_groups') %]any[% ELSE %]all[% END %] the groups with entry This needs a leading [%+ in order to not eat the space. Currently, it renders as "members ofall the". Same for the other occurrences in this file. Also, I would say "at least one of the groups" rather than "any of the groups". @@ +135,5 @@ > </p> > <p> > If any group has <b>Canedit</b> selected, then this product > +will be read-only for any users who are not members of > +[% IF Param('or_groups') %]any[% ELSE %]all[% END %] of the groups with Same here - "one" rather than "any". @@ +138,5 @@ > +will be read-only for any users who are not members of > +[% IF Param('or_groups') %]any[% ELSE %]all[% END %] of the groups with > +Canedit selected. ONLY users who are members of > +[% IF Param('or_groups') %]any[% ELSE %]all[% END %] the canedit groups > +will be able to edit. This is an additional restriction that further The "any" case here is missing a word. I suggest using "at least one of" in the IF case. ::: template/en/default/bug/create/create.html.tmpl @@ +632,4 @@ > <td colspan="3"> > <br> > <strong> > + Only users in [% IF Param('or_groups') %]any[% ELSE %]all[% END %] of the selected groups can view this Same here: "at least one". ::: template/en/default/bug/edit.html.tmpl @@ +627,4 @@ > [% IF NOT emitted_description %] > [% emitted_description = 1 %] > <div id="bz_restrict_group_visibility_help"> > + <b>Only users in [% IF Param('or_groups') %]any[% ELSE %]all[% END %] And here.
Attachment #781498 - Flags: review?(justdave) → review-
> I grepped the source code for the two tables involved in this, bug_group_map > and group_control_map, and found some places where it's possible that you also > need to update the SQL. Could you review these sections (line numbers are for > the start of a section, not the exact line) and either add to the patch or > confirm no change is necessary in that place? > > bug_group_map: > > buglist.cgi line 819 No change needed. Determines whether group security is (always) implied or selected manually. The rules around this remain the same. > Bugzilla/Search.pm line 2823, 2899 No change needed. Both of these are used for the custom field where the field name is 'Group'. Like above, the rules around this remain the same. > Bugzilla/Bug.pm line 3615 No change needed. Determines what groups are added to a bug, not visiblity of the bug itself. > request.cgi line 121 Good spotting. Noone at brc has picked up on this. > group_control_map: > > buglist.cgi line 819 See above. > Bugzilla/Product.pm line 762 No change required. Restrictions around bug entry have not changed (i.e. you need to be in one of the specified ENTRY groups, or no entry groups for the bugs are set) > Bugzilla/Bug.pm line 3615 See above. > Bugzilla/User.pm line 831 No change required. Per product restrictions remain unchanged. > Do we have any plans to write migration code so that the visibility of bugs > does not change when switching from AND to OR? Definitely not. Existing database will have the param set to 'AND' by default, and may change it if they want to. > The other option is a scary > warning like the one I write below, perhaps combined with a migration guide. It > would perhaps be useful to have a place where users could see how many bugs > they had in their installation which are in multiple groups (and therefore > whose visibility would be affected). What do you think? Not needed. There are plenty of scary changes that can be made in the params file (see bug 941915 as an example). Given that this can be easily undone, I don't forsee the need for any special warning to be displayed. > ::: Bugzilla/Search.pm > @@ +1318,5 @@ >> + $security_term .= " (security_map.group_id IS NULL OR > security_map.group_id IN (" . $user->groups_as_string . "))"; >> + } >> + else { >> + $security_term = 'security_map.group_id IS NULL'; >> + } > > It seems like this extra SQL here "matches" the and-only SQL in the higher-up > change to this file. Can you explain why this can't be switched-in in the same > place that the other SQL is switched-out? Can we add comments linking the two? The and-group version needs to go in the WHERE clause. The or-group version needs to go in the JOIN statement. > ::: Bugzilla/User.pm > @@ +880,5 @@ >> + if (Bugzilla->params->{'or_groups'}) { >> + my $groups = $self->groups_as_string; >> + my ($cnt_can_edit, $cnt_group_member) = $dbh->selectrow_array( >> + "SELECT SUM(foo.cnt_can_edit), >> + SUM(foo.cnt_group_member) > > Please don't use "foo" as an identifier in production code. :-) I agree. (brc) svn commit logs tells us dkl added this code in Feburary 2009, so blame him :-) > @@ +882,5 @@ >> + my ($cnt_can_edit, $cnt_group_member) = $dbh->selectrow_array( >> + "SELECT SUM(foo.cnt_can_edit), >> + SUM(foo.cnt_group_member) >> + FROM (SELECT CASE WHEN canedit = 1 THEN 1 ELSE 0 END AS > cnt_can_edit, >> + CASE WHEN canedit = 1 AND group_id IN ($groups) > THEN 1 ELSE 0 END AS cnt_group_member > > Are we sure this SQL works fine across all our DBs? Same Q for all the other > SQL you've added. I'm not sure what our coding standards are here. Works in MySQL and Postgres, which are the two databases I have access to. I'm sure the SQLite or Oracle people will pipe up soon enough if we broke something. CASE-WHEN-THEN-ELSE-END is already used in other places in our code. > @@ +885,5 @@ >> + FROM (SELECT CASE WHEN canedit = 1 THEN 1 ELSE 0 END AS > cnt_can_edit, >> + CASE WHEN canedit = 1 AND group_id IN ($groups) > THEN 1 ELSE 0 END AS cnt_group_member >> + FROM group_control_map >> + WHERE product_id = $prod_id) AS foo"); >> + return ($cnt_can_edit == 0 or $cnt_group_member > 0); > > This took a bit of puzzling out; a comment might elucidate: > > # Check that either a) product is not using group-based canedit, or b) user is > in one of the necessary groups. Noted. Will fix. > @@ +886,5 @@ >> + CASE WHEN canedit = 1 AND group_id IN ($groups) > THEN 1 ELSE 0 END AS cnt_group_member >> + FROM group_control_map >> + WHERE product_id = $prod_id) AS foo"); >> + return ($cnt_can_edit == 0 or $cnt_group_member > 0); >> + } > > Can we please make this an if/else rather than an if/return continue/return, to > make it more clear that one is the or_groups case and the other is the > and_groups case? Yes. Will fix. > @@ +912,5 @@ >> + : $self->_visible_bugs_and(@_); >> +} >> + >> +sub _visible_bugs_or { >> + my ($self, $bugs) = @_; > > This shares too much code with _visible_bugs_and to be an entirely separate > function. There are only two real differences - the SQL executed, and the > decision code for deciding visibility. There's a lot of duplicated boilerplate. > Can you make the split more fine-grained? Yes. Will fix. Additionally, we discovered that the visible_bugs_or SQL was very inefficent. The next version will have completely different SQL in it. > @@ +943,5 @@ >> + $sth ||= $dbh->prepare( >> + # This checks for groups that the bug is in that the user >> + # *isn't* in. Then, in the Perl code below, we check if >> + # the user can otherwise access the bug (for example, by being >> + # the assignee or QA Contact). > > Is this cut-and-pasted comment still valid for the OR case? I think it might > well not be. Probably not :) > @@ +2586,5 @@ >> +=item C<visible_bugs($bugs)> >> + >> +Description: Determines if bugs are visible to the user. Calls > _visible_bugs_or >> + or _visible_bugs_and to do the work depending on the value of > the >> + or_groups parameter. > > If you change how this is done above, this text will need to change also. Noted. Will revert. > ::: template/en/default/admin/params/groupsecurity.html.tmpl > @@ +44,5 @@ >> >> + or_groups => "To view a bug, a user only needs to be a member of one " _ >> + "group the bug is restricted too. If off, a user needs to be " > _ >> + "a member of all groups to view a bug. Exceptions apply to " _ > >> + "users that have a role in a bug in either case." > > Better text: > > <p>Define the visibility of a bug which is in multiple groups. If this is on > (recommended), a user only needs to be a member of one of the bug's groups in > order to view it. If it is off, a user needs to be a member of all the bug's > groups. Note that in either case, if the user has a role on the bug (e.g. > reporter) that may also affect their permissions.</p> > > <p><b>Warning</b>: Changing this parameter on an existing installation may > seriously affect who can view which bugs. Make sure you understand the > consequences.</p> Changed, but without the warning. See above why I didn't add it. > ::: template/en/default/admin/products/groupcontrol/edit.html.tmpl > @@ +129,5 @@ >> </p> >> <p> >> If any group has <b>Entry</b> selected, then this product will >> +restrict [% terms.bug %] entry to only those users who are members of >> +[% IF Param('or_groups') %]any[% ELSE %]all[% END %] the groups with entry > > This needs a leading [%+ in order to not eat the space. Currently, it renders > as "members ofall the". Same for the other occurrences in this file. Noted. Will fix. > Also, I would say "at least one of the groups" rather than "any of the groups". Will fix, along with the other noted occurrences of the wording. I'll get a patch up later today (AEST) hopefully.
Attached patch v4 patch — — Splinter Review
Attachment #8348514 - Flags: review?(gerv)
Comment on attachment 8348514 [details] [diff] [review] v4 patch Review of attachment 8348514 [details] [diff] [review]: ----------------------------------------------------------------- r=gerv with these two nits fixed. Awesome :-) Gerv ::: Bugzilla/Search.pm @@ +1228,4 @@ > push(@joins, $security_join); > > if ($user->id) { > + if (!Bugzilla->params->{'or_groups'}) { Please add a comment at both this change and the one at line 1311 linking the two together as a matching pair. ::: template/en/default/bug/create/create.html.tmpl @@ +633,4 @@ > <td colspan="3"> > <br> > <strong> > + Only users in [% IF Param('or_groups') %]at least one[% ELSE %]all[% END %] of the selected groups can view this Missed a [%+ .
Attachment #8348514 - Flags: review?(gerv) → review+
Flags: approval+
Attachment #781498 - Attachment is obsolete: true
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/ modified request.cgi modified Bugzilla/Config.pm modified Bugzilla/Search.pm modified Bugzilla/User.pm modified Bugzilla/Config/GroupSecurity.pm modified template/en/default/admin/params/groupsecurity.html.tmpl modified template/en/default/admin/products/groupcontrol/edit.html.tmpl modified template/en/default/bug/edit.html.tmpl modified template/en/default/bug/create/create.html.tmpl Committed revision 8832.
Keywords: relnote
See Also: → https://bugzilla.redhat.com/show_bug.cgi?id=985631
Why is this bug left open? Unless I missed a comment, there is no other patch expected.
Comment on attachment 8348514 [details] [diff] [review] v4 patch >=== modified file 'template/en/default/admin/params/groupsecurity.html.tmpl' >+ or_groups => "Define the visibility of a $terms.bug which is in multiple " _ "a $terms.bug" must be "$terms.abug".
Flags: testcase?
LpSolit: you are right; no further patch is expected; except: Simon: I was about to close this bug, but do you want to fix comment 38 before I do? Gerv
(In reply to Gervase Markham [:gerv] from comment #39) > LpSolit: you are right; no further patch is expected; except: So I just forget to change the status. > Simon: I was about to close this bug, but do you want to fix comment 38 > before I do? done. Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/ modified template/en/default/admin/params/groupsecurity.html.tmpl Committed revision 8833.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
If anyone wants to write a migration script around this, please file a new bug (and mention this one). It would be ideal for a script in the contrib directory. Red Hat have no need for such a script, so we won't be writing it.
Blocks: 970228
Blocks: 995988
Added to relnotes for 5.0rc1.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: