Opened 4 years ago

XmlRpcPlugin does not respect ITicketManipulators

Reported by: Owned by: Rob Emanuele Odd Simon Simonsen normal XmlRpcPlugin normal Olemis Lang 1.0

Description

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

comment:1 Changed 4 years ago by Odd Simon Simonsen

Resolution: → worksforme new → closed

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.

comment:2 Changed 4 years ago by 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.

comment:3 Changed 4 years ago by Odd Simon Simonsen

Resolution: worksforme closed → reopened

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

comment:4 Changed 4 years ago by Rob Emanuele

Type: defect → enhancement

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

comment:5 follow-up:  6 Changed 3 years ago by 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 .

comment:6 in reply to:  5 Changed 3 years ago by Olemis Lang

IMO we could (optionally) skip manipulators for TRAC_ADMIN users

or TICKET_ADMIN ... ?

comment:7 Changed 3 years ago by 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.

comment:8 Changed 3 years ago by Odd Simon Simonsen

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.

comment:9 Changed 3 years ago by 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 .

Modify Ticket

Change Properties