923603 - Require editbugs to clear a needinfo flag targeted at a different requestee

Status: RESOLVED DUPLICATE of bug 927778

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

Description

[Justin Dolske [:Dolske]]

I just disabled an account that was spamming BMO. It was also clearing a number of needinfo flags set by other people for other people.

Seems like that should be behind an editbugs check? (Obviously there would need to be a special case to allow someone w/o editbugs to clear a needinfo asked of them.)

Comment 2

[David Lawrence [:dkl]]

This rev. updated Bugzilla::Bug::check_can_change_field to allow flag changes always since normally flags permissions are handled by the flags themselves (i.e. who can request, grant, deny). The needinfo flag doesn't itself have any restrictions set so essentially anyone can clear the flag manually.

http://bzr.mozilla.org/bmo/4.2/revision/8552

We used to require 'canconfirm' rights to even use it in the beginning and then we removed that to allow new users to also utilize needinfo.

Proposal:

* Require 'canconfirm' to clear a needinfo request that is not directed at you. I think 'canconfirm' may be more fitting as canconfirm users should be able to do basic triage and answer questions in bug reports if the user is not able to find the answer. 

Thought before I run off to do a patch?

dkl

Comment 3

[Justin Dolske [:Dolske]]

Using canconfirm wfm, and is a better idea for the reason you mention. :)

Comment 4

[David Lawrence [:dkl]]

Attachment: 923603_1.patch

Also removed check for 'added_comments' as we don't use that anymore and instead look for the "Clear needinfo..." checkbox. 

dkl

Comment 5

[:glob ✱]

Comment on attachment 814603 [details] [diff] [review]
923603_1.patch

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

::: extensions/Needinfo/Extension.pm
@@ +153,5 @@
>          {
> +            # Clear if user has chosen to clear the needinfo for
> +            # the following reasons:
> +            # 1. They are the requestee and the clear needinfo request was checked.
> +            if ($flag->requestee->login eq $user->login) {

compare the id's instead of the login names

@@ +158,5 @@
> +                $clear_needinfo = 1;
> +            }
> +
> +            # 2. They are not the requestee and they chose to override the needinfo request.
> +            if ($flag->requestee->id ne $user->id || $user->in_group('canconfirm')) {

id's are numeric, use == instead of ne

@@ +179,5 @@
>      }
>  }
>  
> +# Blocking clearing of a needinfo flag if the requestee has been
> +# specified, the user is not the requestee, and does not have 

nit: trailing whitespace

@@ +188,5 @@
> +    my $user = Bugzilla->user;
> +
> +    return if !$object->isa('Bugzilla::Bug');
> +
> +    my ($removed) = diff_arrays($old_flags, $new_flags);

Undefined subroutine &Bugzilla::Extension::Needinfo::diff_arrays called

@@ +203,5 @@
> +        my ($name, $status) = $flag =~ /^(.+)(.)$/;
> +        next if ($name ne 'needinfo' && $status ne '?');
> +
> +        if ($user->login ne $requestee && !$user->in_group('canconfirm')) {
> +            ThrowUserError('needinfo_illegal_change');

user error tag 'needinfo_illegal_change' is used at line(s) (207) but not defined for language(s): any

Comment 6

[David Lawrence [:dkl]]

Attachment: 923603_2.patch

Comment 7

[:glob ✱]

Comment on attachment 817256 [details] [diff] [review]
923603_2.patch

following a chat with dkl, i'm going to roll relevant changes from this patch into bug 927778.