Edgewall Software
Modify

Opened 9 years ago

Closed 7 years ago

#11949 closed enhancement (fixed)

Author not identified if new ticket created on behalf of another user

Reported by: Dave Matthews Owned by: Ryan J Ollos
Priority: normal Milestone: 1.2.2
Component: ticket system Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

The ticket author is used in the notification from field when [notification] smtp_from_author = enabled, rather than the ticket reporter. This changes the behavior for users with TICKET_ADMIN, since they are allowed to edit the Reporter field.

API Changes:
Internal Changes:

Description

If someone with TICKET_ADMIN privilege opens a ticket on behalf of another user there is no evidence of who opened the ticket.

This can be very confusing for a user who receives an email notification if they weren't aware that a ticket was being created on their behalf - it can look as though their account has been compromised.

Somehow, the author needs to be identified (if different from the reporter).

(I suppose this could be considered an enhancement? It depends on whether you see the lack of evidence of who actually created the ticket a defect).

Attachments (0)

Change History (13)

in reply to:  description ; comment:1 by Jun Omae, 9 years ago

I don't think that's a defect. TICKET_ADMIN user can set any value to reporter field. You should grant TICKET_ADMIN to only trusted users.

This can be very confusing for a user who receives an email notification if they weren't aware that a ticket was being created on their behalf - it can look as though their account has been compromised.

The reporter field shouldn't be used in the case.

comment:2 by Jun Omae, 9 years ago

Type: defectenhancement

in reply to:  1 comment:3 by Ryan J Ollos, 9 years ago

Resolution: wontfix
Status: newclosed

Replying to jomae:

I don't think that's a defect. TICKET_ADMIN user can set any value to reporter field. You should grant TICKET_ADMIN to only trusted users.

Agreed. The author can be indisputably identified by just preventing anyone from setting the Reporter field, if that's what you desire. If you wish to grant users TRAC_ADMIN but prevent them from setting the reporter field, you have many options:

  • Revoke TRAC_ADMIN for the ticket realm using TracFineGrainedPermissions. TracDev/Proposals/EvenFinerGrainedPermissions would be ideal for the use case of allowing TRAC_ADMIN on the ticket realm, but preventing TRAC_ADMIN for the reporter field.
  • Write a small plugin to reject setting or changing the reporter field to a value that doesn't match the author name.
  • Write a small plugin to modify the template to prevent the reporter field from being set.

comment:4 by Dave Matthews, 9 years ago

That's disappointing. I can't think of any other situation where an admin user can pretend to be someone else. I don't think that should be possible.

To be clear, we want these trusted users to be able to set the reporter - that's not the issue. The problem is that the trusted user may not realise that if they specify the reporter when creating a ticket then they are effectively faking an entry from that user. Why should they realise this? After all, if they use their TICKET_ADMIN privilege to alter the reporter after the ticket is created then that is properly tracked and shown as a change to the ticket. I've had a couple of users get caught out by this and it wasn't the behaviour I would expect which is why I reported it.

I suppose what would help is a TICKET_EDIT_REPORTER permission which would allow you to modify the reporter field after ticket has been created.

Perhaps also the TICKET_ADMIN description could be clarified? It says "… modification of the reporter and description fields". I hadn't realised this implied it allowed the reporter to be faked on creation with no ability to identify the author.

in reply to:  4 comment:5 by Ryan J Ollos, 9 years ago

Replying to Dave Matthews:

That's disappointing. I can't think of any other situation where an admin user can pretend to be someone else. I don't think that should be possible.

What is disappointing about it? I pointed out several ways that you can customize Trac to fit your requirements. You might reply that you would like Trac to function that way out of the box, but we hear that all the time, and everyone wants something different. That is why we make Trac extensible.

It sounds like you are asking for a ticket creator field, in addition to the reporter field. I feel that would be confusing to many users, and doesn't add value for most use cases.

To be clear, we want these trusted users to be able to set the reporter - that's not the issue. The problem is that the trusted user may not realise that if they specify the reporter when creating a ticket then they are effectively faking an entry from that user. Why should they realise this?

Assuming you work at a company, you could document that in your software development process and training materials. If you don't have these, or your users don't read them, then you have a different problem altogether.

Perhaps also the TICKET_ADMIN description could be clarified? It says "… modification of the reporter and description fields". I hadn't realised this implied it allowed the reporter to be faked on creation with no ability to identify the author.

The TracPermissions page is freely editable and the changes are reviewed by the developers. We welcome your contributions to the page.

comment:6 by Dave Matthews, 9 years ago

I think it's the fact that Trac gives such a good experience out of the box which has made it so successful. Personally I feel this ability to pretend to be someone else in this one special case isn't consistent with the normal way that Trac behaves. However, I appreciate that everyone wants something different and this only applies to a user with TICKET_ADMIN.

I've made a change to the TracPermissions page to try to clarify that TICKET_ADMIN provides this ability. I've also changed the description of TICKET_EDIT_COMMENT since this is something that had confused me (I hope I've understood it correctly). Hope that's OK.

[By the way, I agree a separate ticket creator field wouldn't be desirable. I think my preference would be to disallow setting the reporter when creating a ticket and only allow it to be modified afterwards (although that would mean the author receiving notifications for that ticket which they might not want).]

in reply to:  6 comment:7 by Ryan J Ollos, 9 years ago

Replying to Dave Matthews:

I've made a change to the TracPermissions page to try to clarify that TICKET_ADMIN provides this ability. I've also changed the description of TICKET_EDIT_COMMENT since this is something that had confused me (I hope I've understood it correctly). Hope that's OK.

Looks good to me.

[By the way, I agree a separate ticket creator field wouldn't be desirable. I think my preference would be to disallow setting the reporter when creating a ticket and only allow it to be modified afterwards (although that would mean the author receiving notifications for that ticket which they might not want).]

For your use case I think you could just revoke TICKET_ADMIN for the newticket realm using TracFineGrainedPermissions to prevent users from setting the reporter field. Unless the TICKET_ADMIN permission actually grants the ability to perform a workflow action or effects some other behavior on the newticket page, which seems like a rare circumstance, then you should see no side-effects from revoking TICKET_ADMIN for the realm.

More generally, it's worth considering whether only users with TRAC_ADMIN should be able to edit the reporter field from the newticket page, simply because the change is not tracked in the ticket_change table, like you've explained. I'll have to give it more thought and ask for input from the other devs.

Version 0, edited 9 years ago by Ryan J Ollos (next)

in reply to:  1 ; comment:8 by Christian Boos, 9 years ago

Replying to jomae:

This can be very confusing for a user who receives an email notification if they weren't aware that a ticket was being created on their behalf - it can look as though their account has been compromised.

The reporter field shouldn't be used in the case.

I think Jun was suggesting that the author of the ticket creation (and the sender of the notification) should still be clearly identified as being that TICKET_ADMIN user, not the reporter. I'm not sure whether this is already the case or not, but I also think that it should behave that way and this should be enough to address the concerns raised in this ticket.

in reply to:  8 comment:9 by Ryan J Ollos, 9 years ago

Resolution: wontfix
Status: closedreopened

Replying to cboos:

Replying to jomae:

This can be very confusing for a user who receives an email notification if they weren't aware that a ticket was being created on their behalf - it can look as though their account has been compromised.

The reporter field shouldn't be used in the case.

I think Jun was suggesting that the author of the ticket creation (and the sender of the notification) should still be clearly identified as being that TICKET_ADMIN user, not the reporter. I'm not sure whether this is already the case or not, but I also think that it should behave that way and this should be enough to address the concerns raised in this ticket.

Thanks, I had interpreted that comment differently. The author is only stored in the ticket_change table; in the ticket table the author is typically stored in the reporter field. Probably what you are referring to is the behavior when [notication] smtp_from_author is true. I'm unsure as well, perhaps the reporter could test out that behavior for us.

comment:10 by Ryan J Ollos, 9 years ago

Keywords: verify added

in reply to:  6 comment:11 by Ryan J Ollos, 9 years ago

Replying to Dave Matthews:

[By the way, I agree a separate ticket creator field wouldn't be desirable. I think my preference would be to disallow setting the reporter when creating a ticket and only allow it to be modified afterwards (although that would mean the author receiving notifications for that ticket which they might not want).]

What are your thoughts on comment:8? If the ticket creator is identified through the email from field, that should at least make it clear to users who had a ticket created on their behalf, who the actual ticket creator is.

comment:12 by Ryan J Ollos, 7 years ago

Keywords: verify removed
Milestone: 1.2.2
Owner: set to Ryan J Ollos
Release Notes: modified (diff)
Status: reopenedassigned

With the following configuration and the proposed change, the email sender is the user that created the ticket:

[notification]
smtp_from_author = enabled
  • trac/ticket/web_ui.py

    diff --git a/trac/ticket/web_ui.py b/trac/ticket/web_ui.py
    index e1a9231ad..4e3d56794 100644
    a b class TicketModule(Component):  
    13891389
    13901390        # Notify
    13911391        event = TicketChangeEvent('created', ticket, ticket['time'],
    1392                                   ticket['reporter'])
     1392                                  req.authname)
    13931393        try:
    13941394            NotificationSystem(self.env).notify(event)
    13951395        except Exception as e:

Two other potential changes are:

  • Store the authname of the person that created the ticket by adding an author column to the ticket table.
  • Put some text in the template when reporter != creator, such as (Created by …), so that there is clear identification of the author when smtp_from_author = disabled.

Those changes are more extensive, but we can consider them in the future (documented in TracDev/ScratchPad/TicketModule).

comment:13 by Ryan J Ollos, 7 years ago

Resolution: fixed
Status: assignedclosed

Committed to 1.2-stable in r15854, merged to trunk in r15855.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos 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.