pmuller / django-crowd-auth

Atlassian Crowd SSO integration for Django applications
Other
10 stars 6 forks source link

Assuming that request is always an object is incorrect #5

Open tm1000 opened 6 years ago

tm1000 commented 6 years ago

https://docs.djangoproject.com/en/2.0/topics/auth/customizing/#authentication-backends

request is an HttpRequest and may be None if it wasn’t provided to authenticate() (which passes it on to the backend).

Environment:

Request Method: POST
Request URL: https://hostname/accounts/login/

Django Version: 1.11.10
Python Version: 2.7.13
Installed Applications:
(u'django.contrib.auth',
 u'django.contrib.contenttypes',
 u'django.contrib.sessions',
 u'django.contrib.sites',
 u'django.contrib.messages',
 u'django.contrib.staticfiles',
 u'django.contrib.admin.apps.SimpleAdminConfig',
 u'django.contrib.admindocs',
 u'django.contrib.sitemaps',
 u'social_django',
 u'crispy_forms',
 u'compressor',
 u'rest_framework',
 u'rest_framework.authtoken',
 u'weblate.addons',
 u'weblate.trans',
 u'weblate.lang',
 u'weblate.langdata',
 u'weblate.permissions',
 u'weblate.screenshots',
 u'weblate.accounts',
 u'weblate.utils',
 u'weblate.wladmin',
 u'weblate',
 u'weblate.gitexport')
Installed Middleware:
[u'django.middleware.security.SecurityMiddleware',
 u'django.contrib.sessions.middleware.SessionMiddleware',
 u'django.middleware.common.CommonMiddleware',
 u'django.middleware.locale.LocaleMiddleware',
 u'django.middleware.csrf.CsrfViewMiddleware',
 u'weblate.accounts.middleware.AuthenticationMiddleware',
 u'django.contrib.messages.middleware.MessageMiddleware',
 u'django.middleware.clickjacking.XFrameOptionsMiddleware',
 u'social_django.middleware.SocialAuthExceptionMiddleware',
 u'weblate.accounts.middleware.RequireLoginMiddleware',
 u'weblate.middleware.SecurityMiddleware',
 u'weblate.wladmin.middleware.ConfigurationErrorsMiddleware']

Traceback:

File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/exception.py" in inner
  41.             response = get_response(request)

File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py" in _get_response
  187.                 response = self.process_exception_by_middleware(e, request)

File "/usr/local/lib/python2.7/dist-packages/django/core/handlers/base.py" in _get_response
  185.                 response = wrapped_callback(request, *callback_args, **callback_kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in inner
  185.                     return func(*args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/generic/base.py" in view
  68.             return self.dispatch(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in _wrapper
  67.             return bound_func(*args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/cache.py" in _wrapped_view_func
  57.         response = view_func(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in bound_func
  63.                 return func.__get__(self, type(self))(*args2, **kwargs2)

File "/usr/local/lib/python2.7/dist-packages/weblate/accounts/views.py" in dispatch
  572.         return super(WeblateLoginView, self).dispatch(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in _wrapper
  67.             return bound_func(*args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/debug.py" in sensitive_post_parameters_wrapper
  76.             return view(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in bound_func
  63.                 return func.__get__(self, type(self))(*args2, **kwargs2)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in _wrapper
  67.             return bound_func(*args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in _wrapped_view
  149.                     response = view_func(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in bound_func
  63.                 return func.__get__(self, type(self))(*args2, **kwargs2)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in _wrapper
  67.             return bound_func(*args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/decorators/cache.py" in _wrapped_view_func
  57.         response = view_func(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/utils/decorators.py" in bound_func
  63.                 return func.__get__(self, type(self))(*args2, **kwargs2)

File "/usr/local/lib/python2.7/dist-packages/django/contrib/auth/views.py" in dispatch
  90.         return super(LoginView, self).dispatch(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/generic/base.py" in dispatch
  88.         return handler(request, *args, **kwargs)

File "/usr/local/lib/python2.7/dist-packages/django/views/generic/edit.py" in post
  182.         if form.is_valid():

File "/usr/local/lib/python2.7/dist-packages/django/forms/forms.py" in is_valid
  183.         return self.is_bound and not self.errors

File "/usr/local/lib/python2.7/dist-packages/django/forms/forms.py" in errors
  175.             self.full_clean()

File "/usr/local/lib/python2.7/dist-packages/django/forms/forms.py" in full_clean
  385.         self._clean_form()

File "/usr/local/lib/python2.7/dist-packages/django/forms/forms.py" in _clean_form
  412.             cleaned_data = self.clean()

File "/usr/local/lib/python2.7/dist-packages/weblate/accounts/forms.py" in clean
  571.                 password=password

File "/usr/local/lib/python2.7/dist-packages/django/contrib/auth/__init__.py" in authenticate
  70.             user = _authenticate_with_backend(backend, backend_path, request, credentials)

File "/usr/local/lib/python2.7/dist-packages/django/contrib/auth/__init__.py" in _authenticate_with_backend
  116.     return backend.authenticate(*args, **credentials)

File "/usr/local/lib/python2.7/dist-packages/django_crowd_auth/backends.py" in authenticate
  23.         remote_addr = request.META['REMOTE_ADDR']

Exception Type: AttributeError at /accounts/login/
Exception Value: 'NoneType' object has no attribute 'META'
pmuller commented 6 years ago

Hello @tm1000

Thank you for your report.

When we do not have a request object available, I believe we have no way to know the IP address of the remote user. Creating a session for an user without specifying an IP address is rather insure: the token can then be used from any IP address. This makes me really uncomfortable supporting a missing request object.

Maybe I am missing something:

pmuller commented 6 years ago

Haha, just realized YOU added the request to their code base from a PR a few a hours ago. Since they accepted and merged it, I believe it's safer to use your fix (which seems cleaner).

tm1000 commented 6 years ago

@pmuller That's fine but you are still violating the Django API. It specifically says "request is an HttpRequest and may be None if it wasn’t provided to authenticate() (which passes it on to the backend).". So Request can be of type None and this module should be able to deal with that.

This makes me really uncomfortable supporting a missing request object.

Right but the Django API states "request is an HttpRequest and may be None". Regardless it's fixed in Weblate, but this may be an issue with other systems that use the Django framework and this crowd module (since this is pretty much the only good one)

On that note thank you for making this module and saving me much headache in writing it myself. You did a great job!

pmuller commented 6 years ago

I fully understand your concern @tm1000, but as this module is critical for a webapp security, I do not think it's a good idea to compromise on security. Let's reopen the issue to keep track of this in the event someone comes up with a good idea abut how to fix it without creating sessions not tied to a specific source ip address.

NewbiZ commented 6 years ago

I also just ran into this issue while calling Django authenticate(). I understand the concern and use of the REMOTE_ADDR, but I think adding an assert would just save some debugging time.

assert(request, 'django-crowd-auth requires authenticate() to provide a request object to validate the remote address')