Closed Bug 25010 Opened 25 years ago Closed 25 years ago

Need a way to edit the list of available groups

Categories

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

PowerPC
Mac System 8.5
enhancement

Tracking

()

VERIFIED FIXED
Bugzilla 2.12

People

(Reporter: justdave, Assigned: Chris.Yeh)

References

Details

Attachments

(8 files)

Can't find any docs anywhere for how to set up groups.  There's a comment in the 
CHANGES file from when the groups capability was added, that says the 
documentation is in the comments in the makegroupstable.sh file, however, that 
file is no longer in the distribution, and the documentation did not get moved to 
the relavent section of checksetup.pl, or into the README, either.
ok, you're right, there's not much in there.  It does explain a couple of the 

fields in the table, but it doesn't explain how to use them in bugzilla, which is 

what I'm trying to figure out.  As far as I can tell, the current implementation 

permits you to move individual users in and out of groups, and also to move 

individual bugs in and out of groups.  The fact that there is a flag for "user 

has the ability to create and destroy groups" implies that there is somewhere to 

do this, but I can't find it.  The only way I can find to create and destroy 

groups is to log into mySQL and do it yourself.



What I'm really looking for is a way to automatically set a group bit on a bug 

based on what product/version combo the bug is reported under, but if that 

doesn't already exist, that's worthy of a separate bug report for a feature 

request, rather than discussing it here.

OK, from more investigating on this stuff, it looks like there's something 
missing in the current Bugzilla distribution.  As stated above, there is a group 
bit available for "Has the ability to create and destroy groups".  There is 
nowhere in Bugzilla to excercise this privilege.  Did there used to be an 
editgroups.cgi that somehow disappeared out of the tree?  Or was this an intended 
feature that never got implemented?
It never got implemented.
ok, in that case, maybe I'll implement it.  Sounds like an easy enough thing to 
do, just didn't want to duplicate any effort.
Assignee: terry → dave
Severity: normal → enhancement
Summary: Documentation missing for groups → Need a way to edit the list of available groups
Status: NEW → ASSIGNED
QA Contact: matty
Adding Terry to the CC so he sees this.  I just attached my first draft of 
editgroups.cgi to this bug.  Yes, it's been thoroughly tested this time. :)

Everything works except for deleting a group.  I figured people would be mostly 
interested in adding and editing groups, so I did that first.  I'll keep working 
and post a new one as soon as I get the delete stuff in.  (If you try to delete 
one in this version, it'll tell you it's not implimented yet, and explain the way 
I intend to have it work).
This looks very cool, and I'd go ahead and check it in, except for one
thing.

jmrobins@tgix.com (who is now CC'd on this bug, and for whom i just created a
bugzilla.mozilla.org account) just submitted a big patch that optionally allows
groups to automatically be created for each product.  I just checked it in.  I'm
afraid that if his stuff is turned on, then this new editgroups.cgi script might
break some of his things.

I'd like one or both of you to check it out and make sure that your code will
interact well with each other.
A few comments, from a first glance at the code for editgroups.cgi:

1) The TestGroup method does the same thing as the GroupExists method I 
implemented in globals.pl.  You may want to modify this script to just use that 
instead of duplicating the method.

2) When you add a group, you're not matching existing users against the 
userregexp, so people who probably should get put in the group won't be.  Same 
for when you edit a group, and change the regexp.  To see how I'm handling it 
for product bug groups, take a look at editproducts.cgi, where I'm creating and 
editing the groups for products.  (You may or may not want to handle it this 
way, but I thought it worked nicely...)

3) If you rename a bug group associated with a product, it will break my patch.  
There is a simple fix for this, though: When a user tries to rename a group, 
check and see if there is a product by the same name as the group being 
modified.  If so, disallow the change, with a message to the effect of "Cannot 
change the name of a bug group associated with a product."  Changing the 
description is OK, though.

4) One note for the upcoming delete function: When you're deleting group 
permissions from the users, be careful not to delete permissions from any 
superusers, since they should always have membership in all groups.  I handled 
this with a bit of a kludge:  Merely add a check for whether the user's groupset 
is the superuser groupset:
            SendSQL("UPDATE profiles " .
                    "SET groupset = groupset - $bit " .
                    "WHERE (groupset & $bit) " .
                    "AND (groupset != 9223372036854710271)");


Otherwise, it looks pretty nice.  Let me know your thoughts on these comments.
OK, yeah, I don't see much that would break anything other than what Joe already 
mentioned.  I do have comments about Joe's patch though:

1) If you turn this on, it does not create groups for any existing products.  
Each of those products now becomes closed to new bugs because the group-check 
against that user's login fails, not because they aren't in the group, but 
because the group doesn't exist.

2) In the Enter A New Bug page, the popup menu to choose the group state for that 
product appears twice (once defaulting to "only those..." and the other 
defaulting to "those outside of...") along with one popup for each of the other 
groups the user has access to.  This might be from the patch I submitted earlier 
this week, and maybe it didn't get taken into account when Joe did the same 
thing, so now they duplicate each other.

3) If you create a product with spaces in the name, it creates a group name with 
spaces in it.  This breaks bug_email.pl, because it uses a whitespace-separated 
list of group names for the groupset to set the bug to.  There may be other 
things that break because of a space in that name, but I couldn't tell you for 
sure.
I got to thinking about this, and maybe a better way to handle the name of the 

group for a product group, is instead of naming the group to match the product, 

let the user name the group (this will require adding a column to the products 

table).  This could have a number of benefits:



1) If the group already exists, it's used.

2) If the group doesn't exist, it's created.

3) More than one product could conceivably belong to the same group.

4) If the group number is stored in the table instead of the group name,

   then we don't have to worry about the user later renaming the group,

   although it will cause one more thing to check for when deleting a group.



A thought on this...  how about storing the group number in the versions table 

instead of the product table?  Then you could put automatic restrictions on 

visibility to unreleased private beta versions of a product that already has a 

public release.  I actually already submitted this idea in Bug 30189, which comes 

awfully close to this patch you just submitted, but with one more level of 

precision.  (In fact, when I discovered your patch, I almost went and marked that 

bug FIXED, until I realized it couldn't split between versions.)



I'll be happy to work with you on implementing it this way if you're game.

Oh, one thing I forgot:  Some products I like using this with, some I'd rather 
have open to the public and unrestricted.  The present system doesn't allow this.  
it's all or nothing. (thus, my note above about existing products becoming closed 
to new bugs by means of access restriction to everyone when you turn it on).

So, if the user leaves the group name blank when creating a product (or version, 
if we decide to go that route), then no group check would be performed when 
deciding the visibility of that product.
Another thought on this: if we stored the group number with the product or 

version, the usebuggroups and usebuggroupsentry prefs would no longer be needed, 

as this would essentially be set on a product by product basis.  i.e. if you 

didn't want it for that product, just leave the group name blank when you set it 

up.

The diffs I just attached fix the duplicate select-box problem caused by the 
conflict between my patch and Joe's.  My patch from last week was already 
generating a list of select-boxes for each group the user had access to.  Joe's 
patch was creating yet another one if the product had a group defined.  I moved 
Joe's group detection into my list of select-boxes so that if the current select-
box matched the group for the current product, it automatically got set to "only 
people in...".  Consequently, his patch to add the 'groupset' parameter to 
post_bug.cgi got removed, since the 'bit-$bit' parameters were already being 
checked for.  I also fixed the problem here where products without a defined 
group were being denied bug entry.  (it was missing the '&& GroupExists($p)' on 
the initial access check).
A whole bunch of responses to the various thoughts:

> If you create a product with spaces in the name, it creates a group name
> with  spaces in it.  This breaks bug_email.pl, because it uses a
> whitespace-separated list of group names for the groupset to set the
> bug to.  There may be other things that break because of a space in
> that name, but I couldn't tell you for  sure.

I've taken a look at bug_email.pl, and you're right that this would cause a 
problem.  However, (1) bug_email.pl has several other issues already, it would 
appear, (for example, the default group for bugs doesn't exist at all), and (2) 
this is easily remedied with only a minor functional change so that it requires 
a comma or + delimiter, and no longer accepts spaces.  (In fact, I think that it 
should only use commas, but that's another issue entirely.)  I can patch this up 
if you think that might be a good idea.

I don't think that the spaces matter anywhere else.  I did a whole bunch of 
testing on this, including groups with spaces, and didn't notice it anywhere.  I 
didn't test the stuff in contrib, though, which is why I missed this...

> I got to thinking about this, and maybe a better way to handle the name
> of the group for a product group, is instead of naming the group to
> match the product, let the user name the group (this will require adding
> a column to the products table).

I actually dislike the idea of having more than one product in the same product 
group.  It's quite possible to create groups that are not associated with 
products, and put bugs from different products into those groups if you so 
desire.  However, the idea behind this patch was that each product is, in its 
fashion, a separate entity, and should have separate access possible.  If you 
have pieces that are very closely grouped together, maybe they're different 
components of one product?

Similarly, WRT

> how about storing the group number in the versions table instead of
> the product table?

I don't like this as much.  Again, you could easily create an additional access 
group for a version you wanted to keep private.  I think that the product level 
is a more reasonable level for the default permission separation than the 
version level.

> Oh, one thing I forgot:  Some products I like using this with, some I'd
> rather have open to the public and unrestricted.  The present system
> doesn't allow this.  it's all or nothing.  (etc.)

Ooh...  You're right.  I think that this could be easily remedied by the 
addition of a simple flag in editproducts.cgi, for when you add or edit a 
product, for whether or not to create a bug group.  I think it should be 
possible to have a bug group without a userregexp, so just leaving it blank 
doesn't cover it, but a simple yes/no toggle would handle that.  The code 
already can handle products without groups, so being able to continue on with 
products without groups should work fine.  I'll add this in and post the diffs 
here when I get a chance.  (I'm pretty busy at the moment, so it might be a 
couple of days.)

(Although a possible workaround would be to set the userregexp to something that 
would match all users, like "." or "@" or something...  That way, everyone would 
have access to that group.)

> I also fixed the problem here where products without a defined group
> were being denied bug entry.  (it was missing the '&& GroupExists($p)'
> on the initial access check).

Yeah, when I saw your first comment, I looked back at it and realized that that 
was somehow missing...  Not sure how I managed to skip that one, but the simple 
additional check you put in fixes it up just fine.
by the way, Terry, the most-recent fix I posted here needs to be checked in NOW, 

whether we do other stuff here or not.  My previous patch to do the group select 

at bug submission time and Joe's patch for the product groups that have both 

already been checked in break each other, and the most-recent attachment on this 

bug patches up the immediate breakage.  Joe and I are still hashing out the 

editgroups thing so don't worry about that yet.



I have other comments for Joe, but I'm a bit busy here at the moment, and will 

likely get that written up and posted here in an hour or two...

A whole bunch of responses to the various thoughts (back at ya ;) :



> I've taken a look at bug_email.pl, and you're right that this would

> cause a  problem.  (In fact, I think that it  should only use commas,

> but that's another issue entirely.)  I can patch this up  if you think

> that might be a good idea.



I agree here.  Better to fix bug_email to use something more logical. :)



> I actually dislike the idea of having more than one product in the

> same product group.



You do, I don't.  The way I suggested would make it work either way.  It

would be a preference of the site maintainer.



> It's quite possible to create groups that are not associated with

> products, and put bugs from different products into those groups if

> you so desire.



But it doesn't happen automatically without support from the system

we're talking about.  The users of these products would then have to

explicitly set the group every time they report a bug.



> However, the idea behind this patch was that each product is, in its

> fashion, a separate entity, and should have separate access possible.

> If you have pieces that are very closely grouped together, maybe

> they're different components of one product?



No, these are obviously separate products.  I just have the same people

doing internal testing on them, and I don't want group overload with a

billion select-boxes at the bottom of the enter-bug page.  Perhaps what

we really need is a third classification for "isbuggroup".  Right now

it's just 0 or 1.  What if we added a 2 to that possibility and made it

a product group if it's a 2?  Any time you are viewing or entering a

bug, the only #2 select box that would show up is the one related to

that product, but any #1 select boxes they have access to would show up.

(I still favor letting the maintainer pick which group that product gets

set to, personally).



> I don't like this as much. Again, you could easily create an

> additional access group for a version you wanted to keep private. I

> think that the product level is a more reasonable level for the

> default permission separation than the version level.



And once again, the user has to explicitly set the visibility every time

they report a bug, instead of it being set for them, and you don't have

the ability to hide a version number from being visible to the public.



> I think that this could be easily remedied [the all or nothing

> problem] by the addition of a simple flag in editproducts.cgi, for

> when you add or edit a product, for whether or not to create a bug

> group.



Agreed.  Sounds like a good way to do it.  However, if we made it so the

maintainer could choose the name of the group being associated with it,

then just leaving the group name blank would do it.



> I think it should be possible to have a bug group without a

> userregexp, 



It is.  I haven't used any regexps on any groups yet.  Not everyone in

any given domain automatically should have access to it in my situation,

so I've just been setting them individually when they tell me their

account has been created.  All my regexps are just blank.

Seconds on the comment that the patch fix needs checked in. I applied the
stuff in the tree through today, and you can't post a bug with groups
turned on, you get an error in globals.pl @ line 134.

The diffs from dave got line-wrapped and patch chokes on the file, btw.
Please check in the duplicate select-box conflict patch fix.
OK, just a status update for the people who are paying attention to this bug (by 
the way, Terry, the patch to fix the immediate stuff that broke before STILL 
hasn't been checked in!) the major project I was working on is out the door now, 
so I'll be having time available to play with this again.  Sometime in the next 
week or so I'll be revising editgroups.cgi to follow Joe's protocol for group 
definitions, and get the delete stuff added to it.
If you set usebuggroups, play around, and then unset usebuggroups, you'll
still get prompted for the bug's scope.  With the bug_form.pl diff I attached,
you'll no longer get prompted.

The spirit of that diff should probably also be followed in enter_bug.cgi.
Ok, per request from Tara Hernandez I've reviewed and tested and checked in
the patches from 03/09/00 03/11/00 and 04/12/00 and a change to CGI.pl to
make it easy for folks to reach the editgroups.cgi.
Cool, thanks. :)  editgroups.cgi is still a work in progress.  There's still 

stuff I need to do to it to get it to be a little more cooperative with the 

Product Bug Groups stuff, but as long as you're careful, it doesn't actually 

break anything.  I never did get time to work on it again last week.  Life is 

rough.  I'll keep trying.

Was the pref "usebuggroups" added by this feature? After Don checked in the
patch I had to back it out because it broke bugzilla.mozilla.org by not
displaying the "People not in the X group can see this bug" element any more
in the bug display.

During a post mortem I discovered two different buggroup items in our
prefs file, both are turned off and things work just fine. According to
editparams.cgi, the editbuggroups pref associates a bug group with each
product in the database and uses it for querying bugs. By this definition,
it seems correct for us to have it off.

I'm trying to figure out if we need to turn on this pref or if the code
needs changing or what. All we want to do with groups is to restrict
certain bugs so only certain people can view them, but this isn't 
related at all to products.
Yep, he's right.  The 4/12/00 patch on this bug should not have been applied, and 

needs to be backed out.  It breaks what he says here.  Those popups are supposed 

to be there for people that have more than just the product bug groups.  FYI, 

those popups only show up if you have access to that group.  Those who have 

access to nothing won't see them.

Ok, finally the proper stuff is checked in.
Should the link to editgroups.cgi appear in the footer menu if usebuggroups is 
turned off?

If so, should editgroups.cgi check to see if usebuggroups is off and if so 
display a message instead of allowing groups to be created?
That's a thought.  I like that idea.  I think it's worthy of a new bug report 

though because it can be extended to other circumstances as well (for example, I 

have keywords disabled, and edit keywords still shows up in the footer for me).

OK, believe it or not, I *finally* got a free day to work on this again. :)

The following two attachments are the diffs for editusers.cgi and editgroups.cgi 
to get them up-to-date.  My only changes to editusers.cgi is to add a query 
parameter for the list operation, to which allows you to provide the contents of 
the WHERE clause in the query that generates the list of users.  This way I can 
call it from editgroups.cgi and have it generate a list of users that are members 
of that group.  The above patch for the == string comparison is included in this 
(I already applied it on mine), so that one will not need to be checked in, it's 
included in this one.

editgroups.cgi now allows you to delete groups.  It double-checks the user list 
and the bug list for any users or bugs that have that group bit set, and gives 
you override checkboxes to have it nuke them all for you.  It also will generate 
a user list or a bug list showing the items that are still in that group.  It 
will also warn you if you try to delete a group that belongs to a product 
(usebuggroups methodology), but also has an override checkbox for that (warns you 
that the product will become publicly visible).

It does grab a list of people with maintainer privs before it does a global 
"remove this bit from all users", and restores the opblessgroupset value to those 
people afterwards.

This completes the feature list for the scope of this bug.  As soon as these 
patches are checked in, this bug can be marked FIXED (I don't have checkin privs, 
so someone else will have to do that for me). Any additional features needed for 
this file can go in new bug reports.
My part is done here (for the time being).  Reassigning this to Tara so she can 
make sure the attached patches get checked in, or delegate it. (I don't have 
checkin privs).
Assignee: dave → tara
Status: ASSIGNED → NEW
Chris you wanna take a look at this and get it in?
Assignee: tara → cyeh
Status: NEW → ASSIGNED
Oof, there are a lot patches here. Should I start applying from 6/3 or from 
3/11? Where should I start?
The two that were added on 6/17 are the only ones that need to be applied (this 
has been an ongoing project for a while). Everything above that has either 
already been applied or is included within the last two.
great. i'll toss them up on landfill either tonight or tomorrow.
patches applied to http://landfill.bluemartini.com

dave, i am going to enable your create/destroy groups bit so you can play around 
with this and test.
OK, now that I've had the chance to play with it from an almost-user 
perspective...   got some concept issues here now that we need to work out.   The 
editusers and editgroups bits are two separate privileges...  except that due to 
the interrelationship between records in the database, they kind of go hand in 
hand.

1) I would assume that if a user has the authority to edit groups, but not the 
authority to edit users, that they should probably be denied access to delete a 
group if there are users that are members of it?  [unrelated point - I created a 
test group on landfill.  Then I went to delete it, and got a confirmation dialog 
with no warnings.  The current code doesn't check to exclude the maintainer 
except when it does the final delete (it probably should, but it doesn't yet).  I 
should have gotten a warning saying there were users in the group because the 
maintainer would automatically be in it.  So you apparently don't have a 
maintainer set up on landfill]

2) If a user has the ability to create groups, a) should they automatically 
become a member of any group they create?  b) should they automatically be able 
to move users in and out of that group? [can you set me for "able to move others 
in and out of this group" for the test group I just created?]

3) Should a user with the authority to create and destroy groups be allowed to 
delete a group that (s)he's not a member of?  Should they not be allowed to 
delete a group that has members if they don't have the authority to move members 
in and out of the group?

4) If a user has the ability to create groups, but not the ability to edit any 
aspect of a bug, should they be able to delete a group that still has bugs 
belonging to it?

I just realized that the user regexps aren't being handled yet, either (applying 
to existing matching users at the time of group creation - they'll still be 
applied to new signups).

None of this is a showstopper to checking it in (it's still useful as-is).  Just 
stuff that still needs to be done to it.
A few comments:

1) I would assume that if a user has the authority to edit groups, but not the 
authority to edit users, that they should probably be denied access to delete a 
group if there are users that are members of it?

If I read this right, there should be seperation of powers:
1) edit groups is just that, the ability to add and remove users from a group
2) create/destroy groups, the ability to add and remove groups, edit group 
permissions are implied

> So you apparently don't have a maintainer set up on landfill

Actually it was set, and I am the maintainer.

>2) If a user has the ability to create groups, a) should they automatically 
>become a member of any group they create? 

No. They must explicitly add themselves to any group they create. 

>b) should they automatically be able 
>to move users in and out of that group?

Yes, group creation and destruction equates to "super-user priviledges with 
respect to groups" in my way of thinking. This includes setting who is in a 
group or not.

>[can you set me for "able to move others  in and out of this group" for the 
>test group I just created?]

Done.

>3) Should a user with the authority to create and destroy groups be allowed to 
>delete a group that (s)he's not a member of?  Should they not be allowed to 
>delete a group that has members if they don't have the authority to move 
>members 
>in and out of the group?

Yes. If someone has the authority to create groups, they should have the 
authority to destroy it and membership for all users in that group.

>4) If a user has the ability to create groups, but not the ability to edit any 
>aspect of a bug, should they be able to delete a group that still has bugs 
>belonging to it?

I'm thinking that some form of warning should come up, letting them know exactly 
how many bugs belong to the group that is about to be destroyed (in fact, this 
would be a good idea in general)

>I just realized that the user regexps aren't being handled yet, either 
(applying 
>to existing matching users at the time of group creation - they'll still be 
>applied to new signups).

It would be nice to have this ability, but it would need to be tempered with the 
appropriate warnings as any massive db operation would.

>None of this is a showstopper to checking it in (it's still useful as-is).  
>Just stuff that still needs to be done to it.

I agree completely. I think there's a good case here to check in into the trunk 
and then spawn sub-bugs of this so that other people can work on these issues as 
so inclined. There's good work here that can be built upon.

I'm going to play with it some more later on today and probably commit tomorrow.
>If I read this right, there should be seperation of powers:
>1) edit groups is just that, the ability to add and remove users from a group
>2) create/destroy groups, the ability to add and remove groups, edit group 
>permissions are implied

OK, so all of the places throughout bugzilla that test for "caneditusers" or "can 
add/remove other users from this group" should also be checking for "can create/
destroy groups" as well...   or I could just automatically set the blessgroupset 
bits for any group in existence on someone with group create/destroy privs when 
they enter the group editor...  that would save having to edit the rest of 
bugzilla to compensate...

>> So you apparently don't have a maintainer set up on landfill
>
>Actually it was set, and I am the maintainer.

Have you edited your account?  There's a bug in the edituser.cgi that will remove 
your maintainer status if you edit the maintainer account (any bits that aren't 
defined on the form that is shown to you get cleared when you update the 
account).  You still have access to everything that exists at that time, but 
since it cleared the other bits, you won't have access to anything new unless you 
go back in and add yourself to it.

>>4) If a user has the ability to create groups, but not the ability to edit any 
>>aspect of a bug, should they be able to delete a group that still has bugs 
>>belonging to it?
>
>I'm thinking that some form of warning should come up, letting them know exactly how many bugs belong to the group that is about to be destroyed (in fact, this would be a good idea in general)

right now it just has a warning saying "there are still bugs in this group" and 
it has a link to "show me which bugs" which calls buglist.cgi with a query to 
look them up.  Problem here is that above, you said the person should not 
automatically become a member of the new groups they create.  Problem with this 
is if they go to delete said group later, it can't show them which bugs are in 
it, because they aren't a member of the group, so they can't see them.
>I could just automatically set the blessgroupset 
>bits for any group in existence on someone with group create/destroy privs when 
>they enter the group editor...  that would save having to edit the rest of 
>bugzilla to compensate...

That sounds good.


>>Actually it was set, and I am the maintainer.

>Have you edited your account?

Yes. This is probably what happened.

>Problem with this 
>is if they go to delete said group later, it can't show them which bugs are in 
>it, because they aren't a member of the group, so they can't see them.

<insert circus music here> IEEEE. Okay, so if we go so far as above, we should 
probably add the person who can create/destroy groups as a member of each group 
they create by default.

Blocks: 43614
committed changes to trunk. Thanks again Dave.

Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED
Verif feature is present.  All bugs and enhancements should be filed separately.
Status: RESOLVED → VERIFIED
In search of accurate queries....  (sorry for the spam)
Target Milestone: --- → Bugzilla 2.12
Moving closed bugs to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
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: