Modify

Opened 2 years ago

Last modified 10 months ago

#10738 reopened enhancement

XmlRpcPlugin does not respect ITicketManipulators

Reported by: rje@… Owned by: osimons
Priority: normal Component: XmlRpcPlugin
Severity: normal Keywords:
Cc: olemis 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 2 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 years ago by osimons

  • Resolution set to worksforme
  • Status changed from new to 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 2 years ago by rje@…

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

  • Resolution worksforme deleted
  • Status changed from closed to 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 2 years ago by rje@…

  • Type changed from defect to enhancement

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

Changed 2 years ago by anonymous

comment:5 follow-up: Changed 10 months ago by olemis

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 months ago by olemis

Replying to olemis:

IMO we could (optionally) skip manipulators for TRAC_ADMIN users

or TICKET_ADMIN ... ?

comment:7 Changed 10 months ago by olemis

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 months 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 months ago by olemis

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 .

Add Comment

Modify Ticket

Action
as reopened The owner will remain osimons.
Author


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

 
Note: See TracTickets for help on using tickets.