Last Comment Bug 812433 - establish reports and processes for continued auditing of bugzilla security group membership
: establish reports and processes for continued auditing of bugzilla security g...
Status: RESOLVED FIXED
:
Product: bugzilla.mozilla.org
Classification: Other
Component: General (show other bugs)
: Production
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Byron Jones ‹:glob›
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-15 23:31 PST by Byron Jones ‹:glob›
Modified: 2013-01-28 06:32 PST (History)
13 users (show)
See Also:
Due Date:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (12.26 KB, patch)
2012-11-21 06:38 PST, Byron Jones ‹:glob›
dkl: review+
reed: review-
Details | Diff | Splinter Review
patch v2 (12.37 KB, patch)
2012-11-26 06:29 PST, Byron Jones ‹:glob›
no flags Details | Diff | Splinter Review
patch v3 (13.71 KB, patch)
2012-12-13 11:42 PST, Byron Jones ‹:glob›
dkl: review+
Details | Diff | Splinter Review

Description Byron Jones ‹:glob› 2012-11-15 23:31:10 PST
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 Byron Jones ‹:glob› 2012-11-15 23:31:43 PST
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 security@mozilla.org and they update where appropriate.
Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2012-11-16 00:18:53 PST
This kind of thing should be made available upstream.  Giving admins good tools for managing groups just sounds like a good idea.
Comment 3 Byron Jones ‹:glob› 2012-11-21 06:38:39 PST
Created attachment 683996 [details] [diff] [review]
patch v1

add a group membership report (both html and json output).
Comment 4 David Lawrence [:dkl] 2012-11-25 22:32:16 PST
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] (use needinfo?) 2012-11-25 23:02:17 PST
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 Byron Jones ‹:glob› 2012-11-25 23:17:53 PST
(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 Byron Jones ‹:glob› 2012-11-26 06:29:46 PST
Created attachment 685138 [details] [diff] [review]
patch v2
Comment 8 Byron Jones ‹:glob› 2012-12-13 11:42:11 PST
Created attachment 691930 [details] [diff] [review]
patch v3

adds a "last seen" column (see bug 821341)
Comment 9 David Lawrence [:dkl] 2013-01-22 21:06:16 PST
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 Byron Jones ‹:glob› 2013-01-22 22:01:05 PST
(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 Byron Jones ‹:glob› 2013-01-23 00:27:23 PST
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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://bjones%40mozilla.com@bzr.mozilla.org/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] 2013-01-23 07:50:54 PST
(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 Byron Jones ‹:glob› 2013-01-23 22:53:45 PST
https://bugzilla.mozilla.org/page.cgi?id=group_members.html is now live.
Comment 14 Gervase Markham [:gerv] 2013-01-25 03:03:33 PST
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] 2013-01-25 03:34:57 PST
(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 Byron Jones ‹:glob› 2013-01-28 06:32:30 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.