274724 - Edit Attachment link available if user does not have Edit permissions

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Colin Ogilvie [:cso]]

In the attachment table at the top of the show_bug.cgi page, the edit link is
hidden if the user in question does not have permission to edit the attachment.

In the comment related to that attachment, the Edit link is available, even if
the user does not have permission to edit the attachment.

It would be best if these Edit links were consistent.

Comment 1

[Frédéric Buclin]

Attachment: Hide "Edit" links when appropriate, v1

I don't really like this patch because I need to request informations we
already had in show_bug.cgi. But I'm not sure I can access $vars from
globals.pl. Can I?

Comment 2

[Frédéric Buclin]

Attachment: complete patch, v2

I found two files which call quoteUrls:
template/en/default/bug/comments.html.tmpl (via process_bug.cgi) and
template/en/default/pages/linked.html.tmpl via
https://bugzilla.mozilla.org/page.cgi?id=linkify.html

page.cgi does not define any special global variable so that we have to
retrieve all the necessary information from within quoteUrls when this
subroutine is called.

Checks made by GetAttachmentLink are now *exactly* the same as the ones done by
attachment.cgi and Bugzilla/Attachment.pm.

Comment 3

[Gervase Markham [:gerv]]

Comment on attachment 169006 [details] [diff] [review]
complete patch, v2

It would be good to avoid adding more SQL queries to quoteUrls(), as it's
called a great deal...

Gerv

Comment 4

[Gervase Markham [:gerv]]

Oops, didn't mean to cancel the review. Apologies.

Gerv

Comment 5

[timeless]

how about we let everyone visit the edit page? people can comment on attachments even if we don't let 
them make changes to them

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> how about we let everyone visit the edit page? people can comment on
attachments even if we don't let 
> them make changes to them

Well, as soon as we know that the user is not allowed to edit attachments, we
should remove all "Edit" links. This prevents the user from having an error
message when submitting changes. And as said by the reporter, we should be
consistent.

A good thing would be to reuse informations we already have for the attachment
table, so that new queries are not required.

My patch fixes the problem. But I will investigate a way to use informations we
already have. Note that such informations are duplicated in both Attachments.pm
and attachment.cgi! :(

Comment 7

[Frédéric Buclin]

Comment on attachment 169006 [details] [diff] [review]
complete patch, v2

Restoring the request flag for review accidentally removed by gerv.

vladd, do you think we should set %::attachlink already in show_bug.cgi? When
looking at the templates, I find several places where the same information is
requested several times.

Maybe should we open a new bug for that? That is, defining *all* required
informations *once* in show_bug.cgi and then sharing this with templates, such
as UserInGroup("...") among others.

If you agree with that, we could check in this patch to fix the actual problem
and open a new one to optimize it.

To answer to gerv's remark, note that GetAttachmentLink() is called *once* for
each attachment mentioned in the page. That should not be a big problem
(relative to the additional number of SQL requests) and we could even remove
all these requests in GetAttachmentLink() if we could share the necessary
informations between show_bug.cgi and templates (and globals.pl).

Comment 8

[Max Kanat-Alexander]

Comment on attachment 169006 [details] [diff] [review]
complete patch, v2

>Index: mozilla/webtools/bugzilla/globals.pl
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/globals.pl,v
>retrieving revision 1.278
>diff -u -r1.278 globals.pl
>--- mozilla/webtools/bugzilla/globals.pl        2004-12-11 13:50:31.000000000 +0100
>+++ mozilla/webtools/bugzilla/globals.pl        2004-12-18 04:28:55.303143736 +0100
>@@ -981,20 +981,28 @@
>         # is saved off rather than overwritten
>         PushGlobalSQLState();
> 
>-        SendSQL("SELECT bug_id, isobsolete, description 
>+        SendSQL("SELECT bug_id, submitter_id, isobsolete, isprivate, description 
>                  FROM attachments WHERE attach_id = $attachid");

  We don't use SendSQL anymore, we use the $dbh handles If we're going to touch
this code anyway, we should probably move to that.


>         if (MoreSQLData()) {
>-            my ($bugid, $isobsolete, $desc) = FetchSQLData();
>-            my $title = "";
>-            my $className = "";
>-            if (Bugzilla->user->can_see_bug($bugid)) {
>-                $title = $desc;
>+            my ($bugid, $submitterid, $isobsolete, $isprivate, $desc) = FetchSQLData();
>+            my $canedit = Bugzilla->user->can_see_bug($bugid);

  This is really $cansee, not $canedit. The name of the var is confusing, which
makes it hard to read the code and understand what's going on.

>+            if ($canedit
>+                && (!$isprivate
>+                    || !Param("insidergroup")
>+                    || UserInGroup(Param("insidergroup"))))

  You'll want a comment explaining that logic in words, it's a bit confusing.

>+            {
>+                SendSQL("SELECT product_id FROM bugs WHERE bug_id = $bugid");
>+                my $productid = FetchOneColumn();
>+                my $whoid = Bugzilla->user->id;

>+                $canedit = !$whoid
>+                           || ((UserInGroup("editbugs") || $submitterid == $whoid)
>+                               && CanEditProductId($productid));

  OK, now this is *really* $canedit, so we shouldn't re-use the same name for a
different thing.

  Really, I think it would be better to write a sub called
UserCanEditAttachment(), which would clear all this up a LOT and make the code
more modular.

>     if (defined $title) {
>         my $linkval = "attachment.cgi?id=$attachid";
>-        return qq{<a href="$linkval" class="$className" title="$title">$link_text</a> [<a href="$linkval&amp;action=edit">edit</a>]};
>+        my $edit = $canedit ? " [<a href=\"$linkval&amp;action=edit\">edit</a>]" : "";
>+        return qq{<a href="$linkval" class="$className" title="$title">$link_text</a>$edit};
>     }

  You know, I think the reason that we got into this whole mess in the first
place is that this should be generated inside the *template*, not inside perl.
But that's a whole different bug that we should file.

  (I cleared the review, because LpSolit asked me to do the review, and I think
that some of these things are legitimate concerns, but I'm not a qualified
reviewer yet to do review-... you can ask for the review again if you want.)

Comment 9

[Max Kanat-Alexander]

(In reply to comment #7)
> Maybe should we open a new bug for that? That is, defining *all* required
> informations *once* in show_bug.cgi and then sharing this with templates, such
> as UserInGroup("...") among others

  I think we should define the information inside the template, since it's HTML.

  But yes, that would be a new bug.

Comment 10

[Frédéric Buclin]

Due to the fix from bug 274906, now all links to attachments give you an error
message if you don't have editbugs privs:

Unauthorised Action

You are not authorised to edit attachment xxxx.

A bit annoying, isn't it?

Comment 11

[Frédéric Buclin]

Attachment: patch, v3

Per my discussion with justdave, this patch lets any user access the form for
editing an attachment. So I removed everything related to this check from the
code. Validations are still done when the user tries to submit his changes:

(20:35:54) justdave: not letting people into the edit page has always bothered
me.  but I always forget it does that.
(20:36:04) justdave: we should do like show_bug and show it anyway, just not
let them update it

Comment 12

[Frédéric Buclin]

Attachment: patch, v4

Remove unused variable $submitter_id from Attachment::query() and replace old
SendSQL stuff by DBI.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 181924 [details] [diff] [review]
patch, v4

>-# Edit an attachment record.

Nit: Edits an attachment record.
(same nit for other comments describing what functions do)


>-  # This data should be given to attachment/list.atml in an
>+  # This data should be given to attachment/list.html in an

Nit: list.html.tmpl


>-        [% IF !attachment.isprivate || canseeprivate %]
>+   [% IF !attachment.isprivate || canseeprivate %]
>     <tr [% "class=\"bz_private\"" IF attachment.isprivate %]>

Nit: indent two spaces, and indent the enclosed code accordingly.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Polish fix approved for checkin during 2.20 freeze.

This is the right thing to do for now.  Long term we'll want a better solution
that perhaps uses a different name or directs people to click on the attachment
name itself to go to the edit page.

Comment 15

[Frédéric Buclin]

Attachment: polished patch, v4.1

Nits fixed. Forwarding r+

Comment 16

[Frédéric Buclin]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.83; previous revision: 1.82
done
Checking in Bugzilla/Attachment.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Attachment.pm,v  <--  Attachment.pm
new revision: 1.21; previous revision: 1.20
done
Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v
 <--  list.html.tmpl
new revision: 1.20; previous revision: 1.19
done