Modify

Opened 5 years ago

Last modified 2 months ago

#10738 reopened enhancement

XmlRpcPlugin does not respect ITicketManipulators

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

Description

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 5 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by Odd Simon Simonsen

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/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 5 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 5 years ago by Odd Simon Simonsen

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

Type: defectenhancement

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

Changed 5 years ago by anonymous

Attachment: xmlrpcplugin-mps1.diff added

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

Replying to olemis:

IMO we could (optionally) skip manipulators for TRAC_ADMIN users

or TICKET_ADMIN ... ?

comment:7 Changed 4 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 4 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 4 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 6 months 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 2 months ago by Ryan J Ollos

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain Odd Simon Simonsen.

Add Comment


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

 
Note: See TracTickets for help on using tickets.