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

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Attachments & Requests
--
enhancement
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

2.19.3
Bugzilla 5.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

12 years ago
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.
Assignee: attach-and-request → LpSolit
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 2.24
(Assignee)

Comment 2

12 years ago
(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.
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Depends on: 350181
(Assignee)

Comment 3

11 years ago
The fix for this bug conflicts with the patch from bug 346086. The security bug has a higher priority.
Depends on: 346086
(Assignee)

Updated

11 years ago
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2

Comment 4

10 years ago
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.
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
(Assignee)

Updated

7 years ago
Assignee: LpSolit → attach-and-request
(Assignee)

Updated

6 years ago
Status: ASSIGNED → NEW
(Assignee)

Comment 5

5 years ago
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.
Target Milestone: Bugzilla 4.4 → ---

Updated

5 years ago
Assignee: attach-and-request → mtyson

Comment 6

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

Updated

5 years ago
Attachment #670238 - Flags: review?(glob) → review?(LpSolit)
(Assignee)

Comment 7

5 years ago
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.
Attachment #670238 - Flags: review?(LpSolit) → review-

Updated

4 years ago
Assignee: mtyson → nobody
(Assignee)

Comment 8

4 years ago
Created attachment 8351663 [details] [diff] [review]
patch, v1
Assignee: nobody → LpSolit
Attachment #670238 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8351663 - Flags: review?(dkl)
(Assignee)

Updated

4 years ago
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Updated

4 years ago
Attachment #8351663 - Flags: review?(dkl) → review?(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?
Other than the above, this seems good.

Gerv
(Assignee)

Comment 11

4 years ago
(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. :)
(Assignee)

Comment 12

4 years ago
Created attachment 8390747 [details] [diff] [review]
patch, v1.1

I still need %flag_list to pass $flag later. But I managed to remove @flag_ids.
Attachment #8351663 - Attachment is obsolete: true
Attachment #8351663 - Flags: review?(gerv)
Attachment #8390747 - Flags: review?(gerv)
Comment on attachment 8390747 [details] [diff] [review]
patch, v1.1

r=gerv.

Gerv
Attachment #8390747 - Flags: review?(gerv) → review+
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 14

4 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   e477b10..1327ff9  master -> master
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Summary: Allow requestees to set flags → Allow requestees to set attachment flags even if they don't have editbugs privs

Updated

4 years ago
Blocks: 989650
You need to log in before you can comment on or make changes to this bug.