294021 - Allow requestees to set attachment flags even if they don't have editbugs privs

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement)

Description

[Frédéric Buclin]

When a user is set as the requestee for a bug or attachment flag, he should be
able to set/clear this flag in all cases, assuming that:

- the requestee can see the bug/attachment;
- the requestee belongs to the grant group for this flag type.

The actual behavior is that you can ask anybody who can access the
bug/attachment, but this user won't be able to set the flag if:

- he is not in the grant group for this flag type;
- he cannot edit the attachment in the case of an attachment flag.


So I request two things here:

- Flag(Type)::validate() should do better checks and make sure the requestee is
allowed to set the flag (meaning he belongs to the grant group). I will fix this
point as part of bug 293159.

- In a similar request to bug 140999, the requestee should still be able to set
the attachment flag even if he is not allowed to change other fields. Maybe it
would make sense to fix this point together with bug 140999, meaning that the
requestee can set the flag and add a comment to the attachment.

Comment 1

[Frédéric Buclin]

Definitely tired of this bug. If I ask someone for review, there is a good reason for that, and not having editbugs privs shouldn't prevent him from doing his review. After all, he is free to edit bug flags, so why so many restrictions for attachment flags? Let's fix it asap.

Comment 2

[Frédéric Buclin]

(In reply to comment #0)
> - Flag(Type)::validate() should do better checks and make sure the requestee is
> allowed to set the flag (meaning he belongs to the grant group). I will fix this
> point as part of bug 293159.

Note to self. Bug 293159 doesn't include this check. I will have to implement it here.

Comment 3

[Frédéric Buclin]

The fix for this bug conflicts with the patch from bug 346086. The security bug has a higher priority.

Comment 4

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 5

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 6

[Matt Tyson]

Attachment: Patch to allow requestees to set flags.

This patch (against current trunk) will allow someone to set a flag if they are the requstee for that flag.

Please let me know what you think of the current approach, or if it might be better handled by taking a different tack.

Comment 7

[Frédéric Buclin]

Comment on attachment 670238 [details] [diff] [review]
Patch to allow requestees to set flags.

>=== modified file 'Bugzilla/Attachment.pm'

> =item C<validate_can_edit($attachment, $product_id)>
> 
> Description: validates if the user is allowed to view and edit the attachment.
>-             Only the submitter or someone with editbugs privs can edit it.
>+             Only the submitter, requestee or someone with editbugs privs can edit it.

The requestee is not allowed to edit properties of attachments. He can only set flags, nothing more. So this method must not be altered.



>=== modified file 'Bugzilla/User.pm'

> sub can_set_flag {

>+    return 0 unless $self->in_group('editbugs');

Why this change? Being able to set flags has nothing to do with editbugs privs.



>=== modified file 'template/en/default/flag/list.html.tmpl'

>-          [% IF user.can_set_flag(type) || (flag && flag.status == "+") %]
>+          [% IF user.can_set_flag(type) || (flag && flag.status == "+") || (user.id == flag.requestee_id) %]

If it's a new flag, 'flag' is undefined and flag.requestee_id doesn't exist. You must include your check into (flag && ...).


>-          [% IF user.can_set_flag(type) || (flag && flag.status == "-") %]
>+          [% IF user.can_set_flag(type) || (flag && flag.status == "-") || (user.id == flag.requestee_id)%]

Same here.


I only gave a very quick look at your patch, so I didn't test it, nor checked all changes you did. But the problems above need to be fixed first.

Comment 8

[Frédéric Buclin]

Attachment: patch, v1

Comment 9

[Gervase Markham [:gerv]]

Comment on attachment 8351663 [details] [diff] [review]
patch, v1

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

Comment 0 doesn't seem to correctly summarise what this bug is about. Is it correct to say that it is now about "If someone has requested a flag from a user, that user should be able to fulfil that request"?

::: attachment.cgi
@@ +694,5 @@
> +        my %flag_list = map { $_->id => $_ } @$flag_objs;
> +
> +        my @editable_flags;
> +        foreach my $flag (@$flags) {
> +            my $flag_obj = $flag_list{$flag->{id}};

This all seems a bit roundabout. I suppose you are doing new_from_list as a performance optimization, but can't you eliminate %flag_list entirely, and just do:

foreach my $flag_obj (@$flag_objs) {

? Or is there some reason that the flags must be processed in the order they are in @$flags?

Comment 10

[Gervase Markham [:gerv]]

Other than the above, this seems good.

Gerv

Comment 11

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #9)
> Comment 0 doesn't seem to correctly summarise what this bug is about. Is it
> correct to say that it is now about "If someone has requested a flag from a
> user, that user should be able to fulfil that request"?

This is incomplete. Comment 0 is accurate and says more than that. You must still ask someone who is not excluded by the grant group. This part is already fixed, as said in comment 0. The point of this bug is that if you ask someone who hasn't editbugs privs, he won't be able to touch flags in attachments he didn't create. That's what I want to fix here.


> This all seems a bit roundabout. I suppose you are doing new_from_list as a
> performance optimization, but can't you eliminate %flag_list entirely, and
> just do:
> 
> foreach my $flag_obj (@$flag_objs) {

Hum, good idea. Will do it. :)

Comment 12

[Frédéric Buclin]

Attachment: patch, v1.1

I still need %flag_list to pass $flag later. But I managed to remove @flag_ids.

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 8390747 [details] [diff] [review]
patch, v1.1

r=gerv.

Gerv

Comment 14

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   e477b10..1327ff9  master -> master