Reject the requestee if he cannot access private attachments

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Attachments & Requests
RESOLVED FIXED
13 years ago
13 years ago

People

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

Tracking

2.20
Bugzilla 2.20
Bug Flags:
approval +
approval2.20 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.24 KB, patch
Jouni Heikniemi
: review+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
Actually, if a user cannot access a bug, a request asking him to set a flag is
rejected. But if a user cannot access an attachment because the attachment is
private (the user is not in the insidergroup), no error is displayed.

For this user, request.cgi displays the attachment number only, but the
description remains hidden.
(Assignee)

Comment 1

13 years ago
The validation only fails on new attachments, but works correctly on existing
attachments.
Severity: minor → normal
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 2

13 years ago
The reason is in FlagType::validate:

                # Throw an error if the target is a private attachment and
                # the requestee isn't in the group of insiders who can see it.
                if ($attach_id
                    && Param("insidergroup")
                    && $cgi->param('isprivate')
                    && !$requestee->in_group(Param("insidergroup")))


If the attachment is new, $attach_id doesn't exist yet and we ignore this test!
(Assignee)

Comment 3

13 years ago
Created attachment 196108 [details] [diff] [review]
patch, v1

As the attachment has not been inserted into the DB yet, its ID is unknown. We
use '-1' to indicate that the attachment is new. I see no better solution
without invasive changes in attachment.cgi; which we should avoid, especially
on the branch. A solution (for the tip only) would be to first insert the
attachment into the DB, and then validate flags. This would also fix the user
matching problem where the confirmation page makes the attachment data to be
lost. I would say that this is another bug (the important point here being to
validate requestees in all cases, even with a fake attachment ID for now).

The patch applies cleanly to 2.20 and tip.
Attachment #196108 - Flags: review?(myk)

Comment 4

13 years ago
Comment on attachment 196108 [details] [diff] [review]
patch, v1

Looks good from this end.  Sending the -1 (new attachment) to the validate
function does force a check.
Attachment #196108 - Flags: review+
(Assignee)

Comment 5

13 years ago
Created attachment 199914 [details] [diff] [review]
patch, v2

explain the meaning of $attach_id = -1
Attachment #196108 - Attachment is obsolete: true
Attachment #199914 - Flags: review?(jouni)
(Assignee)

Updated

13 years ago
Attachment #196108 - Flags: review?(myk)

Comment 6

13 years ago
Comment on attachment 199914 [details] [diff] [review]
patch, v2

>+    # FlagType::validate() and Flag::validate() should not detect
>+    # any reference to existing flags when creating a new attachment.
>+    # Setting the third param to -1 will force this function to check this point.

Nit: Wrap the third line to avoid exceeding 80 chars.

r=jouni, by inspection and discussion only. The patch is functionally
equivalent to v1 though.
Attachment #199914 - Flags: review?(jouni) → review+
(Assignee)

Updated

13 years ago
Flags: approval?
Flags: approval2.20?
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
(Assignee)

Comment 7

13 years ago
tip:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.99; previous revision: 1.98
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.55; previous revision: 1.54
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.23; previous revision: 1.22
done


2.20:

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.89.2.1; previous revision: 1.89
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.45.2.2; previous revision: 1.45.2.1
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.19.2.2; previous revision: 1.19.2.1
done
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.