276230 - Create UI for assigning groups to chart categories

Status: NEW

(Bugzilla :: Reporting/Charting, defect)

Description

[Gervase Markham [:gerv]]

We need to create a UI to allow admins to define which groups each of the chart
categories is in.

Comment 1

[Gervase Markham [:gerv]]

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

Comment 2

[Gervase Markham [:gerv]]

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

Comment 3

[Joel Peshkin]

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.

Comment 4

[Gervase Markham [:gerv]]

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

Comment 5

[Dave Miller [:justdave]]

Agreed, this needs to be done.  Is someone working on it?

Comment 6

[Gervase Markham [:gerv]]

It's next on my list after bug 73665.

Gerv

Comment 7

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

"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

Comment 8

[Gervase Markham [:gerv]]

Notwithstanding my chronic inability to produce a patch, I suggest this does
fall into the second of those categories.

Gerv

Comment 9

[Dave Miller [:justdave]]

Agreed, this is a serious usability issue for people using charts on systems
that also use groups.

Comment 10

[Max Kanat-Alexander]

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.

Comment 11

[Dave Miller [:justdave]]

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.

Comment 12

[Dave Miller [:justdave]]

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.

Comment 13

[Frédéric Buclin]

Only security and dataloss fixes will be accepted on the 2.20 branch.

Comment 14

[Frédéric Buclin]

Nothing has been done so far... retargetting to a more realistic milestone. :)

Comment 15

[Frédéric Buclin]

I think I can start working on it as soon as gerv has reviewed bug 389396.

Comment 16

[Frédéric Buclin]

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).

Comment 17

[Max Kanat-Alexander]

(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.

Comment 18

[Frédéric Buclin]

/me gives up.

Comment 19

[Max Kanat-Alexander]

Attachment: Outline of Idea

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.

Comment 20

[Gervase Markham [:gerv]]

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

Comment 21

[Max Kanat-Alexander]

(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.

Comment 22

[Max Kanat-Alexander]

  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.

Comment 23

[Frédéric Buclin]

(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.

Comment 24

[Max Kanat-Alexander]

(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.

Comment 25

[Frédéric Buclin]

(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).

Comment 26

[Frédéric Buclin]

(In reply to comment #25)
> makes charts useless IMO (even more accurate if some bugs are deleted).

s/accurate/inaccurate/

Comment 27

[Max Kanat-Alexander]

(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.

Comment 28

[Max Kanat-Alexander]

(In reply to comment #25)
> With your
> suggestion, you get inaccurate data based on the *current* state of bugs,

  Inaccurate how?

Comment 29

[Frédéric Buclin]

(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.

Comment 30

[Max Kanat-Alexander]

(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.

Comment 31

[Max Kanat-Alexander]

  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.

Comment 32

[Max Kanat-Alexander]

  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.)

Comment 33

[Gervase Markham [:gerv]]

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

Comment 34

[Max Kanat-Alexander]

(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.

Comment 150

[Julien Cristau [:jcristau]]

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