Closed Bug 243463 Opened 17 years ago Closed 17 years ago

Use a param and other controls to protect new charts from leaking information


(Bugzilla :: Reporting/Charting, defect, P1)




Bugzilla 2.18


(Reporter: bugreport, Assigned: bugreport)



(1 file, 2 obsolete files)

The New Charts, by default, can divulge information about products to which a
user is supposed to have no access.  This seems unavoidable on migration (see
bug 225687), so we need to prevent inadvertant leaks.

To do this, a new param, "seriesgroup," will define the name of the group of
users who are permitted to use new charts.  If left default (blank), no users
will be able to use new charts.  In general, an administrator will secure any
migrated series' before enabling untrusted users to use the New Charts feature.

To prevent inadvertant creation of new leaks, new products will only have new
series datasets auto-created if the administrator checks a box on the product
creation form.
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
Flags: blocking2.18?
Attached patch The patch (obsolete) — Splinter Review
Adds a param to keep the series feature restricted to appropriate users and
adds a checkbox to the new product add page to determine if series data should
automatically be collecte for a new product.
Attachment #148362 - Flags: review?
My concern here is that we are giving ourselves a backwards compatibility
problem, without really giving admins the tools they need to manage this properly.

I know I've been snowed under and haven't done this "the right way" yet. OTOH,
it's still slightly unclear as to what the right way is. It may even be that the
architecture of this is just wrong, and we need to remove it for this release.
Better that than release something that doesn't work right.

I have to sleep now, and I'm away at the weekend, but I'll put in some heavy
thinking ASAP. Other people's thoughts would be most welcome :-)

As you point out, this is a temporary fix to keep from leaking until bug 225687
can be done.  I do not, however, think that it will be a migration problem. 
Making creation of new series data optional should pose no problem.   In my own
case, there are lots of series' that I would like to have and the ones that
would be autocreated are not among them.

Once bug 225687 lands, it is more likely that sites would be able to open up
access to series data to a larger audience.
OK, how about if we *don't* migrate any data, and keep the old chart system in
place?  The new chart system can exist, but would be devoid of data until people
start creating things to chart.  Perhaps a single sample chart or something.  At
a later date, we can provide an option to migrate the data from the old chart
system when we're ready to get rid of it.

The migration and the series creation when new products are created are the only
reasons we have leaks.  An administrator who makes a chart publicly available
otherwise should know what he's doing. :)
That would work so long as we don't (by default) auto-create new charts for new
Gotta run... back on Sunday evening. Don't check anything in ;-)

WONTFIX in deference to bug 225687.  Let's do it right and get it over with.
Closed: 17 years ago
Flags: blocking2.18? → blocking2.18-
Resolution: --- → WONTFIX
Target Milestone: Bugzilla 2.18 → ---
Looks like it is time to dust this off...

The more comprehensive fix could still be provided on a later 2.18 candidate if
it becomes available.
Resolution: WONTFIX → ---
Target Milestone: --- → Bugzilla 2.18
Attachment #148362 - Flags: review?(justdave)
It's not time to dust this off. I'm in the middle of writing a patch to fix this
the right way. I should have something tonight or tomorrow night.

Target Milestone: Bugzilla 2.18 → ---
Comment on attachment 148362 [details] [diff] [review]
The patch

As mentioned earlier, I'd rather see a specific group for this rather than a
param asking which group.  This way, applying groups to it is a matter of group
inheritance.  The group can be introduced inheriting only 'admin', and with a
blank regexp.  Once the series permissions stuff lands, we can change it to use
'.*' for the default regexp on new installs, like editbugs and canconfirm do

Maybe I can be convinced otherwise, but these things in params where you enter
the name of a group just kind of bug me.
Attachment #148362 - Attachment is obsolete: true
Attachment #148362 - Flags: review?(justdave)
Attachment #148362 - Flags: review?
Attached patch v2 - use a system group (obsolete) — Splinter Review
Attachment #152268 - Flags: review?(justdave)
Comment on attachment 148362 [details] [diff] [review]
The patch

Lets go with the param specifying which group for now, since the group-name
prefix thing for system groups is still up in the air at the moment.  We can
come back and change it later when that's resolved if we ever get around to
fixing timetrackinggroup (we can do them both at once at that point).


>+   name => 'seriesgroup',

Let's call it 'chartgroup'.  It's called Charts in the UI, so it may as well
make sense to the admin who's looking for it. :)

>+   desc => 'The name of the group of users who can use the "New Charts" ' .
>+           'feature. Administrators should ensure that the public categories ' .
>+           'and series definitions do not divulge unwanted information ' .
>+           'enabling this for an untrusted population. If left blank, ' .
>+           'no users will be able to use New Charts.',

Something's missing in this description...

I think it needs the word "before" between "information" and "enabling".  Is
that what you meant?

The rest looks good at first glance.
Attachment #148362 - Attachment is obsolete: false
Attachment #148362 - Flags: review-
Attachment #152268 - Attachment is obsolete: true
Attachment #152268 - Flags: review?(justdave)
Attachment #148362 - Attachment is obsolete: true
Attachment #152281 - Flags: review?(justdave)
Putting this back on the blocking list so I can find it :)
Flags: blocking2.18- → blocking2.18+
Target Milestone: --- → Bugzilla 2.18
Comment on attachment 152281 [details] [diff] [review]
Patch with renamed parameter

>Index: chart.cgi

>+    || ThrowUserError("authorization_failure", 
>+                     {action => "use this feature"});

Ewwww...  but it's already used this way elsewhere, so this can go in now, and
I filed bug 249930 to clean it up.

Otherwise, seems to look and work pretty good.
Attachment #152281 - Flags: review?(justdave) → review+
checked in.

Note:  There is a side agreement that a more comprehensive fix will be permitted
to be landed on the 2.18 branch once it is available.
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.