Determine WWW-Authenticate header using all authentication classes

[dbrgn]

When trying to determine the WWW-Authenticate header (https://github.com/tomchristie/django-rest-framework/blob/12394c9cacef0c92ef5b9d31836612e52a26e57a/rest_framework/views.py#L144-L151), until now the first available authentication class is used. If it doesn't provide any header (see the SessionAuthentication class as an example), None is returned, even though there might be another authentication class which does provide a valid header.

With this commit, get_authenticate_header() iterates through all available authentication classes, until it finds one that provides a valid header (if any).

A use case where the current behavior is a problem is the default configuration:

    'DEFAULT_AUTHENTICATION_CLASSES': (
        'rest_framework.authentication.SessionAuthentication',
        'rest_framework.authentication.BasicAuthentication',
     ),

Even though basic auth provides a valid WWW-Authenticate header, a HTTP 403 status is returned when a permission check fails.

Tests to cover this change are provided in this PR.

(The CSRF fix is needed to properly pass the tests after the WWW-Authenticate commit has been added.)

[xordoquy]

@tomchristie we've been discussing that a bit on IRC. Is there any reason why it was just checking the first authentication class ?

[lovelydinosaur]

I think the reason for going with the first only is that the browser behavior is loud when a WWW-Authentication header is present - if you have BasicAuthentication anywhere in your authenticators you'll end up with the basic auth credentials popup unless you're already logged in by some other method - it's not obvious to me if that'd be desirable if session auth is also in the list and the user is able to login via the standard login screen. Unsure.

[dbrgn]

The reason for this PR is that a HTTP 403 is simply semantically incorrect when a user is not authenticated :) And the primary use of an API is via software, not via the browser. Software usually relies on status codes to determine the actions it should take.

[kevin-brown]

There are two separate issues covered in this pull request. I'm +1 on dbrgn@fd533db (CSRF changes) and +0 on dbrgn@e8c03bc (Header changes).

The CSRF issue passes in tests because they do not cover any cases where basic authentication (or other authentication methods which include a WWW-Authenticate header) is first in the list. In reality, the responses are not actually consistent with the Django documentation or the expected status code for CSRF issues.

The WWW-Authenticate header issue does seem like a problem, though this is expected behaviour that is called out in the documentation. Given that some users return HTML pages through the API (and renderers exist for them), and not just always returning XML/JSON data, I'm not sure what the impact of this change would be.

---

This pull request should probably be split, so the CSRF fix (which is less controversial) can land while we figure out the header issue.

[lovelydinosaur]

@kevin-brown Sounds right to me.

[lovelydinosaur]

@dbrgn "HTTP 403 is simply semantically incorrect when a user is not authenticated" Arguable either way. It is also what the vast majority of sites will return right now.

[dbrgn]

I'll create a separate PR for the CSRF issue.

[dbrgn]

CSRF issue is now removed from this PR. The tests fail now, as long as that other issue is not merged.

[lovelydinosaur]

Marking for 3.0 (or after) given behavioural change and dependance on #1613.

[lovelydinosaur]

Forcing the travis tests to re-run given that #1896 is now merged.

[lovelydinosaur]

Needs updating to latest master.

[lovelydinosaur]

HTTP 403 is simply semantically incorrect when a user is not authenticated :)

Except that's not the case - it depends on the authentication policies in place. Eg [SessionAuth] used alone clearly shouldn't ever be a 401 as it can't fulfil the requirements of the RFC with respect to a www-authenticate header.

I don't actually have an issue with the current behaviour, and switching this so that [SessionAuth, BasicAuth] now becomes loud feels like worse behaviour not better, as we're losing the ability to switch between the two different styles.