Closed
Bug 225687
Opened 21 years ago
Closed 20 years ago
Add group controls to new charts
Categories
(Bugzilla :: Reporting/Charting, defect, P2)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: gerv, Assigned: gerv)
References
Details
Attachments
(3 files, 5 obsolete files)
37.83 KB,
patch
|
bugreport
:
review+
|
Details | Diff | Splinter Review |
38.13 KB,
patch
|
bugreport
:
review+
|
Details | Diff | Splinter Review |
853 bytes,
patch
|
timeless
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•21 years ago
|
Priority: -- → P1
Assignee | ||
Comment 1•21 years ago
|
||
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
Comment 2•21 years ago
|
||
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.
Assignee | ||
Comment 3•21 years ago
|
||
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
Comment 4•21 years ago
|
||
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.
Assignee | ||
Comment 5•21 years ago
|
||
> 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
Assignee | ||
Comment 6•21 years ago
|
||
Joel: anything more to add? I would like to implement this before b.m.o. updates, probably on Thursday. Gerv
Comment 7•21 years ago
|
||
Nope. Just that.
Updated•20 years ago
|
Flags: blocking2.18+
Target Milestone: --- → Bugzilla 2.18
Comment 8•20 years ago
|
||
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
Comment 9•20 years ago
|
||
This also currently leaks by showing ALL products as Categories in the Create Chart UI. It should only show selectable products.
Severity: critical → blocker
Assignee | ||
Comment 10•20 years ago
|
||
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
Comment 11•20 years ago
|
||
We are discussing this on IRC right now. Perhaps converted series should be non-public by default?
Comment 12•20 years ago
|
||
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)
Assignee | ||
Comment 13•20 years ago
|
||
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
Comment 14•20 years ago
|
||
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.
Assignee | ||
Comment 15•20 years ago
|
||
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
Comment 16•20 years ago
|
||
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?
Assignee | ||
Comment 17•20 years ago
|
||
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
Comment 18•20 years ago
|
||
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.
Assignee | ||
Comment 19•20 years ago
|
||
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
Comment 20•20 years ago
|
||
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.
Assignee | ||
Comment 21•20 years ago
|
||
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
Comment 22•20 years ago
|
||
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.
Assignee | ||
Comment 23•20 years ago
|
||
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
Comment 24•20 years ago
|
||
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.
Assignee | ||
Comment 25•20 years ago
|
||
> 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
Comment 26•20 years ago
|
||
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.
Comment 27•20 years ago
|
||
(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).
Assignee | ||
Comment 28•20 years ago
|
||
Dave: I'm in the middle of an implementation. Gerv
Assignee | ||
Comment 29•20 years ago
|
||
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 30•20 years ago
|
||
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 31•20 years ago
|
||
Comment on attachment 151978 [details] [diff] [review] Patch v0.1 bah! wrong bug
Attachment #151978 -
Flags: review-
Updated•20 years ago
|
Severity: blocker → major
Priority: P1 → P2
Assignee | ||
Comment 32•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #151978 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Assignee | ||
Comment 33•20 years ago
|
||
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
Assignee | ||
Comment 34•20 years ago
|
||
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)
Assignee | ||
Updated•20 years ago
|
Attachment #153527 -
Flags: review?(bugreport)
Comment 35•20 years ago
|
||
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-
Assignee | ||
Comment 36•20 years ago
|
||
Here's one against the current 2.18 tip. Gerv
Assignee | ||
Updated•20 years ago
|
Attachment #153527 -
Attachment is obsolete: true
Comment 37•20 years ago
|
||
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.
Assignee | ||
Comment 38•20 years ago
|
||
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
Assignee | ||
Comment 39•20 years ago
|
||
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
Comment 40•20 years ago
|
||
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.
Assignee | ||
Comment 41•20 years ago
|
||
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
Comment 42•20 years ago
|
||
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.
Assignee | ||
Comment 43•20 years ago
|
||
> 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 44•20 years ago
|
||
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-
Comment 45•20 years ago
|
||
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.
Comment 46•20 years ago
|
||
(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.
Assignee | ||
Comment 47•20 years ago
|
||
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
Assignee | ||
Updated•20 years ago
|
Attachment #154628 -
Attachment is obsolete: true
Assignee | ||
Comment 48•20 years ago
|
||
Comment on attachment 156098 [details] [diff] [review] Patch v.1 Making the implicit explicit: marking for review. Gerv
Attachment #156098 -
Flags: review?(bugreport)
Assignee | ||
Updated•20 years ago
|
Attachment #153527 -
Flags: review?(justdave)
Comment 49•20 years ago
|
||
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-
Comment 50•20 years ago
|
||
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.
Comment 51•20 years ago
|
||
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.
Assignee | ||
Comment 52•20 years ago
|
||
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
Comment 53•20 years ago
|
||
Also, when creating a new series.... Undefined subroutine &Bugzilla::Series::UserInGroup called at Bugzilla/Series.pm line 166.
Comment 54•20 years ago
|
||
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?
Assignee | ||
Comment 55•20 years ago
|
||
> 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
Assignee | ||
Comment 56•20 years ago
|
||
Attachment #156098 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #157103 -
Flags: review?(bugreport)
Comment 57•20 years ago
|
||
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+
Comment 58•20 years ago
|
||
Approved for the trunk. This patch doesn't apply cleanly to 2.18.
Flags: approval+
Assignee | ||
Comment 59•20 years ago
|
||
Here's the 2.18 version. For some reason, Dave wanted it before he approved this bug for 2.18 ;-) Gerv
Updated•20 years ago
|
Attachment #157357 -
Flags: review?(bugreport)
Assignee | ||
Comment 60•20 years ago
|
||
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 61•20 years ago
|
||
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+
Assignee | ||
Comment 62•20 years ago
|
||
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
Updated•20 years ago
|
Flags: approval2.18+
Updated•20 years ago
|
Flags: approval2.18+
Comment 63•18 years ago
|
||
Attachment #214027 -
Flags: review?(documentation)
Comment 64•18 years ago
|
||
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+
Comment 65•18 years ago
|
||
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+
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
•