812433 - establish reports and processes for continued auditing of bugzilla security group membership

Status: RESOLVED FIXED

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

Description

[:glob ✱]

following on from discussions in bug 812349, we need to establish reports and processes for continued auditing of bugzilla security group membership.

i propose:

1. ensure each security group has at least one administrator
   https://bugzilla.mozilla.org/page.cgi?id=group_admins.html

2. write a report which allows these admins to easily show members of the group
   - currently only visible via editusers, which is clumsy and doesn't
     distinguish between direct and inherited membership

3. send a report of each group's members to the admins on a monthly basis

Comment 1

[:glob ✱]

the current process for this is when someone's ldap account is disabled, it's included on a nightly report.  if their bmo account is a @mozilla.com address, the account is disabled.  if their bmo account isn't, they are removed from the mozilla-corporation-confidential group.  if i notice they are members of security groups when editing the user, i email [e-mail] and they update where appropriate.

Comment 2

[Dave Miller [:justdave]]

This kind of thing should be made available upstream.  Giving admins good tools for managing groups just sounds like a good idea.

Comment 3

[:glob ✱]

Attachment: patch v1

add a group membership report (both html and json output).

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 683996 [details] [diff] [review]
patch v1

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

Looks good and works well. Just a suggestion for group_members.json.tmpl but not a big deal.

r=dkl

::: extensions/BMO/template/en/default/pages/group_members.json.tmpl
@@ +13,5 @@
> +    [% FOREACH member = type.members %]
> +      "[% member.email FILTER json %]"[% "," UNLESS loop.last %]
> +    [% END %]
> +    [% LAST %]
> +  [% END %]

Maybe instead of adding complexity by performing too loops over the same data, instead do earlier:

[% direct = [] %]
[% indirect = [] %]

[% FOREACH type = types %]
  [% direct.push(type) IF type.name == 'direct' %]
  [% indirect.push(type) IF type.name != 'direct' %]
[% END %]

{
  "group": "[% group FILTER json %]",
  "direct": [
  [% FOREACH type = direct %]
    [% FOREACH member = type.members %]
      "[% member.email FILTER json %]"[% "," UNLESS loop.last %]
    [% END %]
    [% LAST %]
  [% END %]
  ][% "," IF indirect.size > 1 %]
  [% IF indirect.size > 1 %]
    "indirect": {
    [% FOREACH type = indirect %]
      "[% type.name FILTER json %]": [
        [% FOREACH member = type.members %]
          "[% member.email FILTER json %]"[% "," UNLESS loop.last %]
        [% END %]
        ]
      [% END %]
    }
  [% END %]
}

Comment 5

[Reed Loden [:reed]]

Comment on attachment 683996 [details] [diff] [review]
patch v1

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

::: extensions/BMO/template/en/default/pages/group_members.html.tmpl
@@ +61,5 @@
> +            [% type.members.size FILTER html %]
> +          </td>
> +          <td valign="top" width="100%">
> +            [% FOREACH member = type.members %]
> +              <a href="editusers.cgi?action=edit&userid=[% member.id FILTER html %]"

&amp;

FILTER url_quote

@@ +64,5 @@
> +            [% FOREACH member = type.members %]
> +              <a href="editusers.cgi?action=edit&userid=[% member.id FILTER html %]"
> +                 target="_blank">
> +                <span [% 'class="bz_inactive"' UNLESS member.is_enabled %]>
> +                  [% member.name FILTER html %] &lt;[% member.email FILTER html %]&gt;

FILTER email FILTER html

I wonder if any of this stuff can reuse template/en/default/global/user.html.tmpl code ...

::: extensions/BMO/template/en/default/pages/group_members.json.tmpl
@@ +10,5 @@
> +  "direct": [
> +  [% FOREACH type = types %]
> +    [% NEXT UNLESS type.name == "direct" %]
> +    [% FOREACH member = type.members %]
> +      "[% member.email FILTER json %]"[% "," UNLESS loop.last %]

perhaps FILTER email FILTER json ?

@@ +21,5 @@
> +      [% FOREACH type = types %]
> +        [% NEXT IF type.name == "direct" %]
> +        "[% type.name FILTER json %]": [
> +          [% FOREACH member = type.members %]
> +            "[% member.email FILTER json %]"[% "," UNLESS loop.last %]

Same, FILTER email FILTER json ??

Comment 6

[:glob ✱]

(In reply to Reed Loden [:reed] from comment #5)
> I wonder if any of this stuff can reuse
> template/en/default/global/user.html.tmpl code ...

i had that initially, however it displays the identity only.  i found it much more useful to display both the real name and email address when auditing group membership.


will fix the other points.  i've already realised that the json filter is from the bzapi extension, so i'll switch to using the js filter.

Comment 7

[:glob ✱]

Attachment: patch v2

Comment 8

[:glob ✱]

Attachment: patch v3

adds a "last seen" column (see bug 821341)

Comment 9

[David Lawrence [:dkl]]

Comment on attachment 691930 [details] [diff] [review]
patch v3

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

Looks good and works well. r=dkl

::: extensions/BMO/lib/Reports.pm
@@ +565,2 @@
>          || ThrowUserError('auth_failure', { group  => 'editusers', 
>                                              action => 'run', 

nit: remove whitespace on commit

::: extensions/BMO/template/en/default/pages/group_members.json.tmpl
@@ +14,5 @@
> +  [% SET i = 0 %]
> +  [% FOREACH type = types %]
> +    [% FOREACH member = type.members %]
> +      [% SET i = i + 1 %]
> +      { "login": "[% member.login FILTER email FILTER js %]",

FWIW, in 4.2, we have the json filter which would be better suited here.

Comment 10

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #9)
> FWIW, in 4.2, we have the json filter which would be better suited here.

the json filter is injected by the bzapi extension, and shouldn't be used outside of it.

Comment 11

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
modified extensions/BMO/Extension.pm
modified extensions/BMO/lib/Reports.pm
modified extensions/BMO/template/en/default/hook/reports/menu-end.html.tmpl
modified extensions/BMO/template/en/default/pages/email_queue.html.tmpl
modified extensions/BMO/template/en/default/pages/group_admins.html.tmpl
added extensions/BMO/template/en/default/pages/group_members.html.tmpl
added extensions/BMO/template/en/default/pages/group_members.json.tmpl
modified extensions/BMO/template/en/default/pages/group_membership.html.tmpl
modified extensions/BMO/template/en/default/pages/user_activity.html.tmpl
modified extensions/BMO/web/styles/reports.css
Committed revision 8448.

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified extensions/BMO/Extension.pm
modified extensions/BMO/lib/Reports.pm
modified extensions/BMO/template/en/default/hook/reports/menu-end.html.tmpl
modified extensions/BMO/template/en/default/pages/email_queue.html.tmpl
modified extensions/BMO/template/en/default/pages/group_admins.html.tmpl
added extensions/BMO/template/en/default/pages/group_members.html.tmpl
added extensions/BMO/template/en/default/pages/group_members.json.tmpl
modified extensions/BMO/template/en/default/pages/group_membership.html.tmpl
modified extensions/BMO/template/en/default/pages/user_activity.html.tmpl
modified extensions/BMO/web/styles/reports.css
Committed revision 8512.

Comment 12

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #10)
> (In reply to David Lawrence [:dkl] from comment #9)
> > FWIW, in 4.2, we have the json filter which would be better suited here.
> 
> the json filter is injected by the bzapi extension, and shouldn't be used
> outside of it.

I concede that we made a customization and the filter is not in the upstream code, but the filter does exist outside of the bzapi extension if we would like to use it.

http://bzr.mozilla.org/bmo/4.2/annotate/head:/Bugzilla/Template.pm#L648

dkl

Comment 13

[:glob ✱]

https://bugzilla.mozilla.org/page.cgi?id=group_members.html is now live.

Comment 14

[Gervase Markham [:gerv]]

Does this bug need to continue to be confidential?

I'd like to reference it in another bug report; I'd like to discuss whether we should make this report available to everyone.

Mozilla does closed bugs as a sad tradeoff of security against openness. But I don't think there's any reason to keep a secret of _who_ can see secure bugs. That information could probably be worked out anyway by someone paying attention. And making the lists public is a useful accountability measure to help us keep secrecy to the necessary minimum.

Gerv

Comment 15

[Gervase Markham [:gerv]]

(In reply to Gervase Markham [:gerv] from comment #14)
> I'd like to reference it in another bug report; I'd like to discuss whether
> we should make this report available to everyone.

That's not particularly clear :-) Please read:

"I'd like to reference this bug in another bug, which would discuss whether
we should make the group membership report available to everyone."
 
Gerv

Comment 16

[:glob ✱]

(In reply to Gervase Markham [:gerv] from comment #14)
> Does this bug need to continue to be confidential?

nope; i wasn't exactly sure where the discussion would take it.  public this bug now is.