298024 - Incorrect group control check in sanitycheck.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

sanitycheck.cgi uses a wrong SQL statement when checking for bugs not belonging
to groups whereas membercontrol == CONTROLMAPMANDATORY, line 771:

BugCheck("bugs
         INNER JOIN bug_group_map
            ON bugs.bug_id = bug_group_map.bug_id
         INNER JOIN groups
            ON bug_group_map.group_id = groups.id
          LEFT JOIN group_control_map
            ON bugs.product_id = group_control_map.product_id
            AND bug_group_map.group_id = group_control_map.group_id
         WHERE groups.isactive != 0
         AND group_control_map.membercontrol = " . CONTROLMAPMANDATORY . "
         AND bug_group_map.group_id IS NULL",
         "Are missing groups required for their products");

It should be FROM bugs INNER JOIN group_control_map LEFT JOIN bug_group_map.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

I also remove this useless join to the 'groups' table. The existence of the
group has already been made earlier in CrossCheck(), line 411.

Comment 2

[Frédéric Buclin]

Attachment: patch, v2

My previous patch gives false warnings about missing MANDATORY groups as it
also considers inactive groups. Bugs submitted *after* the group has been
disabled do not belong to that group, as expected.

Bugs being in groups not associated with their corresponding product are
checked against inactive groups too, as that means that the group was removed
from that product *before* being inactive and then no bugs should be associated
with it, even when inactive.

Comment 3

[Joel Peshkin]

Comment on attachment 187460 [details] [diff] [review]
patch, v2

Looks good.  We should probably add a check for group_control_map entries when
inactivating a group, but I don't think we want to force removal.

Also, we should check the edit group controls code to see how it handles
inactivated groups.

Comment 4

[Frédéric Buclin]

Attachment: 2.18 backport, v1

same patch as the one for the tip.

Comment 5

[Joel Peshkin]

Comment on attachment 187506 [details] [diff] [review]
2.18 backport, v1

r=joel by inspection.  Relying on LpSolit's test.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

What are the impacts of the bug and its fix?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Per Frederic's explanation on IRC, this seems like something we should have, as
the bug has security implications (even though it is not in and of itself a
security hole).

Comment 8

[Frédéric Buclin]

Tip:

Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.96; previous revision: 1.95
done


2.18.1:

Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.72.2.4; previous revision: 1.72.2.3
done