Closed Bug 68022 Opened 24 years ago Closed 22 years ago

Need more than 55 groups

Categories

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

Product:
Bugzilla
Component:
Bugzilla-General
Version:
2.10
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.18

People

(Reporter: anowak, Assigned: bugreport)

References

(Blocks 1 open bug)

Details

(Whiteboard: [blocker will fix])

Attachments

(5 files, 3 obsolete files)

Take 1 on checksetup.pl - very incomplete
5.87 KB, patch
justdave
: review-
Details | Diff | Splinter Review
First pass at groups schema changes v0.1
106.82 KB, patch
Details | Diff | Splinter Review
Second pass group schema changes (v0.2)
116.49 KB, patch
Details | Diff | Splinter Review
groups schema v0.3 (v0.2 with current cvs conflicts resolved)
12 bytes, patch
Details | Diff | Splinter Review
wip diff -w, take 2
48.34 KB, patch
Details | Diff | Splinter Review 
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.
Assignee: tara → ian
Component: Bugzilla → Bugzilla 3
QA Contact: matty → ian
Target Milestone: --- → Bugzilla 3.0
Taking QA.
QA Contact: ian → matty
Blocks: 49808
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. :-)
Component: Bugzilla 3 → Bugzilla
Target Milestone: Bugzilla 3.0 → Bugzilla 2.14
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.
Blocks: 39816
Blocks: 43619
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.
Assignee: ian → justdave
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... :-)
Whiteboard: code
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.
Blocks: 66235
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.
No longer blocks: 66235
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.
Keywords: helpwanted
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.
No longer blocks: 39816, 43619
Target Milestone: Bugzilla 2.14 → Bugzilla 2.16
Blocks: 90619
No longer blocks: 90619
Blocks: 43619
Blocks: 91761
No longer blocks: 91761
Priority: -- → P1
Blocks: 91761
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → 2.10
Attachment #41207 - Flags: review-
Blocks: 61343
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.
Blocks: 104690
>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?
Blocks: 106884
No longer blocks: 106884
This is going to be a blocker for the 2.16 release
Severity: enhancement → blocker
-> patch author
Assignee: justdave → dkl
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
Blocks: 107718
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.
Blocks: 97471
Depends on: 120980
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?
Attachment #65992 - Attachment is obsolete: true
Keywords: helpwanted → patch, review
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.
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
No longer blocks: 97471
Blocks: 114696
Blocks: 125492
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.
Blocks: 147275
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.
Attached patch Fix for new account fatal error (obsolete) — — Splinter Review 

    
    

    
  
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

    
  
Depends on: 157040

    
    

    
  
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.




    
  
Depends on: 157756

    
    

    
  
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.



    
  
No longer depends on: 157040

    
    

    
  
Comment on attachment 90772 [details] [diff] [review]
Fix for new account fatal error

Way outdated -- see bug 157756

        Attachment #90772 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 90900 [details] [diff] [review]
Patch to Bugzilla_PgSQL_Groups_Branch checksetup.pl


This is way-obsolete

        Attachment #90900 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 90900 [details] [diff] [review]
Patch to Bugzilla_PgSQL_Groups_Branch checksetup.pl


This is way-obsolete

    
  
Keywords: review

    
    

    
  
Finally marking as a dup, per a discussion with joel on IRC.


*** This bug has been marked as a duplicate of 157756 ***
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → DUPLICATE

    
    

    
  
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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Whiteboard: code → [blocker will fix]

    
    

    
  
Joel is the one actually working on this at the moment
Assignee: dkl → bugreport
Status: REOPENED → NEW

    
    

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


Status: NEW → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
No longer depends on: 120980

    
    

    
  
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?
QA Contact: matty_is_a_geek → default-qa

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: