Currently, bugzilla can handle a maximum of 55 user defined groups, because of the 64 bit limit of perl. If one want's to control the visibility of a large number of products, this limit is too scarce. I suggest using the perl packages "Bit::Vector", "Set::IntRange" & "Math::BigIntFast" by Stefen Beyer, which can handle bit vectors of arbitrary size and provides efficient methods for handling them. (see http://www.perl.com/CPAN-local//modules/by-authors/Steffen_Beyer/).

Schema change ... moving to BZ3.

Taking QA.

I think we may need to rethink the targetting on this one. We have a bug targetted for 2.14 (see the block list) that's going to need this done first before we can check it in. One or the other is going to need to be retargetted.

Discussing this with Hixie in IRC, and checking out his existing source for Bz3 (yes, he's really been working on it), Bz3 does this by having a table that matches groups to bugs, and another that matches users to groups. This has precendence in Bugzilla 2.x because this is exactly the way CCs and keywords are handled.

Hixie's approach is a natural solution to the problem. It is the most normative way to represent one-to-many (bugs->groups, users->groups) and many-to-many (bugs<->users) relationships in a SQL database system. It is much more flexible than the current solution and will not present performance problems if the appropriate indexes are established. I wholeheartedly support Hixie's solution. Now I guess I better go look at it. :-)

in light of discussion, retargetting. Myk's recent work on the various confidential viewing holes should keep this easy since there's a function in globals.pl now to tell you if a bug is visible to the user. Can change the group lookups there and just fix everywhere else to call that function.

(Just for the record, and to keep expectations in check: the current bz3 code doesn't actually yet have a table for bugs<->groups, only users<->groups. However, it is indeed my intent for bugs to have a table listing which groups have which rights with respect to viewing or editing the bug, with the default being that if no groups are listed, all groups have access, and if one or more groups are listed, only those groups get any rights. I believe this closely resembles how Bugzilla 2 currently behaves at a UI level.)

Just making notes here so it doesn't get lost... the existing group table can have its indexes renumbered by: UPDATE groups SET bit=(LOG(bit)/LOG(2)) + 1;

The bug this blocks has been retargeted to 2.16, so this one could now drop off the 2.14 bug list if appropriate.

at least one bug this blocks is targetted for 2.14. ian, are you going to be able to look at this any time soon, or should i get someone else to do it.

it's only on Ian because it was targeted for bugzilla 3 originally and I moved it and forgot to reassign. I got the impression either Jake or I were going to tackle it but we never decided which. I'll take it for now, probably get something done on it this week sometime.

I've actually already fixed this bug. In the 3.0 codebase. http://lxr.mozilla.org/mozilla/source/webtools/PLIF/PLIF/Service/User.pm Now if you want it in the 2.x timeframe... :-)

Hmm, something I didn't think about in implementing this... currently we define the bugzilla administrator by the user who has all the bits set in the groupset field. Since the groupset field will be going away, and we'll only have one-to- one mappings for the users->groups and bugs->groups, we need a new way to determine who the administrator is. Anyone have any ideas?

this is my current thinking.... mysql> show columns from bug_groups; +----------+--------------+------+-----+---------+-------+ | Field | Type | Null | Key | Default | Extra | +----------+--------------+------+-----+---------+-------+ | bug_id | mediumint(9) | | PRI | 0 | | | group_id | mediumint(9) | | PRI | 0 | | +----------+--------------+------+-----+---------+-------+ 2 rows in set (0.01 sec) mysql> show columns from user_groups; +----------+--------------+------+-----+---------+-------+ | Field | Type | Null | Key | Default | Extra | +----------+--------------+------+-----+---------+-------+ | user_id | mediumint(9) | | PRI | 0 | | | group_id | mediumint(9) | | PRI | 0 | | +----------+--------------+------+-----+---------+-------+ 2 rows in set (0.00 sec) mysql> show columns from user_blessgroups; +----------+--------------+------+-----+---------+-------+ | Field | Type | Null | Key | Default | Extra | +----------+--------------+------+-----+---------+-------+ | user_id | mediumint(9) | | PRI | 0 | | | group_id | mediumint(9) | | PRI | 0 | | +----------+--------------+------+-----+---------+-------+ 2 rows in set (0.01 sec) This would eliminate the "groupset" field in both the bugs and profiles tables, and the blessgroupset field in the profiles table.

I assume user_groups will be the things like CanConfirm, etc. and the bug_groups will be what is currently defined by isbuggroup. For blessgroups, you only have user_blessgroups... isn't it currently possible to allow somebody to have the bless ability on bug groups? Or will this one table cover both? As for the admin, we could probably add another User Group called Administrator.

Actually, user_groups matches users to groups, and bug_groups matches bugs to groups. The groups themselves are still the same ones defined in the groups table, with the isbuggroup flag and all. The keys will be renumbered to be sequential instead of powers of 2 though.

OK, got another thought on this, as I'm trying to get this implemented. Product groups are currently a bit of a hack. As long as I'm redefining the entire group system, now would be the perfect time to redesign product groups as well to make it actually integrate into the system more cleanly. Here's what I'm proposing: 1) product groups get moved into their own table, eliminating the concept of product groups altogether, and simply matching users to which hidden products they're allow to see 2) Bugs get a new field which is a boolean toggle which simply tells if the bug is restricted in viewing to the people who can see the product the bug is in. 3) Since we don't have the existence of groups to compare to anymore to see if a product is restricted, each product should have a flag to tell whether it can be restricted or not also. ---------------- Pros: - It's a lot cleaner. - It will eliminate most of the challenges involved in moving a product- restricted bug from one product to another (currently if you do this, the bug winds up restricted to the old product instead of the new one) - Will get rid of a lot of the bloat on the bug entry and bug change forms for people who are allowed to see a large number of products, since a product can't be restricted to a different product's group anymore Cons: - one more table to change a product name in if you change a product name - an additional place to verify product names in the sanitycheck. Any comments, complaints, suggestions?

IMO, this is a good thing. Between this and bug 68022 bugzilla's usefulness increases by quite a bit. If it is possible to restrict who can see a bug in a product while still allowing the reporter to see the bug (and allowing new bugs to be added to a restricted product) it will probably fix bug 60769, too (I think there's a couple of similar bugs in extance also, but I don't have the numbers handy). I think 5 places to sanity check/change isn't much worse than 4 so the pros outweigh the cons.

s/bug 68022/bug 39816/; In previous comment :)

In case Joe thinks he's getting out of it, I'm kind of waiting for comments from him, since he created the current groupset system. :) Thoughts Joe? (I'm doing the programming here, so don't worry about getting yourself buried :)

OK... At last, a bit of breathing room. Here's my thoughts on everything we've got here: Mapping bugs and users to groups via ids and mapping tables: Good! Administrator: Perhaps another field on the profiles table to flag administrators? Simple to implement, I'd think... Mapping table layout: Those three tables sound good to me. My one nitpick is the names. I've found that naming mapping tables with "map" in the names helps clarify it, so instead of "user_groups", how about "user_group_map"? Jake's misunderstanding is evidence that this could use a little clarification. Product permissions table: I'm not sure I understand why we need a separate table for product groups. If we're going to map users to products, and we're going to restricted viewing and entering bugs on those products based on those mappings, then that sounds like it's still a group to me. (I'm assuming we would also keep the userregexp, because we want to be able to automatically put the appropriate users into the product groups.) If we want to differentiate product groups from non-product groups (a good idea, I think, so that we don't need to have a long list of irrelevant product groups show up on bug detail pages), we could just use a flag. When you move a bug from one product to another, it doesn't seem like it's that complicated to check if it's in a product group and move that as well... I guess I'm just not seeing how separating out product groups from other groups really gains us anything here that we can't have with just a flag on the groups table. And the con I see that wasn't mentioned is that it's also one more permission check you've got to do, that works differently than the other ones. Instead of doing (check all groups that this bug is restricted to against user permissions), you'd have to do ((check all groups that this bug is restricted to aganst user permissions) and (if(the product that this bug belongs to has restricted viewing access) then (check if this user has permissions to see the bugs that are restricted to this product)). I'm not saying it's necessarily a bad idea... I'm just unconvinced at this point. That's enough for now. Aren't you glad you asked? :-)

the main reason the product groups are mapped to users separately is that there is no mapping of bugs to product groups. Simply a designation of whether the bug is restricted to being viewed by people who can see that product. Yeah, it's one more check, but it vastly improves the likelyhood of avoiding data corruption when moving a bug between products. That was my main idea. :) I could still be talked out of it probably if there's a good enough reason for it :)

The thing is, you save on potential trouble in one place (changing the product a bug is assigned against... does this really happen often? I can't imagine that we'd _ever_ do it here...), and make trouble in another place (checking the permissions for a bug, which happens all over the place all the time). I'm not sure of the real gain here. I definitely _do_ think that designating product groups specially is worthwhile, but that could be done with a flag on the groups table, perhaps. However, you're doing the coding here, so I'll leave it at your discretion. If you think we're better off treating product permissions separately from group permissions, and want to do the coding for it, go ahead. :-) One more question to consider on that point, though: If you do decide to do it that way, would you still display product permissions along with other group permissions, or would they be separated out and displayed separately? I think that having two different sections of the bug form that dealt with restricting viewing permissions might be awkward...

yeah, I see your point. Changing a product happens pretty frequently on mozilla.org... people file things against bugzilla all the time that are really either problems with their browser (gets marked invalid unless it's Mozilla, in which case the bug gets moved to the Browser product), or something that's specific to bugzilla.mozilla.org, in which case it gets moved to the mozilla.org product. But I see your point. Compared to security checks, moving between products happens infrequently. On yet another hand, having a group that's named the same as a product seems a clumsy way to tie a group to a product. I guess that's the main thing I was trying to work around. But looking at it, I think it's still better off that way to minimize database queries on security checks.

> On yet another hand, having a group that's named the same as a product seems > a clumsy way to tie a group to a product. The real problem here is that products don't have anything to key on other than the name. Ideally, there would be a product_id we could use as a key. Then you could have a product_id field in the groups table, and if product_id = 0, it's not a product group, otherwise it's a group for the indicated product. I suspect that this would be an extremely non-trivial change, though, and the payoff may not be worth the effort it would take.

This grand patch is going to touch a total of 24 files. I think it's beyond what I have time for. I'm going to try to get checksetup.pl done and post it here today or tomorrow, so we know what we're looking at schema-wise, then list all the files and ask for volunteers to take and convert individual files. Some will be easier than others. buglist.cgi will likely be the worst because of the boolean chart queries. Some of the files probably do security checks manually and can probably be replaced with UserInGroup() calls or ValidateBugID() calls.

Based on a recent post to netscape.public.mozilla.webtools by Tony Tay, I am now convinced that a new group called "administrator" which is automatically special- cased as being a member of all groups and having all privs, is the way to go. What Tony is requesting in his post is the ability to make a management group which can see all bugs but can't edit them. This would probably be best served by creating a "seebugs" group which is special-cased like "administrator" which automatically makes you a member of all *product* groups (but not all privilege groups). Said people would be given "seebugs" but not "editbugs". This is probably worthy of a separate bug report, but just noting it here to show how what we're doing will make this easier to implement later.

As noted in the patch discription, that's very incomplete, but that's what I've got so far. I may continue working on this, but it's really slow going because I don't have a lot of time right now. So if anyone wants to pick up the ball in the interest of getting 2.14 out the door, go for it.

Since obviously no one has time to work on this, I'm pulling this off 2.14 in order to get 2.14 out. I'm breaking dependencies on the bugs that need to stay on 2.14, we can fix those on the existing schema for now.

It would be useful if a new grouping mechanism were able to apply different sets of permissions to different group memberships (i.e. User X can edit bugs in the "foo" group but can only view bugs in the "bar" group).

I would like the table names and stuff to be extendible. In particular, we may want to at some stage, introduce groups of different sorts of things. Hence I suggest something like: user_groups user_group_assignments bug_user_groups

Currently with the work I have done so far I have the following tables configured. groups * Removed the bit column since no longer needed. * Added a group_id column which is auto incrementing and primary key user_group_map * Consists of two columns user_id and group_id which determines which group each member belongs to. bug_group_map * Consists of two columns bug_id and group_id which determines which groups a bug report is private to. If no entries exist for a current bug report then that bug report is public. bless_group_map * Consists of two columns user_id and group_id which determines which group a user can bless another user into if desired. profiles * I added a column to the profiles table called admin which is a boolean type column. If it is 1 then the member is an admin and cannot have their group permissions altered. Current same function as having have their previous groupset set to all ones. The user still needs to have their user_id added to every group to be able to see everything. editgroups.cgi will look for users designated as admins and add those to a new created group. Everything seems to be working as far as providing same functionality as current 2.14 code. I still need to add a section to checksetup.pl to do a conversion of current bitmasked type permissions to new style but I wanted to make sure everything else worked first. I also propose not having products as just another group and have them user their own table such as products * Add a product_id column auto incrementing and primary key product_group_map * Consists of two columns product_id and group_id which determines which group can enter bugs against which product and also determines product list from the query page. All of the above has been in place for well over a year now at Red Hat and has proven very successful for us.

>products > * Add a product_id column auto incrementing and primary key See bug 43600.

As far as I understand the product groups influence both ability to enter and default groupset, if you have usebuggroupsentry on. Is this the time to separate this into two columns, enter_group, default_group?

Which table are you referring to adding the columns enter_group and default group? product_group_map? If that is the case then I understanding you to be saying that if the product_id in product_group_map is flagged as a enter_group it would show up in the enter_bug.cgi product list, and if it is falled also as a default_group then it would show up on the query.cgi page as well as reports.cgi. Am I understanding this properly?

This is going to be a blocker for the 2.16 release

-> patch author

This now has a branch in cvs, v0.3 was just checked into the branch. To obtain the branch: cvs checkout -d bugzilla-groups -rBugzilla_Groups_Branch Bugzilla or cvs update -rBugzilla_Groups_Branch

I was discussing the selectVisible stuff with dkl last night, trying to find a fast way to map between the three tables (bugs, bug_group_map and user_group_map). However, there are actually two things here: CanSeeBug, and SelectVisible. Currently, the first uses the second, because its easier that way, but theres no need for that to be the case. [lightly code follows] CanSeeBug can be written (w/o the cc stuff) as: SELECT bug_group_map.bug_id FROM bug_group_map LEFT JOIN user_group_map ON group_id AND user_group_map.user_id = $userid WHERE bug_group_map.bug_id = $bugid AND user_group_map.group_id IS NULL LIMIT 1 If this returns any rows, then the user cannot see the bug. Its written that way to make the user-in-one-group, bug-in-two-groups stuff work. We could also write a version of this which takes multiple bugs, for use in the multiple change stuff. With cc/accessible stuff, it becomes: SELECT bugs.bug_id FROM bugs LEFT JOIN cc ON bugs.bug_id = cc.bug_id AND cc.who = $userid LEFT JOIN bug_group_map ON bugs.bug_id=bug_group_map.bug_id LEFT JOIN user_group_map ON bug_group_map.group_id = user_group_map.group_id AND user_group_map.user_id = $userid WHERE bugs.bug_id = $bugid AND user_group_map.group_id IS NULL AND (bugs.reporter_accessible = 0 OR bugs.reporter != $userid) AND (bugs.qacontact_accessible = 0 OR bugs.qa_contact != $userid) AND (bugs.assignee_accessible = 0 OR bugs.assigned_to != $userid) AND (bugs.cclist_accessible = 0 OR cc.who IS NULL) LIMIT 1 ISTR the mysql docs mentioning that ORs weren't optimised well - it may be better to apply De Morgan to change the (a OR b) into NOT (NOT A AND NOT B). We should run EXPLAIN to find out. However, you should add some indexes to the *map* tables, first. Currently, though, I get: EXPLAIN SELECT bugs.bug_id FROM bugs LEFT JOIN cc ON bugs.bug_id = cc.bug_id AND cc.who = 2 LEFT JOIN bug_group_map ON bugs.bug_id=bug_group_map.bug_id LEFT JOIN user_group_map ON bug_group_map.group_id = user_group_map.group_id AND user_group_map.user_id = 2 WHERE bugs.bug_id = 1 AND user_group_map.group_id IS NULL AND (bugs.reporter_accessible = 0 OR bugs.reporter != 2) AND (bugs.qacontact_accessible = 0 OR bugs.qa_contact != 2) AND (bugs.assignee_accessible = 0 OR bugs.assigned_to != 2) AND (bugs.cclist_accessible = 0 OR cc.who IS NULL) LIMIT 1; +-----------------------------------------------------+ | Comment | +-----------------------------------------------------+ | Impossible WHERE noticed after reading const tables | +-----------------------------------------------------+ 1 row in set (0.01 sec) (when the user is teh assignne of a bug they are not in the group for) which does suggest that mysql will optimise this away in most cases. Also, I had to SqlQuote the username is post_bug.cgi to add a comment, plus processmail is complaining about taint errors. As well, teh group stuff isn't an option when entering a new bug. So thats CanSeeBug :) SelectVisible to follow, much later.

OK, try this for select visible. According to EXPLAIN, its done w/o temporary tables (after adding indexes to the _map_ tables) We still need two queries, first this, then one with ... AND (bugs.reporter_accessible = 1 OR .... OR bugs.bug_id IN (list from first query). SELECT bugs.bug_id FROM bugs LEFT JOIN bug_group_map USING (bug_id) LEFT JOIN user_group_map ON bug_group_map.group_id = user_group_map.group_id AND user_group_map.user_id = 2 GROUP BY bugs.bug_id HAVING (COUNT(user_group_map.group_id) = COUNT(bug_group_map.group_id)); Can I avoid the COUNT?

I will give this a try and see what happens? Would the following accomplish the same thing without using the COUNT() function? SELECT bugs.bug_id FROM bugs LEFT JOIN bug_group_map USING (bug_id) LEFT JOIN user_group_map ON bug_group_map.group_id = user_group_map.group_id AND user_group_map.user_id = 2 GROUP BY bugs.bug_id HAVING (user_group_map.group_id IS NOT NULL and bug_group_map.group_id IS NOT NULL); Aren't we just checking to make sure that there is more that 0 in both tables? I am not sure if not using the COUNT() function would yield better performance for really large return sets. May have to benchmark it both ways or use EXPLAIN.

SELECT $selectedCols, bugs.bug_id SV_bugid, bugs.reporter SV_reporter, bugs.reporter_accessible SV_reporter_accessible, bugs.assigned_to SV_assigned_to, bugs.assignee_accessible SV_assignee_accessible, bugs.qa_contact SV_qa_contact, bugs.qacontact_accessible SV_qacontact_accessible, SV_cc.who, bugs.cclist_accessible SV_cclist_accessible FROM bugs LEFT JOIN bug_group_map USING (bug_id) LEFT JOIN user_group_map ON bug_group_map.group_id = user_group_map.group_id AND user_group_map.user_id = $userid LEFT JOIN cc SV_cc ON bugs.bug_id = SV_cc.bug_id AND SV_cc.who = $userid WHERE $wherecond GROUP BY bugs.bug_id HAVING (COUNT(user_group_map.group_id) = COUNT(bug_group_map.group_id)) OR (SV_reporter=$userid AND SV_reporter_accessible=1) OR (SV_assigned_to=$userid AND SV_assignee_accessible=1) OR (SV_qa_contact=$userid AND SV_qacontact_accessible=1) OR (NOT isnull(SV_cc.who) AND SV_cc.who = $userid AND SV_cclist_accessible=1); Will do this in one query. There may be problems with existing HAVING clauses, but the only one which calls selectvisible is in buglist.cgi, and that should probably be DISTINCT instead. I'm not sure how fast this will be, though.

I took a slightly different route that I would like some review on. I am commiting some new changes to the groups branch that handles the way permissions to see a bug are decided for both single bugs and lists of bugs. I modified the CanSeeBug function to no longer calling SelectVisible and instead does its own checking of the permission level of a bug. CanSeeBug can also work with a list of bug numbers and will return a hash containing bug numbers that can be seen by the member. This is useful for buglist.cgi since it need only first do it's normal query to get a list bugs based on the search criteria, feed the list of bug numbers to CanSeeBug and it will return what the user can see. Buglist.cgi then just skips the ones it cannot see. The whole process only adds two more queries to the overall page load. I have not disabled SelectVisible in all places except for CanSeeBug() and buglist.cgi so as to get feedback first. Also the edit multiple bugs feature in buglist.cgi does not work yet for group manipulation. My reason for suggesting to move the permission checking out of SelectVisible was because of performance issues with Oracle and PostgreSQL. According to the Oracle documentation when do 3 or more outer joins in a single query performance starts to degrade substantially. After importing our Oracle database into PostrgreSQL for testing with the PostgreSQL port of the current code base, PostgreSQL would choke on the 50,000 bugs we have currently. When commenting out the SelectVisible() line in buglist.cgi, PostgreSQL return the expected results (minus permission checking) almost instantaneously. And it remained very responsive with the new version of CanSeeBug and seems to perform permission checking properly. I have done a similar change in the groups branch and it also seems to perform nicely with the 50,000 bugs from our Oracle database imported into Mysql. Let me know any feelings on this change.

I discussed something with justdave last night that I would like some other feedback on. Someone mentioned that there really shouldnt be a case where someone would need to be able to bless someone else into a group unless they are are also a member of that group or have 'editusers' privs. If this is the case I can just drop the separate bless_group_map table altogether and add another column to the user_group_map table to track bless information. user_id | group_id | bless ------------------------------- 6 | 10 | 1 The bless column would be a boolean type and if 1 would mean that person can bless others into that group, 0 they cannot. This would only be settable by persons with 'editusers' privs. The only catch would be what was discussed earlier, that a person would need to be a member of a group to also be able to bless others. If everyone thinks this should always be the case I will make the changes.

In bugzilla3, there is a similar map except the "bless" column is a "rank" column and can be one of: non-member (0 or row not there) do not have any rights related to this group member (1) have the rights that all members get. op (2) are allowed to promote non-members or demote members. admin (3) are allowed to change anyone's level within this group. edit-users privs is equivalent to level 2. With this system, however, when a group's administrator leaves the project he does not have to contact the bugzilla administrator to change who can make other people into ops. (This is actually already implemented, although there is no UI for it yet.)

To step back from the technical issues and look at the people activities that this is supposed to support: there are several different team structures that Bugzilla ought to support. One of these is a central Bugzilla administration person and then a designated member of each team who is responsible for administering that team's bugs, users etc. So Bugzilla ought to be able to support this by creating a group for each team and then assigning bugs and users to groups. The motivation may be security (a team is only allowed to see/change its own bugs) or it may be simplification (a team may want to see just its own bugs). In some organisations, a bug may move between teams. For example, one team might be responsible for a set of libraries and another team may be using those libraries to build systems. A bug could start off being assigned to an end system product and then after investigation be reassigned to a library (and so to another team and another group). In other organisations, each team is effectively working in isolation and bugs rarely move across teams. Bugzilla must support both of these structures. As a team may have many (m) members and many products (n), and any member of the team may be called on to work on any of the team's products, it would be advisable to allow the team members and the products to be connected to the group (m+n operations), rather than having to connect each team member to each product (m*n operations).

> Someone mentioned that there really shouldnt be a case where > someone would need to be able to bless someone else into a group unless they > are also a member of that group or have 'editusers' privs That was me, I think. I can't think of a reason, and hixie's solution is sort of similar to what you propose (although his goes further)

>edit-users privs is equivalent to level 2. With this system, however, when a >group's administrator leaves the project he does not have to contact the >bugzilla administrator to change who can make other people into ops. >(This is actually already implemented, although there is no UI for it yet.) This is a good idea. So basically with the current scheme I propose we have the following values for the bless column in the user_group_map table: 0 = Person is a normal member and cannot alter others privs for that group. 1 = Person can place another into the particular group but not alter bless privs for that person. 2 = Person can place another person into the particular group and can also give that person the ability to place others in to that group. Value 2 would allow as you describe for someone to pass on administrator status for a group to someone else.

CCing Asa Dotzler and Mitchell Stoltz, two of the primary users of groups at b.m.o.'s installation of Bugzilla. Asa and Mitchell: read comment 48 and following comments and let us know if these changes meet your needs for the security group and other groups at b.m.o.

I can't think of a case for b.m.o where I would want someone who is not a part of a group to be able to add people to that group. To put it another way, I have no problem with a Bugzilla requirement that a person be a part of a group in order to add other people to that group. If I've misunderstood this please correct me.

Sounds like you understood it fine, at least you understood it the way I also understand it ;) Unless Mitchell Stoltz or anyone else has any objection to implementing it this way for the time being, I will go ahead and make the changes and submit it for review.

Myk, The change mentioned in comment 48 is fine with me as far as the security group in b.m.o is concerned.

Is the latest version of this in CVS? The version in cvs is reasonably broken in lots of ways (people not logged in can see secure bugs if there is no qa contact, the permissoin checkboxes aren't showing up correctly (or at all on enter_bug), Bug.pm gives bogus results if the bug# is invalid, processmail gives taint errors, etc)

OK, I think part of the problem is that the merging between trunk and branch has't worked properly all the time. For example, acording to bonsai the branch was last synced yesterday, but changes form last week like upping the version of TemplateToolkit aren't there. I'm hacking on this now, and will attach a patch later this evening.

OK, I got up to edit*. (I did have to hack on bits of globals.pl, but I haven't looked at that yet in detail.) The diff is -w for ease-of-reading. Most of the stuff is simple to follow. What are you doing with product groups? checksetup has an table in there, but its unused. I hacked the existing stuff in - see the code. Comments on the table schema: groups doesn't need an index on group_id, since its the primary key, which implies an index. user_group_map should have a unique(user_id,group_id) index, instead of the single one for user_id. Similarly for bug_group_map. As I mentioned above, I'm not sure what product_group_map is for.

OK, heres v 2. I'm now gone through everything except edit*, which dkl is going to redo anyway in order to add the admin stuff as discussed on IRC, + the product group stuff. Misc comments: - I've tested this very very briefly; some changes havne't been tested at all - Group changes don't appear on the activity log - Can't search for bugs in group x - Note the locks I added to post_bug.cgi - I definately haven't tested the multiple bugs stuff. - The word groupset no longer appears outside of edit* and checksetup.pl - I'm wondering if this should be pushed off beyond 2.16 Anyone else?

moving this out of 2.16. We're way late already and I wanna get 2.16 out the door. This is major enough of a change that it shouldn't be going in this close to the end of a release cycle anyway. We'll put it in one of the first things during the 2.18 cycle.

In the current security implementation, it is possible to add additional "action" (for want of a better word) groups like "canedit" and "canconfirm" and customize Bugzilla to conditionally permit the user to take certain actions based on their membership in those groups. How does this re-implementation take this kind of customization into account? Is it easier to do this, more difficult, or the same?

You mean by chaning the source code? Adding a group is no different to before. You'd just create an admin group "canfoo", and then add UserInGroup checks at the apropriate places. there isn't any ui to create an admin gruop, but ther isn't any ui currently, so... The only front end change is that since we have to remove the blessgroupset stuff, membership in a group is now at four levels - -1. Not a member 0. Member. 1. Can add others to the group and 2. can add anyone to level 1. (-1 isn't a real level, it corresponded to no entry in the table) This means that it will not be possible for someone to be in the blessgroupset without being in the group, which is currently possible, although silly, since they could just add themselves. buggroups (aka porduct groups) have also been changed to be more fine grained via another table - you can now use the same group (or groups) for multiple bugs. That code still needs a bit of work, though. isadmin is still a separate flag on the profiles table, although it could (as part of a separate bug) just become a group whose members are automatically added to level 2 of every new group.

In regardless to the usegroupsentry stuff I was talking about, I'm not sure how to represent this in the current schema. Originally I was thinking we would continue to give products a group rather than a group set. That's fine I suppose, but we need to figure out a way to not regress the usegroupsentry option. I guess the options that come to mind are another table, or another field on product_group_map. We want to be able to leave products open, we want to be able to give products a groupset but allow anyone to report on them, and we want to be able to give products a groupset and prevent certain people from reporting on them (or seeing them). Do we want to be able to prevent them reporting against a product, but still see the bugs? If entry and viewing are totally independent, perhaps a three valued field is the order of the day, or checkboxes where one must be on. The fourth value/all checkboxes off situation is where there would be no record.

Another question is whether the entry groupset will ever differ from the default groupset.

IMHO the important thing is to be able to put more than one product in a group.

The code on the branch has a table which is a map. It should have a flag for requried vs default on vs default off. Using this, the buggroup sentry stuff vanishes, and becomes more flexable. However, I've proposed to dkl that this stuff be removed, and left for a future bug. This is partly because he creates product ids (which is covered by another bug), and partly because its not needed for this, and is just going to make it harder to review, and land.

OK, so is the proposal here to get the groups extension stuff in, and continue to locate product groups by name, and leave the rest for a separate bug? That sounds like an appropriate course of action to me. I definitely want one group against several products for 2.18, but that can wait for another bug. The only qualm is if it makes checksetup.pl more complicated since it is two schema updates and we need to be able to handle the situation where only the first has occurred as well as before both and after both.

Well, thats my plan, but I'm not writing the code... The schema change isn't a problem - they happen sequentially, just like they happen now. The latter one will have to check the values of some paramaters (eg "usebuggroups"), though, to determine what to put into the new column, but thast ok.

OK, what's the status on this? 2.16 is going to be out soon, and I get the feeling this bug is in general blocking a lot more bugs than actually got set as dependencies on it.

Err, 2.16? This isn't going to be in 2.16. Actually, the code as designed would be a lot easier with bug 43600 fixed. That isn't a blocker for this, though, because its just an 'extra' step which fixes bug 147275. I think we need to pull features out of the groups branch, though, and just get the basics working

Not sure where to put bugs against a branch that isn't merged in yet, so I'm putting this here..... (JustDave: should we have a version for this branch??) New account creation in Bugzilla_Groups_Branch ... Software error: INSERT INTO user_group_map VALUES (2, 6): Column count doesn't match value count at row 1 at globals.pl line 271. Seems to be caused by foreach my $groupid (@groups) { SendSQL("INSERT INTO user_group_map VALUES ($userid, $groupid)"); } when the user_group_map has 3 values. I noticed in checksetup.pl also that the INSERT/UPDATE commands all use the implicit knowledge of what the fields are. I suspect that this could make merges more difficult in the future if fields are added. I'd suggest "INSERT INTO user_group_map ( user_id, group_id ) VALUES ($userid, $groupid"

Yeah, the ocde on the branch is partial. I'm considering starting from scratch, pulling only particular bits from the branch.

Brad, Dave, (( I never know if I should have these sidebars in email, newsgroup, IRC, or on a bug -- let me know if you have a preference)) Several of the things I need to do are starting to coalesce around bug 147275 I am going to need to start work on something that essentially means that, in addition to users having groupsets and blessgroupsets, products will have a number of groupsets (default, required, permitted, editors). I am approaching a decision point where I have to decide if I am going to do this with old bit-math groupsets or new ones. My suspicion is that I will have to do this now with the old groupsets but be prepared to migrate to the new ones. There are several aspects of the new groups that could make my life easier. The same way as editusers has to show 2 columns of checkboxes with the blessgroup and the memberofgroup, I will need to have several columns for each product. (default, required, permitted, editors). If you could make the group_map table... tinyint idtype (0 if id is a user, 1 if id is a product) mediumint id (user_id or product_id - depending on idtype) group_id canwhat ( 0 = canbeamember, 1 = canbless, 2 = isdefault, 3= isrequired, 4 = ispermitted, 5 = iseditors ...) This also leaves the door open to another idtype ( 2 => id is a group) permitting groups to be eventually defined in terms of other groups. [ essential if I start adding "roles" where certain reviews can only be done by certain groups of people ] Naturally, this also would imlply that there could be quite a bit of common code between the editproducts and editusers scripts and templates.

The code on the branch goes half way towards adding a product_group_map table. It uses ids for products, but doesn't do it properly - the correct and full fix is in bug 43600.

In the current tip of CVS, usebuggroups does not result in the creation of new groups when a new product is created. It may not be necessary to fix this if bug 147275 is going to rework the whole product groups issue anyway.

In the tip of Bugzilla_PgSQL_Groups_Branch ( globals.pl 1.170.2.2 ) line 139 uses the variable, "$driver" for the database type. checksetup.pl and pgsetup.pl both define a variable $db_driver in localconfig. This means that the DBI->connect will fail if using a mysql database. This should probably be changed to $db_driver in globals.pl

In the tip of Bugzilla_PgSQL_Groups_Branch, checksetup.pl (1.152.2.1) will fail if the database being converted happens not to have group records in order at EVERY power of 2. This seems to come from the code below which first causes MySQL to assign numbers to the newly added group_id column and then updates them. It looks like this code could simply come out (at least for MySQL). Otherwise, it should either order by group_id then by bit or only do the update if the group_id were not already set. if (&GetFieldDef('groups', 'bit')) { &AddField('groups', 'group_id', 'mediumint primary key auto_increment not nu ll'); &AddField('profiles', 'admin', 'smallint default 0'); my $currentgroupid = 1; my $query = "select bit from groups order by bit"; my $sth = $dbh->prepare($query); $sth->execute(); while (my ($bit) = $sth->fetchrow_array()) { my $query = "update groups set group_id = $currentgroupid where bit = $b it"; my $sth2 = $dbh->prepare($query); $sth2->execute(); $currentgroupid++; }

Patch removing conflicting operation described in comment 79

A new branch implementing this feature is ready for testing on non-production databases and copies of production databases. It is being tracked under bug 157756. Please note test experiences in that bug's comments. There are some hints for copying production databases to test databases in that bug.

I've taken the question of what to do with the 2 main bugs under advisement with dkl and we are leaning towards closing this bug 68022 as a duplicate of bug 157756. If anyone thinks that is the wrong thing to do, please speak up now. Bug 157756 is ready for some serious testing, particularly by people with a large and complex group list and especially by people who move bugs. Naturally, you should not test it on your live database. There are some hints in one of the early comments on how to make a database copy for testing.

Comment on attachment 90772 [details] [diff] [review] Fix for new account fatal error Way outdated -- see bug 157756

Comment on attachment 90900 [details] [diff] [review] Patch to Bugzilla_PgSQL_Groups_Branch checksetup.pl This is way-obsolete

Comment on attachment 90900 [details] [diff] [review] Patch to Bugzilla_PgSQL_Groups_Branch checksetup.pl This is way-obsolete

Finally marking as a dup, per a discussion with joel on IRC. *** This bug has been marked as a duplicate of 157756 ***

sorry, JayPee, we've discussed that in the past, this bug is advertised to the world all over the place, not to mention has a block list like you wouldn't believe which has just gotten confused by being closed as a duplicate. This stays open until it's checked in, then it gets closed as FIXED.

Joel is the one actually working on this at the moment

(Now it's really fixed!!) This has been checked in to HEAD under bug 157756, reoloving this item. There is one known regression that will be tracked under a seperate bug where buglist does not highlight bugs properly. Due to the size of this schema change, I cannot stress enough the importance of making backup snapshots of your database before updating from HEAD.

Could We extend this to include the values inside the fields? Say i want only one group to submit bugs with critical status. Is more that possible with these changes in place?