Closed Bug 39816 Opened 25 years ago Closed 23 years ago

Anyone in CC/reporter/QA Contact fields should see restricted bugs

Categories

(Bugzilla :: Bugzilla-General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.14

People

(Reporter: justdave, Assigned: myk)

References

(Blocks 1 open bug, )

Details

(Whiteboard: security code)

Attachments

(8 files)

Anyone listed in the CC, reporter, QA Contact, or owner fields on a bug should be able to see that bug, regardless of the group permissions set on that bug. If you really don't want that person to see that bug, you should take their name off it, also (since they're going to get emails on it anyway). This would also have the added bonus of being able to set up a group for "private bugs" that only the reporter, QA, and the owner could see, by making sure no one was a member of that "private bugs" group. Main use of this would be for customer support tickets where the customer could still be able to view their own bug, while keeping the general public out of it, and not requiring a group to be created for each customer.
Sounds like a good idea. As for how to do this, I think we could probably manage it pretty easily by modifying the initial select statement in bug_form.pl. The current version has: select ... from bugs left join votes using(bug_id) where bugs.bug_id = $id and bugs.groupset & $::usergroupset = bugs.groupset group by bugs.bug_id We could update it to read: select ... from bugs left join votes using(bug_id) where bugs.bug_id = $id and ((bugs.groupset & $::usergroupset = bugs.groupset) or (something to check the reporter, assigned_to, qa_contact, and cc fields)) group by bugs.bug_id I'd do it myself, but my SQL knowledge is slightly lacking for the task, because cc is in a separate table, and I'm not certain enough of my knowledge of mySQL to do joins and all that. It looks like this would be sufficient to allow the user to see bugs that he was involved in, even if not in the group for it, right? Anyone with a slightly better working knowledge of SQL want to step up and give this a whirl?
This works: select ...... from bugs left join votes on bugs.bug_id = votes.bug_id left join cc on bugs.bug_id = cc.bug_id where bugs.bug_id = 131where bugs.bug_id = $id and ((bugs.groupset & $::usergroupset = bugs.groupset) or (reporter = $::userid) or (assigned_to = $::userid) or (qa_contact = $::userid) or (cc.who = $::userid)) group by bugs.bug_id had to change the using(bug_id) to a specific match since there were now more than 2 bug_id columns and it confused it. My initial testing here presents another problem though... The groupset selectors where you can set what groups it has access to... if the user doesn't have access to a group, they don't get a selector to move the bug in and out of that group. If the selector isn't there, it's not counted when it adds up the groupset when you hit Commit. So if someone in one of those other fields looks at a hidden bug and then changes it, poof, it's no longer hidden. The solution for that part is to put the groupset selectors in the form anyway, but make them hidden if the user doesn't have access, instead of not having them there at all.
Can't forget about buglist.cgi either... they'd have to be able to see their bugs there, too. Chris, I'm dragging you into this because we'll probably have a patch available for this within a few days, and this one is far-reaching enough it'll need some play on landfill before we check it in.
And we also can't forget processmail. Currently, processmail restricts mailings to those who have access to the bug, and ignores CCs if the CC'd people don't have access to it.
see also bug 39067.
Dave, Chris and co: Implementing this RFE would be very useful for security bugs, where the strong suggestion in n.p.m.security (where Frank Hecker has just kicked off that debate again) is that they should be restricted to a certain group _plus_the_reporter_. Is there any chance of munging the thoughts in the comments in this bug into a patch? Gerv
Assignee: tara → dave
I already have it partially implemented on my system as of about a month or two ago, but it didn't get uploaded because I didn't finish it. (i.e. you can view a bug that you're the reporter of (show_bug), but you can't find it in a query (bug_list) and it won't send you mail about it (processmail)). Since I already have part of it done, and had actually forgotten about it (thanks for reminding me) I'll go ahead and grab this bug report and see if I can finish it up.
IMO, this is a bug. SEVERITY -> normal. There were several NS-"confidental" bug on which I was first cced, but then got neither notifications nor could access it (but I got no notification about being removed from cc).
Severity: enhancement → normal
But there's security issues involved in this... First, you might not always want the reporter to see the bug. Sure it sucks, but if you've got all kinds of internal code in the bug report, the reporter can't see it... And what happens when a bug is made confidential, but all non-internal people aren't removed? I can see that happening quite a lot.
> Sure it sucks, > but if you've got all kinds of internal code in the bug report, the reporter > can't see it... (Note: That might be true for closed source project, but it isn't for bugzilla.mozilla.org. There shouldn't be proprietary code posted to Mozilla resources. Which opens the question why *any* non-security bug can be confidental on bugzilla.mozilla.org.) > And what happens when a bug is made confidential, but all non-internal people > aren't removed? I can see that happening quite a lot. Yes, but they should still have access. Otherwise remove them. I should be possible to allow selected "outsiders" to access the bug.
"(Note: That might be true for closed source project, but it isn't for bugzilla.mozilla.org. There shouldn't be proprietary code posted to Mozilla resources. Which opens the question why *any* non-security bug can be confidental on bugzilla.mozilla.org.)" Yes, this is something that has been discussed many times. But we're not getting into that argument again in this bug, because that's not what this bug is for. "Yes, but they should still have access. Otherwise remove them. " But people make mistakes. And it's all tiresome to manually remove 30 non- internal people from a CC list. And how do you remove the reporter?
OK, but how do you allow selected people access? I had cases where Netscape wanted to do changes to my code for N6 only, or checkin the changes to mozilla.org (e.g. a new pref), but the intention was N6 (-> different default prefs). In one case I even revied the changes for a bug I could not access. That's not right.
OK, here's my thoughts on how this should work... good, bad or indifferent. * Completely remove the groupset from the WHERE part of SQL Queries (But leave it in the SELECT part [esp. when a large number of bugs is being returned a la buglist.cgi]). * Create a new sub in globals.pl call "UserCanSeeBug" (See Below). * Change the security dropdown to a series of checkboxes (See Below). * Add a new option in editcomponents to set the initial security setting (New bugs are secure) The UserCanSeeBug subroutine could probably then be used to kill some of the bugs attached to bug 66091. --- Below --- sub UserCanSeeBug { my ($bugid, $grpset) = (@_); if (defined $grpset && $grpset == 0) { return 1; } # A bunch of code to get information about bug number $bugid and the current # user. After that, there should be some code to check permissions. If at # any point it's determined that the user can see the bug, simply # "return 1;" (Basically, assume the user can't see the bug, unless # determined otherwise). # All options are exausted... return 0; } --- Below #2 --- +----------------------------------------------------------------------------+ | Bug Security: | | If none of the checkboxes below are checked, the bug can be seen by all | | [ ] Users in Group "Security" | | [ ] Users in Group "Software Engineers" | | ( ) And (*) Or | +----------------------------------------------------------------------------+ | Allow these additional people to see the bug: | | [X] Reporter [ ] QA Contact [ ] CC List | +----------------------------------------------------------------------------+
Jake put it the way I would like to see it.
I like the way Jake described too. That'll require a bit of reworking of the way groups are handled though.
Whiteboard: security
Blocks: 60769
Whiteboard: security → 2.14, security
moving to real milestones...
Whiteboard: 2.14, security → security
Target Milestone: --- → Bugzilla 2.14
*** Bug 39067 has been marked as a duplicate of this bug. ***
When this feature is added, we need to make sure that someone watching a user CCed on a restricted bug doesn't get mail from the restricted bug unless the watcher is also a part of the group.
no point in doing this twice. Let's get the group management stuff fixed first. Adding dependency on the group table redo bug.
Depends on: 68022
First, some comments about Jake's proposal: I like the use of check boxes for groups instead of drop down lists. That's what Red Hat does and I have modified my local installation of bugzilla to do the same. I also like the addition of the and/or construct. I believe this is easier for the user to understand. Currently, if you check more than one group, only users in all groups can see the bug. Now, a couple of questions about Jake's proposal: 1. Is it optional? I don't want to have to click a bunch of boxes to get a bunch of people to see a bug that currently see the bug by default now. I don't even want to see these controls on the interface. I would want to keep the group UI though. 2. If you don't select CC, does that mean that non of the CC list sees it or only that CCers can only see it if they belong to the proper group?
> I also like the addition of the and/or construct. So did I :) My day job is as a Network Administrator so to me assigning group permissions is normally an "or" statement (If you are in group CCN or SNR then you can see the Security directory). But realizing that the current bugzilla is based on an AND, it wouldn't be very nice of us to suddenly change it to OR. > 1. Is it optional? Well, if you aren't in any bug groups, it won't show up (like the present drop-downs). Anything more than that is really up to Dave (as the owner of this bug). > 2. If you don't select CC ... Then the CC list doesn't get added to the list of ppl that can view the bug. The section labeled "Allow these addition people" would not effect anyone who otherwise would have permission (is in all the correct groups). Also, most of what I proposed for UserCanSeeBug() has been implimented in ValidateBugID() [Validate also makes sure that bug ID's are numeric but it doesn't have the ability to optionally accept $groupset which would be a requirement to use it from buglist (it would also have to be able to cache some information about the logged in user and accept information about the bugs reporter, CC list, etc. in order to be used for this bug, otherwise querying becomes a much more expensive operation)].
Whiteboard: security → security code
No longer depends on: 68022
This bug was previously waiting for groups to be redone in bug 68022, which has since been rescheduled to 2.16. Should this bug also be rescheduled?
maybe. It can be done with the current group code, we just didn't want to have to do it over again with the new group code being so close. Now that it's not so close anymore... I dunno. I suppose this one is mine, though, since I sort of had it working on InTrec...
Blocks: 40885
Blocks: 92257
Dave, are you working on this?
Attached patch patch v1 (deleted) — Splinter Review
This attachment does the second part of what Jake wanted (the first part having already been done in some other bug whose number I don't remember now). It works except for the strange habit of detecting a collision with every change that has previously occurred. I will look into it.
The second/third versions of the patch adds the assignee to the list of roles who can be granted access to an otherwise inaccessible bug, and it fixes the problem with mid-air collisions (Bugzilla apparently doesn't like it when code calls &MoreSQLData and then doesn't call &FetchSQLData). Version three contains very minor adjustments in wording (Cc -> CC for consistency and role -> user for clarity). Patch ready for review.
Assignee: justdave → myk
Keywords: patch, review
adding the URL to the test installation on landfill where you can see the patch in action: http://landfill.tequilarista.org/bz66235/
I like it. But I think we might need to massage the wording a little... it's obvious to me, but probably not blatently obvious to someone looking at it for the first time...
I think the proper way to fix that problem of calling MoreSQLData() but not FetchSQLData() causing problems on the next query would be to add |undef @::fetchahead;| to the SendSQLData() routine. Also, as for the wording, I think saying (possibly in smaller type underneath the "But always..." text) "These aditional options will not be effective unless at least on of the group checkboxes is selected" (or something similar). See also my comments in bug 66235.
Attached patch patch v5: adds explanatory text (deleted) — Splinter Review
Bug 95024 has been filed to allow the Reporter/QA/Assignee/CClist to be able to see these bugs in their buglist also (this patch doesn't allow for that) r= justdave checked in.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
No longer blocks: 60769
test
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
We also may want to only display the QA Contact checkbox (in bug_form.pl) if the Param('useqacontact') is set.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: