231975 - renaming "admin" product renames the group "admin"

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P1)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5) Gecko/20031007 Firebird/0.7

I had created an "admin" product and later on renamed it to "Admin(archived)".
This ended up also changing the group-access "admin" string in the groups table.

This broke the ability to add new groups, for editgroups.cgi calls
GroupNameToId('admin') 

It could be worse, one could imagine accidently deleting the group if I were to
have deleted the product "admin". 

I'm using 2.17.6

Reproducible: Always

Steps to Reproduce:

Comment 1

[Dave Miller [:justdave]]

Ooops.  Did this get left in by accident when the group stuff got redone?

        SendSQL("UPDATE products SET name=$qp WHERE id=$product_id");        #
Need to do an update to groups as well.  If there is 
        # a corresponding bug group, we want to update it so it will         #
match in the future.  If there is no group, this
        # update statement will do nothing, so no harm done.  -JMR, 3/8/00     
  SendSQL("UPDATE groups " .                "SET name = $qp, " .
                "description = " .
                SqlQuote("Access to bugs in the $product product") .
                " WHERE name = $qpold");


product changes shouldn't be changing the groups in the new group architecture.

Comment 2

[Gervase Markham [:gerv]]

> product changes shouldn't be changing the groups in the new group architecture.

Really? I thought we still had groups named after products. Line 366 of
editproducts.cgi seems to think so. 

The problem here, I suspect, is when you create a product with the same name as
an existing magic group, and makeproductgroups is turned on.

But, to be honest, the new groups system baffles me (just look at the SQL in
editproducts.cgi from line 50 onwards), and as far as I know Joel has never
documented it (from a hacker's point of view).

Gerv

Comment 3

[Joel Peshkin]

Gerv's first and second point are correct.  We were maintaining backward
compatibility with product groups.  The right thing to do is to make the rename
conditioned on the group meeting the criteria for a "product group" meaning that
productgroups should be enabled and the group should be used for bugs.  Admin
does not meet those criteria.

Comment 4

[Dave Miller [:justdave]]

would it also make sense to check that the group is actually used by that
product's group controls, too?

Comment 5

[Joel Peshkin]

Attachment: Patch

OK, this does the trick.

As a seperate matter, editproducts needs to ensure that a group name is
available before it creates a product by the same name if makeproductgroups is
on.  But... that's a seperate bug and not a new one at that.

Comment 6

[Gervase Markham [:gerv]]

> As a seperate matter, editproducts needs to ensure that a group name is
> available before it creates a product by the same name if makeproductgroups is
> on.

You may say that, but this fix alone wouldn't have prevented the problem the bug
reporter encountered if he had usebuggroups on. We can argue about whether it's
a new bug or not, but it still needs fixing ASAP. :-)

Gerv

Comment 7

[Christian Reis]

Comment on attachment 139797 [details] [diff] [review]
Patch

Doesn't this patch just avoid one particular instance of this problem? What if
I *do* have makeproductgroups on and create a product called "admin"?

A "more correct" fix is probably to forbid creating the product if
makeproductgroups is on (which is arbitrary from the end-user's standpoint,
unfortunately), and have a similar check for renaming a product (since it can
be renamed to a reserved group name). 

However, there's a related problem where makeproductgroups is initially off, a
product called "admin" is created, and makeproductgroups is then turned on.
It's probably the same bug manifesting itself; I just can't see clearly what
should be done -- structurally -- to avoid this happening. At least not apart
from simply (and inconditionally) forbidding the creation of products with the
same name as a reserved group.

Comment 8

[Joel Peshkin]

That's why this fix only puports to prevent the rename from messing up the admin
group.  It is a seperate matter to prevent the user from creating conflicting
product names and the issue of sites upgrading to versions with new reserved
groups also needs to be addressed.

Comment 9

[Dave Miller [:justdave]]

Keep in mind also that in the new world, having a group named the same as a
product doesn't automatically tie it to that product.  There's no reason that
creating a group with the same name as a product should fail...  It should just
alter the group name somehow (add something on the end to make it unique?) and
tie the alternately-named group to the product.

When deleting a product we should look at the list of groups tied to that
product, and only delete ones that are tied ONLY to that product (and not delete
permission groups).  We should probably also only offer to delete those groups
(as an afterthought to the product deletion - next screen if you will).  "The
following groups were tied exclusively to this product and will be deleted when
you click Continue.  Uncheck the box for a group before clicking continue if you
wish to keep a group.  Clicking any other link will abort the group deletion
altogether."

Comment 10

[Dave Miller [:justdave]]

this won't make 2.17.7

Comment 11

[Joel Peshkin]

So, what do we want to do here?

Using same-name as the criteria for renames requires that we never alter that
logic.  Do we want to add a special field to the group indicating that it is
named after a product so that, even if we append a suffix like "_1" to the
group, we always know later that the group is named after a specific product?

Essentially, 
1) during upgrade, we would set the group's tracked_product_id to the product ID
if the name is the same as a product and productgroups is enabled
2) when products and groups are created together by makeproductgroups, the new
group's tracked_product_id would be the same as the product's id.
3) Somewhere in the editproducts or editgroups page, we would add a selection of
which group should be the one tracked to that product.
4) When new system groups are added, any name conflicts would be resolved by
appending a "_1", "_2",...  to the name of the existing bug group.
5) On product rename, the tracking group would always be renamed to the same
name as the product, but _1, _2,... would be appended to resolve any name collision.

Gerv, Kiko, justdave:  Is that the right way to deal with this?

Comment 12

[Christian Reis]

(In reply to comment #11)
> 3) Somewhere in the editproducts or editgroups page, we would add a selection of
> which group should be the one tracked to that product.

Possibly editproducts -- I'm guessing we'd offer a SELECT box of groups there.

> 5) On product rename, the tracking group would always be renamed to the same
> name as the product, but _1, _2,... would be appended to resolve any name
collision.

Only in the case of name collisions, I assume?

It does seem that the only way to avoid mismatches here is to explicitly define
the association in the database -- using identical names to tie both concepts
together isn't going to be practical (specially when names can be changed easily
via the UI); I think your proposal addresses that nicely.

Comment 13

[Dave Miller [:justdave]]

As much of an edge case as this is, I don't think it'd be right to release 2.18
with this still happening.

Here's what I think:  (let's simplify this a little).

1) Don't ever rename groups when you rename a product, period.  The group is
tied to the product by the group ID, not by the name.  Once the product is
created, the new group system could care less whether the group has the same
name as the product or not, it's all in the group controls.
2) Because we don't care if the name matches the product, if someone tries to
create a product that already has an existing group by that name, just tack
something on the end of the new group's name to make it unique, tie it to the
product, and be done with it.  As above, we don't really care if the name
matches anymore, all we care is that it gets tied to the product when the
product is created.

Comment 14

[Joel Peshkin]

Attachment: Patch does not rename groups to match product names

This patch removes the renaming of groups.  Also, if a new product has the name
of an existing group, the group created for it gets an underscore appended to
its name.
This also removes the automatic association of any non-product groups with
every new product when makeproductgroups is enabled.  That was an unecessary
carryover from 2.16.

Comment 15

[Jouni Heikniemi]

Comment on attachment 146334 [details] [diff] [review]
Patch does not rename groups to match product names

>+        my $productgroup = $product;
>+        while (GroupExists($productgroup)) {
>+            $productgroup .= '_';
>+        }
>         SendSQL("INSERT INTO groups " .
>                 "(name, description, isbuggroup, last_changed) " .
>                 "VALUES (" .
>-                SqlQuote($product) . ", " .
>+                SqlQuote($productgroup) . ", " .

Of course, the code above can overrun the maximum group name length. That's
extremely unlikely, though - product name's max length is 64 and group name has
a max of 255, so it would require quite a few oddly named groups to achieve
that result.

I'd be ready to leave the issue above, so r=jouni. However, my testing platform
was nuked in the middle of trying the patch, so I didn't test it very
thoroughly.

Comment 16

[Joel Peshkin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.48; previous revision: 1.47
done