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

Status: RESOLVED FIXED

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

Description

[Dave Miller [:justdave]]

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.

Comment 1

[Joe Robins]

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?

Comment 2

[Dave Miller [:justdave]]

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.

Comment 3

[Dave Miller [:justdave]]

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.

Comment 4

[Dave Miller [:justdave]]

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.

Comment 5

[Jesse Ruderman]

see also bug 39067.

Comment 6

[Gervase Markham [:gerv]]

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

Comment 7

[Dave Miller [:justdave]]

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.

Comment 8

[Ben Bucksch (:BenB)]

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

Comment 9

[Blake Ross]

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.

Comment 10

[Ben Bucksch (:BenB)]

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

Comment 11

[Blake Ross]

"(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?

Comment 12

[Ben Bucksch (:BenB)]

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.

Comment 13

[Jacob Steenhagen]

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                             |
+----------------------------------------------------------------------------+

Comment 14

[Alex Nowak]

Jake put it the way I would like to see it.

Comment 15

[Dave Miller [:justdave]]

I like the way Jake described too.  That'll require a bit of reworking of the way 
groups are handled though.

Comment 16

[Dave Miller [:justdave]]

moving to real milestones...

Comment 17

[Matthew Tuck [:CodeMachine]]

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

Comment 18

[Jesse Ruderman]

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.

Comment 19

[Dave Miller [:justdave]]

no point in doing this twice.  Let's get the group management stuff fixed first.  
Adding dependency on the group table redo bug.

Comment 20

[Stephen Rasku]

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?

Comment 21

[Jacob Steenhagen]

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

Comment 22

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

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?

Comment 23

[Dave Miller [:justdave]]

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

Comment 24

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

Dave, are you working on this?

Comment 25

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

Attachment: patch v1

Comment 26

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

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.

Comment 27

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

Attachment: patch v2 (adds assignee and fixes mid-air collision problem)

Comment 28

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

Attachment: patch v3 (really minor semantic changes)

Comment 29

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

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.

Comment 30

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

adding the URL to the test installation on landfill where you can see the patch
in action:

http://landfill.tequilarista.org/bz66235/

Comment 31

[Dave Miller [:justdave]]

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

Comment 32

[Jacob Steenhagen]

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.

Comment 33

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

Attachment: patch v4: fixed bug where flags cleared in multiple bug change

Comment 34

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

Attachment: patch v4: the real one (ignore the one above)

Comment 35

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

Attachment: patch v5: adds explanatory text

Comment 36

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

Attachment: patch v6: more improvements to explanatory text

Comment 37

[Dave Miller [:justdave]]

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.

Comment 38

[Dawn Endico]

test

Comment 39

[Dave Miller [:justdave]]

Moving to Bugzilla product

Comment 40

[James A. Laska]

We also may want to only display the QA Contact checkbox (in bug_form.pl) if the
Param('useqacontact') is set.

Comment 41

[James A. Laska]

Attachment: Add Param('useqacontact') check in bug_form.pl