CommitTicketUpdater makes changes on tickets on behalf of users without checking if they have sufficient permissions

[nikolay@…]

We have a setup, where committers can view only their own tickets.

When I reference a ticket, to which I don't have permissions to even see, CommitTicketUpdater still posts a comment on this ticket with my username and I get the notification e-mail, exposing the ticket summary and description.

I expected that if the commit_ticket_update_check_perms option is set, I won't be able to post comments to this ticket and that I won't be able to see the ticket summary and description.

I think we should check if the user has TICKET_APPEND permission before updating the ticket on their behalf.

[Remy Blank]

That makes sense, yes. Can you please provide a patch that adds this check in the loop of _update_tickets()?

[Nikolay Bachiyski <nikolay@…>]

Hey rblank, here is the patch.

In order to have different checks in different commands, I moved the permission checks there and now _update_tickets() just make sure all the commands return True, instead of checking the permissions itself.

[Remy Blank]

Thanks for the patch!

[Ryan J Ollos <ryan.j.ollos@…>]

Perhaps out of scope for this ticket, but I'm thinking it might be useful to log, perhaps at the debug level, the specific causes of a cmd returning False. For example: "%s doesn't have permission to add comments to Ticket #(%s)" % (changeset.author, ticket.id).

[Remy Blank]

Notify outside of transaction.

[Remy Blank]

Can you please test 10911-update-perms-r11496.patch​? It has two changes from your initial patch:

• The notification email must be sent outside of the transaction. You had moved it into the transaction, but this is wrong when the transaction fails.
• Commands returning None don't prevent the change, they must explicitly return False. This should make the change backward-compatible for people who extended the component and added commands.

[Remy Blank]

Modified patch applied in [11531]. The patch above was updating the ticket if all commands succeeded. This was wrong, and the final patch updates the ticket if at least one command succeeds. The refs command is now gated by TICKET_APPEND.

[anonymous]

Thanks for committing that rblank, your changes make total sense.