Skip to content

Commit

Permalink
factors/view: show concise error message when domain is mis-configured
Browse files Browse the repository at this point in the history
  • Loading branch information
BeryJu committed Feb 18, 2020
1 parent 5dad853 commit e2631ce
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 18 deletions.
2 changes: 1 addition & 1 deletion passbook/core/templates/error/400.html
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ <h1>{% trans 'Bad Request' %}</h1>
</header>
<form>
{% if message %}
<p>{% trans message %}</p>
<h3>{% trans message %}</h3>
{% endif %}
{% if 'back' in request.GET %}
<a href="{% back %}" class="btn btn-primary btn-block btn-lg">{% trans 'Back' %}</a>
Expand Down
10 changes: 4 additions & 6 deletions passbook/factors/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,8 @@ def setUp(self):
def test_unauthenticated_raw(self):
"""test direct call to AuthenticationView"""
response = self.client.get(reverse("passbook_core:auth-process"))
# Response should be 302 since no pending user is set
self.assertEqual(response.status_code, 302)
self.assertEqual(response.url, reverse("passbook_core:auth-login"))
# Response should be 400 since no pending user is set
self.assertEqual(response.status_code, 400)

def test_unauthenticated_prepared(self):
"""test direct call but with pending_uesr in session"""
Expand Down Expand Up @@ -71,9 +70,8 @@ def test_authenticated(self):
"""Test with already logged in user"""
self.client.force_login(self.user)
response = self.client.get(reverse("passbook_core:auth-process"))
# Response should be 302 since no pending user is set
self.assertEqual(response.status_code, 302)
self.assertEqual(response.url, reverse("passbook_core:overview"))
# Response should be 400 since no pending user is set
self.assertEqual(response.status_code, 400)
self.client.logout()

def test_unauthenticated_post(self):
Expand Down
44 changes: 33 additions & 11 deletions passbook/factors/view.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
"""passbook multi-factor authentication engine"""
from typing import List, Tuple
from typing import List, Optional, Tuple

from django.contrib.auth import login
from django.contrib.auth.mixins import UserPassesTestMixin
from django.shortcuts import get_object_or_404, redirect, reverse
from django.http import HttpRequest, HttpResponse
from django.shortcuts import get_object_or_404, redirect, render, reverse
from django.utils.http import urlencode
from django.views.generic import View
from structlog import get_logger

from passbook.core.models import Factor, User
from passbook.core.views.utils import PermissionDeniedView
from passbook.lib.config import CONFIG
from passbook.lib.utils.reflection import class_to_path, path_to_class
from passbook.lib.utils.urls import is_url_absolute
from passbook.policies.engine import PolicyEngine
Expand Down Expand Up @@ -44,18 +46,34 @@ class AuthenticationView(UserPassesTestMixin, View):
current_factor: Factor

# Allow only not authenticated users to login
def test_func(self):
def test_func(self) -> bool:
return AuthenticationView.SESSION_PENDING_USER in self.request.session

def handle_no_permission(self):
def _check_config_domain(self) -> Optional[HttpResponse]:
"""Checks if current request's domain matches configured Domain, and
adds a warning if not."""
current_domain = self.request.get_host()
config_domain = CONFIG.y("domain")
if current_domain != config_domain:
message = (
f"Current domain of '{current_domain}' doesn't "
f"match configured domain of '{config_domain}'."
)
LOGGER.warning(message)
return render(
self.request, "error/400.html", context={"message": message}, status=400
)
return None

def handle_no_permission(self) -> HttpResponse:
# Function from UserPassesTestMixin
if NEXT_ARG_NAME in self.request.GET:
return redirect(self.request.GET.get(NEXT_ARG_NAME))
if self.request.user.is_authenticated:
return _redirect_with_qs("passbook_core:overview", self.request.GET)
return _redirect_with_qs("passbook_core:auth-login", self.request.GET)

def get_pending_factors(self):
def get_pending_factors(self) -> List[Tuple[str, str]]:
"""Loading pending factors from Database or load from session variable"""
# Write pending factors to session
if AuthenticationView.SESSION_PENDING_FACTORS in self.request.session:
Expand All @@ -67,6 +85,7 @@ def get_pending_factors(self):
)
pending_factors = []
for factor in _all_factors:
factor: Factor
LOGGER.debug(
"Checking if factor applies to user",
factor=factor,
Expand All @@ -81,10 +100,13 @@ def get_pending_factors(self):
LOGGER.debug("Factor applies", factor=factor, user=self.pending_user)
return pending_factors

def dispatch(self, request, *args, **kwargs):
def dispatch(self, request: HttpRequest, *args, **kwargs) -> HttpResponse:
# Check if user passes test (i.e. SESSION_PENDING_USER is set)
user_test_result = self.get_test_func()()
if not user_test_result:
incorrect_domain_message = self._check_config_domain()
if incorrect_domain_message:
return incorrect_domain_message
return self.handle_no_permission()
# Extract pending user from session (only remember uid)
self.pending_user = get_object_or_404(
Expand Down Expand Up @@ -117,23 +139,23 @@ def dispatch(self, request, *args, **kwargs):
self._current_factor_class.request = request
return super().dispatch(request, *args, **kwargs)

def get(self, request, *args, **kwargs):
def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse:
"""pass get request to current factor"""
LOGGER.debug(
"Passing GET",
view_class=class_to_path(self._current_factor_class.__class__),
)
return self._current_factor_class.get(request, *args, **kwargs)

def post(self, request, *args, **kwargs):
def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse:
"""pass post request to current factor"""
LOGGER.debug(
"Passing POST",
view_class=class_to_path(self._current_factor_class.__class__),
)
return self._current_factor_class.post(request, *args, **kwargs)

def user_ok(self):
def user_ok(self) -> HttpResponse:
"""Redirect to next Factor"""
LOGGER.debug(
"Factor passed",
Expand All @@ -160,14 +182,14 @@ def user_ok(self):
LOGGER.debug("User passed all factors, logging in", user=self.pending_user)
return self._user_passed()

def user_invalid(self):
def user_invalid(self) -> HttpResponse:
"""Show error message, user cannot login.
This should only be shown if user authenticated successfully, but is disabled/locked/etc"""
LOGGER.debug("User invalid")
self.cleanup()
return _redirect_with_qs("passbook_core:auth-denied", self.request.GET)

def _user_passed(self):
def _user_passed(self) -> HttpResponse:
"""User Successfully passed all factors"""
backend = self.request.session[AuthenticationView.SESSION_USER_BACKEND]
login(self.request, self.pending_user, backend=backend)
Expand Down

0 comments on commit e2631ce

Please sign in to comment.