Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RemoteUserAuthentication, docs, and tests #5306

Merged
merged 2 commits into from Aug 11, 2017

Conversation

alexdutton
Copy link
Contributor

@alexdutton alexdutton commented Aug 4, 2017

Support for delegating authentication to a web server.

This is a little thing, but I thought it would be worth providing an obvious way to do it. The alternative is to use RemoteUserBackend, RemoteUserMiddleware and SessionBasedAuthentication, but then one has to mess with working out how/whether to disable CSRF protection.

Support for delegating authentication to a web server.
alexdutton added a commit to alexdutton/idm-core that referenced this issue Aug 4, 2017
This has also been submitted upstream
(encode/django-rest-framework#5306) so can
hopefully be removed at some point.
@tomchristie
Copy link
Member

tomchristie commented Aug 4, 2017

This is great - nice & tidy.

Normally we'd try to push something like this into a third party package. Even tho the implementation is small, there's still the extra overhead of docs and tests that it introduces to the project. I'm somewhat in two minds tho, since it really is a small implementation.

Not sure what the rest of the team think?

@rpkilby
Copy link
Member

rpkilby commented Aug 4, 2017

I'm in favor of the change. The PR seems straightforward and exposes builtin behavior. That said, I don't use remote user authentication and can't really review the PR.

@jpadilla
Copy link
Member

jpadilla commented Aug 4, 2017

Ah nice, I didn't even know about REMOTE_USER in Django at all. Although simple enough, IMHO I'd rather see this be its own third party package instead. Seems like one of those things that I haven't heard of many people using, so not a big fan of adding to core, but I might be wrong.

@alexdutton
Copy link
Contributor Author

alexdutton commented Aug 5, 2017

My argument in favour of inclusion 😁: I'll agree it's a bit more "traditional" to use e.g. apache for authentication, but REMOTE_USER support does open up a number of possibilities that don't necessarily have mature implementations in Python or as drf auth classes. It's an enabling thing that mirrors functionality available in Django core. In the team in which I work, API authentication is mostly a configuration management, so pushing it out of the application like this is seen as a Good Thing (though obviously other viewpoints/drivers exist).

@xordoquy
Copy link
Collaborator

xordoquy commented Aug 7, 2017

I'm definitively in favor of adding this to prevent some weird workarounds in developer's code base.

@tomchristie tomchristie merged commit e80b78d into encode:master Aug 11, 2017
1 check passed
@tomchristie tomchristie added this to the 3.6.4 Release milestone Aug 11, 2017
@tomchristie
Copy link
Member

tomchristie commented Aug 11, 2017

Ace, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants