Validate ticket "select" fields

[Matthew Good]

The spammers seem to have grown fond of submitting tickets with a blank "type". It may be good to start validating that the values submitted for "select" fields actually contain a valid value. This component provides that basic functionality:

class FieldOptionsValidator(Component):
    implements(ITicketManipulator)

    def prepare_ticket(self, req, ticket, fields, action):
        pass

    def validate_ticket(self, req, ticket):
        for field in ticket.fields:
            if 'options' not in field:
                continue
            value = ticket[field['name']]
            if value not in field['options']:
                yield field['name'], '"%s" is not a valid value' % value

If it's too close to the 0.10 release to add this maybe we can stick it in the SpamFilter since that seems to be it's main purpose ATM.

[Christian Boos]

Well, spammers will quickly learn to get around that, but OTOH I don't see in which case it would make sense to accept invalid field values, so my take would be to put it in trunk, for milestone:0.10 as the lack of checking looks more like a defect to me.

I tested the FieldOptionsValidator and had to make a few changes in order to make it work, please test further.

Index: api.py
===================================================================
--- api.py	(revision 3692)
+++ api.py	(working copy)
@@ -130,6 +130,7 @@
                      'options': options}
             if name in ('status', 'resolution'):
                 field['type'] = 'radio'
+                field['optional'] = True
             elif name in ('milestone', 'version'):
                 field['optional'] = True
             fields.append(field)
Index: web_ui.py
===================================================================
--- web_ui.py	(revision 3692)
+++ web_ui.py	(working copy)
@@ -637,3 +637,24 @@
                 current['fields'][field] = {'old': old, 'new': new}
         if current:
             yield current
+
+
+class FieldOptionsValidator(Component):
+    implements(ITicketManipulator)
+
+    def prepare_ticket(self, req, ticket, fields, action):
+        pass
+
+    def validate_ticket(self, req, ticket):
+        for field in ticket.fields:
+            if 'options' not in field:
+                continue
+            name = field['name']
+            if name in ticket.values:
+                value = ticket[name]
+                if value:
+                    if value not in field['options']:
+                        yield name, '"%s" is not a valid value' % value
+                elif not 'optional' in field:
+                    yield name, 'must be set'
+

[Christopher Lenz]

I definitely agree with the idea, but don't see why that validation logic should be in a separate component. If that's just to share between TicketModule/NewticketModule, we could simply add the code to TicketModuleBase._validate_ticket(), no?

[Matthew Good]

Replying to cmlenz:

I definitely agree with the idea, but don't see why that validation logic should be in a separate component. If that's just to share between TicketModule/NewticketModule, we could simply add the code to TicketModuleBase._validate_ticket(), no?

Yes, it would work there, but I was figuring that it should use the validation API in case some other component would want to use the validation in the future.

cboos: I don't understand why you assigned this to yourself. I guess I should've set myself as the owner when I created it, but I don't think you need to take it over.

[Christian Boos]

Replying to mgood:

cboos: I don't understand why you assigned this to yourself. I guess I should've set myself as the owner when I created it, but I don't think you need to take it over.

As the owner was jonas, it wasn't obvious that you intended to complete the implementation yourself, so I volunteered to finish it. No harm done, I hope ;)

[Christian Boos]

So… any progress on this?

[Christian Boos]

See #1744 recent changes and there are lots of similar spam that could have been prevented by the above changes. I'll propose an updated patch shortly, as I think we need this to get done for 0.10 and tested a.s.a.p on t.e.o before the release.

[Christian Boos]

Moved the validation of ticket fields in TicketModuleBase._validate_ticket

[Christian Boos]

Please double check that everything is OK with attachment:always_validate_fields-r3739.patch​, so that we can integrate it and upgrade.

One issue I've come across with this change however was that if Trac is configured with

[ticket] 
restrict_owner = true

then an attempt to modify a ticket assigned to someone being no longer a valid owner will fail. Not an issue with t.e.o, but must be addressed before 0.10.

[Matthew Good]

skips validation on unmodified fields

[Matthew Good]

Replying to cboos:

One issue I've come across with this change however was that if Trac is configured with

[ticket] 
restrict_owner = true

then an attempt to modify a ticket assigned to someone being no longer a valid owner will fail. Not an issue with t.e.o, but must be addressed before 0.10.

The new patch I attached should fix that since it skips validation on fields that were unmodified. I had to move the validation check after the processing of the "Action" items, since otherwise it didn't check changes to the owner.

[Matthew Good]

cboos: I think this is ok, so unless someone else finds a bug, go ahead and commit it.

[Christian Boos]

Fine, so I've committed it as r3740, with only a minor change​.

jonas/cmlenz, please consider upgrading t.e.o, so that:

• the change gets more exposure before we release 0.10 (which should be real soon now, shouldn't it?)
• hopefully reduce further the spam that still gets through

Thanks!