836713 - Make group membership reports publicly-available

Status: RESOLVED FIXED

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

Description

[Gervase Markham [:gerv]]

In bug 812433, we implemented "group membership reports", which tell you who is in which Bugzilla group. Those reports are currently only available to users who are in the groups "editusers" or "infrasec".

The Mozilla organization permits non-public bugs to exist as a sad tradeoff of security against our desire for openness - and it's a position which I support and do not argue with. But I don't think there's any reason to keep a secret of _who_ can see classes of secure bugs. 

Making the lists public is good thing because it's a useful accountability measure to help us keep secrecy to the necessary minimum, it shows what 'forms' of secrecy there are (the group names) and it helps to make sure that all and only the right people have certain accesses. 

The principle of publicising a list of people with secure bug access has been accepted for a long time when it comes to the security group.
https://www.mozilla.org/projects/security/secgrouplist.html

The main argument against doing this that I see is that it increases the security risk because an attacker might know which accounts to target in order to try and break into one which has a particular permission. However, information about group memberships could probably be worked out anyway by someone paying attention, from public lists like the list of security group members, or from observing behaviour or status (e.g. the Mozilla lawyers are obviously in the "legal" group).

I initially propose making all group memberships public, but if people want to give reasons for keeping certain ones private, go right ahead.

Gerv

Comment 1

[Reed Loden [:reed]]

I think this should be opt-in only and require explicit approval from each group owner. I also don't see the benefit to improving "openness" in doing this aside from maybe a few small number of groups, especially considering so many groups are MoCo-only and have no outside benefit to the community (finance, hr, legal, partner groups, etc.). Also, you're actually making it easier for an attacker to target somebody by giving them the exact bugmail addresses, as well as "last seen time" (which can be useful for attacking somebody's account without that person noticing).

Comment 2

[Gervase Markham [:gerv]]

I think most groups would benefit from being transparent, although I agree the case is less compelling for the few which are related to the administering of MoCo. BTW, "hr" has one member - I suspect it's not used. "finance" has 8, but only 4 seem to use Bugzilla.

I think transparency into who can see bugs in partner groups is a useful thing. (Note: there are only 2 pairs of such groups anyway.) In one sense, the membership of the "qualcomm" group is not much of a secret - it's all people @qualcomm.com and their other domain. But in another sense, it's a legitimate point of query from the community as to why we have a group in our Bugzilla which seems to be inaccessible to all Mozillians except mrz (for some reason) and the Bugzilla admins. What's that group for? Are we letting them use our Bugzilla for their proprietary development?

We could suppress "last seen time"; I agree that doesn't add anything from a transparency perspective.

Gerv

Comment 3

[:glob ✱]

doing this will provide a near complete list of email addresses of mozilla staff and contractors, likewise for partners who collaborate with us to address issues (eg. qualcomm, adobe, etc).  i'm not sure where we'd stand on a legal/privacy perspective.

broadly i agree with reed and think this should be per-group opt-in, not opt-out.

Comment 4

[Gervase Markham [:gerv]]

Would some of the fears expressed be allayed if the lists were to give Bugzilla real names (if present) rather than email addresses?

Much of this information is gatherable, with a little effort, from publicly-available (or available after registering for an account) Bugzilla data. I can certainly search for all bugs assigned to/CC/etc. "*@mozilla.*" and get a pretty good list of Mozilla email addresses.

It's not really possible to have a "secret Bugzilla account" if you interact with public bugs (which pretty much everyone does).

Gerv

Comment 5

[Gervase Markham [:gerv]]

Tom: can you give us a privacy perspective on this?

The proposal is to make public a list of the names of the groups in Bugzilla, and a list of members of each group. 

I seem to remember you telling me that our view is (given the warnings on signup) that people had no expectation of privacy of their account data in Bugzilla. Is that correct?

Gerv

Comment 6

[Michael Coates [:mcoates] (acct no longer active)]

(Need info flag makes the bugzilla world go round)

Comment 7

[Tom Lowenthal]

I do not think that there is any privacy interest at stake here: I do not think that there is any expectation of privacy in bugzilla account info like the name you use and your email address. I completely agree with Gerv that openness suggests that group membership should be universally visible.

Comment 8

[:glob ✱]

taking.

in summary so far, if you are a member of the editusers or infrasec group, the report remains (mostly) unchanged.

if you are not so privileged, the following changes occur to html and json reports:
  - the user's real name is displayed instead of their email address
  - if a user hasn't provided a real name, their email address will be shown
  - remove the 'last seen' column

for performance reasons, i will add editbugs-team to the "always excluded" group list (editbugs is already excluded).

also, due to the size of some of these groups and the impact this places on bmo to generate, i plan on generating the reports at most once per day for unprivileged requests.

Comment 9

[Reed Loden [:reed]]

Again, I feel this should be group opt-in.

Comment 10

[:glob ✱]

(In reply to Reed Loden [:reed] from comment #9)
> Again, I feel this should be group opt-in.

i thought both of your concerns were addressed:

> you're actually making it easier for an attacker to target somebody by giving them
> the exact bugmail addresses

we'll only show the bugmail address if a real name is missing.  we could, of course, show only the username part of the email address in this case (eg. "[e-mail]" would be displayed as "byron").

> as well as "last seen time"

this will not be available.

Comment 11

[Reed Loden [:reed]]

(In reply to Byron Jones ‹:glob› from comment #10)
> (In reply to Reed Loden [:reed] from comment #9)
> > Again, I feel this should be group opt-in.
> 
> i thought both of your concerns were addressed:

They were, and I don't have any remaining issues with it being used for security groups, but there are a ton of bugzilla groups (just look at https://bugzilla.mozilla.org/editgroups.cgi). Not sure it makes sense at all to make the membership of each of those groups publicly visible.

Comment 12

[Tom Lowenthal]

Openness should be the default position. I think that there should be a compelling reason *not* to have all groups visible. Is there such a reason?

Comment 13

[Daniel Veditz [:dveditz]]

I would have thought groups like partners-confidential and payments-confidential could reveal the existence of certain partnerships before marketing was ready to do so. Bugs in the related components are not public. Knowing that these relationships couldn't be kept private could encourage people to migrate issue tracking off of bugzilla.

> The principle of publicising a list of people with secure bug
> access has been accepted for a long time when it comes to the
> security group.
> https://www.mozilla.org/projects/security/secgrouplist.html

I'm trying to kill that page actually. I don't think it has compelling benign uses and it gives a nice list of targets for APT. Sure, you can easily find the people who are most active in security bugs without such a list, but the active people may also be the most aware and paranoid.

Comment 14

[Gervase Markham [:gerv]]

I agree that the principle of partner confidentiality means that groups which include members from undeclared partners will need to have secret membership. Sad, but true.

I think that a consideration of utility misses the point in some ways. I think we should be "open by default" as an organization, and when we cannot be fully open, we should be as open as possible. So if there must be e.g. a private mailing list, its membership and general topic should be public. So at least someone can say "ah, that's where they are discussing sensitive issue X. I'm interested in that" and investigate ways to be involved. 

Similarly, bug information is public on principle. When we have to compromise that, we at least tell people who the group is who are looking at these secret bugs on behalf of everyone.

Gerv

Comment 15

[Gervase Markham [:gerv]]

glob: comment 8 suggested you might have written some code here. Did you?

It seems like the criteria for a group whose existence and membership should not be revealed is either:

a) the name of the group reveals the name of a partner who has not announced they are working with us; or
b) the group relates to a project and we are working on that project with an unannounced partner.

We should really try and avoid situation a), but there you go.

Looking through the list, I suggest we do not reveal the memberships of the following groups:

* tako-confidential
* tako-team
* woodduck-confidential
* woodduck-team

Both of these pairs relate to projects that I think may still be unannounced.

The following groups, mentioned above as potentially needing hiding, do not appear to have any secret members (in some cases, they have no members):
* partner-confidential
* payments-confidential
so I think we are OK to list those.

So, apart from the four groups listed above, I think we are fine to reveal group memberships.

Gerv

Comment 16

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #15)
> glob: comment 8 suggested you might have written some code here. Did you?

unfortunately not.

> So, apart from the four groups listed above, I think we are fine to reveal
> group memberships.

as we add more groups for governing confidentially of bugs with our b2g partners, i'm concerned that an opt-out would result in exposing a membership of a group which we shouldn't.  there are definitely more partner groups on the horizon.

this is a change in how bugzilla is being used -- a year and a half ago we didn't have as much of a need for partner confidentially as we do today.


in light of that, i would prefer for this to be opt-in.
my proposed list of groups is:

amo-drivers
b2g-drivers
b2g-feature-drivers
b2g-ux-drivers
bugzilla-approvers
bugzilla-reviewers
calendar-drivers
can_restrict_comments
community-it-team
e10s-drivers
fennec-drivers
firefox-backlog-drivers
kilimanjaro-drivers
l10n-drivers
loop-drivers
marketplace-drivers
mozilla-drivers
mozilla-employee-confidential
mozilla-foundation-confidential
mozilla-next-drivers
mozilla-reps
mozilla-stable-branch-drivers
qa-approvers
thunderbird-drivers
weave-drivers

the list is focused on groups which govern permissions of fields within bugzilla, where we can be reasonably assured of some level of accuracy in the lists in addition to the direct utility value of showing their members.  any group which ends in "-drivers" should automatically be included in this list.

i've explicitly not included any of the security groups over concerns from reed and dveditz that these lists "provide a nice list of targets".  when trying to balance openness against security, i have to tip the scales in favour of security.

Comment 17

[Gervase Markham [:gerv]]

Attachment: 836713.diff

How about this? (If there's a better place to put the data list, let me know.)

Gerv

Comment 18

[:glob ✱]

Comment on attachment 8526801 [details] [diff] [review]
836713.diff

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

this approach generally looks good, thanks for picking this up gerv.

- rather than manually iterating all the -drivers groups, it would be simpler to grep for them (it's highly likely we'll forget to update this list the next time we add a -drivers group).
- you need to update the reports/menu-end hook to make the two reports visible to users with editbugs.
- the "last seen" data needs to be removed for users who are not editusers/infrasec (make sure you update both html and json templates).
- all the usernames link to 'editusers', which doesn't make sense for users without that ability.  non-editusers views should link to that user's profile (continue to link to editusers for viewers who are blessed).
- if one of the groups in @public_membership_groups isn't configured on the system, the following error is throw instead of the group being ignored:
> Can't call method "members_direct" on an undefined value at ../Bugzilla/Extension/BMO/Reports/Groups.pm line 229

::: extensions/BMO/lib/Reports/Groups.pm
@@ +40,5 @@
> +mozilla-reps
> +mozilla-stable-branch-drivers
> +qa-approvers
> +thunderbird-drivers
> +weave-drivers

nit: indent the group names

@@ +56,5 @@
>  
> +    my @grouplist = @public_membership_groups;
> +    if ($user->in_group('editusers') || $user->in_group('infrasec')) {
> +        @grouplist = map { lc($_->name) } Bugzilla::Group->get_all;
> +    }

nit: no need to assign to @grouplist twice; use a ternary if

@@ +59,5 @@
> +        @grouplist = map { lc($_->name) } Bugzilla::Group->get_all;
> +    }
> +
> +    my $groups = join(',', map { $dbh->quote($_) } @grouplist);
> +    

nit: remove trailing whitespace

@@ +71,5 @@
>                      AND user_group_map.grant_type = 0
>                 LEFT JOIN profiles
>                      ON user_group_map.user_id = profiles.userid
>           WHERE groups.isbuggroup = 1
> +         AND groups.name IN ($groups) 

nit: remove trailing whitespace, and make the A of "AND" line up with the "g" of "groups." in the previous line

Comment 19

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Review comments addressed.

There was one additional privacy nit - when someone was a member of a group via another group, the report revealed the name of the other group, thereby revealing partial group membership for non-whitelisted groups. I've changed it to just say "another group" in those circumstances, so people know it's transitive membership, but not how.

Gerv

Comment 20

[:glob ✱]

Comment on attachment 8531212 [details] [diff] [review]
Patch v.2

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

most of this looks ok, however it doesn't pass tests.

t/008filter.t ........ 1/626
#   Failed test '(en/default) extensions/BMO/template/en/default/pages/group_members.html.tmpl has unfiltered directives:
#   83: member.login FILTER url
# --ERROR'
#   at t/008filter.t line 130.

that should be "FILTER uri"

ps. you really should configure your editor to warn on trailing whitespace.  you've added a bit with this patch.

::: extensions/BMO/lib/Reports/Groups.pm
@@ +272,5 @@
> +        mozilla-reps
> +        qa-approvers
> +    );
> +    my $groups_re = join("|", @hardcoded_groups);
> +    push(@public_groups, grep { $_ =~ /^($groups_re)$/ } @all_groups);

using a regex to intersect two arrays is a bit odd..

how about..
    my @hardcoded_groups = qw( ... );
    my @all_groups = map { lc($_->name) } Bugzilla::Group->get_all;
    my %hardcoded = map { $_ => 1 } @hardcoded_groups;
    return grep { /-drivers$/ || exists $hardcoded{$_} } @all_groups;

::: extensions/BMO/template/en/default/pages/group_members.html.tmpl
@@ +10,5 @@
>    title = "Group Members Report"
>    style_urls = [ "extensions/BMO/web/styles/reports.css" ]
>  %]
>  
> +[% SET privileged = (user.in_group('editusers') || user.in_group('infrasec')) ? 1 : 0 %]

nit: no need for "? 1 : 0" as it's already boolean.

Comment 21

[Gervase Markham [:gerv]]

(In reply to Byron Jones ‹:glob› from comment #20)
> ps. you really should configure your editor to warn on trailing whitespace. 
> you've added a bit with this patch.

Unfortunately, in my editor (geany) I only have the following options:

* Display line endings (but they use a really ugly and intrusive character on every line)
* Show whitespace (opposite problem - the tiny grey dot is almost invisible)
* Remove trailing whitespace on save (awesome, except when a file already has trailing whitespace, when 
  it creates noise in diffs)

Suggestions?

Gerv

Comment 22

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Review comments addressed.

Gerv

Comment 23

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #21)
> Suggestions?

write a script which you always use to generate your diffs, and add check for added whitespace to that script: /^\+\+\+/ && /\s+$/

in my dev environment manager i have https://github.com/globau/bugzilla-dev-manager/blob/app/Bz/Repo.pm#L363

Comment 24

[:glob ✱]

Comment on attachment 8539317 [details] [diff] [review]
Patch v.3

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

r=glob, with the trailing whitespace removed on commit.

::: extensions/BMO/lib/Reports/Groups.pm
@@ +28,5 @@
> +    my @grouplist =
> +                  ($user->in_group('editusers') || $user->in_group('infrasec'))
> +                  ? map { lc($_->name) } Bugzilla::Group->get_all
> +                  : _get_public_membership_groups();
> + 

there's trailing whitespace here

@@ +176,5 @@
> +    my @grouplist =
> +                  ($user->in_group('editusers') || $user->in_group('infrasec'))
> +                  ? map { lc($_->name) } Bugzilla::Group->get_all
> +                  : _get_public_membership_groups();
> + 

trailing whitespace here too

::: extensions/BMO/template/en/default/pages/group_members.html.tmpl
@@ +64,5 @@
> +            [% ELSE %]
> +              via 
> +              [% IF privileged %]
> +                [% type.name FILTER html %]
> +              [% ELSE %]another group[% END %]                

and more trailing whitespace

Comment 25

[Gervase Markham [:gerv]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   f7afaa0..17a4afe  master -> master

Gerv

Comment 26

[Mark Côté [:mcote]]

(In reply to Gervase Markham [:gerv] from comment #21)
> Suggestions?

On my system, "git diff" shows trailing whitespace.