1211377 - needinfo from someone not in sec group shows warning message even when bug is being removed from sec group

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[:Gijs (he/him)]

STR:

1. find sec-sensitive bug
2. go into edit mode
3. untick relevant sec-sensitive flag
4. also request needinfo from someone not in the sec group that previously applied to the bug (and not on CC list)
5. click 'save changes'

ER:
it works

AR:
it gives me a red-bordered big-font warning that this person isn't in the sec group and am I really sure I want to do this?


(also, I find that warning generally really annoying because I see it multiple times a day, and the only effect it has on me is having to go "back", expand the 'people' section, and add the person to the CC list manually, and then re-execute some of my changes, which often leads to me forgetting to do one of the latter things, which means I have to change the bug multiple times, which is frustrating - can I please please please have an option in that warning to just say "yes, I know, please CC this person for me" ?)

Comment 1

[:Gijs (he/him)]

This is still hitting me regularly. I realize people with (some) sec group access are a minority, so it's not a priority. Where does this code live so I can contribute a patch myself?

Comment 2

[:glob ✱]

so what needs to happen is the needinfo extension needs to check needinfo requestees, and cc them if they cannot view the bug anymore.

you'll need to add a bug_end_of_update hook to the needinfo extension
https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/Needinfo/Extension.pm

eg. https://github.com/mozilla/webtools-bmo-bugzilla/blob/74c6f9974fddc33163ae78c5927c6569f2eb28c0/extensions/MyDashboard/Extension.pm#L190

iterate over the requestees and check if each user can see the bug.
there's sample flag iteration code at line 187 of NeedInfo/Extension.pm, you can see it skipping flags based on name and flags without a requestee.

to test if a user can see a bug you should be able to use $flag->requestee->can_see_bug($bug->id).
and cc'ing the user to the bug should just be $bug->add_cc($flag->requestee).

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to :Gijs from comment #0)
> it gives me a red-bordered big-font warning that this person isn't in the
> sec group and am I really sure I want to do this?

NB: it's not warning you against adding people to the visibility of the bug, it's warning you that the person you're asking won't be able to see your request. Adding them to the CC list automatically is probably the right thing to do. If it were worried about adding people to hidden bugs then CC'ing them would have a warning also. It doesn't, because CC'ing is how folks trusted to view a bug are able to say "You need to see this, too".

That still leaves an edge case for bugs where someone has un-checked the "Users in the roles selected below can always view this bug:" boxes -- CC'ing the person won't help. In that case the warning makes sense. CC'ing the user won't help, and having those boxes unchecked is so very very rare that I wouldn't want us to automatically re-enable them.

There's another edge case: you needinfo someone in a public bug and later someone adds a group the requestee can't see. Maybe needinfo? (and review, etc requests) should always CC the requestee just in case.

Comment 4

[:Gijs (he/him)]

(In reply to Byron Jones ‹:glob› from comment #2)
> so what needs to happen is the needinfo extension needs to check needinfo
> requestees, and cc them if they cannot view the bug anymore.
> 
> you'll need to add a bug_end_of_update hook to the needinfo extension
> https://github.com/mozilla/webtools-bmo-bugzilla/blob/master/extensions/
> Needinfo/Extension.pm
> 
> eg.
> https://github.com/mozilla/webtools-bmo-bugzilla/blob/
> 74c6f9974fddc33163ae78c5927c6569f2eb28c0/extensions/MyDashboard/Extension.
> pm#L190
> 
> iterate over the requestees and check if each user can see the bug.
> there's sample flag iteration code at line 187 of NeedInfo/Extension.pm, you
> can see it skipping flags based on name and flags without a requestee.
> 
> to test if a user can see a bug you should be able to use
> $flag->requestee->can_see_bug($bug->id).
> and cc'ing the user to the bug should just be $bug->add_cc($flag->requestee).

FWIW, doing this in bug_end_of_update doesn't seem to be sufficient and still gets me errors. I think this is because the flag _check_requestee validation stuff bails well before we reach bug_end_of_update. So I put this in the begin_of_update code instead. Which I'm sure is wrong somehow, but it seemed to wfm in some naive manual testing in the vagrant env.