Open Bug 276230 Opened 20 years ago Updated 2 years ago

Create UI for assigning groups to chart categories

Categories

(Bugzilla :: Reporting/Charting, defect)

2.17.7
defect
Not set
normal

Tracking

()

People

(Reporter: gerv, Unassigned)

References

Details

Attachments

(1 file)

We need to create a UI to allow admins to define which groups each of the chart
categories is in.
The table we are manipulating here is category_group_map. A user has to be in
all the groups for that category in order to be able to see or plot it.

Q1) Is it right that we have a large matrix of every chart category and every
group which we need to allow admins to manipulate? Or is the list of groups
restricted in some way? I can't think of one.

Q2) Is a big table of checkboxes the best UI? There could be a fairly large
number of both groups and categories... But any restricted view makes it harder
for an admin to get an overview of what the settings are, or copy settings from
one column/row to another by eye.

Q3) Is this a new function of editgroups.cgi, chart.cgi or a new
editchartgroups.cgi?

Gerv
Severity: enhancement → major
Target Milestone: --- → Bugzilla 2.18
Requesting blocking 2.20. Without this, we are still in the situation that
charts and groups are incompatible, because you can't keep chart data properly
secure. IMO, doing a stable release where this is true is doing a disservice to
our users.

Gerv
Flags: blocking2.20?
I think that the solution for this wants to be closely coupled to bug 257350. 
Once you have a dataset that is created using only the bugs accessible to a
certain user (instead of the administrator), the question of who should see that
dataset becomes a bit less arbitrary.

The mechanisms that could come into play here might include 
a) user visibility (Only let users "see" the data if they are permitted to "see"
the user onder whose privileges the chart is run??) 
b) Create a "hypothetical" user object under whose permissions the search is run
and require that the users viewing a series have permissions that are a superset
of the hypothetical user.
c) keep the series_group_map completely distinct from the permissions under
which the data is run.

One additional note.  In sites where it is necessary to keep user communities
totally isolated from each other (visibilitygroups on), you would not want to
let a user make a series that is shared with anyone who does not have permission
to "see" that user.  It is not a big problem if this is not accounted for in
this bug because we can always restrict who can make public queries, but it
would be really cool if we had a good way to do that as well.


Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Joel: you seem to be talking about what the restriction mechanism should be. We
already have a mechanism; this bug is about providing a UI for it.

Gerv
QA Contact: mattyt-bugzilla → default-qa
Agreed, this needs to be done.  Is someone working on it?
Flags: blocking2.20? → blocking2.20+
It's next on my list after bug 73665.

Gerv
Flags: blocking2.18.2?
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
Flags: blocking2.18.2? → blocking2.18.2+
"If it's not a regression from 2.18 and it's not a critical problem with
something that's already landed, let's push it off." - Dave
Flags: blocking2.20+
Flags: blocking2.18.2+
Notwithstanding my chronic inability to produce a patch, I suggest this does
fall into the second of those categories.

Gerv
Flags: blocking2.20?
Whiteboard: [wanted for 2.20]
Agreed, this is a serious usability issue for people using charts on systems
that also use groups.
Flags: blocking2.20?
Flags: blocking2.20+
Flags: blocking2.18.2+
I can't think of a bug larger than this that we need to land for 2.20. That
makes this pretty much the most important 2.20 bug there is that I know of.
Severity: major → critical
Blocks: 267541
Flags: blocking2.18.2+ → blocking2.18.4+
Whiteboard: [wanted for 2.20] → bug awaiting patch [wanted for 2.20]
The infrastructure for this already exists, we just need UI to edit the fields,
right?

Is there a screen in chart.cgi for editing a specific category?  If so, this
should probably go on that screen, just a list of checkboxes for all of the
available groups, like you have on a bug.  This gets rid of the need for a
matrix.  The groups should only be visible to admins and the owner of the
category, and in the case of a non-admin, should only show groups the user is a
member of.

Gerv is on vacation again, someone else is going to have to take this if we want
it to see 2.20.
We can still take this on a branch if it's done cleanly in a non-intrusive way,
but we're not going to hold any release for it or we'll be hear until next year
waiting.
Flags: blocking2.20-
Flags: blocking2.20+
Flags: blocking2.18.4-
Flags: blocking2.18.4+
Severity: critical → major
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Only security and dataloss fixes will be accepted on the 2.20 branch.
Severity: major → normal
Whiteboard: bug awaiting patch [wanted for 2.20]
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Nothing has been done so far... retargetting to a more realistic milestone. :)
Target Milestone: Bugzilla 2.22 → ---
Assignee: gerv → charting
I think I can start working on it as soon as gerv has reviewed bug 389396.
Depends on: 389396
Here is my opinion about this:

- The category, if it matches a product name, should have the same group restrictions as the product itself. $user->get_selectable_products() will let us quickly know if you can access a category or not. If you want to make the category public, then make the product visible by everybody.

- The category, if it doesn't match a product name, should be private by default, unless otherwise specified by the creator of the category. The category can only be restricted to groups the creator belongs to, as for shared queries. If no group is selected, the category is only visible by its creator (same behavior as for shared queries). Note that as a user must be in the chartgroup in order to use new charts, sharing your category with this chartgroup is a synonym for "public", so there is no need for a magical hack to make a category public.

- The subcategory behaves the same way as categories with no matching name (the option above): private by default, and can be shared with groups you belong to. We don't care if a subcategory matches a component name; there is no group restriction at the component level.

- The series itself can be public or not. If not, only the creator of the series can view the series. If public, it's accessible to users being in both groups defined by the category and subcategory (AND relationship).


Now about some related bugs:

- bug 257350: everybody having access to the new charting system (i.e. users in the chartgroup) should be allowed to make his series public. Users can already share their queries with other users, and we never got any bug report about users abusing this feature (probably because such users must be in the querysharegroup, which limits undesired people from using the feature). No reason to think users are going to abuse this feature here.

- bug 267240: renaming a product should rename the category having the same name, to keep group restrictions intact. Renaming a component should rename the corresponding subcategory as well.

- bug 451716: if a product is deleted, the admin can either delete the series at the same time, or accept to make the corresponding category public. I already attached a patch there. For now, all categories are public so the patch doesn't enforce this statement yet. This will be done in a separate bug.


I hope to have fixed all these bugs on time for Bugzilla 3.8 (or 3.6 if I have enough free time, and gerv is around for prompt reviews).
Assignee: charting → LpSolit
Status: NEW → ASSIGNED
(In reply to comment #16)
> - The category, if it matches a product name, should have the same group
> restrictions as the product itself. $user->get_selectable_products() will let
> us quickly know if you can access a category or not. If you want to make the
> category public, then make the product visible by everybody.

  No, security based around the name of the category is a bad idea, because the product name can change without the category name changing.

  If you want to more directly link categories to products (such as by a category_product_map table) then I'd accept that solution in terms of security, though it would seem like an odd solution in terms of flexibility.

  I think we may have to think a bit about the architecture of categories and series, and perhaps about the architecture of New Charts in general, before proceeding with adjusting its security model.

  One option is to have the charting system note, every time it gets a result list of bugs from a series, what groups the bugs are in, and then only allow access to that day's data to users in that group. (That's a fairly complex but rather airtight security solution.)

> - bug 257350: everybody having access to the new charting system (i.e. users in
> the chartgroup) should be allowed to make his series public.

  That would make me a bit nervous, as a corporate manager, because it gives people the power to expose private data to the public. Shared searches do not.

> - bug 267240: renaming a product should rename the category having the same
> name, to keep group restrictions intact. Renaming a component should rename the
> corresponding subcategory as well.

  Hmm. I think this is only acceptable if they are linked in some fashion more than their name, such as by a mapping table, because otherwise we could be renaming things that were named by coincidence, or named after an old version of the same Product.

> - bug 451716: if a product is deleted, the admin can either delete the series
> at the same time, or accept to make the corresponding category public. I
> already attached a patch there. For now, all categories are public so the patch
> doesn't enforce this statement yet. This will be done in a separate bug.

  That sounds reasonable.
/me gives up.
Assignee: LpSolit → charting
Status: ASSIGNED → NEW
Attached patch Outline of IdeaSplinter Review
Here's a rough outline (in code) of the idea that I discussed with LpSolit on IRC. It actually seems to perform pretty well, though I haven't tested it on anything as large as bmo. The actual implementation wouldn't take much more than this, except that there's a fair bit of rearchitecture we need to do around shared searches first, and there would be some pretty involved upgrade code.
Assignee: charting → mkanat
Status: NEW → ASSIGNED
Max: my understanding is the idea is to remove charts and replace them with stored data about each user's saved searches? Is that right?

If so, that doesn't sound good to me. If a user comes and says "tell me how the NEW bug count has changed in my component over the last six months", then telling them "go back in time six months and define a search for that" isn't a particularly helpful answer.

You also lose the useful hierarchical classification we have now, which allows a large number of charts to be organized in a way people can find them.

Gerv
(In reply to comment #20)
> Max: my understanding is the idea is to remove charts and replace them with
> stored data about each user's saved searches? Is that right?

  No. What's attached is the absolute bare minimum of a rough outline of a system that is designed only to replace New Charts. LpSolit and I had a pretty involved IRC discussion about the design. The rough idea is that we can enhance Saved Searches to the point that we can re-use the Shared Search code for what's currently being used by Series, and we can do security by storing bug ids for each chart run and checking access to them when rendering the chart. (This is a fool-proof security system as far as exposing bug data.)
  
> If a user comes and says "tell me how the
> NEW bug count has changed in my component over the last six months", then
> telling them "go back in time six months and define a search for that" isn't a
> particularly helpful answer.

  You're describing an existing fundamental problem with basic concept of New Charts. I'm just talking about fixing the *architecture* of the extant system.

> You also lose the useful hierarchical classification we have now, which allows
> a large number of charts to be organized in a way people can find them.

  That's accounted for by the discussed design--we would allow users to categorize their Shared Searches by Product.
  To be clear, what I attached is an experiment to see how the underlying architecture would perform, it's not an implementation of the user-facing system design.
(In reply to comment #21)
> we can do security by storing bug
> ids for each chart run and checking access to them when rendering the chart.

During our discussion, I said that I was against this idea, i.e. storing bug IDs themselves rather than counts only, because you would compare these IDs to the current group restrictions of the bugs rather than group restrictions set when the query was run. We should store counts only, not bug IDs.
(In reply to comment #23)
> During our discussion, I said that I was against this idea, i.e. storing bug
> IDs themselves rather than counts only, because you would compare these IDs to
> the current group restrictions of the bugs rather than group restrictions set
> when the query was run. We should store counts only, not bug IDs.

  Yes, and I pointed out that this expects everybody sharing a search to know that not only are there no confidential bugs in their search *now*, but that there never *will* be any confidential bugs in their search (or that the entire contents of their search will not some day become confidential to some of the users its shared to, without their realizing it). That is of course an unreasonable and impossible burden to place on a user when it comes to security.
(In reply to comment #24)
>   Yes, and I pointed out that this expects everybody sharing a search to know
> that [...]

This doesn't expect anything from those sharing their queries. With your suggestion, you get inaccurate data based on the *current* state of bugs, which makes charts useless IMO (even more accurate if some bugs are deleted).
(In reply to comment #25)
> makes charts useless IMO (even more accurate if some bugs are deleted).

s/accurate/inaccurate/
(In reply to comment #25)
> This doesn't expect anything from those sharing their queries. 

  We can't store the per-group count, because of bugs in multiple groups. We talked about that.
(In reply to comment #25)
> With your
> suggestion, you get inaccurate data based on the *current* state of bugs,

  Inaccurate how?
(In reply to comment #27)
>   We can't store the per-group count, because of bugs in multiple groups. We
> talked about that.

We talked about that, yes. We didn't deeply investigate if that's unsolvable or not.


(In reply to comment #28)
>   Inaccurate how?

Data changes every time you add or remove a bug from a group. And completely miss them if the bug is deleted. Data also change when your privileges change. So you would never get reliable charts.
(In reply to comment #29)
> We talked about that, yes. We didn't deeply investigate if that's unsolvable or
> not.

  If it's solveable, I'd definitely prefer to store the bug counts--it'd be way easier in terms of performance. But I don't think it is--SQL databases are just not good at storing data where one column has to contain multiple values.

> Data changes every time you add or remove a bug from a group. And completely
> miss them if the bug is deleted. Data also change when your privileges change.
> So you would never get reliable charts.

  I think that's OK, actually. You can see the whole History of a bug on show_activity when it's unlocked. Why not show its history in the chart, as well? Same for a hidden or deleted bug--you can't see its history, it doesn't show up in the chart. That seems like consistent behavior on Bugzilla's part.
  And by the way, in my experiments so far, the bug_id system is very fast, actually, even when the chart_run table becomes huge, because we can use indexes very efficiently. I do still have to experiment with years of data on a bmo-sized installation, though.
  Mmm, there is one way we could store a per-group count--we could run the search once for every single group that any bug was restricted to in the results, and store a count of how many bugs were visible if you were in the group, and then take the MAX() of all those numbers for every group the current user is in. But that could actually be really slow on the rendering side, because it would involve an IN() clause of the user's groups. It's another experiment that could be done, though.

  It would greatly increase the time of running the searches, though. (Although currently they run really fast, so I don't know how much of an issue that would be.)
I think you guys are over-engineering this.

A better solution is what happens now ;-) That is, each _search_ is in a group. If you are a member of the group, you can see the results. If you aren't, you can't. 

This way, users get to decide whether the numbers themselves (as opposed to the individual bugs) are sensitive and, if so, how sensitive. E.g. a plot of "security bugs in Firefox" might be restricted to the security group, but a plot of "NEW bugs in Firefox" might include security bugs, but still be not in a group and available to anyone, because it was decided that this figure is not sensitive.

This is much better than showing different people different numbers. I don't want a situation where I say "Fred, check out this chart" and his version looks different to mine.

Gerv
(In reply to comment #33)
> A better solution is what happens now ;-) That is, each _search_ is in a group.
> If you are a member of the group, you can see the results. If you aren't, you
> can't. 

  That's already the way that shared searches work. It doesn't allow people to expose sensitive data because each bug is checked for security when the search is run, not when it's saved.

  It actually effectively isn't the way that charts currently work, because there is currently no UI that allows users to restrict charts to groups.

> This way, users get to decide whether the numbers themselves (as opposed to the
> individual bugs) are sensitive and, if so, how sensitive.

  That might work for Firefox, but it doesn't work in certain corporate environments where a large subset of bug data is confidential and users are restricted to seeing only a small portion of it.
Assignee: mkanat → charting
Status: ASSIGNED → NEW
Flags: needinfo?(pubggamer131)
Flags: needinfo?(keyurverma.keyur)
Flags: needinfo?(allseotol)

Restricting comments on this old bug to cut down on spam.

Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.