Edgewall Software
Modify

Opened 15 years ago

Closed 15 years ago

Last modified 8 years ago

#8592 closed defect (fixed)

'Replace attachment of same name' visible even though user has not enough privileges

Reported by: Felix Schwarz <felix.schwarz@…> Owned by: Remy Blank
Priority: normal Milestone: 0.11.6
Component: general Version: 0.11-stable
Severity: normal Keywords:
Cc: ryano@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

In the "Add attachment" page, the checkbox "Replace existing attachment of the same name" is visible even if the user has no privileges to delete attachements (needed for replace).

Attachments (0)

Change History (14)

comment:1 by Remy Blank, 15 years ago

Milestone: 0.12
Owner: set to Remy Blank

I was also bitten by that. I saw you post quite a few patches lately, Felix, do you happen to have one for this issue as well?

in reply to:  description ; comment:2 by Ryan Ollos <ryano@…>, 15 years ago

Replying to Felix Schwarz <felix.schwarz@…>:

In the "Add attachment" page, the checkbox "Replace existing attachment of the same name" is visible even if the user has no privileges to delete attachements (needed for replace).

Seems like you would need to handle the required permissions differently for ticket attachments and for wiki attachments. For wiki attachments you'd need the WIKI_DELETE permission, and for ticket attachments you'd need the TICKET_ADMIN permission. Is that correct?

in reply to:  2 comment:3 by Remy Blank, 15 years ago

Replying to Ryan Ollos <ryano@…>:

Seems like you would need to handle the required permissions differently for ticket attachments and for wiki attachments. For wiki attachments you'd need the WIKI_DELETE permission, and for ticket attachments you'd need the TICKET_ADMIN permission. Is that correct?

No, the permission to check is ATTACHMENT_DELETE, which is a "composite" permission that checks WIKI_DELETE for wiki attachments and TICKET_ADMIN for tickets, as you noticed.

But looking at the attachments code, I see that replacing an attachment only requires ATTACHMENT_DELETE if either the old or new attachment author was anonymous, or if both authors are different. So you can replace your own attachments without having ATTACHMENT_DELETE permission.

The trouble is, you can't know before submitting if you are allowed to replace the attachment or not, so the check box will have to stay. It could be hidden if the user is anonymous and anonymous doesn't have ATTACHMENT_DELETE, which currently doesn't seem to be done.

comment:4 by Remy Blank, 15 years ago

Milestone: 0.120.11.6
Resolution: fixed
Status: newclosed

Mmh, there's something wrong here. The message in [1874] states that an authenticated user should be able to replace her own attachments. However, the way the test is written, any anonymous user can replace an attachment where the author was "anonymous". Not quite what was expected.

I have fixed the check in [8505], and also hidden the checkbox in the only case where we are sure a user can't replace an attachment (an anonymous user).

OT: I see a few tests like this in the code for checking if a user is authenticated:

req.authname and req.authname != 'anonymous'

But when can req.authname actually be empty? From what I understand, it is either the username, or 'anonymous' if the user is not authenticated.

in reply to:  4 comment:5 by Christian Boos, 15 years ago

But when can req.authname actually be empty?

The code is not crystal clear (for ... else construct…), but yes, it looks like we always would return at least 'anonymous', so I think we can clean-up the tests you pointed out (in trunk).

comment:6 by Ryan Ollos <ryano@…>, 15 years ago

Cc: ryano@… added

I had an interest in this request because more than once a user has come to me with the question of why they can't upload and over-write an attachment, even though they are checking the Replace existing attachment of the same name box.

From a usability standpoint, I think it would be better to remove this checkbox from the Add Attachments page. Rather, if the user is trying to upload an attachment of the same name as an existing attachment, then check whether they have permissions to over-write the attachment. If they do have the requisite permissions, ask the user if they want to overwrite the existing attachment.

I think this is preferable because you aren't giving the user the option to do something that you aren't actually going to allow them to do. However, this is probably a pretty significant change and thus a lot of coding.

comment:7 by Remy Blank, 15 years ago

Resolution: fixed
Status: closedreopened

I understand what you mean, and I agree with the usability argument. Implementing it, however, is far from trivial. The attachment is uploaded in the first step, and at the confirmation screen, you would have to either:

  • re-upload the attachment
  • store it at the first step in a temporary location and retrieve it in the second step

The first option would be reasonably simple to implement, but requires uploading the attachment twice. The other option requires disproportionate effort for the added value.

I have a compromise solution that could at least help the user understand what's happening. Instead of displaying the standard "This operation requires ATTACHMENT_DELETE permission…" error message, display a more meaningful message when replacing an attachment fails, something along the lines of:

You don't have permission to replace the attachment my_file.txt. You can only replace your own attachments. Replacing other's attachments requires ATTACHMENT_DELETE permission.

Good enough?

in reply to:  7 comment:8 by Ryan Ollos <ryano@…>, 15 years ago

Replying to rblank:

I have a compromise solution that could at least help the user understand what's happening. Instead of displaying the standard "This operation requires ATTACHMENT_DELETE permission…" error message, display a more meaningful message when replacing an attachment fails, something along the lines of:

You don't have permission to replace the attachment my_file.txt. You can only replace your own attachments. Replacing other's attachments requires ATTACHMENT_DELETE permission.

Good enough?

Yeah, I think so. The message seems clear enough that most people should understand what is going on.

in reply to:  7 ; comment:9 by Christian Boos, 15 years ago

Replying to rblank:

You don't have permission to replace the attachment my_file.txt. You can only replace your own attachments. Replacing other's attachments requires ATTACHMENT_DELETE permission.

But wouldn't that be also shown to anonymous users? They won't necessarily understand the above and think but it is my attachment ?


Of course, solving that would require fixing #948. By the way, what about storing anonymous:<session_id> as the the author for the attachment? That could also work for #454. We would have to truncate the :<session_id> part in other places. A better solution (in light of #1890) would be to have an additional session column where needed (attachment table and ticket_change at least).

in reply to:  9 ; comment:10 by Remy Blank, 15 years ago

Replying to cboos:

But wouldn't that be also shown to anonymous users? They won't necessarily understand the above and think but it is my attachment ?

No, since [8505] the check box is not shown at all for anonymous users.

By the way, what about storing anonymous:<session_id> as the the author for the attachment? That could also work for #454. We would have to truncate the :<session_id> part in other places. A better solution (in light of #1890) would be to have an additional session column where needed (attachment table and ticket_change at least).

I'm not sure. If I use the same Trac instance as anonymous from two computers (or even two browsers), chances are my session IDs are different, except if I go to the trouble of copying the ID from one to the other in the preferences (which is quite advanced functionality).

I guess #1890 will bring a complete solution to identifying anonymous users uniquely. For now, I'll just make the error message as informative as possible for this particular ticket.

in reply to:  10 ; comment:11 by Christian Boos, 15 years ago

Replying to rblank:

since [8505] the check box is not shown at all for anonymous users.

OK.

By the way, what about storing anonymous:<session_id> as the the author for the attachment? That could also work for #454. We would have to truncate the :<session_id> part in other places. A better solution (in light of #1890) would be to have an additional session column where needed (attachment table and ticket_change at least).

I'm not sure. If I use the same Trac instance as anonymous from two computers (or even two browsers), chances are my session IDs are different, except if I go to the trouble of copying the ID from one to the other in the preferences (which is quite advanced functionality).

Yes, but this solution would be adequate for the most common use case, fix one's error after the fact, which usually happens quickly, in the same session. I think that alone would already be very helpful.

If really there's a need to go beyond that, then as you said, copying the session ID would be possible for advanced users.

I guess #1890 will bring a complete solution to identifying anonymous users uniquely.

No idea how, but I'm interested in hearing suggestions.

For now, I'll just make the error message as informative as possible for this particular ticket.

Sure, I just wanted to get a quick feedback on the idea above, so I hijacked the ticket ;-)

in reply to:  11 comment:12 by Remy Blank, 15 years ago

Replying to cboos:

Yes, but this solution would be adequate for the most common use case, fix one's error after the fact, which usually happens quickly, in the same session. I think that alone would already be very helpful.

Ah, sorry, I didn't get the use case you were writing about. Yes, for that use case, it would be very useful (and indeed also for #454).

If really there's a need to go beyond that, then as you said, copying the session ID would be possible for advanced users.

It's still not very practical, and if you happen to loose your session ID at some point, there's no way to get it back. But that doesn't really matter. Your use case still covers about 90% of the cases.

I guess #1890 will bring a complete solution to identifying anonymous users uniquely.

No idea how, but I'm interested in hearing suggestions.

Me neither, and that's why I didn't want to pull a "complete" solution into this ticket ;-)

Sure, I just wanted to get a quick feedback on the idea above, so I hijacked the ticket ;-)

Now that I understand the use case, I find it's an excellent idea. I'll still commit the better error message from comment:7 first, then I'll try your idea.

comment:13 by Remy Blank, 15 years ago

Ah, one more thing about the anonymous:<session_id> thing: 0.11-stable or trunk? I vote for trunk.

comment:14 by Remy Blank, 15 years ago

Resolution: fixed
Status: reopenedclosed

The error message has been improved in [8516], and #8605 has been opened for allowing non-authenticated users to replace their own attachments.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.