Failure on 403 Forbidden doesn't work with how we expect the failure redirect

[peterbe]

I'm currently using 07369a5f7 and it has this code::

    def login_failure(self, error=None):
        ...
        return JSONResponse({'redirect': failure_url}, status=403)

Nothing happens because jQuery AJAX thinks of this as an error. So the .then() from browserid.js isn't firing.

        django_browserid.login().then(function(verifyResult) {
            window.location = $link.data('next') || verifyResult.redirect;
        }).fail(function() {  // added by me to demonstrate
            ????
        });

I think that if the server returns a 403 Forbidden, any content it sends is quite superficial and not something to rely on. In the jquery deferred fail for AJAX you get sent (qXHR jqXHR, String textStatus, String errorThrown) (from the docs)

[peterbe]

My gut tells me we ought to change the server to instead do something like

    def login_failure(self, error=None):
        ...
        return JSONResponse({'failure_redirect': failure_url})

And in the javascript do something like this:

        django_browserid.login().then(function(verifyResult) {
            if (verifyResult.failure_redirect) window.location = verifyResult.failure_redirect;
            window.location = $link.data('next') || verifyResult.redirect;
        });

[peterbe]

oops. clumsy fingers

[Osmose]

I disagree with avoiding the 403, I think the real question is that should we do with the default JS when a login error occurs.

If we never have the promise fail, there's no point in even using a promise and we might as well just go back to having users pass us a callback. But if we use a promise and let it fail, things like jQuery.when() can be used effectively.

I think that if the server returns a 403 Forbidden, any content it sends is quite superficial and not something to rely on.

We control the server's response, so we determine if this is true or not. The one case I can think of where we might get bitten is a 403 from CSRF issues, but that just means we need to change the view to send a custom response when CSRF fails.

---

As for what the default JS should do, I'm not convinced it should do anything. If a site wants to show an error message on a login failure, perhaps we should encourage them to include api.js but not browserid.js and write their own click handlers? It takes the burden of "handle every different case for every different website" off of browserid.js, and instead just makes it workable for super-minimal cases. I kind've like that reduction in complexity/responsibility.

Thoughts?

[peterbe]

I'm confused. Are we talking about two different things?

First of all, something has to do be done about the return JSONResponse({'redirect': failure_url}, status=403) because in $.ajax this will cause the fail promise (aka. error callback) to fire. When that happens we don't get the response as a first argument turned into an object (thanks to the content type of the response). Meaning that within the fail promise we can't kick off the redirect. Or is that possible in some way I don't know about?

Generally I don't trust response body when 4xx or 5xx errors happen. For example, if you do return http.HttpResponse("This is an important message", status=500) in Django, Django will wrap that in its own handler500 view (which by default just serves the 500.html template). And the message "This is an important message" won't be passed to the user viewing the web page. Something similar happens with CSRF 403 errors and happens too with 404 errors.

Now with regards to unexpected JS errors, that's a different matter. One thing I see being very likely is a Bad Gateway or Gateway TImeout error. For example, for the server to send the assertion verification it could get stuck in the network IO and just take forever, thus making the AJAX POST ultimately fail with something like a 504 Gateway Timeout which reaches our browserid.js via the fail promise. At that point I think the only sensible thing to do is something like...:

request.fail(function(jqXHR, textStatus) {
    alert('Something unexpected happened on the server. ("' + textStatus + '")\nTry again later.');
});

[jgmize]

As for what the default JS should do, I'm not convinced it should do anything. If a site wants to show an error message on a login failure, perhaps we should encourage them to include api.js but not browserid.js and write their own click handlers? It takes the burden of "handle every different case for every different website" off of browserid.js, and instead just makes it workable for super-minimal cases. I kind've like that reduction in complexity/responsibility.

Thoughts?

How about implementing 403 handling just for the Django admin, and point to that as a working example of error handling?

[Osmose]

I'm confused. Are we talking about two different things?

No, but I didn't quite explain the first few stops my train of thought hit. Your suggestion is to avoid making browserid_login trigger the fail callback on the Deferred (I was saying Promise, but meant jQuery's Deferreds that our API functions return), and instead just have the success callback check if there's an attribute on the response that signals failure. My argument against that had two parts:

1. Avoiding the failure callback removes a very useful part of Deferreds, which is being able to use jQuery.when() to wait for multiple Deferreds to resolve before running some code.
2. If a failure has occurred, semantically it makes more sense to trigger a failure callback then to trigger a success callback and make users double-check whether it was actually successful or not.

When that happens we don't get the response as a first argument turned into an object (thanks to the content type of the response). Meaning that within the fail promise we can't kick off the redirect. Or is that possible in some way I don't know about?

It is! You use xhr.responseText. See this StackOverflow thread for details. It's not quite as convenient but still completely workable.

For example, if you do return http.HttpResponse("This is an important message", status=500) in Django, Django will wrap that in its own handler500 view (which by default just serves the 500.html template). And the message "This is an important message" won't be passed to the user viewing the web page.

I just tested this on Django 1.6 and 1.5 and I get "This is an important message" and the correct HTTP status code. Works for 403 too.

[peterbe]

Michael Kelly wrote:

I'm confused. Are we talking about two different things?

No, but I didn't quite explain the first few stops my train of thought hit. Your suggestion is to avoid making |browserid_login| trigger the fail callback on the Deferred (I was saying Promise, but meant jQuery's Deferreds that our API functions return), and instead just have the success callback check if there's an attribute on the response that signals failure. My argument against that had two parts:

1. Avoiding the failure callback removes a very useful part of Deferreds, which is being able to use |jQuery.when()| to wait for multiple Deferreds to resolve before running some code.

2. If a failure has occurred, semantically it makes more sense to trigger a failure callback then to trigger a success callback and make users double-check whether it was actually successful or not.

   When that happens we don't get the response as a first argument turned into an object (thanks to the content type of the response). Meaning that within the |fail| promise we can't kick off the redirect. Or is that possible in some way I don't know about?

It is! You use |xhr.responseText|. See this StackOverflow thread for details http://stackoverflow.com/questions/1637019/how-to-get-the-jquery-ajax-error-response-text. It's not quite as convenient but still completely workable.

Aha! Doable but not convenient. I guess we'll have to do something like

.fail(function(xhr) {
     try {
        var response = JSON.parse(xhr.responseText);
        if (response.redirect_url) {
           location.href = response.redirect_url
        }
     } catch(ex) {
        alert('Unable to redirect you. Something else happened.');
     }
});

For example, if you do |return http.HttpResponse("This is an
important message", status=500)| in Django, Django will wrap that in
its own handler500 view (which by default just serves the |500.html|
template). And the message "This is an important message" won't be
passed to the user viewing the web page.

I just tested this on Django 1.6 and 1.5 and I get "This is an important message" and the correct HTTP status code. Works for 403 too.

My bad. I assumed. I guess the handler500 view only kicks in if you have an unhandled exception happening. I'm assuming you tested in both DEBUG and !DEBUG mode.

— Reply to this email directly or view it on GitHub https://github.com/mozilla/django-browserid/issues/231#issuecomment-36178621.