In environments where a development team is supporting multiple customers, each sharing some of their own bugs with the development team and also interested in tracking bugs the development team is working on, some finer-grain access controls are needed to prevent cross-contamination. I am in the process of implementing (hacking, really) the following changes... #1 is nearly done and will soon have a patch (my first) for the whole world to throw rocks at. #2 is in planning. Beyond that is more sketchy. 1) New parameter - "insidergroup" naming a group whose members are considered insiders. Members of this group can mark any comment private. Comments marked private will not be visible to any user not part of "insidergroup" and email notifications to non-insiders will be supressed when the associated comment is private. (for simplicity, auto-generated comments about duplicate bugs and attachments are considered private -- this may change later) 2) New attribute of a project -- "inside" If a project is flagged as "inside" then no non-insiders may edit any bugs while they are in that project, even if the bug is public and/or they are on the CC list. If a project is non-"inside", then all bugs in that project will be forced to be in that project's group. 3) Limit visibility of email addresses and assignees by non-insiders. Exact rules TBD (suggestions anyone??)

Patch (needs review!!!) that enables a new parameter, insidergroup, to specify which people can restrict comments to be visible only to their fellow insiders (warning!! this does not prevent search matches based on the text of private comments)

This is a dupe of bug 7415. Once we have real groupids, then then would probably be easier, I suspect. The patch has the problem that: private doesn't need to be a full int, if it sjust boolean. The search probelm you mentioned too, is an issue. What do others think?

I'll change the int to a boolean on the next pass. I presume I should have BOTH the Addfield (using the new type) AND the ChangeFieldType so that I don't mess up anyone who already used my patch (like me). I already have a cut of a search that looks like it does the trick.... --- bugz/buglist.cgi Sat May 11 21:42:41 2002 +++ bugbase/buglist.cgi Fri May 10 15:55:09 2002 @@ -531,11 +531,7 @@ "^long_?desc," => sub { my $table = "longdescs_$chartid"; push(@supptables, "longdescs $table"); - my $morewhere = ""; - if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) { - $morewhere = "AND $table.private < '1'" ; - } - push(@wherepart, "$table.bug_id = bugs.bug_id $morewhere"); + push(@wherepart, "$table.bug_id = bugs.bug_id"); $f = "$table.thetext"; }, "^attachments\..*," => sub { I was torn between subverting #7415 and opening a new one.

Who is implementing the new security stuff? I have a need for something almost exactly like this (presuming I can get my company to consider Bugzilla), but I was going to get around to making sure the new security stuff supported this. Gerv

Replaces first rough patch. Ensures that private comments are not used to match outsider queries and uses a TINYINT instead of an INT for the private flag in longescs. (No sense in being the first to use "BOOL")

gerv: dkl is doing the group stuff

Comment on attachment 83284 [details] [diff] [review] Improved Insiders patch on 2.17 > AppendComment($id, DBID_to_name($who), >- "*** This bug has been confirmed by popular vote. ***"); >+ "*** This bug has been confirmed by popular vote. ***",1); In general, we try and use descriptive values for TRUE. So: >+ "*** This bug has been confirmed by popular vote. ***", "public"); Although it would be a better interface if: - parameter missing == public - parameter present == private because then normal calls would not change, and private calls could have , "private"); > "^long_?desc," => sub { > my $table = "longdescs_$chartid"; > push(@supptables, "longdescs $table"); >- push(@wherepart, "$table.bug_id = bugs.bug_id"); >+ my $morewhere = ""; >+ if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) { >+ $morewhere = "AND $table.private < '1'" ; >+ } >+ push(@wherepart, "$table.bug_id = bugs.bug_id $morewhere"); Why not: > push(@wherepart, "$table.bug_id = bugs.bug_id"); >+ if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) { >+ push(@wherepart, "$table.private < '1'"); >+ } ? Also, your patch has tab characters in it. You will need to replace these with four spaces. >@@ -598,4 +598,9 @@ > "t" , > '1000'); > >+DefParam("insidergroup", >+ "The group of users who can see/change private descriptions", >+ "t", >+ '') Is it correct that this Param defines the name of the insiders group? It would be better if the param merely switched it on or off, and it was always called "insiders", and had a fixed "bit" in the Groups table. This removes confusion. >- SendSQL("INSERT INTO longdescs (bug_id, who, bug_when, thetext) " . >- "VALUES($bugid, $whoid, now(), " . SqlQuote($comment) . ")"); >+ SendSQL("INSERT INTO longdescs (bug_id, who, bug_when, thetext, private) " . >+ "VALUES($bugid, $whoid, now(), " . SqlQuote($comment) . ", " . SqlQuote($private) . ")"); You don't need to SqlQuote numbers. > if ($count) { > $result .= "\n\n------- Additional Comments From $who".Param('emailsuffix')." ". > time2str("%Y-%m-%d %H:%M", str2time($when)) . " -------\n"; > } >+ if (($private > 0) && Param("insidergroup")) { >+ $result .= "PARAM_PRIVATE\n"; >+ } > $result .= $text; > $count++; > } >@@ -1164,12 +1167,16 @@ > sub GetComments { > my ($id) = (@_); > my @comments; >- >+ my $morewhere = ""; >+ if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) { >+ $morewhere = "AND private < '1'" ; >+ } > SendSQL("SELECT profiles.realname, profiles.login_name, > date_format(longdescs.bug_when,'%Y-%m-%d %H:%i'), > longdescs.thetext > FROM longdescs, profiles > WHERE profiles.userid = longdescs.who >+ $morewhere > AND longdescs.bug_id = $id > ORDER BY longdescs.bug_when"); Better to unconditionally do AND private < $private and set $private appropriately. > my %comment; > ($comment{'name'}, $comment{'email'}, $comment{'time'}, $comment{'body'}) = FetchSQLData(); > >- $comment{'email'} .= Param('emailsuffix'); >+ $comment{'email'} .= Param('emailsuffix') ; Nit: you added a space here. >diff --recursive -u old/process_bug.cgi bugz/process_bug.cgi >--- old/process_bug.cgi Wed Apr 24 13:07:57 2002 >+++ bugz/process_bug.cgi Sat May 11 20:42:21 2002 >@@ -1012,7 +1012,8 @@ > $timestamp = FetchOneColumn(); > > if (defined $::FORM{'comment'}) { >- AppendComment($id, $::COOKIE{'Bugzilla_login'}, $::FORM{'comment'}); >+ my $private = $::FORM{'commentprivacy'}; >+ AppendComment($id, $::COOKIE{'Bugzilla_login'}, $::FORM{'comment'},$private); Why bother copying the value into a variable? However, you'll need to do some checking here otherwise taint mode will complain. Won't it? >- >+ >+ >+ # Drop any non-insiders if the comment is private >+ if ( Param("insidergroup") && ($newcomments =~ /PARAM_PRIVATE/)) { >+ ConnectToDatabase(); >+ PushGlobalSQLState(); >+ SendSQL("select (bit & $groupset ) != 0 from groups where name = " . SqlQuote(Param("insidergroup"))); >+ my $bit = FetchOneColumn(); >+ PopGlobalSQLState(); >+ if (!$bit) { >+ return; >+ } >+ } >+ > # We shouldn't send changedmail if this is a dependency mail, and any of > # the depending bugs is not visible to the user. > foreach my $dep_id (@depbugs) { >diff --recursive -u old/template/en/default/bug/edit.html.tmpl bugz/template/en/default/bug/edit.html.tmpl >--- old/template/en/default/bug/edit.html.tmpl Wed May 8 11:32:33 2002 >+++ bugz/template/en/default/bug/edit.html.tmpl Sat May 11 20:46:31 2002 >@@ -301,6 +301,14 @@ > > <br> > <b>Additional Comments:</b> >+ [% IF Param("insidergroup") && UserInGroup(Param("insidergroup")) %] >+ <br> >+ <input type="radio" name="commentprivacy" value="0" > Public >+ <br> >+ <input type="radio" name="commentprivacy" value="1" checked> Internal This should be a checkbox, marked "Public" and checked by default. I think it's possible that people will want to withdraw comments after the fact, though - we need to work out how this could be done. Also, we need to decide the effect this will have on comment numbering. Do we just have gaps in the numbers. I can't spot which bit of code this is but, for displaying comments, you should pass all comments to the template with their privacy information (add it to the hash representing a comment; this all happens in GetComments() ) and let the template decide what to display. I also haven't seen anything in this patch about private attachments or private groups, as you mention initially. Private groups already exist, so that's fine - but do you have plans for private attachments? Gerv

>In general, we try and use descriptive values for TRUE. That's not really correct. Bugzilla uses Perl's boolean equivalents (numeric zero and one) almost everywhere; only recently has new code been added that uses descriptive instead of boolean values. Although I liked the descriptive values at first, I've changed my mind after going back to code that has them. Descriptive values try to document parameters at the function call but end up being more confusing than the combination of an unambiguous boolean value and the documentation within the function definition. They would only work in this case if "public" and "private" make more sense than "is public" and "is not public".

Gerv: Thanks for the feedback. I'll roll in the changes on the next go-round. (I would obsolete the first patch with the second, but I need some privilege I am missing) The one area where I am not clear is on adding a new group. If I do this, do I just need to add the call to AddGroup to checksetup and the rest happens by the usual magic? I avoided adding a fixed group for this because I was concerned about breaking other things. On the checkboxes, I am of 2 minds on the correct default (another parameter?). Comment numbering currently only numbers the comments it shows which only seems to break the ability to refer to comments by the comment number as an anchor. Perhaps the anchor should be based on something other than the sequence? I haven't figured out a good way to go back and adjust the comments short of going into a mysql client and flipping the bit. (That would work for me, but is probably bad form). For private attachments, I am trying to decide between using aspecial status bit or adding another field to the attachment table. I'm leaning toward another private flag like I added to the long_descs table. I will probably tackle that after I clean up the initial patches and add the changes to permit insiders to make a bug exposed to a specific non-inside group, prevent anyone from accidentally making outside bugs public, and preventing outsiders from editing or commenting on public bugs. The other open issue for this effort is how much to expose/hide the email addresses from outsiders.

We can't add a new system group until the max groups restiction is removed - otherwise we'd break stuff. Lets keep this as a 'normal' group, the name of which is a param. Then we can add a mapping table later, once groups have ids, not bits.

It makes sense to use a regular product group for this anyway. In actual use, there would be an internal product and associated group that was populated by the exact same people as the insiders list. So, after the clean-up of the patch based on Gerv's review, I'll go onto adding in the goof-proofing items. I'm going to neglect the editability of the private comment flag for now.

> Thanks for the feedback. I'll roll in the changes on the next go-round. (I > would obsolete the first patch with the second, but I need some privilege I am > missing) Fixed. > On the checkboxes, I am of 2 minds on the correct default (another > parameter?). Certainly not a parameter. The default will be defined in the template - i.e. which one has "checked" next to it - so if you want to change the default, you edit the template. The default default, as it were, should be openness. > Comment numbering currently only numbers the comments it shows > which only seems to break the ability to refer to comments by the comment > number as an anchor. Perhaps the anchor should be based on something other > than the sequence? It numbers the comments it shows because it shows all comments. This is why you should pass all comments to the template and let the template decide which ones to display - this way, it can count the ones it's not displaying and increment the counter anyway. > I haven't figured out a good way to go back and adjust the comments short of > going into a mysql client and flipping the bit. (That would work for me, but # > is probably bad form). Yes; this definitely isn't ideal. If this was rare enough, we could just have an admin page for withdrawing a comment - you either paste a link, or give bug number and comment number. > For private attachments, I am trying to decide between using aspecial status > bit or adding another field to the attachment table. I'm leaning toward > another private flag like I added to the long_descs table. That makes most sense to me. Consistency is good. > I will probably tackle that after I clean up the initial patches and add the > changes to permit insiders to make a bug exposed to a specific non-inside > group, prevent anyone from accidentally making outside bugs public, and > preventing outsiders from editing or commenting on public bugs. Sorry, I don't understand that sentence. But it sounds a bit like these features should be separate patches, or template customisations. > The other open issue for this effort is how much to expose/hide the email > addresses from outsiders. That's easy. In the template, you only print the email address if UserInGroup('insider'). No Bugzilla changes are required. Gerv

Initial description of insiders feature. Comments welcomed

This is a combination of the completion of the work started in attachment 82384 [details] with some of the feedback from Gerv's review. Since much of this is security-related, the more eyes and the more testers the better. A new file is included, editprivacy.cgi, that permits a maintainer to adjust privacy settings. If anyone has a good suggestion on how to permit the history without a security risk, please comment.

I'm away this weekend. I will comment on it on my return :-) Gerv

Gerv: Thanks. I should be clear on attachment 84021 [details] [diff] [review]. I will have furether revs before I would consider this a candidate for checkin. However, I am eager to get comments about the direction and coding style, any security holes, and feedback from potential users. One key question for the community. Going back in to revise privacy settings is a function that I initially assigned to anyone with tweakparams access. I am not sure if I should define a parameter that decides who has this poewer. Also, I could implement a delete function from the same screen. Once again, there is a decision to make.

TODO: For attachment 84021 [details] [diff] [review] Add protections to prevent cross-contamination by means of duplication and dependency notifications and viewing of dependency trees. Barring a better suggestion, this will be done by blocking dependency mechanism from outsiders in both the bug viewing/reporting/editing and in processmail.

Regarding bbaetz's comment #10, we will hopefully remove all system groups anyway, it's much more flexible to make them all params. This is because it potentially reduces the burden on admininstrators if they want one group and only that group to have a number of different privileges. And similar for product groups. We need a UI for editparams so the admin can deal with names while at the same time the params is stored as an ID, which is going to be an issue for this patch when the group is renamed. Regarding the patch, I think putting groupsets on things is more flexible solution than boolean flags. That's bug #118736 & bug #118738. I shudder at adding a UI to each comment though, which would occur if you could change the privacy settings after attachment/commenting. Perhaps that could be made to fit in nicely with the header line somehow. A checkbox for privacy would definitely be easier from this POV but I honestly can't see everyone being satisfied with the implementation.

Regarding comment #18, I think I have a reasonable model for adjusting privacy settings. When a comment or attachment is first added, there is a checkbox. I then added a seperate cgi that permits an appropriately privileged user (currently the maintainer, but I think I am going to change this to anyone with blessgroupset <> 0) to paste the URL of a comment or attachment into a box and check/change the privacy settings. I am considering adding a "delete" function to this, but this poses a philisophical question regarding revisionism. The other complication with this... I have a dilemma about numbering comments. I can number all comments and leave outsiders knowing that they missed something. This preserves the ability to have comment cross-references work. Or, I can change the comment URLs to use the timestamp. (This is what I do now, and the URLs work, but referring to comment #40 would fail) The todo list for this is non-trivial. This feature has HELP WANTED status.

Regarding the privacy UI, I was primarily referring to editing an existing attachment or comment. I wouldn't expect the content to be changeable, but the privacy settings should be able to be. Yes I was going to make a comment on comment links, it's a thorny issue, and more so than I think you realise. Firstly, what if comment comes up that refers to a private comment? The reference should probably fail to be linked as bug links are when a bug doesn't exist (but see bug #146752). Secondly, what if I refer to a comment that has greater permissions than my current comment, yet I can see the comment I'm referring to. I will probably want to be warned about this situation - perhaps my comment should be under tighter security. And for that matter if I can't see the comment I should probably get a warning, see new bug #146768. Perhaps the implementation solution lies in explicitly numbering comments, this would make bug #146752 simpler to implement, as well as comment deletion. This also points to a possible solution for hiding the comment numbers of private comments - you could set them to be comment 40A or 40.1. This would work for a single insider group but not for general groupset functionality.

Hmm... reading comment 15, that was a long weekend :-) I think your model for privacy changing could be simplified, as follows: If you are a member of the insider group, each comment has a "public" checkbox next to it when you view a bug. To change the privacy settings, change the checkboxes and commit the bug. This ability is available to all in the insider group, and no-one else. </me reads more> This seems to be what MattyT is suggesting. Excellent The fact that we came up with the same idea should indicate it's a good one. ;-) I think pasting URLs into separate CGIs is very clunky, and should be avoided. It also makes it hard to quickly change multiple comments at once. > Firstly, what if comment comes up that refers to a private comment? The > reference should probably fail to be linked as bug links are when a bug > doesn't exist (but see bug #146752). This requires a DB lookup for every comment link (although we already do one for every bug link, if what you say is true.) I don't think this is a win - the fact that the words "bug 56, comment 3" are unlinked is not going to stop a curious person heading over to bug 56 to try and look at comment 3. He'll just assume Bugzilla's comment-text-linking is broken. I think that we are going to have to accept that you can't do "insider group" without outsiders knowing that the insider group exists (which, in the standard company-with-customers scenario, is the case.) If we accept that they can know that fact, then we don't have to worry about comment numbering at all. We just continue as normal, with gaps in the numbering for outsiders. Gerv

I'm leaning toward Gerv's point on the comment numbering. There seems to be no perfect solution and that seems the most straightforward. This leaves the TODO list down to 1) cleaning up the comment numbering issue 2) cleaning up the public/private editing issue (Gerv - how strongly do you feel about this? I have the paste-the-url approach working, but I do want to see this patch find its way into the main branch eventually) 3) protecting dependency trees and duplicate actions from traversal 4) preventing reports from exposing excessive information

> 2) cleaning up the public/private editing issue (Gerv - how strongly do you feel > about this? I have the paste-the-url approach working, but I do want to see > this patch find its way into the main branch eventually) Pretty strongly; because I know that if we implemented it as now, the first comment everyone would make would be: "How clunky. Why can't each comment have a checkbox?" ;-) As for the security issues, it would probably be fair to lock everything down in the initial patch, and then loosen things gradually in further patches. Given that you are the only person using this at the moment, this also follows the principle of minimum code changes to provide a feature. Gerv

This version permits insiders to control public/private comments and attachments whenever they are edited. It also uses CSS to color-code the private comments and attachments. Reports and viewing of the bug activity have been disabled for non-insiders until they can be made truly safe. This is ready for review and serious testing. Please add feekback to this bug.

Minor change to attachment 85932 [details] [diff] [review]. Apply patch from bug 148679 first, then this patch. This respects the new interface to header.html.tmpl but does not implement it within this patch file.

Doh! Here we go again. Anyone know offhand how to shut off vim's automatic tab insertion?

I used to have the same problem. Here is my .vimrc file if you need to add some lines to yours. Lines of interest are the set sts=4 and set ts=4. sts means soft tab stops and inserts 4 actual spaces in place a tab character. set nocompatible syntax on version 5.0 set sts=4 set ts=4 set sw=4 set expandtab set ruler set laststatus=2

Gerv: I think this is ready for review again. I did have to require CGI.pl in processmail so that quietly_check_login() would work. I've put in all the essential functionality. * Processmail now redirects output of sendmail into stderr for outsiders and shows them email addresses as *interested parties* * Reports are disabled for outsiders

> Gerv: I think this is ready for review again. I'll do my best to get to it soon; I'm currently doing bug 98801 and other bugs for the b.m.o upgrade on Monday, and preparing my talk on Bugzilla for Linux 2002 in Bristol in July :-) Gerv

After playing with the test site: If gerv@mozilla.org is an insider, how come he can't see bugs 2, 3 or 4? If gerv@gerv.net is in custa, how come he can't see bug 6? UI: The private checkbox UI for comments should be: ------- Additional Comment #2 From Joel 2002-05-11 06:40 ------- [ ] Private The one next to the "Additional Comments" entry field is probably best as: ([X] Private) Additional Comments: with Private also in bold. Implementing Attachment privacy as a status is an excellent idea. On the Create New Attachment screen, the private checkbox should be Private: [ ] as one other "line" of the things you can set about the attachment. After "Comment" would probably be fine. I'll do a code review soon. Gerv

Gerv: WRT comment 30, bugs 2, 3, and 4 belong to yet another prouct group, "aproduct" that I had not given your insider access to (I just added that as well). Bug 6 was one that I used to test out additonal restrictions early in the development. It also had the "aproduct" restriction. This would be impossible to get into that state with the current code. Moving it to internal and then back to custa took care of that one. I'll plan to make the change you suggest to the UI. How do I avoid either blocking the review or wasting your time reviewing it twice? -Joel

> bugs 2, 3, and 4 belong to yet another prouct group, > "aproduct" that I had not given your insider access to (I just added that as > well). I thought insiders automatically had access to everything? Perhaps you need to summarise the current permissions model for me. > How do I avoid either > blocking the review or wasting your time reviewing it twice? The review's not blocked; I'll review the latest patch (whatever it is) when I get time. If you've had time to make the UI changes before then, great. Otherwise, I'll review the Jun8 patch. Gerv

To clarify the permissions model, the normal permissions model applies to bugs, with the following changes... 1) Only insiders can edit bugs in the insider product. 2) Insiders can add/edit/view private attachments and comments on any bug they can edit/view. 3) Buggroupsentry has teeth. Nobody can make a bug outside the insider product public. Only insiders even see the permissions of bugs in the insider product. In general, insiders would be configured to be a member of each customer's product's group as well as the insider group while customers would be members of only their own product's group. The only application I can think of for having insiders that can access inside bugs and a subset of customer items is if a company has account personnel with access to the inside bugs and their own customer's bugs who might be interested in telling their client how a competing client is doing if they had too much access. Personally, I'd rather keep the sales force out of the inside bug log altogether.

> 1) Only insiders can edit bugs in the insider product. What is "the insider product"? The way I envisaged it, there would be multiple products accessible only to insiders. And do you mean "view and edit"? I'm not sure you've completely outlined your changes to the model... Or, at least, you need to expand the scope of your explanation beyond permissions. ;-) > 2) Insiders can add/edit/view private attachments and comments on any bug they > can edit/view. Right. So insiders can be excluded from bugs by not being members of a group, just like anyone else. Gerv

On item 2, we are in sync. On item 1, the way I have things set up, any product where (product name) != (insider group name) will be treated as if it is one of the customer products. Essentially, I use a product group as the insider group and that makes the corresponding product the insider product. So, it would be possible to have a number of other products that various subsets of the insiders could access, but those additional products are unlikely to be terribly useful. If important, it would be possible to add a field to the product that designates the product itself as an insider product and decouple the question of who is in a group of insiders from the question of which products are inside versus outside. This is not needed for any of the scenarios I normally think about. Since customers have access to a single product, I have to use platforms, OSs, and components to catagorize bugs anyway so there is no need for me to have multiple internal products.

> On item 1, the way I have things set up, any product where (product name) != > (insider group name) will be treated as if it is one of the customer products. > Essentially, I use a product group as the insider group and that makes the > corresponding product the insider product. But product groups are just groups whose names happen to match the names of products. Surely it's possible to create a new group, foo, and set the value of Param('insidergroup') to foo? Then you wouldn't have an Insider Product. > So, it would be possible to have a > number of other products that various subsets of the insiders could access, but > those additional products are unlikely to be terribly useful. I'm talking about a number of other products that all the insiders could access. The scenario I'm thinking of here is that an organisation may have a number of products, corresponding to products that they make. They also have a number of customers. For simplicity's sake, we'll assume each customer has a single email account. So, CustomerA buys ProductA and ProductB. His email is added to group ProductA and ProductB. Bugs in this product have the usual insider stuff - the possibility for private comments and attachments - but are otherwise normal. Customer B buys ProductA and ProductC. His situation is the same as above. He can see CustomerA's comments and vice versa. This is all as expected. George is an insider. He's a member of group ProductA, ProductB and ProductC. There are two pre-release products with no customers, ProductD and ProductE. He wants to see those bugs too. How do you set the permissions on those products? They can't be the "insider product" because a) there can only be one, and b) it has to have the same name as the insider group. And they can't have no group, because Customers could see them. My point is that using Bugzilla with insiders is fundamentally different to the open development model. The way I envisage it working is that: Bugs with no groupset are visible only to insiders (ProductD and ProductE). Bugs with a groupset are visible only to people in the correct groups (Products A-C). This way, with usebuggroupsentry on, Customers can only file bugs in products they can see, and they stay in those products, whereas Insiders can file bugs anywhere - but bugs filed in ProductD don't have a group, because there's no product group for ProductD. Make any sense? Gerv

I think that the place where the model diverges is this.... To make the insiders model work, components are used where a completely open model uses products. It is essential for this to ensure that there is no way that Customer A and Customer B can see anything of each other's even if they attempt to make something public or they are customers of the same thing. To do this, each Customer is given a "product." When a single customer uses multiple "things," we use components to tell them apart. This prevents cross-contamination. The insiders deal with a set of components that are a superset of all the components in the various customer "products." Once we do this, "Product" has become associated with the scope of who can access instead of being the top level of hierarchy in classifying bugs. At that point, it becomes less relevent (to me) to have multiple insider products, but it could be done.

> Once we do this, "Product" has become associated with the scope of who can > access instead of being the top level of hierarchy in classifying bugs. Right. Now we have reached the nub of the problem :-) This is the difficulty - we should not be changing the meaning of Products in this way. It is often said that the two-level classification (Product, Component) is not really enough, and there should be a third. Reducing the two levels to one is a big step backwards in usability. <later> I've just spent a long time thinking about how best to do this. I ended up reading bug 68022, which is the rewrite of the groups system. This is how I now see it. It seems we are trying to do two things here: - Improve the groups system so that we can control exactly which users see which products and which bugs. This is bug 68022. - Make it possible that only a select few ("insiders") can see particular comments and attachments on bugs, over and above the improved group system. That's this bug. So the bit of this code that creates an "insiders" group, and restricts access to comments and attachments of any bug based on membership of it - I like that. But I'm not convinced about the "turn products solely into an access control mechanism and make components the new products" bit... I need to do some more hard thinking and look at your patch some more. I'll try and do that in the next few days. Gerv

I've also been looking at bug 68022, but I would rather not have to wait for all of that work to complete before I can start this. Once I open that particular can of worms, I am not sure exactly where to stop (or when that work would reach fruition). I agree that using product as the group mechanism is sub-optimal. I find the same thing with my main installation using groupsentry. It would be nice to have a distinct mechanism. The additions I would need to what 68022 seems to be doing are as follows... 1) When outsiders create a bug, they need to be forced to restrict it to their own group. (all their groups??) 2) Outsiders need to be kept from editing any bug that is not rescticted to their own group. 3) Protect email notifications, reports, and bug activity logs from disclosing unwanted information. 4) Private comments and attachments. 5) Insiders should be able to generate reports based on the group restrictions of the bugs (show all bugs visible to xxxx)

Yes, I think bug 68022 and this bug are complementary; that bug is obviously the right way to do permissions on a bug-to-bug level, and this one is the right way to do "permissions" inside a bug - private comments and attachments. > The additions I would need to what 68022 seems to be doing are as follows... > 1) When outsiders create a bug, they need to be forced to restrict it to > their own group. (all their groups??) Yes. That shouldn't be hard. > 2) Outsiders need to be kept from editing any bug that is not rescticted to > their own group. Not really. If they can't see it, they can't edit it. :-) I think they should be able (if they have editbugs) to edit any bugs they have permissions to see. > 3) Protect email notifications, reports, and bug activity logs from > disclosing unwanted information. The groups system in 68022 should do this for you (apart from the private comments/attachments stuff... 4) Private comments and attachments. ...which is what this bug is all about. > 5) Insiders should be able to generate reports based on the group > restrictions of the bugs (show all bugs visible to xxxx) Hmm. This is a separate bug on the query interface, really. When reporting gets rewritten (oh, that glorious day), I'll try and make this possible. So my conclusion is that the private comments/attachments stuff from this bug, combined with the fine-grained permissions stuff over in bug 68022 should be able to combine together to do what you want, and what I (might) want, and is the right way for the main Bugzilla codebase. If you can't wait, that's fine - do your own thing. No-one will complain. The other option is to throw your weight behind 68022 :-) Gerv

OK... sounds like the following... 1) One patch strictly for private comments and attachments kept private to an insider group. This would be a candidate for near-term commit into the main branch of CVS. 2) The additional protections here would be bundled together as a seperate patch. This second patch may have to stay in patch form until after 68022 is all done. 3) Included in those additional protections will be the restiction against users editing or commenting on bugs that they can see but whose product is one that they could not create a bug in. This could be made selectably by a param or by a flag on the product itself and could be kept seperate from #2 above. Gerv: I'll keep this unbundled from the others if it is a likely early merge as well.

Gerv: Here's the patch to the current tip with just the private comment and private attachment portions isolated.

Comment on attachment 89508 [details] [diff] [review] Patch for only the comment and attachment privacy >+++ new/Attachment.pm Thu Jun 27 21:05:12 2002 >@@ -49,7 +49,7 @@ > # of hashes in which each hash represents a single attachment. > &::SendSQL(" > SELECT attach_id, creation_ts, mimetype, description, ispatch, >- isobsolete, submitter_id >+ isobsolete, submitter_id, private > FROM attachments WHERE bug_id = $bugid ORDER BY attach_id > "); > my @attachments = (); >@@ -57,7 +57,8 @@ > my %a; > my $submitter_id; > ($a{'attachid'}, $a{'date'}, $a{'contenttype'}, $a{'description'}, >- $a{'ispatch'}, $a{'isobsolete'}, $submitter_id) = &::FetchSQLData(); >+ $a{'ispatch'}, $a{'isobsolete'}, $submitter_id, $a{'private'}) >+ = &::FetchSQLData(); Nit: you should put private before $submitter_id. >- "*** This bug has been confirmed by popular vote. ***"); >+ "*** This bug has been confirmed by popular vote. ***",1); Nit: please put a space between function parameters. > # Make sure the attachment exists in the database. >- SendSQL("SELECT bug_id FROM attachments WHERE attach_id = $::FORM{'id'}"); >+ SendSQL("SELECT bug_id, private FROM attachments WHERE attach_id = $::FORM{'id'}"); > MoreSQLData() > || DisplayError("Attachment #$::FORM{'id'} does not exist.") > && exit; > > # Make sure the user is authorized to access this attachment's bug. >- my ($bugid) = FetchSQLData(); >+ my ($bugid,$private) = FetchSQLData(); Nit: space, please. > ValidateBugID($bugid); >+ if (($private > 0 ) && Param("insidergroup") && !(UserInGroup(Param("insidergroup")))) { >+ DisplayError("Attachment -- Denied") ; Can we have a more explanatory error, please? :-) And you need to be using ThrowUserError now. Which calls exit() for you. > # Retrieve the attachments from the database and write them into an array > # of hashes where each hash represents one attachment. >- SendSQL("SELECT attach_id, creation_ts, mimetype, description, ispatch, isobsolete >- FROM attachments WHERE bug_id = $::FORM{'bugid'} ORDER BY attach_id"); >+ my $privacy = ""; >+ if (Param("insidergroup") && !(UserInGroup(Param("insidergroup")))) { >+ $privacy = "AND private < 1 "; >+ } >+ SendSQL("SELECT attach_id, creation_ts, mimetype, description, ispatch, isobsolete, private >+ FROM attachments WHERE bug_id = $::FORM{'bugid'} $privacy ORDER BY attach_id"); You appear to have indentation issues here (and elsewhere in this patch.) > my $description = SqlQuote($::FORM{'description'}); > my $contenttype = SqlQuote($::FORM{'contenttype'}); > my $thedata = SqlQuote($::FORM{'data'}); >- >+ my $private = ($::FORM{'private'} == 1) ? 1 : 0; Can you do this the same way as before? my $private = $::FORM{'private'} ? 1 : 0; > # Insert the attachment into the database. >- SendSQL("INSERT INTO attachments (bug_id, filename, description, mimetype, ispatch, submitter_id, thedata) >- VALUES ($::FORM{'bugid'}, $filename, $description, $contenttype, $::FORM{'ispatch'}, $::userid, $thedata)"); >+ SendSQL("INSERT INTO attachments (bug_id, filename, description, mimetype, ispatch, submitter_id, thedata, private) >+ VALUES ($::FORM{'bugid'}, $filename, $description, $contenttype, $::FORM{'ispatch'}, $::userid, $thedata, $private)"); Again, private probably shouldn't be on the end. > > # Retrieve the ID of the newly created attachment record. > SendSQL("SELECT LAST_INSERT_ID()"); >@@ -492,7 +509,7 @@ > > AppendComment($::FORM{'bugid'}, > $::COOKIE{"Bugzilla_login"}, >- $comment); >+ $comment,1); Give the 1 its 15 seconds of fame with its own line. :-) >- SendSQL("SELECT description, mimetype, bug_id, ispatch, isobsolete >+ SendSQL("SELECT description, mimetype, bug_id, ispatch, isobsolete, private > FROM attachments WHERE attach_id = $::FORM{'id'}"); >- my ($description, $contenttype, $bugid, $ispatch, $isobsolete) = FetchSQLData(); >+ my ($description, $contenttype, $bugid, $ispatch, $isobsolete, $private) = FetchSQLData(); I'm getting the feeling the field should be called isprivate rather than private, to match ispatch and isobsolete. >+ >+# 2002-05-10 - enhanchment bug 143826 >+# add private flag for each comment ... and attachment. Please explain what it's for, not what you are doing. >+DefParam("insidergroup", >+ "The group of users who can see/change private descriptions", >+ "t", >+ ''); "The name of the group of users who can see and change private comments and attachments." >+ if (($private > 0) && Param("insidergroup")) { >+ $result .= "PARAM_PRIVATE\n"; >+ } This looks like a hack to me :-) >diff -u --recursive old/processmail new/processmail >--- old/processmail Thu May 23 01:08:53 2002 >+++ new/processmail Thu Jun 27 21:55:35 2002 >@@ -30,6 +30,7 @@ > use lib "."; > > require "globals.pl"; >+require "CGI.pl"; Why is this necessary? >+ [% IF count > 0 >+ && mode == "edit" >+ && Param("insidergroup") && UserInGroup(Param("insidergroup")) %] >+ <input type=hidden name="oprivate-[% count %]" value="[% comment.private %]"> >+ <input type=hidden name="when-[% count %]" value="[% comment.when %]"> >+ <input type=checkbox name="private-[% count %]" value="1" >+ [% " checked=\"checked\"" IF comment.private %]> Private >+ <br> >+ [% END %] A few formatting issues here, too (and in other places throughout the patch.) Copy indent and other style from other templates. What's the "oprivate" business? > <br> > <b>Additional Comments:</b> >+ [% IF Param("insidergroup") && UserInGroup(Param("insidergroup")) %] >+ <br> >+ <input type="checkbox" name="commentprivacy" value="1"> Private >+ [% ELSE %] >+ <input type="hidden" name="commentprivacy" value="0"> >+ [% END %] not much point putting the hidden field in, is there, given that we assume an absence to be a zero value? > [% PROCESS global/header.html.tmpl > title = "Full Text Bug Listing" >+ style_urls = [ "css/show_multiple.css" ] > %] Some of another patch got caught up here? :-) Gerv

Revised patch with adjustments per Gerv's review. (the style URLs are being used here, not defined here - this was the motivation for bug 148679 )

Applied the July 12 patch, so far works without problems for me.

Comment on attachment 91216 [details] [diff] [review] Patch for July 12 tip >- # Make sure the user is authorized to access this attachment's bug. >- my ($bugid) = FetchSQLData(); >- ValidateBugID($bugid); >+ # Make sure the user is authorized to access this attachment's bug. >+ my ($bugid, $isprivate) = FetchSQLData(); >+ ValidateBugID($bugid); >+ if (($isprivate > 0 ) && Param("insidergroup") && !(UserInGroup(Param("insidergroup")))) { >+ ThrowUserError("Access to attachment -- Denied") ; "You are not permitted access to this attachment." > # Append the comment to the list of comments in the database. >- AppendComment($bugid, $who, $wrappedcomment); >+ AppendComment($bugid, $who, $wrappedcomment,1); Nit: missing space. >+ if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) { >+ push(@wherepart, "$table.isprivate < '1'") ; >+ } Use a bare 1, for consistency with earlier code. >+DefParam("insidergroup", >+ "The name of the group of users who can see/change private comments >+ and attachments.", "Note: this group must exist." (if that's true.) >- return $result; >+ return ($result,$anyprivate); Nit: need space. >+if ( $::FORM{'id'} && >+ (Param("insidergroup") && UserInGroup(Param("insidergroup")))) { >+ detaint_natural($::FORM{'id'}); >+ foreach my $i (keys %::FORM) { >+ if ( $i =~ /when-([0-9]+)/ ) { >+ my $n = $1; >+ my $p = $::FORM{"isprivate-$n"} ? 1 : 0 ; >+ if ( $p != $::FORM{"oisprivate-$n"}) { >+ detaint_natural($::FORM{"$i"}); >+ SendSQL(qq[UPDATE longdescs SET isprivate = $p >+ WHERE bug_id = $::FORM{'id'} AND bug_when = $::FORM{"$i"}]); >+ } >+ } >+ >+ } >+} This took me a while to figure out. Can we please have more descriptive variable names, double quotes in SendSQL() as is normal elsehwere, Hmm. This is going to make form submissions larger on bugs with a lot of comments, because you are submitting a date and an old value for each comment. But I don't see an obvious way to avoid this. Is there not a comments table index you can use, which is shorter than the date? >+ AppendComment($duplicate, $::COOKIE{'Bugzilla_login'}, "*** Bug $::FORM{'id'} has been marked as a duplicate of this bug. ***",1); Nit: missing space. >- my $newcomments = GetLongDescriptionAsText($id, $start, $end); >+ my ($newcomments,$anyprivate) = GetLongDescriptionAsText($id, $start, $end); Nit: missing space. But, overall, this is pretty good :-) r=gerv. Gerv

Comment on attachment 91216 [details] [diff] [review] Patch for July 12 tip Hmm. I expected granting the review to do this for me... Myk? Gerv

I haven't set that up yet. Ack.

OK.. Incorporated all of Gerv's comments. I tested what happens if insidergroup points at a group that doesn't exist. Everything works, but nobody is given access to private comments and attachments.

Comment on attachment 91949 [details] [diff] [review] Patch to July 19 tip with review changes folded in >Index: CGI.pl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/CGI.pl,v >retrieving revision 1.162 >diff -u -r1.162 CGI.pl >--- CGI.pl 10 Jul 2002 06:27:11 -0000 1.162 >+++ CGI.pl 19 Jul 2002 12:53:45 -0000 >@@ -1034,7 +1034,7 @@ > } > > AppendComment($id, DBID_to_name($who), >- "*** This bug has been confirmed by popular vote. ***"); >+ "*** This bug has been confirmed by popular vote. ***", 1); Does this mean a vote confirmation statement is going to be private? If so, why? >Index: attachment.cgi >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v >retrieving revision 1.15 >diff -u -r1.15 attachment.cgi >--- attachment.cgi 4 Jul 2002 09:12:33 -0000 1.15 >+++ attachment.cgi 19 Jul 2002 12:53:45 -0000 >@@ -244,6 +248,14 @@ > $::FORM{'isobsolete'} = $::FORM{'isobsolete'} ? 1 : 0; > } > >+sub validatePrivate >+{ >+ # Set the isobsolete flag to zero if it is undefined, since the UI uses >+ # an HTML checkbox to represent this flag, and unchecked HTML checkboxes >+ # do not get sent in HTML requests. >+ $::FORM{'isprivate'} = $::FORM{'isprivate'} ? 1 : 0; >+} >+ We have a copied/pasted comment here that didn't get adjusted for its new surroundings. (isobsolete != isprivate) I have NOT done a complete review (I'm a bit busy at the moment) so I won't mark this needs-work, but I just wanted to point out the couple things I noticed in a quick glance at it.

WTR justdave's comments.. 1) Comment is fixed 2) Revisited hard-coded private settings. Made dupe comments defualt to public. Made attachment comments track the private/public setting of the attachment itself.

Comment on attachment 91949 [details] [diff] [review] Patch to July 19 tip with review changes folded in denying review because attachment is obsolete

Comment on attachment 91953 [details] [diff] [review] Update with justdave's point taken... denying review because attachment is obsolete

Comment on attachment 91954 [details] [diff] [review] Doh! Same patch with the 2 new CSS files included attachment is obsolete

Comment on attachment 92189 [details] [diff] [review] Revised patch with timeless's suggestions >+ [% IF (Param("insidergroup") && UserInGroup(Param("insidergroup"))) %] >+ <th></th> >+ <td> >+ <input type="checkbox" name="isprivate" value="1"> private<br> >+ </td> >+ </tr> >+ [% END %] Small nit: indent inside <td> >+ [% IF (Param("insidergroup") && UserInGroup(Param("insidergroup"))) %] >+ <input type="checkbox" name="isprivate" value="1"[% " checked" IF isprivate %]> private<br><br> >+ [% END %] And inside IF. Testing whether Myk's implemented auto-status-changing... Gerv

Comment on attachment 92189 [details] [diff] [review] Revised patch with timeless's suggestions Nope. :-) Gerv

Joel, can you produce a new patch? The last was does not work against the current trunk.

WRT comment 59: It shouldn't be too far off if you want to tweak it for now. I will re-merge this after bug 157756 lands.

Joel: if you merge this, I'll check it in with just my review. The timings for the groups patch are uncertain, so I'd like to get this in now :-) Gerv

OK, I've had enough of this :-). r=gerv. Checked in, with a few UI tweaks (placement changes for the checkboxes) for consistency and beauty. Joel: thanks for your patience. Gerv

*** Bug 118736 has been marked as a duplicate of this bug. ***

*** Bug 118738 has been marked as a duplicate of this bug. ***

Any plans to log who changes a comment's privacy setting?

*** Bug 7415 has been marked as a duplicate of this bug. ***