Works with mobile 832569

[willkg]

So, there are a few things here worth looking at review-wise.

1. I tweaked the simple table. Pretty sure I defined the fields right, but ... worth looking at.
2. It's worth verifying it works from FxOS, Firefox for Android and Firefox Desktop. I checked and it seems fine on all three for me.
3. I tweaked the /feedback/ route pattern so the / at the end is optional. Firefox for Android uses http://m.input.mozilla.org/%LOCALE%/feedback which I tweaked to http://192.168.1.9:8000/%LOCALE%/feedback in about:settings. Because there's no / at the end, I had to tweak the route pattern.
4. I tossed two hidden fields into SimpleForm so I could (ab)use form data validation. Maybe that's a lousy idea. I don't know.
5. Technically, Firefox for Android has two kinds of feedback: sad and idea. I'm lumping them both into sad. Maybe we should instead just ignore the ideas?

Anyhow, thoughts and r?

[mythmon]

Is _type what the old input used for happy/sad/idea? Maybe we should verify that it doesn't contain the value for happy, just in case something else is using the "api" like this?

[willkg]

http://hg.mozilla.org/mozilla-central/file/169e0492b03b/mobile/android/chrome/content/aboutFeedback.xhtml#l187

Happy feedback goes the the Google Play Store review thing, I think. Only sad feedback and ideas get piped to Input.

[willkg]

I mentioned to Mike that I was surprised Firefox for Android feedback posting worked with the csrf stuff. Mike pointed out it's very likely I tested wrong.

I need to re-test and if I hit problems, probably rework how I implemented things.

[willkg]

Also, this should have a test.

[mythmon]

I did some initial testing of this. The functionality works for me. Then I tested csrf. Oddly, a csrf error can be easily triggered on the desktop (I simply removed the csrf form element before submitting), but about:feedback on my Firefox for Android nightly did not cause a csrf failure, and in fact was able to successfully submit feedback. I verified that it was not somehow magically sending a csrf token (by making it POST to an instance of netcat).

In other words, my simple tests indicate that Firefox for Android can break the csrf protection somehow. I don't think this is really true, but I have no data to support that thought yet.

I'm really confused. I bet we are just missing something small and silly.

[jsocol]

Is it sending an X-CSRFToken header? Before 1.2.5, CSRF tokens weren't required if request was Ajaxy (i.e. had the X-Requested-With: XmlHttpRequest but that's been fixed for... a while now.

[willkg]

Some things about csrf in fjord. First, we're using django-session-csrf. Second, we have ANON_ALWAYS set to True (this is a playdoh default) which causes all anonymous users to pick up a csrf token. I think this is set up the way we want.

I think my problem was just shoddy testing on my part. When testing with DEBUG=True, csrf stuff is ignored. When testing in unit tests, csrf stuff is ignored. I didn't set up and test in an environment that was set up correctly.

Firefox for Android neither picks up a csrf token nor sends one. However we really want to csrf_protect the feedback view. Thus, I followed the advice in the Django docs and exempted the main view function and provided a csrf_protect path.

https://docs.djangoproject.com/en/dev/ref/contrib/csrf/#view-needs-protection-for-one-path

Then I wrote some tests that verify the csrf setup is correct. I also wrote a few tests verifying that Fjord can handle the Firefox for Android feedback data.

I'm pretty sure that covers all our use cases. I added some notes about when we can nix the goofy code. I'll do some extra-special testing when we get to input-dev.

r?

[mythmon]

Hmm. I don't really like that now we have additional "routing" logic both before our router (for CSRF things) and after (special cases for _type). Something that comes to mind is to make our router understand that Firefox for Android makes posts that use _type, and route it accordingly. We can then make a new view just for FfA that is csrf_exempt, and keep the csrf enforcement on the other views. To be clear: what I am suggesting is removing the CSRF decorator from the route function, and moving it to desktop_stable_feedback and mobile_stable_feedback.

Does the above sound messy to you? Right now the logic for the CSRF selection bothers me, but it is only temporary. If you think that this pull request is a better way to handle these different paths, then we can go with it. I'll go test the CSRF stuff now and read through it more carefully.

[willkg]

You need the @csrf_exempt decorator on the feedback_router function. Otherwise it doesn't even go through the view if it doesn't have a csrf token and dies in the CsrfViewMiddleware.

[mythmon]

Hmm. I will try and explain my idea better with some code later. For now, this code looks ok.

r+

[mythmon]

For completeness, here is what I did: I verified that CSRF protection is still working in general by tampering with a POST request from the feedback form and removing the CSRF bits. This failed as expected. Then I made another request, again with no CSRF bits, but I added _type=1. This worked. So it seems this hole through CSRF is working as expected.

[willkg]

Landed in 9505740