200957 - CanSeeBug should check if the user will have access after changes

Status: RESOLVED WORKSFORME

(Bugzilla :: Bugzilla-General, defect)

Description

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

CanSeeBug should check if the user will have access to the bug after the
currently submitted changes go through.  Otherwise it'll report incorrectly that
the user will or won't be able to see the bug when that access is changing (f.e.
when the user is being added to or removed from the CC: list).

See bug 179510.

Comment 1

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

Some code I was including in my patch for bug 179510 before I decided to spawn a
separate bug:

          if (# the user can't see the bug
              !&::CanSeeBug($bug_id, $requestee_id)
              # and they won't be able to see it after these changes
              && (# either because they're being removed from the cc: list
                  ($data->{'removecc'}
                   && grep($_ eq $requestee_email, @{$::MFORM{'cc'}}))
                  # or because they aren't being added to it
                  || !($data->{'cclist_accessible'}
                       && $data->{'newcc'} =~ /\Q$requestee_email\E/)))

Comment 2

[Bradley Baetz (:bbaetz)]

'CanSeeBug should check if the user will have access to the bug after the
currently submitted changes go through.'

Well, not CanSeeBug, since thats going to be a method on Bug.pm which knows
nothign about process_bug yet.

See, if we had transactions, we could make the change, test, and then rollback
if needed, but.... :)

Does this affect anything apart from this now, though?

Comment 3

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

As far as I know, this is the first case of something affected by this.

Comment 4

[Dave Miller [:justdave]]

This isn't making the 2.17.4 train unless it's resolved in the next 24 hours.

Comment 5

[Dave Miller [:justdave]]

Does this affect 2.16.3?

Comment 6

[Dave Miller [:justdave]]

myk/bbaetz: status?

Comment 7

[Gervase Markham [:gerv]]

Oops. Windows in a twist. Sorry. De-requesting approval.

Gerv

Comment 8

[Gervase Markham [:gerv]]

Can someone explain why this is a security-sensitive bug blocking 2.17.5? 

From the description, it seems that all that'll happen is that e.g. someone may
get a notification mail saying "this bug is being secured, and you can't see it
any more" rather than it just disappearing off their radar.

Gerv

Comment 9

[Dave Miller [:justdave]]

pushing to 2.17.6, since we obviously don't have code yet, and this isn't quite
a show-stopper.

Comment 10

[Dave Miller [:justdave]]

so, is it, or isn't it?   Looks like we're doing 2.17.6 this weekend because of
the js buglists thing.  Can this go with it?

Comment 11

[Dave Miller [:justdave]]

I seem to recall this being debated back when cclist_accessible was first
implemented, and seems like we chose to do it the way it is now for some reason...

Comment 12

[Christian Reis]

If Gerv's comment #8 describes precisely what happens (including the gist of the
message), I'd suggest WONTFIX, on the ground that it provides decent user feedback.

Comment 13

[Gervase Markham [:gerv]]

Having just run a test:

If a person is removed from the CC list of a secure bug, it simply drops off
their radar with no notification. 

I believe that this was the original behaviour designed in by dmose several
years ago, because I helped him test it. See this comment on line 715 of BugMail.pm:

    # XXX - This currently means that if a bug is suddenly given
    # more restrictive permissions, people without those permissions won't
    # see the action of restricting the bug itself; the bug will just 
    # quietly disappear from their radar.

I think this is reasonable, to be honest. It's also safe - someone who doesn't
think too hard about how this might work may well add a sensitive comment at the
same time they remove the person in question from the CC list.

Gerv

Comment 14

[Dave Miller [:justdave]]

ok, so this bug is a WORKSFORME then?  Or did it regress?

Comment 15

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

I believe the problem this bug is meant to address is that request mail gets
sent to cc: users who are being removed from the cc: list.  I'll do some more
testing to verify.

Comment 16

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

Nope, not a flag problem.  Flag emails get sent after CC list changes have
already happened.

Comment 17

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

After updating the CC list (and making all other changes besides),
process_bug.cgi processes bug/process/results.html.tmpl, which processes
bug/process/bugmail.html.tmpl, which calls SendBugMail() (defined in
Bugzilla/Template.pm), which calls Bugzilla::BugMail::send(), which calls
ProcessOneBug(), which calls NewProcessOnePerson(), which calls CanSeeBug() and
doesn't send the mail if the user can't see the bug.

So this isn't a problem with non-Flag emails either.  So this was either a brain
fart or a problem with an earlier version of the code that doesn't exist now. 
Either way, WORKSFORME.

Comment 18

[Dave Miller [:justdave]]

clearing target on worksforme/duplicate/invalid so they'll show up as untriaged
if they get reopened