If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ability to get attachments by attachments ids

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
WebService
--
enhancement
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: timello, Assigned: timello)

Tracking

unspecified
Bugzilla 3.6
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

8 years ago
Implement a method to allow users to get attachments by the ids. A list of attachment ids could be passed as we do for bug ids.

Comment 1

8 years ago
assigned to ...?
(Assignee)

Updated

8 years ago
Assignee: webservice → timello
(Assignee)

Comment 2

8 years ago
Created attachment 387279 [details] [diff] [review]
Adds the ability to pass attachment_ids to the Bug.attachments method.
Attachment #387279 - Flags: review?(mkanat)

Comment 3

8 years ago
Comment on attachment 387279 [details] [diff] [review]
Adds the ability to pass attachment_ids to the Bug.attachments method.

>Index: Bugzilla/WebService/Bug.pm
>===================================================================

>+    if (!(defined $params->{bug_ids}
>+          || defined $params->{attachment_ids})) {

  Nit: Open-brace goes on the next line.

>+    foreach my $attach (@{Bugzilla::Attachment->new_from_list($attach_ids)}) {
>+        $attachments{$attach->id} =
>+            $self->_attachment_to_hash($attach, $params);
>+    }

  I'm pretty sure you're bypassing security restrictions there. (Both the ability to see the bug and the ability to see the attachment.)
Attachment #387279 - Flags: review?(mkanat) → review-
(Assignee)

Comment 4

8 years ago
Created attachment 387457 [details] [diff] [review]
Fixes.
Attachment #387279 - Attachment is obsolete: true
Attachment #387457 - Flags: review?(mkanat)

Updated

8 years ago
Attachment #387457 - Flags: review?(mkanat) → review-

Comment 5

8 years ago
Comment on attachment 387457 [details] [diff] [review]
Fixes.

By default you should be throwing an error if a user requests a private attachment. You can add a "permissive" argument like Bug.get has, for the times when people don't want that error thrown. (You would then also have to handle failures of Bug->check, so maybe we should do that in a separate bug.)
(Assignee)

Comment 6

8 years ago
Created attachment 388632 [details] [diff] [review]
Throwing an error if a user requests a private attachment.
Attachment #387457 - Attachment is obsolete: true
Attachment #388632 - Flags: review?(mkanat)
(Assignee)

Comment 7

8 years ago
Hi Max, could you review this patch?

Thank you!

Updated

8 years ago
Attachment #388632 - Flags: review?(mkanat) → review-

Comment 8

8 years ago
Comment on attachment 388632 [details] [diff] [review]
Throwing an error if a user requests a private attachment.

  Yeah, code-wise this looks fine. However, I changed "bug_ids" to "ids" a few months back, so you'll have to re-write the patch to account for that.

  As a side note, we also need to fix the API here so that the return value for bugs looks like:

  { bugs => { attachments => \%attachments } }

  That will keep it consistent with the other Bug sub-functions (that is, functions that get Bug data that aren't Bug.get).

>Index: template/en/default/global/user-error.html.tmpl
>+        the attachment [% attach_id FILTER html %]

  Say "attachment #[% attach_id FILTER html %]" instead.

  You also need to add auth_failure to the possible error messages that attachments() can throw.
(Assignee)

Comment 9

8 years ago
Created attachment 399099 [details] [diff] [review]
Fix
Attachment #388632 - Attachment is obsolete: true
Attachment #399099 - Flags: review?(mkanat)
(Assignee)

Comment 10

8 years ago
Hi Max,

What additional auth_failure should we add other than the private attachment?

Thank you.

Comment 11

8 years ago
Comment on attachment 399099 [details] [diff] [review]
Fix

The auth_failure needs to be in the *docs*, in the Errors section.
Attachment #399099 - Flags: review?(mkanat) → review-
(Assignee)

Comment 12

8 years ago
Created attachment 404886 [details] [diff] [review]
Adds the auth_failure in the Errors section.

Hey Max,

Could you take a look?

Thank you!
Attachment #399099 - Attachment is obsolete: true
Attachment #404886 - Flags: review?(mkanat)

Comment 13

8 years ago
Comment on attachment 404886 [details] [diff] [review]
Adds the auth_failure in the Errors section.

  This looks great! :-)

>Index: Bugzilla/WebService/Bug.pm
>+A hash containing two elements: C<bugs> and C<attachments>.
>+The C<bugs> hash has the numeric bug id as key and the C<attachments> hash
>+has the numeric attachment id as key. Both containing the following items:

  On checkin, I'll make it clearer that one return value comes from one input parameter, and the other return value comes from the other input value.
Attachment #404886 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval+
Target Milestone: --- → Bugzilla 3.6

Comment 14

8 years ago
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.50; previous revision: 1.49
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.293; previous revision: 1.292
done
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.