XmlRpcPlugin does not respect ITicketManipulators

[Rob Emanuele]

I have an ITicketManipulator to restrict ticket creation for a specific component but the JSON RPC interface skips past the ITicketManipulator.

[osimons]

No it shouldn't. Not if you use the 'new-style' update that supports workflow, complete with manipulators and listeners.

As the current ticket.update docs say:

Update a ticket, returning the new ticket in the same form as get(). 'New-style' call requires two additional items in attributes: (1) 'action' for workflow support (including any supporting fields as retrieved by getActions()), (2) '_ts' changetime token for detecting update collisions (as received from get() or update() calls). Calling update without 'action' and '_ts' changetime token is deprecated, and will raise errors in a future version.

You can check the TicketRPC.update() method in source:/xmlrpcplugin/trunk/tracrpc/ticket.py​ to see exactly what steps are taken by update. It calls to Trac to make all necessary validations and checks before saving, including manipulators. There should be tests that shows how this works in source:/xmlrpcplugin/trunk/tracrpc/tests/ticket.py​

I'm not aware of breaking changes in Trac 1.0, but please reopen ticket if there is something I've missed.

[Rob Emanuele]

That looks really interesting. Can I use update to create a new ticket? Can I pass update a None id and how would that happen over JSON RPC? Thanks a lot.

[osimons]

Oh. No, update() cannot create tickets - there is a create() method for that. However, reviewing my logic for create() I see now that manipulators are not in fact part of the call chain. For update() it is handled by Trac, but for create() it is not - nor is it implemented explicitly in RPC code.

We really should call TicketSystem._validate_ticket() for new tickets too (create()). The code can likely be used as-is from the update() implementation.

However, this may break various 'dumb insert' implementations that make no such assumption. Do we need to make the same distinction between 'new-style' and 'old-style' that we do for update()? Like maybe adding a custom parameter/field like {'action': 'new'} or similar that triggers the more advanced logic? Or adding a new keyword argument for the method, like check=True so that checks are enabled by default?

Patch with tests welcome ;-)

[Rob Emanuele]

Something like the attached diff is what I'm working with right now.

[Olemis Lang]

IMO we could (optionally) skip manipulators for TRAC_ADMIN users without introducing a new method doing exactly the same as an existing method. All other calls should trigger manipulators chain to filter new ticket submissions .

[Olemis Lang]

Replying to olemis:

IMO we could (optionally) skip manipulators for TRAC_ADMIN users

or TICKET_ADMIN ... ?

[Olemis Lang]

I have prepared ​a patch (with test cases) implementing this . It suggests raising plugin version to 1.2.0 , but I leave that up to @osimons to decide . For details please review changesets in ​t10738 branch.

Please review.

[osimons]

Thanks! Patch looks good, but I've spent some time thinking about validation. Also looking at the coming change in ticket.update for finally forcing API usage to follow the same rules as posting from web.

Basically: I no longer think validation should be optional.

For ticket.create and ticket.update I propose:

1. Creating with validation. Forced validation.
2. Update with validation and old-style non-checking code removed. Forced validation.
3. Notification True by default for both create+update so that API follows web rules, and notification mails sent if enabled for the project. Notifications can of course still be explicitly disabled for an API call.

A version 1.2 would be natural for these major changes.

[Olemis Lang]

Ok , I'll write another patch to be applied on top of the one proposed in comment:7 implementing the features mentioned in comment:8 , then it'd be possible to either qfold it or commit two separate changesets .

[Ryan J Ollos]

Copied comment by Ryan J Ollos:

I'll just mention for future reference, ​[trac 15861#file0] (​trac:#11723) moved the ticket validation into an ITicketManipulator implementation which XmlRpcPlugin could probably utilize in a future version that is only compatible with Trac 1.3.2+.

[Ryan J Ollos]

Implementing calls to manipulators would also allow xml-rpc requests to be passed through the ​SpamFilter.

[anonymous]

Is there any progress on this? Looks like the cause of #13515, possibly amongst others?