Closed
Bug 243463
Opened 20 years ago
Closed 20 years ago
Use a param and other controls to protect new charts from leaking information
Categories
(Bugzilla :: Reporting/Charting, defect, P1)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: bugreport, Assigned: bugreport)
Details
Attachments
(1 file, 2 obsolete files)
5.83 KB,
patch
|
justdave
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•20 years ago
|
Priority: -- → P1
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Updated•20 years ago
|
Flags: blocking2.18?
Assignee | ||
Comment 1•20 years ago
|
||
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.
Assignee | ||
Updated•20 years ago
|
Attachment #148362 -
Flags: review?
Comment 2•20 years ago
|
||
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 :-) Gerv
Assignee | ||
Comment 3•20 years ago
|
||
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.
Comment 4•20 years ago
|
||
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. :)
Assignee | ||
Comment 5•20 years ago
|
||
That would work so long as we don't (by default) auto-create new charts for new products
Comment 6•20 years ago
|
||
Gotta run... back on Sunday evening. Don't check anything in ;-) Gerv
Comment 7•20 years ago
|
||
WONTFIX in deference to bug 225687. Let's do it right and get it over with.
Status: NEW → RESOLVED
Closed: 20 years ago
Flags: blocking2.18? → blocking2.18-
Resolution: --- → WONTFIX
Target Milestone: Bugzilla 2.18 → ---
Assignee | ||
Comment 8•20 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Target Milestone: --- → Bugzilla 2.18
Assignee | ||
Updated•20 years ago
|
Attachment #148362 -
Flags: review?(justdave)
Comment 9•20 years ago
|
||
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. Gerv
Assignee | ||
Updated•20 years ago
|
Target Milestone: Bugzilla 2.18 → ---
Comment 10•20 years ago
|
||
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 currently. Maybe I can be convinced otherwise, but these things in params where you enter the name of a group just kind of bug me.
Assignee | ||
Updated•20 years ago
|
Attachment #148362 -
Attachment is obsolete: true
Attachment #148362 -
Flags: review?(justdave)
Attachment #148362 -
Flags: review?
Assignee | ||
Comment 11•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #152268 -
Flags: review?(justdave)
Comment 12•20 years ago
|
||
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). >Index: defparams.pl >+ 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-
Updated•20 years ago
|
Attachment #152268 -
Attachment is obsolete: true
Attachment #152268 -
Flags: review?(justdave)
Assignee | ||
Comment 13•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
Attachment #148362 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #152281 -
Flags: review?(justdave)
Comment 14•20 years ago
|
||
Putting this back on the blocking list so I can find it :)
Status: REOPENED → NEW
Flags: blocking2.18- → blocking2.18+
Target Milestone: --- → Bugzilla 2.18
Comment 15•20 years ago
|
||
Comment on attachment 152281 [details] [diff] [review] Patch with renamed parameter >Index: chart.cgi >+UserInGroup(Param("chartgroup")) >+ || 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+
Updated•20 years ago
|
Flags: approval+
Assignee | ||
Comment 16•20 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•