Form Wizard keeps state on subsequent requests and provides no way to cut short inside parse_params
|Reported by:||Michael P. Jung||Owned by:||nobody|
|Severity:||Normal||Keywords:||FormWizard, state, parse_params, shortcut, HttpResponse|
|Cc:||mpjung@…, rajesh.dhawan@…, magma.chambers@…||Triage Stage:||Design decision needed|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
Since this issue is subtle I'll describe my concrete usecase first and than describe the issue and possible solutions to this.
I have a form that contains a ChoiceField which depends on the currently logged in user and some request parameters. Since I need a special behaviour I subclassed the FormWizard and did overwrite the methods parse_params, done, process_step, get_form and get_template. Inside parse_params I set some attributes of self. For some reason I assumed that returning anything but None would cause the Wizard to abort. However the return value is ignored and the wizard code continues by calling get_form and process_step. Since get_form is the only way of putting the stuff which was set earlier in parse_params I have a hook that calls some helper methods on the form to set up the options of the ChoiceField depending on the stuff set by parse_params earlier. Since the only way of injecting custom data there is self one has no chance of telling wether the value comes from the current request or something earlier which might just have failed. Thus no AttributeError is raised as the attribute is set. The data just does not come from the current request.
Even worse: In my case - since I was assuming that returning from parse_params with an HttpResponse would stop the wizard and return the HttpResponse immediately - caused invalid requests to be rendered with the state from a previous requests. A horrid leak of sensible data, as the wizard is part of a checkout process and the ChoiceField is used to pick an address. I'm just glad our QA noticed that glitch before putting this stuff live. It would not be hard to exploit and I don't like the idea of leaking sensible data.
- By overwriting the __call__ method one can inject any attribute into self and return any HttpResponse object if something is wrong. I consider this rather ugly as the FormWizard explicitly states parse_params for getting stuff from the request object, but provides no way of breaking out of the wizard and returning a HttpResponse. I would like to see a way to return a HttpResponse object from within parse_params for shortcutting requests to a Wizard. (see attached patch)
- The Wizard could be made immutable, but provide a custom 'state' attribute for transfering data between parse_params, process_step, etc. Anyhow this still would not enable the user of the FormWizard to shortcut a request with a HttpRequest. I would consider it a good idea not to be able to reuse a state from a previous request by accident. Another idea would be to discourage the users to store such state information as attribute, but rather enhance the form wizard to implement __getitem___, __setitem__, etc. so that it can be used to transfer state between the hook methods. Thus instead of doing "self.my_state = args" one would do "self['my_state'] = args".
- In my case I solved this issue by wrapping the wizard inside a custom request method which creates a new instance of the Form wizard on every request. Maybe it would be a good idea to make it possible to have a class inside the url config rather than an instance. e.g. the url handler code could just check if the contained code is a class/type which needs to be instanciated first. This would make it possible to get a fresh instance of the object on every request.
The FormWizard documentation needs to state that the Wizard object will be reused. Thus one must be aware that subsequent requests might get the same wizard instance. Thus any logic depending on a state set to self should be considered unsafe. I can imagine quite a lot of people using the FormWizard wrong without even knowing it.
I posted this as a 1.0 issue as I think this one should be figured out or at least documented properly. It gave me a hard time and caused a critical software bug that was hard to spot. It surely was my mistake to assume that parse_params would be able to cut short the request, but the way the FormWizard uses self to transfer state from one method to another rounded it up to some rather unpleasing issue.
I attached a simple patch which enhances parse_params so that it is capable of cutting short a request. The solutions (2.) is a bit more complex and needs some tweaking of the source and changing of the documentation so I did not provide a patch. Solution (3.) is just a quick fix that I don't consider very pretty, but use it for now.
Change History (10)
Changed 6 years ago by Michael P. Jung
comment:1 Changed 6 years ago by jacob
- milestone changed from 1.0 to post-1.0
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
comment:4 Changed 5 years ago by jacob
- Triage Stage changed from Unreviewed to Design decision needed