All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED DUPLICATE of bug 927778

Status

()

RESOLVED DUPLICATE of bug 927778
5 years ago
5 years ago

People

(Reporter: Dolske, Assigned: dkl)

Tracking

Production

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.)
Assignee: create-and-change → nobody
Component: Creating/Changing Bugs → Extensions: Needinfo
OS: Mac OS X → All
Product: Bugzilla → bugzilla.mozilla.org
QA Contact: default-qa
Hardware: x86 → All
Version: unspecified → Production
See Also: → bug 875613
Duplicate of this bug: 923833
Summary: Add editbugs check to needinfo? → Require editbugs to clear a needinfo flag targeted at a different requestee
(Assignee)

Comment 2

5 years ago
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
Assignee: nobody → dkl
Status: NEW → ASSIGNED
(Reporter)

Comment 3

5 years ago
Using canconfirm wfm, and is a better idea for the reason you mention. :)
(Assignee)

Comment 4

5 years ago
Created attachment 814603 [details] [diff] [review]
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
Attachment #814603 - Flags: review?(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
Attachment #814603 - Flags: review?(glob) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 817256 [details] [diff] [review]
923603_2.patch
Attachment #814603 - Attachment is obsolete: true
Attachment #817256 - Flags: review?(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.
Attachment #817256 - Flags: review?(glob)
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 927778
You need to log in before you can comment on or make changes to this bug.