Opened 11 years ago

Last modified 4 years ago

#10738 reopened enhancement

XmlRpcPlugin does not respect ITicketManipulators

Reported by: Rob Emanuele Owned by: osimons
Priority: normal Component: XmlRpcPlugin
Severity: normal Keywords:
Cc: Olemis Lang Trac Release: 1.0


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

Attachments (1)

xmlrpcplugin-mps1.diff (3.9 KB) - added by anonymous 11 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 years ago by osimons

Resolution: worksforme
Status: newclosed

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/ 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/

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 11 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 11 years ago by osimons

Resolution: worksforme
Status: closedreopened

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 11 years ago by Rob Emanuele

Type: defectenhancement

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

Changed 11 years ago by anonymous

Attachment: xmlrpcplugin-mps1.diff added

comment:5 Changed 10 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 10 years ago by Olemis Lang

Replying to olemis:

IMO we could (optionally) skip manipulators for TRAC_ADMIN users


comment:7 Changed 10 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.

Please review.

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

comment:9 Changed 10 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 .

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

comment:11 Changed 6 years ago by Ryan J Ollos

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

comment:12 Changed 4 years ago by anonymous

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

Modify Ticket

Change Properties
Set your email in Preferences
as reopened The owner will remain osimons.

Add Comment

E-mail address and name can be saved in the Preferences.

Note: See TracTickets for help on using tickets.