This bug will be used to track issues on the Groups_20020716_Branch implementing a new groups system that should address the items in bug 68022

Implementation note: To properly handle administrators, a group called "admin" will, by default, be able to bless all groups except for admin itself. Admin itself will only be blessable by users who have been explicitly authorized to bless admin itself. Checksetup will automatically give both membership in the admin group and the ability to bless the admin group to the administrator specified when running checksetup or any administrators that it finds in the database being converted.

The Groups branch is now ready for initial testing. It should be able to take A COPY of your original database and convert it to the new schema or start a new database. If you would like to try it, I suggest the following... (from memory - adjust as needed. Skip loading the test database if you want to try this starting from an empty database. If you want to go back to square 1, you can always drop testdb, create testdb, and load it again. 1) mysqldump -u dbuser -p --opt -O max_allowed_packet=16M -O net_buffer_length=16M production_database > database.sql and provide your password when prompted 2) In mysql... grant ALL on testdb.* to dbuser@localhost; flush privileges; create database testdb; use testdb; source (originaldir)/database.sql 3) In a new directory..... cvs -d:pserver:anonymous@cvs-mirror.mozilla.org:/cvsroot login (hit enter) cvs -z9 -d:pserver:anonymous@cvs-mirror.mozilla.org:/cvsroot co -rGroups_20020716_Branch -d newgroups Bugzilla cd to the new directory.... 4) Run checksetup.pl (yes, I know you get a warning) 5) Edit localconfig to point to your database 6) Run checksetup.pl again 7) Point your webserver at the new installation. 8) Try it. 9) If you find unexpected bugs, file them and mark them as blocking this bug (bug 157756) Known problems: It is possible for the administrator, with editusers privilege, to take away his own privileges. When deleting groups, some of the warnings sound a bit ominous. queryhelp.cgi has not been converted yet. (volunteers?)

Hmm, filing a separate bug would cause an unreasonable amount of spam for this sort of trivial thing, so... Nit: checksetup.pl has an Addgroup line with the description of "Adminstrators". Make that "Administrators". :-)

These are the diffs from the tip of the branch to the base

I'll play with this later; I've just got back home and have been up for 36 hours streight, and I have to go into uni this afternoon.

Comment on attachment 92951 [details] [diff] [review] Changes to enable > 55 groups Way stale ... cvs is getting mature

The tip of Groups_20020716_Stage3_Branch now has the stage 3 implementation working well enough that I do not easily locate the next issue. A lot of testing is, no doubt, needed. Major changes: * No limit to the number of groups * Groups can bless groups * Group "Admin" can bless all groups by default, but no group can bless itself * Groups can contain other groups * The 5-level product-group permission system is implemented * BZ no longer asks if a bug should be shifted from one group to another when the product is changed, but it does enforce the new required/permitted groups. * Each product can speciy if edit permission is needed to comment or if all can edit. Remaining Known issues: queryhelp.cgi has not been touched, but needs to be. It is still possible for an administrator to lock another administrator out. When deleting groups, there is a warning about remaining users even if there are none left.

General comments, as partly discussed on irc: - Rather than use some map type hashes, |use constant| instead (the values won't change) Do this in a pm file if that won't work from globals.pl, although it should. (Constants.pm for now, and we'll discuss creating a Bugzilla dir for .pm's later) This will involve giving them slightly more informative names, but we can cope with that :) - As I mentioned, I don't like the DeriveGroup stuff. editgroups.cgi should handle updating everyone it needs to. The reason for this is that if I take someoen out of a group, then add a comment to a bug which they used to be able to see but now can't, they shouldn't get mail. (Hacking processmail for this is a workarround, not a fix...) - You can move DeriveGroup into sanitycheck.cgi though, to handle those problems - The alternative is to walk the group tree manually in UserInGroup, each time (possibly caching it so you only do so once per user per script). I'll have to look at more of the patch to work out which is the best way to do it. - schema: - groups table doesn't need unique(group_id) - its implied by the primary key - group_id should just be id, to match other uses (and I think that the admin rerwite uses that to make code more generic) - You've added a product_id field to the products table. Don't do that, review bug 43600 instead, which will make stuff easier for you anyway - As I mentioned, I really don't like all teh refreshed timestamping - member_group_map: I think this shoild be two tables, one to map users to groups, and the other to map groups to parent/child groups. That makes it column which is only used once. You'll also be able to lose almost all of the conditionals on the type in the code, which will make it cleaner. - Rather than a type for blessing, just add a column for canbless. Blessing implies membership, really. - The indexes on that tabel are odd, too - group_control_map: You don't need to prefix field names with ctl_ - Add the $table defintions with all the rest at the top of checksetup - don't rerequire data/params. Its been done above. Also, you may be checking values after they've been removed from data/params; I don't know if they're removed from the hash, too. - UserCanActOnProduct: Don't use an ORDER BY then just grab the top row, use an extra WHERE clause - Don't use mysql-specific features like INSERT INGORE - DeriveGroup doesn't LOCK, and its clalers don't, so there is a race condition where a user could get access to something they couldn't (since you DELETE) - CanSeeBug needs to check that the user is logged in, because otherwise a system without qa contacts enabled will have a userid of 0 for the qa_contact, and an unlogged in user will be able to see secure bugs. That should kepp you busy for a while :)

Marking this as blocking bug 68022. Following the tree, that makes a pretty broad set of things that ultimately depend on bug 43600.

"Default Restrictions: When a new bug is created in this product, it will default to being visible only to users who are members of ALL of the groups specified even if the user creating the bug is not a member of one or more of those groups." ITs actually optional if you're in the group, and it just happens anyway. Maybe forced groups should be displayed, just without a checkbox, similar to how show_bug does it. Also, the conversion code needs to make all non-product groups permitted for all products., and the product groups permitted+defaulted for that particular product. I'm also wondering if you need a per-group switch for defaults for new products and/or a 'change all products' thing, like an indeterminate (tri-option) checkbox on the product page. Browsers don't support those, though, so you'll need another ui.

Fram a few of bbatetz's comments and a bit of early stuff from MattyT, Sounds like the following changes need to go in.... If buggroups is not enabled, checksetup should make all groups permitted by default for each product. However, checksetup should warn/confirm before doing this if data/params was not present when checksetup was started. Instead of 5 checkboxes and bits (Entry, Default, Required, Permitted, Canedit), Default, Required, and Permitted should be combined into a one-of selection from {'Off','DefaultOff','DefaultOn','On'} and a seperate Entry and Canedit bit. In the group record, there will be a field indicating if all existing and new products should have that group 'off' or 'defaultoff' by default. New groups will apply this to existing prodects when the group is created. Group membership rederivation should be incremental rather than a full delete-the-old/add-the-new job and should be revisited whenever the appropriate fields (like regexp) in the group change OR when the user's email changes. Forced groups will be displayed without a checkbox, though it will be easy for sites that do not want this to remove them from the template. For constants, I am waiting for something resembling a consensus. One concern about use constant is that it forces a lot of code to have WHERE field = " . CONSTANT_NAME . "AND other stuff where the ideal mechanism would be more like WHERE field = $constant_name AND other stuff Granted, though, hashes are at least as clunky That, with the schema and style comments noted in comment 9 will keep me busy for a bit just as soon as bug 43600 is comitted.

You don't need the constants, anyway. You should split out the maps into separate tables. The table columns will be different anyway - it doesn't make sens to combine them.

Arghh SQL regexps turn out not to behave at all consistantly with perl regexps, so this makes the update of the whole user list from editgroups back into a problem. So, I am adding the table locks to protect against users editing bugs while momentarily without their privileges. To address the processmail question, it actually only requires a single "SELECT MAX(groups_when) FROM groups" to verify that an entire list of users is current, so long as we have to fetch something (like their email address) from profiles anyway. There has been mixed feedback on the issue of bless privilege implying group membership. I am leaving this as it stands until some sort of consensus starts to emerge. That said, this has now incorporated the latest version of bug 43600 into the pre-stage3 portion, so hopefully that will change very little before it goes in.

Well, we could just use SQL regexps. I'd guess that most people have |.*\.foo\.com| as the regexp. OTOH, that causes a conversion issue.

These are the diffs that would be applied AFTER 43600 is merged in incorporating all of the products of the negotiation with bbaetz on stage 1 & 2. The full code is available on the Groups_20020716_Branch. This diff was producted by taking Groups_20020716_Sync, merging 43600, then diffing the result with Groups_20020716_Branch.

I'd like to remind you that there are no "SQL regexps". There may be "MySQL regexps" and other miscellaneous variants, but building functionality that counts on them only makes it harder to make Bugzilla cross-db-compliant. So please carefully consider the issue before using them.

sub CanEditBug { my ($id, $userid) = @_; PushGlobalSQLState(); SendSQL("SELECT product FROM bugs WHERE bug_id = $id"); my ($product) = FetchSQLData(); my $ret = UserCanActOnProduct($product, 'canedit'); PopGlobalSQLState(); return $ret; } This sub in globals.pl has $userid being passed in but not used. UserCanActOnProduct() should take the argument of $userid instead of using $::userid which we should avoid if possible for all new code. Otherwise I like the changes that you have made and is looking good.

WRT comment 19: I'll check on that. Also: Feedback on the question of admins automatically being members of any group they can bless seems to want automatic membership. This will be done, but kept contained for easy customization so that sites that don't want it (like my own!) can change it.

The following seems to appear every time I run checksetup: "Searching for duplicate entries in the profiles table ... OK, changing index type to prevent duplicates in the future ..." Your post to the newsgroup saying "please test my branch" might have added a note that people should use a fresh DB. This seems obvious in hindsight, but I just downloaded the branch, ran checksetup, and now none of my other Bugzilla installs will work :-( UI -- It is inconsistent that you edit some aspects of a group on the editgroups.cgi page, and have to click an Edit link to edit other aspects. editgroups.cgi should present a non-editable summary of the groups, and the links should provide the full edit interface. You don't need to describe the name or description fields. The Active flag is known to us as Active, but should be known to users as "Accepts New Bugs" or something more explanatory. "User Regexp" should become "Automatically add new users<br>matching regexp:". editgroups.cgi?action=changeform: Text: "Users become members of this group in one of three ways: - by being explicitly included - by matching this regexp: <regexp> - by being a member of one of the groups whose boxes are checked below." Then, have a table with proper columns, like the current email prefs. There are three columns, as follows: 1) Members of any of the following groups: (list group names and descriptions in this column) 2) "Can grant membership of<br> the <groupname> group" (checkboxes) 3) "Are automatically members of<br> the <groupname> group" (checkboxes) I can't find where you say which groups can file a bug into this group. That would be a fourth column in the table - "Can file bugs into <br>the <groupname> group". Attempting to switch a group from "bug" to "user" gives: Can't use an undefined value as a HASH reference at globals.pl line 1238. I can't see any difference between user groups and bug groups. What is it? When I go to enter a bug, or even edit one, it doesn't give me any groups to put it in, even though I'm a member of several. Right. I'm off to restore my DB from backup. Gerv

OK, comments (Again) on the schema: groups table - 'id', not 'group_id', so that common code will work when edit* is redone There should be a unique index, probably over (user_id, group_id, isderived) group_group_map: change the index to unique(child_id, parent_id) Once again, I'm not happy with the flattening thing. Instead of using regexps, maybe we can use sql LIKE notation? Thats all people really use it for, right? Conversion code can auto-convert nothing, '.*', and '.*@<domain>', and ask the admin. Alternately, we could use the posix regexp stuff - mysql supports it as an extention, and its in SQL99 (and postgres) as the SIMILAR TO operator. See http://developer.postgresql.org/docs/postgres/functions-matching.html#FUNCTIONS-REGEXP) On upgrade, simply check each user against each regexp with sql and perl, and see if any match in one but not the other. If so, advise the admin to change the regexp. I seriously doubt that anyone is doing zero-length assertions or stuff like that. At least, I hope not.

Bug 162311 filed on the regexp thing. joel: How does your scheme handle the case where I want someone in the regexp to _not_ have the permissions for whatever reason?

WRT comment 23, currently the proposed groups system allows any group to include members who DO match a regexp but, like the groupset system, do not have a faciliy to use a regexp to exclude privileges. Under the proposed system, it would be necessary to write a regexp that includes the appropriate users and avoids including the users to be excluded. Is there a particular scenario that should be used as an example of the need to do this?

Groups_20020716_Branch has been updated to incorporate today's HEAD (including backing out the old patch for 43600 and including what is checked in). This may have introduced new bugs, but seems to work suprisingly well. Next, updates from Gerv and Bbaetz feedback (where we do have consensus) will roll in and then evolve to the more contraversial portions. The stage3 branch will bitrot a bit while this happens.

Not sure, but the old code could do it...

Then I have misunderstood something. How does one set a regexp under the current system to deny access to users matching the expression? I thought the regexp would have to be set to not match users who are supposed to be denied access to the group. (just like the proposed system)

The checksetup.pl conversion code has the bug where if you update from an old version without one of the system groups, creation will fail, since the schema will still be the 'old' stuff. As we discussed a while ago, you need to reorder that so that it happens later.

Here it is..... the end of groupsets. The question of forcing blessers to be a member of all groups they can bless is still open, but there are only 2 lines of code that change if we decide to change this. Currently, it is possible for someone to have bless privs without have access themselves. For my own site, that is the right thing because it keeps admins from being flooded with unwanted options. There is a signigicant schema change that should convert old DBs properly, but I strongly suggest using a Db that can easily be rolled back for testing.

OK, usability comments, based on whats in cvs: - userprefs.cgi missed one group_id->id change (the perms panel) After that, you should sort by name, not internal id. you also need to select DISTINCT, else you get double entries. - editusers is also missing that in a few places, and it also needs to add some DISTINCT stuff in there. The UI for this is also broken - I add myself to a group, but then it doesn't update. - The 'admins' system group should be a member of all groups, as well as have bless ability. I know that you don't want that for your site, but you could create a separate group which doesn't have the membership. - The editgroups page's usability sucks. Don't worry about this too much, since the latter tages of this stuff are going to rewrite it anyway. However, you should add in text so that the user can, when editing a group, see what group that group is a member of (you only show the other way) - The user regexp example should really use '^[^@]+@example\.com$' as its example regexp. Mention somehwere that its a perl regexp. - The isbuggroup stuff is really unclear; esp re the obsolete stuff which is currently there. - the 'Updating group hierarchy' page makes no sense - it should be at least pritning names not numbers, and I'm not even sure what its doing. You should also change checksetup so that 'groupset' isn't added to the fielddefs table - if it was there, it should remain, but theres no point for new installations. Actually using groupset gives an SQL error - you should instead have any groupset query which is _not_ a changed* one give a ThrowUserError page. Searching on groups is totally busted, too. (wrong fieldname constraints) Plus, if you need to know the group name (not desc) to do searching, it should be displayed to the user somewhere (userprefs, perhaps?)

Comment on attachment 95267 [details] [diff] [review] v1 - Patch implementing stage 1/2 That should be enough to get you started :) I'll start looking trhrough the code now

Tip of Branch in CVS now has fixes from comment 30 with the exception of ... fixing the query-by-group which I will hit after the next sync The regexp example doesn't really need to validate a correct email address. It is choosing among already-validated addresses, so the simpler form may be preferrd (opinions??) Rather than hard-coding admins to be members of everything, the admin group is made a default member of all new groups. This way, it can be subsequently cleared by sites that wish to and avoids the need for customization.

OK... query is fixed also. Checksetup now updates bugs_activity to the new schema as well. This ignites a broader debate about maintaining updates in bugs_acitivity when products, components, users, versions, groups, etc... are renamed.

Nope, you can't change past bug_activity values, because groups can be removed, then recreated. Just leave the groupset in there for the historical stuff.

OK, heres the review comments, done w/o testing. The edit* stuff didn't get much review; I'll have to go over that again.

represents tip of branch

Responses to review items (confirmations and further questions) inline with original review comments.

On the isbuggroup/isactive distinction.... Currently, these 2 bits represent 3 a total of 3 states, but there is no use for an inactive group. In stage2, we do use inactive groups as something other than an alternatove to deletion. I am reluctant to just rename the fields in the schema and all the things that are named after the schema bacause the use of these shifts again in stage3. I'd suggest we leave the fields named as they are for the time being.

Comments on latest cvs stuff: - you need some way of pointing out on the bug_activity page that converted groups may not be accurate. The cc corruption stuff was done with a prepended ?, which will caue historical search problems. Hmm - xml.cgi doesn't identify the groups (remember to change bugzilla.dtd too!) - please file a separate bug on reordering checksetup.pl to incorporate the needed changes we discussed. - in checksetup.pl, don't AddFDef the bug_group between the reporter and cc accessible bits (the order there affects the order shown on the query.cgi screen). - In the bug 157704 workarround, print a LARGE MESSAGE if there is anyone you are matching because of that, to notify the admin. You may want to only do that if such a group does exist, but it may not be worth it. Err on teh side of warning rather than not, though - You removed AddField calls for groupset/blessgroupset. Ideally, those should be created if the bug_group_table doesn't exist, for consitency. Ntohing in the middle uses that, though, so you can probably jsut comment them out (with explanitory text). Don't deltee the lines entirely, though - In editusers, if I inherit bless perms but not membership, then the row doesn't have a grey background. Is this by design? - I can't seem to be able to get results from historical bug group queries (changed by, in particular) Apart from that, its looking good....

Oh, and heres a major bug - I can log in as any username I want :) quietly_check_login does: if (@row = FetchSQLData()) { ($userid, my $loginname, my $ok, my $disabledtext) = (@row); if ($ok) { ... } } but if its not $ok, then $userid has already been reset to a non-zero number from the sql query... move the $ok part into the WHERE condition - don't do what the db can do for you.

WRT comment 40: Fixed in HEAD under bug 164623. Will pick up on merge with HEAD. >> you need some way of pointing out on the bug_activity page that converted >>groups may not be accurate. The cc corruption stuff was done with a prepended ?, >>which will caue historical search problems. Hmm Actually, this has no choice but to be accurate. I will add code to handle cases where a groupset change references a group that no longer exists. >>- xml.cgi doesn't identify the groups (remember to change bugzilla.dtd too!) There is no group or groupset in xml.cgi >>- please file a separate bug on reordering checksetup.pl to incorporate the needed changes we discussed. Will do >>- in checksetup.pl, don't AddFDef the bug_group between the reporter and cc >>accessible bits (the order there affects the order shown on the query.cgi >>screen). Done >> - In the bug 157704 workarround, print a LARGE MESSAGE if there is anyone you >> are matching because of that, to notify the admin. You may want to only do that >>if such a group does exist, but it may not be worth it. Err on teh side of warning rather than not, though Done >>- You removed AddField calls for groupset/blessgroupset. Ideally, those should >>be created if the bug_group_table doesn't exist, for consitency. Ntohing in the >>middle uses that, though, so you can probably jsut comment them out (with >>explanitory text). Don't deltee the lines entirely, though Done >> - In editusers, if I inherit bless perms but not membership, then the row >> doesn't have a grey background. Is this by design? This is by design. >> - I can't seem to be able to get results from historical bug group queries >> (changed by, in particular) Under investigation. This may be another manifestation of bug 163362.

> WRT comment 40: Fixed in HEAD under bug 164623. Will pick up on merge with > HEAD. err. Bug 164623 is the xml.cgi thing. comment 40 is only on the branch, as discsused on irc last night > [bug_activity history rewrite] What if I delete a group, then create a new one which gets the same id? It won't be accurate in that case. Remember, the groupsets can be reused, which is the issue here. > [changed by searches don't work] Well, yes, but 163362 is an issue. But just copy what I did for products. This will catch changed by and changed when stuff. Changed to/from can be made to work too if you make it do a direct string comparison for the moment, and actually make it behave like a 'change removed' and 'change added' search (which is what its going to have to become at some point, anyway)

>> err. Bug 164623 is the xml.cgi thing. comment 40 is only on the branch, as >> discsused on irc last night Right. I missatated that. I fixed that one in the branch. >> What if I delete a group, then create a new one which gets the same id? It won't >> be accurate in that case. Remember, the groupsets can be reused, which is the issue here. I added a check that adds a '?' to the list of names if a group is not present at all. Today, if I see that someone added bit 2048 to a groupset, I would go to the groups page to see what group 2048 is called. This really should be the correct thing to call it even if the group had been renamed or reincarnated at some point. Since the text is a joined list of group names, I could put a '?' at the beginning if the groups were names converted from groupsets, though I would rather not. > [changed by searches don't work] Yes. Will fix

OK... here it is. All the items I know of are taken care of and this is in sync with HEAD as of Sync_20020716_Sync4 (today)

Hmm. groups come out as: | Bug Group | Foo, ? | Lose the trailing comma. Note that putting this at the front will trigger an explanitory message on the show_activity page. Also, why is the description "Bug Group", instead of just "Group" ? checksetup.pl still needs a header for the changed. Also, the changes need to be moved to _below_ subsequent schema changes. In globals.pl, please add a big comment explaining why the ORDER BY is needed. (Is the ordering of NULL vs a number guaranteed, though?) In quietly_check_login, HAVING w/o GROUP BY is probably invalid, and if not its odd. What you need to do is _not_ select that whole branch, and rather _move_ it into the WHERE clause. Then you can go back to directly setting $userid.

>> | Bug Group | Foo, ? | >> >> Lose the trailing comma. Note that putting this at the front will trigger an >> explanitory message on the show_activity page. Comma eliminated. We certainly don't want the message triggered by putting the '?' at the start. In this case, there is no data that has been lost. >> Also, why is the description "Bug Group", instead of just "Group" ? Changed >> checksetup.pl still needs a header for the changed. Also, the changes need to be >> moved to _below_ subsequent schema changes. rearranged (heavily). Schema changes now happen before groups just like the comment block near the beginning says they do. >> In globals.pl, please add a big comment explaining why the ORDER BY is needed. >> (Is the ordering of NULL vs a number guaranteed, though?) Adding comment. MySQL puts NULLs first in ascending. Postgresql puts them "highest" so that they are only first if descending. >> In quietly_check_login, HAVING w/o GROUP BY is probably invalid, and if not its >> odd. What you need to do is _not_ select that whole branch, and rather _move_ it >> into the WHERE clause. Then you can go back to directly setting $userid. WHERE cannot operate on a derived field. HAVING is perfectly valid without GROUP BY, but lets the field be derived before running. The current SQL systax does permit directly setting userid.

This includes a major re-ordering of checksetup. Checksetup now follows the ordering its comments claim it followed all along.

Comment on attachment 97395 [details] [diff] [review] Revised patch - Sync to HEAD r=bbaetz based on what was in CVS if: a) s/DISTINCTROW/DISTINCT/ in editusers b) Add the ? to the profiles_activity table too c) change line 764 of Bugzilla::Search to use &::ThrowUserError instead of Error (yes, this is present on the trunk) d) You can remove the ^groupset,(?!changed) stuff, because its caught by the check I mentioned in (c). And thats it!

OK WRT comment 49, I'll take care of the 4 items and promote bbaetz's r= to that. bbaetz: What else needs to happen to this pre-checkin?

You need another reviewer :) I've tested this, but with a patch this large.... Lets get this in, give it a week or so, and release a 2.17.1 test release.

OK.... comment 49 change is in CVS head.

As of today, CVS head now includes most of MattyT's general code comments. In particular, the query in Search.pm now builds selecting only visible bugs into the query itself. This is an interface change to the query results, but it shortens searches where a small number of bugs are visible out of 50000 bugs from 140 seconds down to 10 seconds. After this is re-syned to HEAD and tested out, I will extract a new patch.

This includes fixes to buglist and search to permit the sql server to select visibility at the same time as the search is being done instead of returning extra records to be filtered later. There is a paranoia check in buglist.cgi that checks each record returned by the query against CanSeeBug() and dies if the query goofs. If this ever happens, please log exactly what happened. The check will come out of the code before this is checked in. At some point, buglist should have some code added to throw away matches after a few thousand records, (a seperate bug)

This incorporates everything on my TODO list. It is now possible to have sanitycheck force derived group permissions to be outdated and rederived as well as to discard derived permissions for users who need to be rederived anyway. (A space saver for extremely large sites) Patch is extracted relative to Groups_20020716_Sync3

Comment on attachment 98362 [details] [diff] [review] Revised - ready for review and test r=bbaetz

Joel, (Yes, I said this also on bug 98801). I'd like to review this patch, but (as you know) it's fairly large. It would probably save me a lot of time if you spent five minutes writing up how it works, and the design philosophy. I could then read the patch thinking "Ah, that bit fits _there_ in the big picture" and "Hang on - if this section does X, then why doesn't it do Y, because that's needed" and so on. Any chance of that? Gerv

Developer/Reviewer information on how the new groups system works.... All aspects of groupsets fro users and bugs have been replaced. bug groupets have been replaced by bug_group map. bug_group_map has an entry for each bug for each group to which that bug is restricted. The CanSeeBug() function had been updated to use the new map so code that relied on that function works unmodified. Queries that previously used groupset math themselves are discussed below. user groupsets have been replaced by user_group_map. This table contains records... a) indicating that a user has been explicitly included in a group b) indicating that a user has been explicitly given the ability to bless a group c) indicating that a user is a member of a group, but by derived permissions rather than by explicit grant. Since there is no longer an all-ones groupset for administrators, administrators are put in a group called "admin" and the group "admin" is given permission to bless all groups by default as well as made a member of any new group by default. The group_group_map controls which groups are made members of other groups an which groups may bless other groups. queries that do not use CanSeeBug do 2 left joins. The first identifies which groups each bug is in and the second identifies the present or absent(null) user_group_map record putting the user in each of those groups (or not). If the number of distinct groups a bugs is in matches the number of distinct groups matches the number of those group in which the user is a member, then the bug is visible.(it is also visible for the ususal reporter_accessible, etc reasons). Having mysql do this instead of calling CanSeeBug in perl is in excess of 10x faster for big queries. This has been tested with 1000 groups and 50000 bugs. In order for all of this to work, the derived group information as well as the explicit group information must be up to date prior to any check of a particular user's access rights. Since it can take nearly a second per user on huge sites to update this information (and it needs to happen with some tables locked), we only want to do this during page loads if the information has changed. During login checks, a new field, refreshed_when, in the profile is checked against the most recent group update. (Checkgroup()) If the profile is not newer than the most recent group change, then DereiveGroup is called for that user and deletes and rederives the group permissions for that user. This must be done with the table locked because there are a few cases now (and will be more later when grep exclusions are added) where a user NOT being a member of a group actually weakens security. Today this might cause a user not to be able to restrict a bug to a group of which he should have been a member. In any case, these need to be locked. CheckGroup() is also called by processmail before it calls CanSeeBug() about a user other than the user invoking processmail. On giant sites (like BMO), the user_group_map could get quite huge. Sanitycheck has new functions to prevent this. ?rederivegroups deliberately sets a group's when field to now() effectively invalidating the user_group_map derived entries. We still do not want to delete them because a user could have just established that they were up to date and could still be in the same cgi. We only want to delete entries if the profile was already out of date long enough ago that the user could not possibly be caught between the login check and the completion of that page. Essentially, no data is purged by the ?cleangroupsnow option unless it was clear that it was out of data an hour ago. checksetup was a bit of a mess previosuly. It had diverged from its original design and was adding groups and the admin BEFORE updating the schema. That violates the rules stated in checksetup itself and makes it impossible to update AddGroup so it will work beyond the first group schema change. This has been fixed. I'm often on IRC during the day for real-time questions.

This has a sync to head (at Groups_20020716_Sync4) as well as a change to CanSeeBug() to have it use the same SQL technique as Search.pm which should be more protable between DBs. The CVS checkins are done seperately, so reviewers who have already read most of this may find it preferable to use Bonsai on recent changes to the Groups_20020716_Branch

Phew! I spent over an hour on this, and it seems like I only reviewed about 1/5 of it. This is an extremely complex patch; you should be asking yourself two questions: 1) Is all this complexity _really_ necessary? 2) Is it hidden behind a sensible interface such that, if we were hypothetically to change the way groups worked again, it would be a lot easier? :-) I'm particularly worried that e.g. SelectVisible() has gone away, and the groups querying seems scattered throughout the code. + if (CheckGroup($userid)) { + DeriveGroup($userid); + } + return $userid; If CheckGroup() returns a boolean indicating whether the group needs rederiving, it should have a name which reflects that. Alternatively, make CheckGroup() call DeriveGroup() if it would fail, and call it ConfirmGroups() or something like that. Index: checksetup.pl =================================================================== @@ -1465,16 +1464,7 @@ index(dependson)'; +$table{group_group_map} = + 'child_id mediumint not null, + parent_id mediumint not null, + isbless smallint not null default 0, + + unique(child_id,parent_id,isbless)'; Nit: missing spaces. + my $sth = $dbh->prepare('INSERT INTO groups + (name, description, userregexp, isbuggroup) + VALUES (?, ?, ?, ?)'); + $sth->execute($name, $desc, $userregexp, 0); Huh? :-) + +# bug 157756 - groupsets replaced by maps +# AddField('bugs', 'groupset', 'bigint not null'); Are you sure code further down doesn't expect this field to be there before it goes away again? +# 2002-08-XX - bugreport@peshkin.net - bug 157756 +# +# If the whole groups system is new, but the installation isn't, +# convert all the old groupset groups, etc... This is a very long bit of conversion code. It needs a comment at the top to explain what it's doing and why. +# ListBits(arg) returns a list of UNKNOWN<n> for all bits set in arg Yes, I can see that. Why is it doing it? :-) + while (my ($bit, $gid) = $sth->fetchrow_array) { + # create u2g memberships for old groupsets Please expand abbreviations. They will then make more sense in 2 years time :-) + $sth = $dbh->prepare("SELECT bug_id, bug_when, who, added, removed + FROM bugs_activity WHERE fieldid = $gsid"); + $sth->execute(); + while (my ($bid, $bwhen, $bwho, $added, $removed) = $sth->fetchrow_array) { Again, eschew obfuscation. my ($bug_id, $bug_when, $who... +AddGroup 'tweakparams', 'Can tweak operating parameters'; +AddGroup 'editusers', 'Can edit or disable users'; +AddGroup 'creategroups', 'Can create and destroy groups.'; +AddGroup 'editcomponents', 'Can create, destroy, and edit components.'; +AddGroup 'editkeywords', 'Can create, destroy, and edit keywords.'; +AddGroup 'admin', 'Administrators'; Please line this up properly or not at all. + $sth = $dbh->prepare("SELECT id FROM groups"); + $sth->execute(); + while ( my ($id) = $sth->fetchrow_array() ) { + # admins can bless every group + $dbh->do("INSERT INTO group_group_map + (child_id, parent_id, isbless) + VALUES ($adminid, $id, 1)"); + # admins are initially members of every group + $dbh->do("INSERT INTO group_group_map + (child_id, parent_id, isbless) + VALUES ($adminid, $id, 0)"); + } This will create a circular loop on the "admin" group - i.e. it will be made a member of itself. +my @groups = (); +$sth = $dbh->prepare("select id from groups"); +$sth->execute(); +while ( my @row = $sth->fetchrow_array() ) { + push (@groups, $row[0]); Isn't there a fetchrow_something to get just a single item? + print "\nLooks like we don't have an administrator set up yet. Either this is your\n"; + print "first time using Bugzilla, or your administrator's privs might have accidently\n"; + print "gotten deleted at some point.\n"; Can we please make this sentence proper English? "may have been deleted accidentally...". + $login = $dbh->quote($login); + $sth = $dbh->prepare(<<_End_Of_SQL_); + SELECT login_name + FROM profiles + WHERE login_name=$login +_End_Of_SQL_ Can we remove the uses of <<? It's inconsistent with the rest of the file. + if(! $pass1 ) { + print "\n\nIt's just plain stupid to not have a password. Try again!\n"; + } elsif ( $pass1 !~ /^.{3,16}$/ ) { + print "The password must be 3-16 characters in length."; + } Why do we limit it to 16? + # Put the admin in each group if not already + my $query = "select userid from profiles where login_name = $login"; + $sth = $dbh->prepare($query); + $sth->execute(); + my ($userid) = $sth->fetchrow_array(); + + foreach my $group ( @groups ) { Nit: random spaces. + print "\n$login is now set up as the administrator account.\n"; _An_ administrator account. The above implies you can only have one. +my ($adminuid) = $sth->fetchrow_array; +if (!$adminuid) { die "No administator!" } # should never get here Administ_r_ator. Index: duplicates.cgi =================================================================== @@ -160,9 +160,7 @@ # Limit to a single product if requested $query .= (" AND bugs.product_id = " . $product_id) if $product_id; - SendSQL(SelectVisible($query, - $userid, - $usergroupset)); + SendSQL($query); while (MoreSQLData()) { # Note: maximum row count is dealt with in the template. @@ -170,6 +168,7 @@ my ($id, $component, $bug_severity, $op_sys, $target_milestone, $short_desc, $bug_status, $resolution) = FetchSQLData(); + next if (!CanSeeBug($id, $::userid)); This is a performance regression; doing this means we do far more SQL queries than we need to. You need to provide functionality equivalent to SelectVisible(). Why did it go away, anyway? Index: globals.pl =================================================================== + return (($found_groups == 0) || ($userid && (($assigned_to == $userid) + || ($qa_contact == $userid) + || (($reporter == $userid) && $rep_access) + || ($found_cc && $cc_access) + || ($found_groups == $found_members)))); Can you indent this to make the structure more clear, please? } @@ -799,6 +722,86 @@ return $cryptedpassword; } +sub CheckGroup { + my ($user) = (@_); + PushGlobalSQLState(); + SendSQL("SELECT userid FROM profiles, groups WHERE userid = $user " . + "AND profiles.refreshed_when <= groups.group_when "); Nit: random extra spaces. + sub UserInGroup { + my ($groupname, $userid) = (@_); + PushGlobalSQLState(); + $userid ||= $::userid; + SendSQL("SELECT groups.id FROM groups, user_group_map + WHERE groups.id = user_group_map.group_id + AND user_group_map.user_id = $userid + AND isbless = 0 + AND groups.name = " . SqlQuote($groupname)); + my $rslt = FetchOneColumn(); + PopGlobalSQLState(); + return defined($rslt); +} $rslt? $result, please :-) +sub GroupIdToName { + my ($groupid) = (@_); + PushGlobalSQLState(); + SendSQL("select name from groups where id = $groupid"); Please capitalise SQL keywords throughout. It makes the SQL so much easier to read. Index: Bugzilla/Search.pm =================================================================== @@ -29,7 +29,7 @@ # The caller MUST require CGI.pl and globals.pl before using this -use vars qw($userid $usergroupset); +use vars qw($userid ); Nit: extra space. package Bugzilla::Search; @@ -751,8 +757,9 @@ "If you think you're getting this in error, please copy the " . "entire URL out of the address bar at the top of your browser " . "window and email it to <109679\@bugzilla.org>"; + $::vars->{'fieldname'} = html_quote($f); Use FILTER html in the template rather than html_quote here. This is for consistency. @@ -807,11 +814,27 @@ $suppseen{$str} = 1; } } - my $query = ("SELECT DISTINCT " . join(', ', @fields) . + my $query = ("SELECT DISTINCT " . + join(', ', @fields) . + ", COUNT(DISTINCT ugmap.user_id) AS cntuseringroups, " . + " COUNT(DISTINCT bgmap.group_id) AS cntbugingroups, " . + " ((COUNT(DISTINCT ccmap.who) AND cclist_accessible) " . + " OR ((bugs.reporter = $::userid) AND bugs.reporter_accessible) " . + " OR bugs.assigned_to = $::userid ) AS canseeanyway " . " FROM $suppstring" . - " WHERE " . join(' AND ', (@wherepart, @andlist))); - - $query = &::SelectVisible($query, $::userid, $::usergroupset); + " LEFT JOIN bug_group_map AS bgmap " . + " ON bgmap.bug_id = bugs.bug_id " . + " LEFT JOIN user_group_map AS ugmap " . + " ON bgmap.group_id = ugmap.group_id " . + " AND ugmap.user_id = $::userid " . + " AND ugmap.isbless = 0" . + " LEFT JOIN cc AS ccmap " . + " ON ccmap.who = $::userid AND ccmap.bug_id = bugs.bug_id " . + " WHERE " . join(' AND ', (@wherepart, @andlist)) . + " GROUP BY bugs.bug_id " . + " HAVING cntuseringroups = cntbugingroups" . + " OR canseeanyway" + ); This is quite scary. Has an SQL guru reviewed this? I seem to remember reading that SQL performance drops off rapidly if you have more than three JOINs in a query - and there's three here even before any more that might be added. Index: template/en/default/sidebar.xul.tmpl =================================================================== @@ -73,7 +73,7 @@ [%- IF UserInGroup('tweakparams') %] <text class="text-link" onclick="load_relative_url('editparams.cgi')" value="edit params"/> [%- END %] - [%- IF UserInGroup('editusers') || blessgroupset %] + [%- IF UserInGroup('editusers') || canblessanything %] You should be using $user{'canblessany'} here. Index: template/en/default/account/prefs/permissions.html.tmpl =================================================================== @@ -20,36 +20,48 @@ [%# INTERFACE: - # has_bits: array of strings. May be empty. - # Descriptions of the permission bits the user has. - # set_bits: array of strings. May be empty. - # Descriptions of the permission bits the user can set for + # has_bits: array of hashes. May be empty. + # name => Descriptions of the permissions the user has. + # desc => Descriptions of the permissions the user has. Oops. You mean "Names of". Same for set_bits. -<table> +<table align=center> Please quote attributes. Index: template/en/default/global/messages.html.tmpl =================================================================== @@ -56,6 +56,126 @@ [% title = "Change columns" %] Resubmitting your query with new columns... + [% ELSIF message_tag == "account_disabled" %] <snip> There's a load of stuff in here with +s next to it which you didn't add... Gerv

gerv: Actually, doing the queyr separtely is not a perf regression - see bug 114696. Basically, the extra joins are slow. SelectVisible was a hack, anyway.

Just to make sure I didn't miss any of Gerv's comments....

Still relative to Groups_20020716_Sync4

Doh!

This is an unrotted version of the previous patch and integrates Gerv's recent optimization of UserInGroup with the patch.

Here's what happened when I ran ./checksetup.pl after installing the patch on my b.m.o test installation: Creating table group_group_map ... Creating table bug_group_map ... Creating table user_group_map ... Adding new field group_when to table groups ... Adding new field id to table groups ... DBD::mysql::db do failed: Multiple primary key defined at ./checksetup.pl line 2012. Adding new field refreshed_when to table profiles ... DBD::mysql::st execute failed: Unknown column 'id' in 'field list' at ./checksetup.pl line 3074. DBD::mysql::st fetchrow_array failed: fetch() without execute() at ./checksetup.pl line 3075. DBD::mysql::st execute failed: Unknown column 'id' in 'field list' at ./checksetup.pl line 3196. DBD::mysql::st fetchrow_array failed: fetch() without execute() at ./checksetup.pl line 3197. Deleting unused field groupset from table profiles ... Deleting unused field blessgroupset from table profiles ... Deleting unused field groupset from table bugs ... Deleting unused field bit from table groups ... Adding group admin ... DBD::mysql::st execute failed: Unknown column 'id' in 'field list' at ./checksetup.pl line 3281. DBD::mysql::st fetchrow_array failed: fetch() without execute() at ./checksetup.pl line 3282. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3284. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 0, 0)' at line 3 at ./checksetup.pl line 3284. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3291. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 1, 0)' at line 3 at ./checksetup.pl line 3291. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3284. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 0, 0)' at line 3 at ./checksetup.pl line 3284. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3291. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 1, 0)' at line 3 at ./checksetup.pl line 3291. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3284. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 0, 0)' at line 3 at ./checksetup.pl line 3284. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3291. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 1, 0)' at line 3 at ./checksetup.pl line 3291. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3284. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 0, 0)' at line 3 at ./checksetup.pl line 3284. Use of uninitialized value in concatenation (.) or string at ./checksetup.pl line 3291. DBD::mysql::db do failed: You have an error in your SQL syntax near ' 1, 0)' at line 3 at ./checksetup.pl line 3291. DBD::mysql::st execute failed: Unknown column 'id' in 'field list' at ./checksetup.pl line 3296. DBD::mysql::st fetchrow_array failed: fetch() without execute() at ./checksetup.pl line 3297. DBD::mysql::st execute failed: Unknown column 'id' in 'field list' at ./checksetup.pl line 3314. DBD::mysql::st fetchrow_array failed: fetch() without execute() at ./checksetup.pl line 3315. DBD::mysql::st execute failed: Unknown column 'id' in 'where clause' at ./checksetup.pl line 3324. DBD::mysql::st execute failed: Unknown column 'groups.id' in 'where clause' at ./checksetup.pl line 3510. DBD::mysql::st fetchrow_array failed: fetch() without execute() at ./checksetup.pl line 3511. No administrator! at ./checksetup.pl line 3512.

OK, this is a full review (well, pretty full) of the patch itself. Once this lot of comments are addressed (and Myk's problems), then I will apply it to a tree and give it some serious testing. Please "reply" to this review in the normal style; that is, for items which you do *not* say "Oops, yes, fixed", please list the items and the reason why you haven't made the change, or the consequences, or whatever. This makes it much easier to have a focussed dicussion on what's left to do. <snip> General point: not enough comments. Each non-trivial SQL query should have a one-line comment like: # Get the foo info about user bar, which we need for baz Bug.pm: =================================================================== @@ -143,8 +139,7 @@ $count++; } } else { - &::SendSQL("select groupset from bugs where bug_id = $bug_id"); - if (@row = &::FetchSQLData()) { + if (@row) { Can you use elsif here? (I'm assuming the previous value of @row is the right one to be checking now you've removed the code which changes it.) @@ -359,18 +354,20 @@ sub UserInGroup { my $self = shift(); my ($groupname) = (@_); Why is this function in Bug.pm? if (@row = FetchSQLData()) { - my ($userid, $groupset, $loginname, $ok, $disabledtext) = (@row); - if ($ok) { + ($userid, my $loginname, my $disabledtext) = (@row); + if ($userid > 0) { The usual idiom here is: if (MoreSQLData() { ($userid, my $loginname, my $disabledtext) = FetchSQLData(); Index: bug_form.pl =================================================================== + # If the bug is restricted to a group, display checkboxes that allow + # the user to set whether or not the reporter + # and cc list can see the bug even if they are not members of all + # groups to which the bug is restricted. Could you fix this comment to not say "display", as it now doesn't? :-) Index: buglist.cgi =================================================================== - return if !$groupset; + return if !$userid; Nit: unless. :-) Index: checksetup.pl =================================================================== RCS file: /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v retrieving revision 1.189 diff -u -r1.189 checksetup.pl --- checksetup.pl 18 Sep 2002 07:20:13 -0000 1.189 +++ checksetup.pl 20 Sep 2002 17:54:37 -0000 @@ -1610,6 +1597,38 @@ index(userid)'; +# group membership tables for tracking group and privilege +# +# This table determines the groups that a user belongs to +# directly or due to regexp and which groups can be blessed +# by a user +# maptype: +# isderived: +# if 0 - record was explicitly granted +# if 1 - record was created by evaluating a regexp or group hierarchy What does the word "maptype:" mean in this comment? Nit: please full-stop-terminate English sentences. +$table{user_group_map} = + 'user_id mediumint not null, + group_id mediumint not null, + isbless smallint not null default 0, + isderived tinyint not null default 0, Any reason isbless is not a tinyint? +# This table determines in which groups a user must be a member to have +# permission to see a bug "This table determines which groups a user must be a member of in order to see a bug." @@ -2125,19 +1933,20 @@ $sth->execute; my ($product_id) = $sth->fetchrow_array; $dbh->do(qq{INSERT INTO versions (value, product_id) VALUES ("other", $product_id)}); + # note: since amin user is not yet known, components gets a 0 for + # initialowner and this is fixed during final checks Typo, missing full stop. "'TestComponent', $product_id, " . "'This is a test component in the test product database. " . "This ought to be blown away and replaced with real stuff in " . - "a finished installation of bugzilla.', $adminuid, 0)"); + "a finished installation of bugzilla.', 0, 0)"); While you are there, please capitalise this instance of Bugzilla. @@ -2237,8 +2046,10 @@ # really old fields that were added before checksetup.pl existed # but aren't in very old bugzilla's (like 2.1) # Steve Stock (sstock@iconnect-inc.com) + +# bug 157756 - groupsets replaced by maps +# AddField('bugs', 'groupset', 'bigint not null'); AddField('bugs', 'target_milestone', 'varchar(20) not null default "---"'); -AddField('bugs', 'groupset', 'bigint not null'); AddField('bugs', 'qa_contact', 'mediumint not null'); AddField('bugs', 'status_whiteboard', 'mediumtext not null'); AddField('products', 'disallownew', 'tinyint not null'); @@ -2672,7 +2483,8 @@ } AddField('products', 'maxvotesperbug', 'smallint not null default 10000'); AddField('products', 'votestoconfirm', 'smallint not null'); -AddField('profiles', 'blessgroupset', 'bigint not null'); +# bug 157756 - groupsets replaced by maps +# AddField('profiles', 'blessgroupset', 'bigint not null'); # 2000-03-21 Adding a table for target milestones to # database - matthew@zeroknowledge.com As I said in my last review, messing with old checksetup.pl code is unwise. There's no harm in it being added, because it'll get removed further down. And some of the intermediate code might expect it. +# 2002-08-XX - bugreport@peshkin.net - bug 157756 +# +# If the whole groups system is new, but the installation isn't, +# convert all the old groupset groups, etc... This piece of code needs far more comments within it. Didn't I say that in my last review? Five pages of dense database access with one comment per page is just not on :-) Anyone else reviewing this patch should review the migration code particularly carefully; I don't understand all of it. + AddField('groups', 'group_when', 'datetime not null'); + $dbh->do("ALTER TABLE groups DROP INDEX bit") if GetIndexDef("groups","bit"); + AddField('groups', 'id', 'mediumint not null auto_increment primary key'); + AddField('products', 'id', + 'mediumint primary key auto_increment not null'); Er... is this supposed to be in here? +AddGroup 'tweakparams', 'Can tweak operating parameters'; +AddGroup 'editusers', 'Can edit or disable users'; +AddGroup 'creategroups', 'Can create and destroy groups.'; +AddGroup 'editcomponents', 'Can create, destroy, and edit components.'; +AddGroup 'editkeywords', 'Can create, destroy, and edit keywords.'; +AddGroup 'admin', 'Administrators'; If these are function calls, can they have brackets, please. +if (!GroupDoesExist("editbugs")) { + my $id = AddGroup('editbugs', 'Can edit all aspects of any bug.', ".*"); + my $sth = $dbh->prepare("SELECT userid FROM profiles ORDER BY userid"); + $sth->execute(); + while ( my ($userid) = $sth->fetchrow_array() ) { + $dbh->do("INSERT INTO user_group_map + (user_id, group_id, isbless, isderived) + VALUES ($userid, $id, 0, 0)"); + } + +} Why are we ordering by userid? Nit: remove newlines between adjacent brackets. This code gives everyone "editbugs" (and "canconfirm", further down) on new installations. That is not the current default, is it? +if (@admins) { + # identify admin group + my $sth = $dbh->prepare("SELECT id FROM groups + WHERE name = 'admin'"); Can we give the admin group ID 0 instead? The problem with this is that it means the name of the admin group isn't changeable. + $sth->execute(); + my ($adminid) = $sth->fetchrow_array(); + foreach my $userid (@admins) { + $dbh->do("INSERT INTO user_group_map + (user_id, group_id, isbless, isderived) + VALUES ($userid, $adminid, 0, 0)"); + # existing administrators are made blessers of group "admin" + # only explitly defined blessers can bless group admin + # other groups can be blessed by any admin or additional + # defined blessers Capitals, full stops, etc ;-) It does make comments easier to read. I had to have three goes at that one. + $dbh->do("INSERT INTO user_group_map + (user_id, group_id, isbless, isderived) + VALUES ($userid, $adminid, 1, 0)"); + } + $sth = $dbh->prepare("SELECT id FROM groups"); + $sth->execute(); + while ( my ($id) = $sth->fetchrow_array() ) { + # admins can bless every group + $dbh->do("INSERT INTO group_group_map + (child_id, parent_id, isbless) + VALUES ($adminid, $id, 1)"); + # admins are initially members of every group + next if ($id == $adminid); + $dbh->do("INSERT INTO group_group_map + (child_id, parent_id, isbless) + VALUES ($adminid, $id, 0)"); + } + +} Here, you appear to have the admin group being the child of all these groups. Surely it should be the parent? + print "\nLooks like we don't have an administrator set up yet. Either this is your\n"; + print "first time using Bugzilla, or your administrator's privs might have accidently\n"; + print "gotten deleted.\n"; Please fix this horrible grammar while you are there. "...or your administrator's privileges might have accidentally been deleted." Index: editgroups.cgi =================================================================== NOT REVIEWED AS PATCH. It's too big a change - I want to review it as a whole file. In general, I haven't been able to review edit* as well as the rest; it's all too horrible. :-| Index: editproducts.cgi =================================================================== - SqlQuote($userregexp) . ")"); + SqlQuote($product . " Bugs Access") . ", 1, NOW())"); Can we change this to "Access to bugs in the $product product"? It's quite a bit clearer. Index: enter_bug.cgi =================================================================== +if(Param("usebuggroups") && GroupExists($product)) { + SendSQL("SELECT id FROM groups ". + "WHERE name = " . SqlQuote($product) . + " AND isbuggroup = 1"); + ($group_id) = FetchSQLData(); +} This does the same query twice, under the covers. Why does GroupExists not return the group ID? + # If this is the group for this product, make it checked. + if(formvalue("maketemplate") eq + "Remember values as bookmarkable template") We really need to localise that string. <sigh> (Not your problem, though.) Index: globals.pl =================================================================== +sub ConfirmGroup { + my ($user) = (@_); + PushGlobalSQLState(); + SendSQL("SELECT userid FROM profiles, groups WHERE userid = $user " . + "AND profiles.refreshed_when <= groups.group_when "); + my $ret = FetchSQLData(); + PopGlobalSQLState(); + if ($ret) { + DeriveGroup($user); + } +} OK, I've tried hard, but I don't quite get this. group_when is a per-group setting which is updated... when, exactly? When it's updated on any group, every user's group settings have to be rederived in case they are different in what way? I need a more detailed explanation here, I'm afraid :-| + # each group needs to be checked for parent memberships once + while (@groupidstocheck) { + my $group = shift @groupidstocheck; foreach (my $group (@groupidstocheck)) { is the usual idiom here. + if (!defined($groupidschecked{"$group"})) { + $groupidschecked{"$group"} = 1; + SendSQL("SELECT parent_id FROM group_group_map WHERE" + . " child_id = $group AND NOT isbless"); + while (MoreSQLData()) { + my ($groupid) = FetchSQLData(); + if (!defined($groupidschecked{"$groupid"})) { + push(@groupidstocheck,$groupid); + } + if (!$groupidsadded{$groupid}) { + $groupidsadded{$groupid} = 1; + PushGlobalSQLState(); + SendSQL("INSERT INTO user_group_map" + . " (user_id, group_id, isbless, isderived)" + . " VALUES ($user, $groupid, 0, 1)"); + PopGlobalSQLState(); + } + } + } + } OK, now I'm more confused. Being a member of a group automatically confers membership of all its _parent_ groups? I would really expect it to confer membership of all its _child_ groups. Like selecting a subtree in a tree. sub UserInGroup { - return $::vars->{'user'}{'groups'}{$_[0]}; + my ($groupname, $userid) = (@_); + return $::vars->{'user'}{'groups'}{$groupname} unless $userid; + PushGlobalSQLState(); Nit: Add a space before and after that return statement, for readability. Index: post_bug.cgi =================================================================== # Groups +my @groupstoadd = (); foreach my $b (grep(/^bit-\d*$/, keys %::FORM)) { Did you make a conscious decision not to rename the form elements from "bit-*"? + if ($member) { + push(@groupstoadd,$v) + } Nit: space after array name. Index: process_bug.cgi =================================================================== + SendSQL("SELECT group_id FROM bug_group_map WHERE bug_id = $::FORM{'id'}"); + my ($havegroup) = FetchSQLData(); + if ( $havegroup ) { Nit: extra spaces. + SendSQL("DELETE FROM bug_group_map + WHERE bug_id = $id AND group_id = $grouptodel"); + } Nit: extra space after AND. You can find these yourself by just reading your diff :-) SendSQL("select now()"); $timestamp = FetchOneColumn(); - + + $groupDelNames =~ s/, $//; + $groupAddNames =~ s/, $//; If you push onto a list for groupDelNames, instead of appending to a string, you can use join(',', @groupDelNames) and avoid this messing about with commas. + LogActivityEntry($id,"bug_group",$groupDelNames,$groupAddNames); Missing spaces. Index: processmail =================================================================== - my ($userid, $groupset) = (FetchSQLData()); + my ($userid, $current) = (FetchSQLData()); Nit: extra space. I know it's not your fault :-) - if (Param("insidergroup") && ($anyprivate != 0)) { - ConnectToDatabase(); - PushGlobalSQLState(); - SendSQL("select (bit & $groupset ) != 0 from groups where name = " . SqlQuote(Param("insidergroup"))); - my $bit = FetchOneColumn(); - PopGlobalSQLState(); - if (!$bit) { - return; - } - } + return if (Param("insidergroup") && ($anyprivate != 0) && (!UserInGroup(Param("insidergroup"), $userid))); Align this as follows: return if (Param("insidergroup") && ($anyprivate != 0) && (!UserInGroup(Param("insidergroup"), $userid))); In general, please try and stay inside 80 chars. +SendSQL("SELECT MAX(group_when) FROM groups"); +($::group_when) = FetchSQLData(); You do this, but appear not to make use of the information. Index: query.cgi =================================================================== +my $userid = 0; if (defined $::FORM{"GoAheadAndLogIn"}) { # We got here from a login page, probably from relogin.cgi. We better # make sure the password is legit. - confirm_login(); + $userid = confirm_login(); } else { - quietly_check_login(); + $userid = quietly_check_login(); } Are you really managing to eliminate all uses of the $::userid global? Index: sanitycheck.cgi =================================================================== +if (exists $::FORM{'rederivegroups'}) { + Status("OK, now setting a group_when so all users will redrive groups on next access."); This isn't particularly admin-friendly :-) How about: "All users' inherited group permissions will be rechecked when they next access Bugzilla." or something like that? + SendSQL("UPDATE groups SET group_when = NOW() LIMIT 1"); For my own information, what does LIMIT 1 do? +if (exists $::FORM{'rederivegroupsnow'}) { + Status("OK, now rederiving groups."); Same here for the text. Why would anyone want to rederivegroupsnow? +if (exists $::FORM{'cleangroupsnow'}) { + Status("OK, now cleaning stale groups."); ... + SendSQL("LOCK TABLES user_group_map WRITE, profiles READ"); This basically brings Bugzilla to a halt, right? No-one can read from the user_group_map. Index: showdependencytree.cgi =================================================================== - SendSQL(SelectVisible("SELECT 1, + my $bug = {}; + if (CanSeeBug($id,$::userid)) { Nit: missing space. Index: Bugzilla/Search.pm =================================================================== my @legal_fields = ("product", "version", "rep_platform", "op_sys", "bug_status", "resolution", "priority", "bug_severity", "assigned_to", "reporter", "component", - "target_milestone", "groupset"); + "target_milestone", "bug_group"); foreach my $field (keys %F) { if (lsearch(\@legal_fields, $field) != -1) { @@ -322,6 +322,12 @@ push(@wherepart, "$table.bug_id = bugs.bug_id"); $f = "$table.thetext"; }, + "^bug_group,(?!changed)" => sub { + push(@supptables, "LEFT JOIN bug_group_map bug_group_map_$chartid ON bugs.bug_id = bug_group_map_$chartid.bug_id"); + + push(@supptables, "LEFT JOIN groups groups_$chartid ON groups_$chartid.id = bug_group_map_$chartid.group_id"); + $f = "groups_$chartid.name"; + }, You change "groupset" to "bug_group", and add a stanza for "bug_group" - do you need to remove one for "groupset"? + $::vars->{'fieldname'} = html_quote($f); die "Internal error: $errstr" if $chart < 0; - return &::DisplayError($errstr); + &::ThrowUserError("illegal_fieldname"); Please use FILTER html in the error template rather than pre-filtering it here. Consistency and convention. } Index: contrib/bug_email.pl =================================================================== -# $Id: bug_email.pl,v 1.13 2002/08/26 06:17:21 bbaetz%student.usyd.edu.au Exp $ +# $Id: bug_email.pl,v 1.9.16.4 2002/08/27 04:50:18 bugreport%peshkin.net Exp $ Is this supposed to be in here? It's the only change to that file. Index: template/en/default/global/useful-links.html.tmpl =================================================================== [% ', <a href="editusers.cgi">users</a>' IF user.groups.editusers - || (user.blessgroupset > 0) %] + || user.canblessany %] Can we not handle this by automatically adding users to editusers when they acquire the ability to bless? We can then ditch all of this UserCanBlessAny infrastructure. Same for sidebar.xul.tmpl. Gerv

I attached a full reconcilliation of the changes. The ones that were real answers are below.... > > As I said in my last review, messing with old checksetup.pl code is unwise. > There's no harm in it being added, because it'll get removed further down. And > some of the intermediate code might expect it. > Actually, this was discussed previously with others. That is why it is commented instead of being deleted. Here are the key reasons. 1) If we add and then remove this field, it will require 2 full file copies every time checksetup is run in the future. For large sites, that is a big deal. 2) I checked and there is nothing lower that cares. 3) See bug 123957 comment 10 > This code gives everyone "editbugs" (and "canconfirm", further down) on new > installations. That is not the current default, is it? > Yes, that is the current default > +if (@admins) { > + # identify admin group > + my $sth = $dbh->prepare("SELECT id FROM groups > + WHERE name = 'admin'"); > > Can we give the admin group ID 0 instead? The problem with this is that it means > the name of the admin group isn't changeable. > I really can't without breaking all sorts of things. The name of the admin group could be moved to a param, but this may as well wait until all the system groups are made configurable. That is a seperate bug. Meanwhile, "admin" is no more hardcoded than "editusers" > > Here, you appear to have the admin group being the child of all these groups. > Surely it should be the parent? > No. All members of a child are included in the parent. We want all groups to include the admins. I'll add a general comment to clarify. > > OK, I've tried hard, but I don't quite get this. group_when is a per-group > setting which is updated... when, exactly? When it's updated on any group, every > user's group settings have to be rederived in case they are different in what > way? I need a more detailed explanation here, I'm afraid :-| > Adding explanation in routine, but group_when is updated whenever a group is updated. If ANY group has been updated since the last time a user was rederived, the rederive is redone. > > OK, now I'm more confused. Being a member of a group automatically confers > membership of all its _parent_ groups? I would really expect it to confer > membership of all its _child_ groups. Like selecting a subtree in a tree. > Yes. Parents are composed of children. > +my @groupstoadd = (); > foreach my $b (grep(/^bit-\d*$/, keys %::FORM)) { > > Did you make a conscious decision not to rename the form elements from "bit-*"? Yes. There are enough merge conflicts as it is. This makes it uneccessary to change the bug templates at all from 2.16. Since the templates are the most likely thing for sites to customize, this is a good thing. > Index: process_bug.cgi > =================================================================== > + SendSQL("SELECT group_id FROM bug_group_map WHERE bug_id = $::FORM{'id'}"); > + my ($havegroup) = FetchSQLData(); > + if ( $havegroup ) { > > Nit: extra spaces. > This entire cgi uses spaces in such places. I may as well leave this consistent with the surrounding code > +SendSQL("SELECT MAX(group_when) FROM groups"); > +($::group_when) = FetchSQLData(); > > You do this, but appear not to make use of the information. > It gets used each time a person is processed. A comment has been added to clarify this. > Index: query.cgi > =================================================================== > +my $userid = 0; > if (defined $::FORM{"GoAheadAndLogIn"}) { > # We got here from a login page, probably from relogin.cgi. We better > # make sure the password is legit. > - confirm_login(); > + $userid = confirm_login(); > } else { > - quietly_check_login(); > + $userid = quietly_check_login(); > } > > Are you really managing to eliminate all uses of the $::userid global? > Actually, confirm_login and quietly_check_login will update the global as well, but query.cgi needs to have a userid of 0 if we never bothered to log in. > + SendSQL("UPDATE groups SET group_when = NOW() LIMIT 1"); > > For my own information, what does LIMIT 1 do? > We only need to make one group newer than the users. LIMIT 1 instructs the DB to do one row and not continue on to the rest of the rows. > +if (exists $::FORM{'rederivegroupsnow'}) { > + Status("OK, now rederiving groups."); > > Same here for the text. > > Why would anyone want to rederivegroupsnow? > They wouldn't unless they were benchmarking. I added a comment to that effect. > +if (exists $::FORM{'cleangroupsnow'}) { > + Status("OK, now cleaning stale groups."); > ... > + SendSQL("LOCK TABLES user_group_map WRITE, profiles READ"); > > This basically brings Bugzilla to a halt, right? No-one can read from the > user_group_map. > I'm changing this to only do 1000 users at a time. > Index: Bugzilla/Search.pm > =================================================================== > You change "groupset" to "bug_group", and add a stanza for "bug_group" - do you > need to remove one for "groupset"? > There wasn't a stanza for groupset since it was flat right from bugs. Removing the field from the field lists makes it dissapear automagically. > } > > Index: contrib/bug_email.pl > =================================================================== > -# $Id: bug_email.pl,v 1.13 2002/08/26 06:17:21 bbaetz%student.usyd.edu.au Exp $ > +# $Id: bug_email.pl,v 1.9.16.4 2002/08/27 04:50:18 bugreport%peshkin.net Exp $ > > Is this supposed to be in here? It's the only change to that file. > CVS replaces this everytime anything is checked in or out. It's just an artifact of that. > Index: template/en/default/global/useful-links.html.tmpl > =================================================================== > [% ', <a href="editusers.cgi">users</a>' IF user.groups.editusers > - || (user.blessgroupset > 0) %] > + || user.canblessany %] > > Can we not handle this by automatically adding users to editusers when they > acquire the ability to bless? We can then ditch all of this UserCanBlessAny > infrastructure. Same for sidebar.xul.tmpl. > > Gerv Editusers lets a user edit all aspects of all users. Users can be autorized to bless specific groups without getting that privilige.

OK... This has the Gerv feedback + the myk fix + sync with HEAD I did back out the unless syntax because unless violates some reviewer's sense of style.

Synced to HEAD, includes fix for myk and changes suggested by Gerv. I did back out the unless statements since they are controversial among reviewers.

Joel: I am sold on all of your replies, apart from the following two :-) > > Here, you appear to have the admin group being the child of all these groups. > > Surely it should be the parent? > > No. All members of a child are included in the parent. We want all groups > to include the admins. I'll add a general comment to clarify. I really have a problem with this (assuming I understand what is going on) :-( The groups system is a graph, right? (By the way, do you have any code to prevent cycles? Or doesn't it matter?) If we are talking about privileges "cascading" down the graph, where membership of one group confers membership of another, I would strongly expect the terminology to be such that membership of a parent confers membership of its children. So the admin group would be the "root node" in the tree, with everything below it. This expectation comes from working in similar hierarchical systems; I fear we are in for trouble if we use terminology which is not only different, but also inverted, to standard practice (e.g. in Unix processes.) As a backup to what seems to be just my view, do a Google search for "parent "inherits permissions from its child" and "child inherits permissions from its parent". Because Google doesn't care about word order, they both return the same 3560 results, and each of the top ten is a phrase similar to "child inherits permissions from its parent" rather than the other way around. > Adding explanation in routine, but group_when is updated whenever a group > is updated. If ANY group has been updated since the last time a user was > rederived, the rederive is redone. Right. So why is there one value per-group, and not one global value? Also, it seems that the group_when is set to NOW() when a group record is changed. Surely it needs to be set to NOW() when the group_group_map changes? Or is that case covered? Gerv

I haven't looked at the code or even the design very much, but I think making the admin group the member of all other groups is (perhaps only mostly, see below) the right approach. Groups may be a graph, but graphs are not inherently hierarchical, and neither would I expect groups to be (parent-child terminology notwithstanding). The Unix process analogy is misleading because Unix processes cannot exist except in the context of their parent process, while groups can (and often do) exist independently. Mailing lists are a better analogy. Users receive mail from lists to which they are subscribed, and lists (in some cases) are subscribed to other lists. If a user is subscribed to list A, and list A is subscribed to list B, the user gets mail from both list A and list B. If a group of users wanted to receive mail from all mailing lists, they would subscribe themselves to a list and then subscribe that list to all other lists. >As a backup to what seems to be just my view, do a Google search for "parent >"inherits permissions from its child" and "child inherits permissions from >its parent". Because Google doesn't care about word order, they both return >the same 3560 results, and each of the top ten is a phrase similar to "child >inherits permissions from its parent" rather than the other way around. This just supports Joel's approach. If we implement the popular "child inherits permissions from its parent" design, we wouldn't want groups to be children of the admin group, because they would then inherit the admin group's permissions. The reason I say this is perhaps only mostly the right approach is that a system which treats admins as "just another group" is prone to failure when admins accidentally don't get added to a new group or get removed from an existing one (or all of them). For this reason, permissions systems often special-case the admin user/group and shortcut out of the authorization process for them.

> This just supports Joel's approach. If we implement the popular "child inherits > permissions from its parent" design, we wouldn't want groups to be children of > the admin group, because they would then inherit the admin group's permissions. Hmm. Maybe the problem is that I'm thinking in terms of memberships - "admin membership implies membership of all the child groups" rather than in terms of permissions, which would flow the other way in my model - "the parent has a sum of the permissions of the children". So Joel is doing "the child has the sum of the permissions of the parents"? Gerv

WRT the comments on the parent-child relationship... There is code to prevent loops. In DeriveGroup(), there is a list of groups to check and a hash of groups already checked. As each group membership is checked, any new parents found are added to the list only if they have not already been explored. It sounds like the real problem, as myk points out, is that this is only a parent-child relationship if you include incest. I am open to suggestions on a name change for these to make it something less confusing. I don't think that reversing the names solves that problem. The group_when question is correct. We really only need one group_when row (the newest one) so a global kept in the database would be ideal. The problem I have is that this needs to be in the database and there really is no table appropriate for a single field like that. Any suggestions?

Joel: what about my query as to whether we are updating group_when at the correct time(s)? Gerv

Gerv: I'll recheck that tonight. I think so.

I am going to London this evening, and am on the Countryside March (http://www.march-info.org) tomorrow. When I get back, I'll probably go straight to church and on to house group. So it'll be 10pm GMT Sunday before I get back to this bug. If Joel can find a better naming convention for the parent/child stuff, and sort out the two group_when issues, I'm pretty happy with this. So, I suggest that whoever was going to 2xr= this patch, starts now. According to IRC, Joel is back at 8pm PDT from wherever he's gone. A second review awaiting him when he arrives would go a long way towards getting this patch in by the end of the weekend :-) Gerv

BTW, two possible naming conventions we came up with were: "inheritor"/"inheritee": If you are a member of the admin group (the inheritor) you inherit membership of the foo group (the inheritee) and the bar group (another inheritee). If you are a member of the foo group (inheritor), you inherit membership of the bar group (inheritee). ...and "grantor"/"member". Gerv

Grantor:Heir

Those changes all look fine to me. I'm curious as to the primary key thing - I couldn't find anything in the mysql manual. Oh well. r=bbaetz - we're going to have to get this in, and see what breaks :)

The latest version fails the same way for me on checksetup.pl.

grantor/member sounds like a winner. I checked the update of group_when and it is OK. I finally think I have a fix for myk's item. I reproduced it here by following myk's suggestion to force my "bit" to primary key and I added a DROP PRIMARY KEY to the sequence before adding the new field. I'll roll that change and a name change from parent/child to grantor/member.

OK - this has the fix for myk and the last open item from Gerv's review covered. If bbaetz can re-update his r= and we take Gerv's from the comment above, then we're ready to roll as soon as myk is confirmed OK.

permission originators/permission aggregators (couldn't help it -- I've been thinking about it all day) :-)

*** Bug 68022 has been marked as a duplicate of this bug. ***

I couldn't really be bothered reviewing the whole patch, but here's a few comments: > my ($userid, $current) = (FetchSQLData()); Nit: Should be: my ($userid, $current) = FetchSQLData(); > ########################################################################### > # Perform group checks > ########################################################################### > > -Status("Checking groups"); Shouldn't the comment be removed? > + if ($result) { > + return 1; > + } > + return 0; Is there something wrong with return $result ( or return $result eq 1 )? > + group_when datetime not null, Could this have a better name, like cached_when or edited_when ? Also the cache table's name should end with _cache_map rather than just _map. > eligable eligible

Fix mattyT's nits, and rename group_when to delta_ts (preferably; for consistency with the bugs table), last_changed (second choice) or edited_when, if it really _must_ have when on the end (why?), and r=gerv. Please check it in ASAP. We can't check anything else in until you do. Gerv

r=bbaetz on the latest set of changes to get this to work with myks's odd schema.

OK... this incorporates the last few nits. I changed group_when to last_changed and did the nits from MattyT with the exception of changing the map name to add the term cache. I think that anything with cache in its name should be something that has exclusively disposable information in it. Splitting the table into a map an a cache would add a huge number of extra db lookups, so that is not an option and calling it a cache strikes me as the wrong thing to do.

CHECKED in with confirmation of successfull conversion on a BMO copy from myk and r=bbaetz, gerv Known problem with highlighing of restricted bugs in buglist will be handled as a speerate bug. Checking in Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.21; previous revision: 1.20 done Checking in CGI.pl; /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v <-- CGI.pl new revision: 1.176; previous revision: 1.175 done Checking in attachment.cgi; /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v <-- attachment.cgi new revision: 1.24; previous revision: 1.23 done Checking in bug_form.pl; /cvsroot/mozilla/webtools/bugzilla/bug_form.pl,v <-- bug_form.pl new revision: 1.104; previous revision: 1.103 done Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.195; previous revision: 1.194 done Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.191; previous revision: 1.190 done Checking in duplicates.cgi; /cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v <-- duplicates.cgi new revision: 1.27; previous revision: 1.26 done Checking in editgroups.cgi; /cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v <-- editgroups.cgi new revision: 1.21; previous revision: 1.20 done Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.29; previous revision: 1.28 done Checking in editusers.cgi; /cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v <-- editusers.cgi new revision: 1.40; previous revision: 1.39 done Checking in enter_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v <-- enter_bug.cgi new revision: 1.73; previous revision: 1.72 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.203; previous revision: 1.202 done Checking in index.cgi; /cvsroot/mozilla/webtools/bugzilla/index.cgi,v <-- index.cgi new revision: 1.8; previous revision: 1.7 done Checking in long_list.cgi; /cvsroot/mozilla/webtools/bugzilla/long_list.cgi,v <-- long_list.cgi new revision: 1.31; previous revision: 1.30 done Checking in post_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v <-- post_bug.cgi new revision: 1.66; previous revision: 1.65 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.146; previous revision: 1.145 done Checking in processmail; /cvsroot/mozilla/webtools/bugzilla/processmail,v <-- processmail new revision: 1.89; previous revision: 1.88 done Checking in query.cgi; /cvsroot/mozilla/webtools/bugzilla/query.cgi,v <-- query.cgi new revision: 1.106; previous revision: 1.105 done Checking in sanitycheck.cgi; /cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v <-- sanitycheck.cgi new revision: 1.48; previous revision: 1.47 done Checking in show_activity.cgi; /cvsroot/mozilla/webtools/bugzilla/show_activity.cgi,v <-- show_activity.cgi new revision: 1.11; previous revision: 1.10 done Checking in showdependencygraph.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v <-- showdependencygraph.cgi new revision: 1.22; previous revision: 1.21 done Checking in showdependencytree.cgi; /cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v <-- showdependencytree.cgi new revision: 1.20; previous revision: 1.19 done Checking in sidebar.cgi; /cvsroot/mozilla/webtools/bugzilla/sidebar.cgi,v <-- sidebar.cgi new revision: 1.9; previous revision: 1.8 done Checking in token.cgi; /cvsroot/mozilla/webtools/bugzilla/token.cgi,v <-- token.cgi new revision: 1.11; previous revision: 1.10 done Checking in userprefs.cgi; /cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v <-- userprefs.cgi new revision: 1.41; previous revision: 1.40 done Checking in votes.cgi; /cvsroot/mozilla/webtools/bugzilla/votes.cgi,v <-- votes.cgi new revision: 1.6; previous revision: 1.5 done Checking in Bugzilla/Search.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v <-- Search.pm new revision: 1.17; previous revision: 1.16 done Checking in contrib/bug_email.pl; /cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v <-- bug_email.pl new revision: 1.14; previous revision: 1.13 done Checking in docs/sgml/administration.sgml; /cvsroot/mozilla/webtools/bugzilla/docs/sgml/administration.sgml,v <-- administration.sgml new revision: 1.16; previous revision: 1.15 done Checking in template/en/default/sidebar.xul.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/sidebar.xul.tmpl,v <-- sidebar.xul.tmpl new revision: 1.6; previous revision: 1.5 done Checking in template/en/default/account/prefs/permissions.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/permissions.html.tmpl,v <-- permissions.html.tmpl new revision: 1.3; previous revision: 1.2 done Checking in template/en/default/global/useful-links.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v <-- useful-links.html.tmpl new revision: 1.10; previous revision: 1.9 done

Comment on attachment 213990 [details] [diff] [review] docs patch about 'Bugzilla Database Tables' section, for 2.18 >> which bugs are belong This should be either present ("which bugs belong"), or present continuous ("which bugs are belonging"). Not sure which one did you mean. >> +group_group_map: This table stores membership of the groups. "membership" is not enough (as a word) to describe the relationship. Maybe we should should we 'relationship', 'inheritance', and explain that this table actually reflects which additional groups are inhereted based on a specific group membership. http://www.ravenbrook.com/project/p4dti/tool/cgi/bugzilla-schema/index.cgi?action=single&version=2.18&view=View+schema says: >> Groups may be configured so that membership in one group automatically confers membership or the "bless" privilege for another group. This is controlled by the group_group_map table. << (not sure if that document is MPL and we can copy from it)

Comment on attachment 214025 [details] [diff] [review] docs patch about 'Bugzilla Database Tables' section, for 2.18 v2 >+group_group_map: This table stores which groups can grant membership >+to others, to other people or groups? (replace "others" with "other users" or "other groups") lose the comma, commas are for lists containing more than two items and certain other things. I don't believe this falls into the latter category. ... and which groups inherit the privilege of the groups. the groups from which another group inherits privileges ?