Closed Bug 225687 Opened 21 years ago Closed 20 years ago

Add group controls to new charts

Categories

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

2.17.5

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: gerv, Assigned: gerv)

References

Details

Attachments

(3 files, 5 obsolete files)

The new charts did not get group access controls in their first iteration. We
need to think how to add these without making things too complicated.

Gerv
Priority: -- → P1
OK, here's the plan (review requested please, Joel):

A new chart_group_map table - chart_id against group_id, with the two together
being the PK. This is exactly like bug_group_map. The Edit Chart screen allows
you to change the group membership of a chart.

Most of the charts initially created by the system are for bugs in a particular
product. These charts will automatically be placed into any groups for which
either MemberControl or OtherControl is "Mandatory" for that product.

When editing, all possible groups in the system will be listed. This isn't ideal
UI, but as charts aren't actually linked to products in any way more than their
naming text, we have no option but to let users put the charts in any
combination of groups they like.

If a chart is in a group, then a user needs to be a member of that group to have
the chart in their list, or do anything else with it. Any charts on URLs to
which they don't have access are silently dropped.

After we've fixed this bug, we can safely fix bug 211164.

Gerv
Blocks: 211164
Most of the plan sounds good.  The only complication is listing all possible
groups in the system when editing.  That would be an information leak on my site
and would also permit a user to create charts that he himself could not see. 
Let's list only the groups of which the user editing the chart is a member.

Joel: will the list of groups a person is a member of always be a superset of
the groups the chart is a member of, given that the person concerned is able to
edit it? If so, that sounds a much better plan.

Gerv
Right.  A user should never have any edit capability to a chart that is a member
of any groups of which that usr is not a member.

The interesting question is....
  If I am a member of group X as well as group Y and I create a series
accessable by all members of Group Y.  
a) Should bugs where membership in group X is needed to see the bug still be
counted in the series?
b) Should I have control over which bugs get counted in the series?
c) What about bugs that I don't have rights to see?  Should they be counted?

  This raises some interesting cases.  If the answers to A and C are yes and no
respectively, that would mean that the behavior of the series depends on who
authored the series.   If C is yes (and, to a lesser extent if A is yes), there
is some information potentially being leaked.

>   If I am a member of group X as well as group Y and I create a series
> accessable by all members of Group Y.  
> a) Should bugs where membership in group X is needed to see the bug still be
>    counted in the series?

Yes.

> b) Should I have control over which bugs get counted in the series?

The bugs that get counted are the ones you can see.

> c) What about bugs that I don't have rights to see?  Should they be counted?

No.

The rule is that if the creator can see it, it's counted. That person is
responsible for putting the series in the correct groups to avoid information
leakage.

There are simpler examples of leakage. For example, if you have a public series
which counts bugs in a private product, even if the category and subcategory
don't give it away, the name of the product appears in the series URL, and so
leaks. The answer is not to put yourself in this situation :-)

Agreed, we may need to warn about this in the manual.

Gerv
Joel: anything more to add? I would like to implement this before b.m.o.
updates, probably on Thursday.

Gerv
Nope.  Just that.
Flags: blocking2.18+
Target Milestone: --- → Bugzilla 2.18
dropping the blocker, because it's already resolved...

this is kinda sorta a security issue, so we really need to have this fixed
before we RC.  Any status here?
No longer blocks: 211164
Severity: normal → critical
This also currently leaks by showing ALL products as Categories in the Create
Chart UI.  It should only show selectable products.
Severity: critical → blocker
Joel: the issue is that the list of products is just a list of bits of text.
People can rename them at will. We can't dynamically detect which are selectable
products; we can only set permissions on each at creation time. And even that's
tricky - do we hide a top-level category if everything under it is hidden? Lots
of questions...

Gerv
We are discussing this on IRC right now.

Perhaps converted series should be non-public by default?  
This needs a fix rather urgently.

How about...
a) If a category has series' in it, but you can't see any of them, you can't see
the category name either.
b) Checksetup should make converted products' series data private to the (first)
administrator.
c) When creating products, have a check box to control autocreation of standard
series stuff (default=no)

Joel:

a) Fine. Good plan.
b) Why can't we go with the current plan of record in this bug, which is that
they should have the same groupset as the product which they represent (see
comment #1)?
c) Why? If we do b), then there's no privacy leak.

If you want to have an IRC discussion about this, cool, but I need more warning
than "now" :-)

Gerv
That would work with one exception we need to cover.

When someone creates a new product and it is not yet in any groups, editproducts
will auto-create a set of series queries for it.  When the product is
subsequently placed in groups, there is no coupling to keep the series
restrictions in sync with the product restrictions.  We need a strategy for
this.  That could be to make editproducts default to NOT creating the series
stuff or else we could invent a coupling by something other than name.  I faver
the former.
Hmm. Drat. You're right.

Alternative: how about an option on the Edit Product page, which basically says
"Find any series whose Component is equal in name to my product, and change the
groupset of any public queries inside it to match the current state of my product."

This would provide a way of keeping them in sync optionally, but maintain the
loose coupling between products and groups.

Gerv
That approach is fine for the conversion, but I think we should make creating
new series' for new products optional and not enabled by default. 

After that, I do not think we can assume that the names will track each other at
all.  If we want to make soemthing in a series follow the name of a product, we
should do it by product ID and add a field to the table to indicate the
"coupled" product id.

I suspect a comprehensive solution to this will not make 2.18.  Should we use
the other approach in the meantime?
Progress: some way through the patch. More on Tuesday evening.

Thoughts on exactly how to manage the (optional?) linkage of series with one (or
more?) products from a security point of view, and how this interacts with any
other groups the user may set (assuming they are allowed to) would be very
welcome. We need this linkage because of the issue raised in comment 14, and to
avoid making management of series security an administrative nightmare.

Gerv
I think that, rather than giving a series it's own group controls, you associate
it with a product (by ID) and the same group controls used for the product apply
to the series.
joel: but what if a series spans multiple products? Can you associate it with
the permissions of multiple products? How are those permissions combined? And
what if a series is not product-related at all, but you still want to restrict
access in some way? 

Also, if we are just using products, how does the mapping work? The series is
considered to be in a group if that group is Mandatory/Mandatory for that
product? Or either relationship is Mandatory? Or perhaps Default/Default?

Gerv
A series that spans several products can still be assocated with the access
restrictions of a product.  However, none of the autocreated series stuff would
do that.
OK. New approach based on implementation attempts and more thought:

We remove subscriptions - they are premature optimisation anyway. A series is
either private to its creator or public. Admins can make series they create
public. This is necessary to simplify things.

Each _category_ (top-level classification) has a groupset, which covers all the
series in that category. This makes permissions manageable - the number of
top-level categories will be much smaller than the number of series. But it also
allows you to have a category which is not linked with a particular product,
unlike previous product-linking ideas.

Admins have a special screen to change the groupsets of categories. They can put
a category into any set of groups in the system.

When migrating, the groupset of a category based on a product is "all the groups
which are Mandatory for either Member or Other" (as discussed before in comment 1).

If you create a product, it will auto-create series, as now. However, once you
set up the groups for that product, you only have to change one set of group
controls (the ones for the top-level category) to cover all the auto-created
series. When editing the groups for a product, you will be warned that you may
need to update series category groups. So it's manageable, and fixes the comment
14 problem.

The above is simpler to implement, and dodges several complexity problems
without much loss of flexibility. 

Gerv
That would work with 2 key items needed....

1) "admins" should not be required to be the "admin" group but should be a
distinct group.  (We shoule really decide if this should be a group specified by
a param or if we should have a section of group namespace reserved for system
groups.  If we do the latter, it should be something that is a reserved prefix
followed by the groupname, like "bz_role_seriesadmin"  These groups need not
exist if nobody is in them.

2) Having anything series-related autocreated when a new product is created
should be optional. This could be a param or a checkbox.  If it is a checkbox,
the default state of the checkbox should be easily controlled from within a
template.
Joel:

1) I don't see this is necessary. The creation of new top-level public groups
will be rare enough that having an admin fix the permissions is not a burden.
Any other mechanism leads to greater complication (like params for "magic
groups", which I really don't like.)

2) Why? I suppose there's a few-second window where the name of your new product
might leak, but doing it any other way is - again - more hassle, as you have to
provide some UI for generating them later.

Gerv
1)
Requiring that any user authorized to create a public series (like a PHB) must
have be a member of a group that can tweak parameters and edit users is not a
good thing.  I am quite certain that this needs to be a distinct group.

2)
Since you don't want to tie the series to a product beyond its initial creation,
there is no reason to force a new series to be created just because a product is
created.
Creating a security issue just because an administrator forgets to follow up on
something that is auto-created in an insecure state is not a good thing. 
Providing a checkbox on the new product form so that the administrator can
opt-out of having this security leak auto-created is extemely simple and
necessary.  As you point out, you have to provide the ability to rename the
category anyway.  


> Requiring that any user authorized to create a public series (like a PHB) must
> have be a member of a group that can tweak parameters and edit users is not a
> good thing.  I am quite certain that this needs to be a distinct group.

You are just restating your point here without engaging with mine :-) But
anyway, how would you solve the issue? Another "magic group" parameter?

> Since you don't want to tie the series to a product beyond its initial 
> creation, there is no reason to force a new series to be created just because 
> a product is created.

That's a non sequitur. And I don't see it as a forcing thing, I see it as a
convenience thing.

> Creating a security issue just because an administrator forgets to follow up 
> on something that is auto-created in an insecure state is not a good thing. 

We already have that if you create a product and forget to put it into any
groups. When they put the product into groups, they'll be prompted to do the
same for the category.

> Providing a checkbox on the new product form so that the administrator can
> opt-out of having this security leak auto-created is extemely simple and
> necessary.

But this also requires some other mechanism for creating them at a later date if
he decides to change his mind. Either that, or you say "sorry, you've missed
your chance, do them all by hand."

> As you point out, you have to provide the ability to rename the
> category anyway. 

I don't understand why this is relevant to the question.

Gerv
Gerv,

  Your plan will work as is and the tree has been closed too long.  Go ahead and
get this in and we'll see if we want to make it fancier later.
(In reply to comment #25)
> > Requiring that any user authorized to create a public series (like a PHB)
> > must have be a member of a group that can tweak parameters and edit users
> > is not a good thing.  I am quite certain that this needs to be a distinct
> > group.
> 
> You are just restating your point here without engaging with mine :-) But
> anyway, how would you solve the issue? Another "magic group" parameter?

editbugs only got overloaded because it was hard to add groups.  I hate "magic
groups" but I have no qualms about adding new permission groups.  Instead of
declaring what group is allowed to set them, just make a group for it. 
"editseries" or something.  Group inheritance can be used to make other groups
get the privilege.

> > Creating a security issue just because an administrator forgets to follow up 
> > on something that is auto-created in an insecure state is not a good thing. 
> 
> We already have that if you create a product and forget to put it into any
> groups. When they put the product into groups, they'll be prompted to do the
> same for the category.

This is something else we should probably fix.  Since you need editcomponents to
create a product, a new product should probably be created restricted to the
editcomponents group by default.  That or put the group controls on the product
creation screen and set them up at the same time, which might be more sane from
an ease of use standpoint, but probably a pain in the butt to implement.

Having it open for a few seconds is better than forever, and that's another bug.


Anyhow, what's the status right now?  This is effectively what's blocking the
2.18rc1 release now (other than the release notes).
Dave: I'm in the middle of an implementation.

Gerv
Attached patch Patch v0.1 (obsolete) — Splinter Review
This doesn't completely work yet; it's just evidence that I am doing something
:-)

This patch eliminates subscriptions, and does the migration.

It doesn't yet (for example) provide any mechanism for editing the groupset of
a category, and doesn't enforce the groups-based restrictions at either
series-choosing or series-displaying time. That's for tomorrow night.

Gerv
Comment on attachment 151978 [details] [diff] [review]
Patch v0.1

If the mail is not accepted (for example, if the return address is too bogus),
the error is fatal and non-helpful.
Attachment #151978 - Flags: review-
Comment on attachment 151978 [details] [diff] [review]
Patch v0.1

bah! wrong bug
Attachment #151978 - Flags: review-
Severity: blocker → major
Priority: P1 → P2
Attached patch Patch v0.5 (obsolete) — Splinter Review
More work in progress. This has everything apart from a UI for changing the
groupsets on categories. I'm taking a 24-hour break so people can have a look
and comment.

Note that this patch will delete and re-migrate all your series data. I agreed
with Dave way back when that this was reasonable, due to data corruption bugs
in the first iteration of the series code. This is what the big red warning was
there for :-) We were expecting it to happen before now, of course...

There is currently no way to delete series. This is a "feature". ;-)

Gerv
Attachment #151978 - Attachment is obsolete: true
Blocks: 241230, 247457, 247544
Status: NEW → ASSIGNED
Attached patch Patch v0.6 (obsolete) — Splinter Review
Here's one with the fixes to bug 247544, bug 241230 and bug 247547
incorporated.

Still to do: UI for editing the groups associated with a category. Comments
still very welcome :-)

Gerv
Attachment #153147 - Attachment is obsolete: true
No longer blocks: 247457
Comment on attachment 153527 [details] [diff] [review]
Patch v0.6

Actually, it's probably best to do this in two parts anyway, seeing as they are
nicely separated.

Gerv
Attachment #153527 - Flags: review?(justdave)
Attachment #153527 - Flags: review?(bugreport)
Comment on attachment 153527 [details] [diff] [review]
Patch v0.6

This wont apply to any version I can identify.

See the previous comments about PHBs.  It looks like this still presumes that
all shared queries will be created by someone in the admin group.

I'll look more when it is derotted.
Attachment #153527 - Flags: review?(bugreport) → review-
Attached patch Patch v0.7 (obsolete) — Splinter Review
Here's one against the current 2.18 tip.

Gerv
Attachment #153527 - Attachment is obsolete: true
Its going to take a while to get through the code itself, but I beat this up a
bit.  

It would really help if you'd put a test flag in the code that causes time to
speed up by a factor of at least 240 so I don't have to wait 2 weeks between
making a change and seeing the results.

If I create a non-public series in a new category when I am logged in as an
admin user and then create a series in a new category by the same name as
another user, the new category and series do not show up when I go right back to
the create another new series, but the category does appear a while later.

The hard-coded "admin" designation is still a problem.  See comment 24 and
comment 27.

This does leave us in the following unenviable situation....

1) Only an admin can make a series that is useful to anyone other than himself.
 Therefore, on BMO, only a tiny handful of people will be allowed to create
series at all because otherwise myk will wind up processing the same series for
each of 40000 users every time he runs.  After all, timeless, timely, and bbaetz
would each need their own version of the same queries.

2) It appears that you are passing the user who created the series to Search. 
That means that this no longer counts only public bugs, right?  The templates
still say that only public bugs are counted.  If you fix the creators = "admin"
problem, it becomes possible to have some special users who are in the public
series group but who have privileges distinct from the admin.

3) Fetching the bug list with 250K bugs in collectstats just to turn around and
count it is a bit much.  Why dont we add an optional param to new
Bugzilla::Search so that we pass " count => 1 " with the params and it returns
SQL that returns COUNT(DISTINCT bug_id)  instead of the rows themselves.  

4) If you fix #2 above, we can control (i, 2.19) which users can "see" specific
special users (see bug 251837) and it becomes possible to create queries that
are public to a group of users rather than to the entire universe.

5) There doesn't seem to be a way for the administrator to clean up an endless
list of large queries.  If I searched for attachment.thedata matches regexp
"Gerv\s+M" in a bunch of queries, I would think that you would want a decent way
to clean up.

6) On the same note, even if you dont provide a UI for it and just leave the
information in the table, we should log the time the query took to run.

7) In collectstats, you need to keep Search.pm exceptions and sql errors from
becoming fatal to the entire process.  IIRC, BMO also has a limit on the query
time so slow queries would also get killed and need collectstats to recover.  Am
I correct?

I'll dig a bit deeper later. I'm not quite awake ATM

Really, this is a very cool feature and it is *almost* useful to me the way you
currently are planning to do it for 2.18.  I think it needs some real honing in
2.19 for it to really be a home-run.

1) Your logic doesn't follow, and I think you overestimate the problem. Even if
one could _only_ make private series, why would that be a problem? Given the
small number of people who have saved searches, I somewhat doubt b.m.o. will be
brought to its knees by 40,000 people all creating saved series.

2) and 3) I'll look into.

4) Oh, for goodness sake, no. KISS! We're trying to simplify here.

5) A DOS can be done by someone running an automated script against b.m.o.
today. Yes, we may need ways to find people who've accidentally messed things
up, but I don't think it's urgent.

6, 7) Sure - we need to make it work so it continues if there are errors.

Several of these are not really comments on the groups/security aspect of this
feature. While such comments are welcome, earlier might have been better ;-)

Gerv
Sorry, that sounded a bit snappy. Put it down to me being in a rush - I'm just
off to camp for nine days (http://www.interaction-france.org).

Gerv
No problem.

We can break this into a series of improvments to the feature.  Since it is only
enabled for those who explicitly enable it, there is nothing that blocks some of
these improvments from landing on 2.18

What I really want is to see all of these improvments land on 2.19 but we can
negotiate them bit by bit.

I like this feature enough to have very high demands of it.
Joel: what is "this" in your last message? Is it your list of suggestions in
comment 37?

I plan to look into 2, 3, 6 and 7 and post an updated patch if necessary, for
inclusion in 2.18 RC3. Shout if that's not the right thing.

Gerv
There are several instances if "this."  The main point is that we don't need to
fix everything right now.  The off-by-default policy relieves us of requiring
perfection on the 2.18 branch.

The test feature would really help, though, so please do slide it in.

Eventually, all of the enhancments I suggested will be needed by most sites, so
let's start plugging away at them.  They do not need to be on 2.18.  If I were
you, I would not even be targetting 2.18 and would really drive this forward on
2.19.  But, that's just me. 

> There are several instances if "this."

Actually, there are two, and one of them is "this feature", which obviously
doesn't need disambiguating. 

> The main point is that we don't need to fix everything right now.

Indeed not. However, there is a patch on this bug which I would like to check in
to 2.18, and which is marked as blocking 2.18, and which you are reviewing for
2.18. Could you please tell me what, if any, markups I need to make, and have
that information separate from your wishlist of new features in 2.19? :-)

Gerv
Comment on attachment 154628 [details] [diff] [review]
Patch v0.7


Well, I still can't properly test this. Please do add the test mode in the next
patch.	Then, I'll do a complete review.

   elsif ($arg_count >= 6 && $arg_count <= 8) {
While we're fixing thigs here,	you should really get rid of the argument count
subtleties.  How about adding an initial parameter that identifies which
mechanism you are trying to use.  I know this would have better been caught by
whoever did the initial reviews, but it wasn't.

  Since each search runs with the permissions of its creator, you no longer
need to claim it only reports public bugs, right?

   Please add a note explaining what you are doing.  It appears that the
migration code sets category_group_map, but nothing else maintains that data. 
Is that an oversight?



>--- Bugzilla/Chart.pm	22 Jan 2004 00:02:27 -0000	1.3
>+++ Bugzilla/Chart.pm	29 Jul 2004 06:50:47 -0000
>@@ -69,12 +69,12 @@ sub init {
>         # Store all the lines
>         if ($param =~ /^line(\d+)$/) {
>             foreach my $series_id ($cgi->param($param)) {
>                 detaint_natural($series_id) 
>                                      || &::ThrowCodeError("invalid_series_id");
>-                push(@{$self->{'lines'}[$1]}, 
>-                     new Bugzilla::Series($series_id));
>+                my $series = new Bugzilla::Series($series_id);                    
>+                push(@{$self->{'lines'}[$1]}, $series) if $series;
>             }
>         }

Goaing a bit nuts with trailing whitespace....


> sub initFromParameters {
>     # Pass undef as the first parameter if you are creating a new series.
>     my $self = shift;
> 
>     ($self->{'series_id'}, $self->{'category'},  $self->{'subcategory'},
>      $self->{'name'}, $self->{'creator'}, $self->{'frequency'},
>-     $self->{'query'}, $self->{'public'}) = @_;
>+     $self->{'query'}, $self->{'public'}) = @_;     
> }
> 

Whitespace only change



>+        if (defined($category_id) && defined($product_id)) {
>+          
>+            # Get all the mandatory groups for this product
>+            my $group_ids = 
>+                $dbh->selectcol_arrayref("SELECT group_id " . 
>+                     "FROM group_control_map " . 
>+                     "WHERE product_id = $product_id " . 
>+                     "AND (membercontrol = " . CONTROLMAPMANDATORY . 
>+                       " OR othercontrol = " . CONTROLMAPMANDATORY . ")");
>+                                            
>

This may be over-paranoid.  membercontrol is sufficient, I think.

>
>Index: defparams.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v
>retrieving revision 1.128
>diff -5 -p -u -r1.128 defparams.pl
>--- defparams.pl	6 Jul 2004 01:12:29 -0000	1.128
>+++ defparams.pl	29 Jul 2004 06:50:55 -0000
>@@ -1036,15 +1036,15 @@ Reason: %reason%
> 
>   {
>    name => 'chartgroup',
>    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 ' .
>+           'and series definitions do not divulge confidential information ' .
>            'before enabling this for an untrusted population. If left blank, ' .
>            'no users will be able to use New Charts.',
>    type => 't',
>-   default => ''
>+   default => 'editbugs'
>   },
>   
I believe that we agreed with jusdave that this would not enable by default for
any community of ordinary users.  If you want, make it default to "admin"
Attachment #154628 - Flags: review-
Please don't interpret my failing to repeat myself on all previous items as r+
on those.  The call to search and the execution of the query need to be eval'd
(contrary to popular belief, there is a form of eval that still compiles ahead
of time and just lets you catch errors).  Roll back through the prior comments
and you'll see them.


(In reply to comment #44)
> >--- defparams.pl	6 Jul 2004 01:12:29 -0000	1.128
> >+++ defparams.pl	29 Jul 2004 06:50:55 -0000
> >@@ -1036,15 +1036,15 @@ Reason: %reason%
> > 
> >   {
> >    name => 'chartgroup',
> >    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 ' .
> >+           'and series definitions do not divulge confidential information ' .
> >            'before enabling this for an untrusted population. If left blank, ' .
> >            'no users will be able to use New Charts.',
> >    type => 't',
> >-   default => ''
> >+   default => 'editbugs'
> >   },
> >   
> I believe that we agreed with jusdave that this would not enable by default
> for any community of ordinary users.  If you want, make it default to "admin"

No, this is the param for who can see charts, not who can share them.  The only
lets people make charts for themselves, iirc.
Attached patch Patch v.1 (obsolete) — Splinter Review
This patch additionally does the following:

- As requested, adds a mode for changing time; pass a date to collectstats.pl
in ISO format (YYYY-MM-DD) to have collectstats pretend it's that date (for new
charts, at least.)

- Removes the comment about it only counting public bugs; you are right, this
is no longer true.

- Adds an eval{} around the collectstats.pl search to prevent the whole thing
dying on bad SQL. Note: I've never done this before; tell me if it's wrong.

- As in the last patch, series are accessible by default to anyone with
editbugs. As discussed with justdave on IRC a while back, we shouldn't have a
feature this useful turned off by default.

- Add proper URL quoting to the series creation code in editproducts.cgi and
editcomponents.cgi.

Things to do in a future patch, if Dave deems them necessary for 2.18:

- Add an admin UI for the groups each category is in (this is why, as Joel
noticed, no-one is currently changing category_group_map; I suspect this will
be deemed necessary :-)

- Add a group to control who can make charts public (in the meantime, I've
added comments at the two places in the code that need changing)

- Add a mode to Search.pm which makes it return counts only (performance
optimisation)

- Fix argument count subtleties in Series.pm (code cleanup).

Gerv
Attachment #154628 - Attachment is obsolete: true
Comment on attachment 156098 [details] [diff] [review]
Patch v.1

Making the implicit explicit: marking for review.

Gerv
Attachment #156098 - Flags: review?(bugreport)
Attachment #153527 - Flags: review?(justdave)
Comment on attachment 156098 [details] [diff] [review]
Patch v.1

I didn't track down exactly why, but products to which I have no visibility are
still shown in the auto-created category list
Attachment #156098 - Flags: review?(bugreport) → review-
Instructions to dumplicate...

Create a product That has ENTRY, MANDATORY/MANDATORY for a group in which sime
users are not a member.

Drop the series tables

run checksetup

run collectstats for a few days

log in as the use who cannot see that product

try to plot a series... note that you can see the product you should never see.
OK, in spite of the fact that there is no interface to examine/edit the
retsrictions, I did check the database.  The category_group_map does have the
restriction.  The UI just seems to be ignoring it.
Joel: I'll look into the possible bug. I can't update the patch until I get back
from EuroFoo; in the mean time, do you have any other comments on the code?

Gerv
Also, when creating a new series....

Undefined subroutine &Bugzilla::Series::UserInGroup called at Bugzilla/Series.pm
line 166.
If I hack past that and create a series with "product" "matches" "foo"
then, collectstats still croaks because Search.pm throws errors.  Using an eval
could also help with that or else we could add an optional param to Search.pm
instructing it to return error codes rather than throwing errors. 

The best bet, I think is to do thwe following....

Add "die" => 1 to the hash being passed to Search.pm
Wherever Search.pm is about to Throw*Error(), have it only do that if die was
not passed.  If die was passed, then really use die() instead.

After the eval, check $@ for the failure notice and print information for the
administrator (cron will email it to the administrator) identifying the bad
query by user, id, etc... so that the administrator can easily delete it in mysql.

As I previously stated, 'admin' is the wrong group to which to restrict the
public queries.  That will have to be fixed, but it can be fixed udner a
different bug.

I previosuly commented on the use of arg_count to determine the entire meaning
of new().  

With one exception, all the series-related tables have names that begin with
"series."  That exception is the categtory_group_map.  Can you come up with a
name starting with "series" for that as well?  When testing, I tripped over that
myself.

The "Create Chart" page is full of invalid html.  Try the developer bar in
firefox. It makes validating local html very easy.

Would it make more sense to open a "new charts cleanup meta-bug" or track all
these things here?
> I didn't track down exactly why, but products to which I have no visibility 
> are still shown in the auto-created category list

Fixed, I hope.

> Also, when creating a new series....
> Undefined subroutine &Bugzilla::Series::UserInGroup called at 
> Bugzilla/Series.pm
> line 166.

Fixed.

> With one exception, all the series-related tables have names that begin with
> "series."  That exception is the categtory_group_map.  Can you come up with a
> name starting with "series" for that as well? 

Well, no :-) One problem is that we'll hit the 31-char column name limit again
if the table name gets any longer. It fits the "map" convention, rather than the
"series" convention - at least it fits one :-)

> The "Create Chart" page is full of invalid html.  Try the developer bar in
> firefox. It makes validating local html very easy.

Thanks for the tip. I've fixed any that might impact display; but I want to keep
this patch as small as possible to get it through review, as it blocks 2.18.

> Would it make more sense to open a "new charts cleanup meta-bug" or track all
> these things here?

New bug, please. :-)

New patch coming up. Please note that I'm away on holiday from Monday 30th for a
week, so would really like to either check this in or rev it again before then.
Even if it's only 95%, let's get it in and let people bang on it, then do a
follow-up patch. Hopefully that should speed the process up a bit - fewer
iterations.

Known issues: I haven't yet fixed the problem that occurs if Search.pm dies.

Gerv
Attached patch Patch v.2Splinter Review
Attachment #156098 - Attachment is obsolete: true
Attachment #157103 - Flags: review?(bugreport)
Blocks: 257346
Comment on attachment 157103 [details] [diff] [review]
Patch v.2

I opened bug 257346 to track all the things that this patch does not address
and are still needed.  That said, this is a major improvment and needs to land.
Attachment #157103 - Flags: review?(bugreport) → review+
Approved for the trunk.  This patch doesn't apply cleanly to 2.18.
Flags: approval+
Attached patch Patch v.2 - 2.18Splinter Review
Here's the 2.18 version. For some reason, Dave wanted it before he approved
this bug for 2.18 ;-)

Gerv
Attachment #157357 - Flags: review?(bugreport)
Fixed on trunk:

Checking in template/en/default/reports/create-chart.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v
 <--  create-chart.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/reports/series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series.html.tmpl,v
 <--  series.html.tmpl
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/Chart.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v  <--  Chart.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.6; previous revision: 1.5
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.300; previous revision: 1.299
done
Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.39; previous revision: 1.38
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.137; previous revision: 1.136
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.59; previous revision: 1.58
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.44; previous revision: 1.43
done
Checking in template/en/default/search/search-create-series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-create-series.html.tmpl,v
 <--  search-create-series.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.22; previous revision: 1.21
done

Gerv
Flags: approval2.18?
Comment on attachment 157357 [details] [diff] [review]
Patch v.2 - 2.18

This looks the same, but I did not have a chance to test this.	r=joel so long
as someone assures us they have tested it.
Attachment #157357 - Flags: review?(bugreport) → review+
Checked in on branch.

Checking in template/en/default/reports/create-chart.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v
 <--  create-chart.html.tmpl
new revision: 1.8.2.1; previous revision: 1.8
done
Checking in template/en/default/reports/series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series.html.tmpl,v
 <--  series.html.tmpl
new revision: 1.4.2.1; previous revision: 1.4
done
Checking in Bugzilla/Chart.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v  <--  Chart.pm
new revision: 1.3.2.1; previous revision: 1.3
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.5.2.1; previous revision: 1.5
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.3; previous revision: 1.289.2.2
done
Checking in collectstats.pl;
/cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v  <--  collectstats.pl
new revision: 1.38.2.1; previous revision: 1.38
done
Checking in defparams.pl;
/cvsroot/mozilla/webtools/bugzilla/defparams.pl,v  <--  defparams.pl
new revision: 1.128.2.1; previous revision: 1.128
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.53.2.3; previous revision: 1.53.2.2
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.41.2.2; previous revision: 1.41.2.1
done
Checking in template/en/default/search/search-create-series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-create-series.html.tmpl,v
 <--  search-create-series.html.tmpl
new revision: 1.8.2.1; previous revision: 1.8
done
Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.21.2.1; previous revision: 1.21
done

Gerv
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Flags: approval2.18? → approval2.18+
Resolution: --- → FIXED
Flags: approval2.18+
Flags: approval2.18+
Comment on attachment 214027 [details] [diff] [review]
docs patch about 'Bugzilla Database Tables' section, for 2.18

it's really the relationship of groups to products and not the relationship between groups and products?
Attachment #214027 - Flags: review?(documentation) → review+
Doc patch submitted on the 2.18 branch:

Checking in docs/xml/customization.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/customization.xml,v  <--  customization.xml
new revision: 1.12.2.14; previous revision: 1.12.2.13
done

This part of the documentation no longer exists on newer branches.
Flags: documentation2.18+
Blocks: 672173
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.