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

[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).

[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?

[Ryan Ollos <ryano@…>]

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?

[Remy Blank]

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.

[Remy Blank]

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.

[Christian Boos]

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).

[Ryan Ollos <ryano@…>]

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.

[Remy Blank]

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?

[Ryan Ollos <ryano@…>]

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.

[Christian Boos]

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).

[Remy Blank]

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.

[Christian Boos]

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 ;-)

[Remy Blank]

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.

[Remy Blank]

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

[Remy Blank]

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