change the insider group from core-security to core-security-release

RESOLVED FIXED

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: dylan)

Tracking

Production

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
This makes it a pain to say something security sensitive on a bug that we don't want to close down...  I resorted to emailing people manually today.
I see a "private" checkbox above and to the right of the comment text box. Unless you're talking about retroactively setting a comment as private?
Are you using the new bug form? If so, you need to click 'Edit' first at the top to see the private checkboxes.
Flags: needinfo?(ehsan)
(Reporter)

Comment 3

3 years ago
No, I am talking about show_bug.cgi.  I cannot find a private checkbox here...
Flags: needinfo?(ehsan)
(In reply to Ehsan Akhgari (don't ask for review please) from comment #3)
> No, I am talking about show_bug.cgi.  I cannot find a private checkbox
> here...

Ah sorry for not thinking of this before, but you are no longer in the insider group (core-security) which would explain why you do not see the checkbox any more. Please see dveditz if this is in error as there was some cleanup work that happened not too long ago.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
(Reporter)

Comment 5

3 years ago
(In reply to David Lawrence [:dkl] from comment #4)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #3)
> > No, I am talking about show_bug.cgi.  I cannot find a private checkbox
> > here...
> 
> Ah sorry for not thinking of this before, but you are no longer in the
> insider group (core-security) which would explain why you do not see the
> checkbox any more. Please see dveditz if this is in error as there was some
> cleanup work that happened not too long ago.

Well, but I still need to be able to make private comments, even if I'm not part of core-security (which I shouldn't be, I have talked to dveditz about this before), and should see the private comments on the bugs in the components for which I am in the right security group.

Lacking this gets in the way of me getting work done when I am dealing with a security bug (which can be time sensitive.)  Note that this need doesn't arise very often for people, but at the times where it's needed, it can be critical.  One example is a private comment on a bug saying "Stop talking about this in a public bug, comment in bug whatever instead.".  :-)
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Summary: It seems no longer possible to mark a comment as private → It seems no longer possible to mark a comment as private unless if you are a core-security member
> [i] should see the private comments on the bugs in the components for which I am in the right security group

bugzilla only supports a single group for private comments and attachments, and right now that group is core-security.  if you aren't a member of this 'insider group', then that feature is not available to you.

an option here is to create a new group named "can-use-private" or similar, place all of the new security groups into it, then change to using that as the insider group.

it's dveditz's call if this should change.  if we want to take that path we'll need to audit the code to ensure core-security isn't hardcoded in places where it should be using the insidergroup parameter.
Assignee: nobody → dveditz
Component: User Interface: Modal → Administration
Flags: needinfo?(dveditz)
Summary: It seems no longer possible to mark a comment as private unless if you are a core-security member → consider using a broader group as the insider-group (instead of core-security)
It's unfortunate that the same feature controls both private comments and private attachments, or that there isn't another setting for "super-sekret" attachments. One of the things we use this for is for hiding exploit attachments (not mere testcases, but working exploits) when security bugs are fixed and unhidden, because these exploits can be used against people who have not yet upgraded. We know one of the things the recent unauthorized access led to was an unknown person looking at some of these old exploits. Since these aren't the useful testcases for the bugs they weren't necessary for QA or anyone once the bug has been fixed, so access is pure risk. To that extent having the insidergroup stick with the smaller uber-access group (client-security-triage-team) was a benefit.

Everyone else losing the ability to mark comments as private has hurt, and I've received feedback along those lines from several people. (I also think it has been over-used, but definitely used to good purpose in many cases.)

Creating a can-use-private group would certainly be the most flexible. For instance, admins might also want to be able to use it, and people who handle web site bugs. Another choice might be to move the hardcoded linkage of insidergroup from core-security to core-security-release. This would restore the previous state of all the people with any level of client security bug access being able to do this.

The last option (a terrible one with lots of wasted extra work) would be to reverse the inheritance direction again so everyone inherits core-security instead of core-security-release, then move all the FIXED bugs from core-security-release back to core-security. Then change the default new security group from core-security to core-security-triage (a new group, or rename the now-useless core-security-release). That seems like a crazy amount of work compared to the other options.

I think the simplest option to restore the status quo would be to change the insidergroup to 'core-security-release' here:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/bb5c5a5e62d2fd96f9b4a7339cbb0cbb33209745/docker/generate_bmo_data.pl#L416

I didn't find anywhere in the codebase that hardcoded 'core-security' for that functionality.
Flags: needinfo?(dveditz)
(In reply to Daniel Veditz [:dveditz] from comment #7)
> I think the simplest option to restore the status quo would be to change the
> insidergroup to 'core-security-release'

That seems to me like a good start.

Gerv
(Reporter)

Comment 9

3 years ago
(In reply to Gervase Markham [:gerv] from comment #8)
> (In reply to Daniel Veditz [:dveditz] from comment #7)
> > I think the simplest option to restore the status quo would be to change the
> > insidergroup to 'core-security-release'
> 
> That seems to me like a good start.

Sounds good to me as well!
ok - there's actually quite a few places that need to be updated, with the actual change being an admin setting.
Assignee: dveditz → glob
Created attachment 8667308 [details] [diff] [review]
1207721_1.patch

actually fewer changes than i initially thought :)
Attachment #8667308 - Flags: review?(dkl)
Comment on attachment 8667308 [details] [diff] [review]
1207721_1.patch

Review of attachment 8667308 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl
Attachment #8667308 - Flags: review?(dkl) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   d9d7e95..6050008  master -> master
Summary: consider using a broader group as the insider-group (instead of core-security) → change the insider group from core-security to core-security-release
backed this change out for causing test failures

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   6050008..a7a445a  master -> master
(Assignee)

Updated

3 years ago
Assignee: glob → dylan
(Assignee)

Comment 15

3 years ago
One of the places this is failing is the test_strict_isolation.t test:

my $master_gid = $sel->get_attribute('//input[@type="checkbox" and @name="groups" and @value="Master"]@id');
$sel->check_ok($master_gid);
$master_gid =~ s/group_//;

$master_gid should end up being a number, like 21, but is instead the string Master.
What's not clear is how this change triggers this.

I also note this patch didn't change every occurrence of core-security in the qa tests -- only some of them. That might be what is tripping up this test.

I'll work on isolating the cause of the other failure. 

Question for glob: 
Do we need to test strict isolation in BMO?
Flags: needinfo?(glob)
(In reply to Dylan William Hardison [:dylan] from comment #15)
> Do we need to test strict isolation in BMO?

no.
Flags: needinfo?(glob)
(Assignee)

Comment 17

3 years ago
i've isolated the breakage to the generate_bmo_data.pl script.
(Assignee)

Comment 18

3 years ago
Created attachment 8679108 [details] [diff] [review]
1207721_2.patch

Removing test isolation seems to make this not fail. Does this sound sane?
Attachment #8667308 - Attachment is obsolete: true
Attachment #8679108 - Flags: review?(dkl)
Comment on attachment 8679108 [details] [diff] [review]
1207721_2.patch

Review of attachment 8679108 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

note to self: this will require a rebuild of the docker image at https://hub.docker.com/r/dklawren/webtools-bmo-bugzilla
Attachment #8679108 - Flags: review?(dkl) → review+
(Assignee)

Comment 20

3 years ago
Should I wait to commit this until the docker image is updated?
(In reply to Dylan William Hardison [:dylan] from comment #20)
> Should I wait to commit this until the docker image is updated?

No it needs to be committed first and then I will rebuild the docker image once github syncs up. 

Dkl
(Assignee)

Comment 22

3 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   5852295..2c08375  master -> master
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
reopening: the parameter still needs to be changed once this passes tests.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Byron Jones ‹:glob› from comment #23)
> reopening: the parameter still needs to be changed once this passes tests.
 
Tests passed. Param changed.

dkl
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.