:glob ✱ Reporter
	Description • 9 years ago

in order to reduce the attack surface, it should be possible to configure a group so users that have not visited bugzilla in more than the specified duration are automatically removed from the group.  this only applies to groups that the user is a direct member of.

- add optional "idle_member_removal" integer field (days)
- default 0 (feature disabled)
- create a nightly triggered script which:
  - find direct members of groups with idle_member_removal set with a last_seen_date older than the removal period
  - remove the user from the group
  - write an [audit] log entry
  - email the group's owner and the user

	:glob ✱ Reporter
	Comment 2 • 9 years ago

Comment on attachment 8667560 [details] [diff] [review]
1196620_1.patch

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

::: scripts/remove_idle_group_members.pl
@@ +38,5 @@
> +       FROM profiles JOIN user_group_map ON profiles.userid = user_group_map.user_id
> +            JOIN groups ON user_group_map.group_id = groups.id
> +      WHERE user_group_map.grant_type = ?
> +            AND groups.idle_member_removal > 0
> +            AND TO_DAYS(LOCALTIMESTAMP(0)) - TO_DAYS(profiles.last_seen_date) > groups.idle_member_removal

this needs to accommodate old users where profiles.last_seen_date is null

@@ +50,5 @@
> +my %group_cache = ();
> +my %user_cache  = ();
> +foreach my $data (@$expired) {
> +    my $user  = $user_cache{$data->{user_id}}   ||= Bugzilla::User->new($data->{user_id});
> +    my $group = $group_cache{$data->{group_id}} ||= Bugzilla::Group->new($data->{group_id});

don't create your own caches - use the object cache.

@@ +59,5 @@
> +# 1. Remove users from the group
> +# 2. $user->update will add audit log and profile_activity entries
> +# 3. Send email to group owner showing users removed
> +foreach my $group_id (keys %remove_data) {
> +    my $group = $group_cache{$group_id};

ie.
my $group = Bugzilla::Group->new({ id => $group_id, cache => 1 });
(same for users)

@@ +62,5 @@
> +foreach my $group_id (keys %remove_data) {
> +    my $group = $group_cache{$group_id};
> +
> +    # nobody@mozilla.org cannot recieve email
> +    next if $group->owner->name eq 'nobody@mozilla.org';

this should be $group->owner->login, and this check should only prevent sending of the email (ie. users should still be removed from groups if the owner is nobody).

::: template/en/default/admin/groups/edit.html.tmpl
@@ +115,5 @@
> +      <th>
> +        Idle Member Removal:
> +      </th>
> +      <td>
> +        <input type="text" size="20" maxlength="20" id="idle_member_removal"

maxlength 20?  that's a lot of days :)

::: template/en/default/admin/groups/email/idle-member-removal-header.txt.tmpl
@@ +3,5 @@
> +  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +  #
> +  # This Source Code Form is "Incompatible With Secondary Licenses", as
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]

this header is missing "X-Bugzilla-Type: admin"

@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +From: [% Param('mailfrom') %]
> +To: [% group.owner.email %]
> +Subject: [[% terms.Bugzilla %]] Idle Group Member Removals

terms.Bugzilla is not defined, resulting in a subject of "[] Idle Group Member Removals".
i'd also like to see the group name in the subject.

	David Lawrence [:dkl] Assignee
	Comment 4 • 9 years ago

Comment on attachment 8668435 [details] [diff] [review]
1196620_2.patch

Ugh. left in some debugging crud. New patch coming.

	:glob ✱ Reporter
	Comment 6 • 9 years ago

Comment on attachment 8668448 [details] [diff] [review]
1196620_3.patch

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

r=glob

commit just the schema changes, and attach a patch with the following revisions.

::: template/en/default/admin/groups/create.html.tmpl
@@ +62,5 @@
>  
> +  <tr>
> +    <th>Idle Member Removal:</th>
> +    <td colspan="3">
> +      <input type="text" size="20" maxlength="20" id="idle_member_removal" name="idle_member_removal">

maxlength needs to be fixed here too

::: template/en/default/admin/groups/email/idle-member-removal-header.txt.tmpl
@@ +6,5 @@
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +From: [% Param('mailfrom') %]
> +To: [% group.owner.email %]
> +Subject: [[% terms.BugzillaTitle %]] Idle group members removed from [% group.name %]

terms.BugzillaTitle is also undefined.

you need to [% PROCESS global/variables.none.tmpl %]

::: template/en/default/admin/groups/email/idle-member-removal.html.tmpl
@@ +5,5 @@
> +  # This Source Code Form is "Incompatible With Secondary Licenses", as
> +  # defined by the Mozilla Public License, v. 2.0.
> +  #%]
> +
> +[% PROCESS "global/field-descs.none.tmpl" %]

this template (and the plain text version) doesn't use field-descs

@@ +11,5 @@
> +<!doctype html>
> +<html>
> +
> +<head>
> +  <title>[[% terms.Bugzilla %]] Ilde Group Member Removal Notification</title>

terms isn't defined here either (or in the plain text version), due to a missing [% PROCESS global/variables.none.tmpl %]

	David Lawrence [:dkl] Assignee
	Comment 8 • 9 years ago

The rest

	David Lawrence [:dkl] Assignee
	Comment 9 • 9 years ago

Schema only:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   02a5cb5..fcc39a8  master -> master

	:glob ✱ Reporter
	Comment 10 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   f8f537a..3822973  master -> master

	:glob ✱ Reporter
	Comment 11 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=bmo-master&revision=38229739b7cee263208d39b2c55b942e4d21c632

fixed by defaulting idle_member_removal to 0 when creating groups:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   3822973..8ab60b2  master -> master