Closed danizen closed 2 years ago
So, guess the view could use the LOGOUT_REDIRECT_URL Django setting in the event of a failure. In the events of a success, it could use the LOGIN_REDIRECT_URL or "next".
I think that's a valid feature request.
Let's go back to the use case in order to clarify the constraints. The goal is to provide an alternative login mechanism via django-sesame, typically for "service account"-style access, when adding the django-sesame middleware isn't desirable.
(Here the reason for not adding the django-sesame middleware is to avoid deploying that login mechanism to all URLs. I see it as a layer of defense-in-depth on top of django-sesame, which is a more risky authentication mechanism.)
For testing purposes, you don't really care about redirecting. Your testing script can simply navigate to the next URL. Redirecting after login is a rats nest — see Django's LoginView
— so I'd rather avoid it.
Your implementation looks correct to me (although I don't understand why you need the if not request.user.is_anonymous
conditional). Here's a slightly more generic version, without redirects (untested):
from http import HTTPStatus
from django.contrib.auth import authenticate, login
from django.http import HttpResponse
from . import settings
def login(request, scope="", max_age=None):
sesame = request.GET.get(settings.TOKEN_NAME)
if sesame is None:
return HttpResponse(
b"login failed: missing token",
content_type="text/plain; charset=utf-8",
status=HTTPStatus.UNAUTHORIZED,
)
user = authenticate(request, sesame=sesame, scope=scope, max_age=max_age)
if user is None:
return HttpResponse(
b"login failed: invalid token",
content_type="text/plain; charset=utf-8",
status=HTTPStatus.FORBIDDEN,
)
login(request, user) # always updates the last login date.
return HttpResponse(status=HTTPStatus.NO_CONTENT)
Going through the higher-level get_user
function isn't really simpler. The primary purpose of get_user
is to update the last login date, but login
does that already, so you have to tell get_user
not to do it. This isn't an improvement over the previous version (untested):
from http import HTTPStatus
from django.contrib.auth import login
from django.http import HttpResponse
from .utils import get_user
def login(request, scope="", max_age=None):
# login(request, user) will update the last login date.
user = get_user(request, update_last_login=False, scope=scope, max_age=max_age)
if user is None:
return HttpResponse(
b"login failed: invalid or missing token",
content_type="text/plain; charset=utf-8",
status=HTTPStatus.FORBIDDEN,
)
login(request, user)
return HttpResponse(status=HTTPStatus.NO_CONTENT)
Probably that solves your use case. However, I'm not sure it's sufficiently widely useful to be provided in django-sesame. For non-scripted use cases, I need to support redirecting to a customizable URL.
I agree with you on avoiding get_user().
I understand that many users of django-sesame use it not for functional testing, or to login for dynamic security analysis, but to be able to enable email based login for tickler emails. This reduces the costs of conversions in much the same way as social login does. No one likes it when the first thing an email does is demand that you login.
What about setting that restricts the middleware to work only on certain request PATHs? That is present in django-cas-ng, which is what our federated login uses on-premise (see https://www.danizen.net/anti-social-auth/). This for instance allows login to be restricted to the django admin.
Sorry, I meant "a setting" that restricts the middleware, such as SESAME_PATH_STARTS_WITH,
which would default to None. It seems to me that this limits the amount of code surface for you to maintain while also enabling django-sesame to be used out of the box for this use case. To be honest, it is not just my code that restricts this login to a certain subnet - my DevOps does this as a control so that public apps can have "internal-only" URLs - sort as a "technical control" if you speak that compliance/security language.
When I first used django-sesame, set_unusable_password
didn't exist, and it wasn't the default. My management commands in the common code that supports my applications probably do not call set_unusable _password
. I will fix that :)
If you do not like the idea of adding a setting like SESAME_PATH_STARTS_WITH
, please go ahead and close with such a comment. If you do like it, I will be happy to craft a pull request or simply wait for you or one of your maintainers/contributers.
Yesterday I attempted to clean up how Django implements redirects after login/logout: https://github.com/django/django/pull/15608. This makes it much more realistic to implement a custom login view in django-sesame that redirects securely, by backporting or importing the mixin that I created in this PR. Let's see where that goes.
Then, I've been giving this further thought. Actually, it's a really good issue because it forces a more structured understanding of use cases for django-sesame.
Users may want:
The corresponding solutions are:
/target?sesame=...
instead of /sesame_login?sesame=...&next=%2ftarget
)user = get_user(request)
Long story short, here's one possible plan:
One thing I didn't address — I dislike the idea of a setting containing of whitelist of URLs; this is too far from the URLconf or the view. I'd rather apply a decorator in the URLconf or directly on the view.
I dislike the idea of a setting containing of whitelist of URLs; this is too far from the URLconf or the view. I'd rather apply a decorator in the URLconf or directly on the view.
I see that - spreading it out into middleware, URLconf, and the views undermines the clarity of the code. A security professional might talk about increasing attack surface. I used to advocate that many developers apply security decorators directly in URLconf so that they can validate security in one file - but I found it too convenient personally to do that in the views once I understood "@method_decorator", and also DRF with authentication classes makes that sort of difficult anyway.
Hello @danizen, what do you think of #83?
I haven't written the documentation yet; you can look at the tests for intended usage.
I should have followed up on this. NIH Central IT created an "Automated Login Utility" which works for both Django and other web frameworks. The way it works is by saving a cookie (which can be moved from the browser to some automated session or used via Selenium so that when Django redirects to the usually interactive federated login, the login flows right through.
This addresses the issue I had. However, I will still try this out and get you feedback.
On a previous project, I used django-sesame as a "backdoor" for functional testing. One problem I ran into however is that the middleware makes all views login views, and some views are public. My solution was to implement a view that functions as a login view and avoid the middleware, then I could secure this login view to only be allowed from a certain subnet via Httpd or nginx. I am wondering whether such a view can be provided and documented in this next release.
Note that the code below includes my own decorator
remote_ip_permitted
and also integration withdjango.contrib.messages
which may or may not be desired.