From 5b22f9b6c35960b99b3840e99bf85c6c8fc9041b Mon Sep 17 00:00:00 2001
From: Jens Langhammer <jens.langhammer@beryju.org>
Date: Tue, 18 Feb 2020 10:57:30 +0100
Subject: [PATCH] providers/saml: transition to dataclass from dict, cleanup
 unused templates, add missing autosubmit_form

---
 passbook/providers/saml/exceptions.py         |   4 -
 passbook/providers/saml/models.py             |   2 +-
 passbook/providers/saml/processors/base.py    |  24 +--
 passbook/providers/saml/processors/types.py   |  11 ++
 .../templates/saml/idp/autosubmit_form.html   |  39 ++++
 .../saml/templates/saml/idp/invalid_user.html |   5 -
 .../saml/templates/saml/idp/logged_out.html   |  10 +-
 .../saml/templates/saml/idp/login.html        |  12 +-
 .../saml/templates/saml/idp/settings.html     |  47 -----
 passbook/providers/saml/urls.py               |   9 +-
 passbook/providers/saml/views.py              | 177 +++++++++++-------
 passbook/root/settings.py                     |   2 +-
 12 files changed, 190 insertions(+), 152 deletions(-)
 create mode 100644 passbook/providers/saml/processors/types.py
 create mode 100644 passbook/providers/saml/templates/saml/idp/autosubmit_form.html
 delete mode 100644 passbook/providers/saml/templates/saml/idp/invalid_user.html
 delete mode 100644 passbook/providers/saml/templates/saml/idp/settings.html

diff --git a/passbook/providers/saml/exceptions.py b/passbook/providers/saml/exceptions.py
index 98fea6dec..a78218ca6 100644
--- a/passbook/providers/saml/exceptions.py
+++ b/passbook/providers/saml/exceptions.py
@@ -3,7 +3,3 @@
 
 class CannotHandleAssertion(Exception):
     """This processor does not handle this assertion."""
-
-
-class UserNotAuthorized(Exception):
-    """User not authorized for SAML 2.0 authentication."""
diff --git a/passbook/providers/saml/models.py b/passbook/providers/saml/models.py
index d69c2d189..4a1668b77 100644
--- a/passbook/providers/saml/models.py
+++ b/passbook/providers/saml/models.py
@@ -82,7 +82,7 @@ class SAMLProvider(Provider):
         self._meta.get_field("processor_path").choices = get_provider_choices()
 
     @property
-    def processor(self):
+    def processor(self) -> Processor:
         """Return selected processor as instance"""
         if not self._processor:
             try:
diff --git a/passbook/providers/saml/processors/base.py b/passbook/providers/saml/processors/base.py
index 13d51b4ba..c407d44d0 100644
--- a/passbook/providers/saml/processors/base.py
+++ b/passbook/providers/saml/processors/base.py
@@ -7,6 +7,7 @@ from structlog import get_logger
 
 from passbook.core.exceptions import PropertyMappingExpressionException
 from passbook.providers.saml.exceptions import CannotHandleAssertion
+from passbook.providers.saml.processors.types import SAMLResponseParams
 from passbook.providers.saml.utils import get_random_id
 from passbook.providers.saml.utils.encoding import decode_base64_and_inflate, nice64
 from passbook.providers.saml.utils.time import get_time_string, timedelta_from_string
@@ -133,14 +134,13 @@ class Processor:
             self._response_params, saml_provider=self._remote, assertion_id=assertion_id
         )
 
-    def _get_django_response_params(self) -> Dict[str, str]:
+    def _get_saml_response_params(self) -> SAMLResponseParams:
         """Returns a dictionary of parameters for the response template."""
-        return {
-            "acs_url": self._request_params["ACS_URL"],
-            "saml_response": self._saml_response,
-            "relay_state": self._relay_state,
-            "autosubmit": self._remote.application.skip_authorization,
-        }
+        return SAMLResponseParams(
+            acs_url=self._request_params["ACS_URL"],
+            saml_response=self._saml_response,
+            relay_state=self._relay_state,
+        )
 
     def _decode_and_parse_request(self):
         """Parses various parameters from _request_xml into _request_params."""
@@ -183,7 +183,7 @@ class Processor:
         # Read the request.
         try:
             self._extract_saml_request()
-        except Exception as exc:
+        except KeyError as exc:
             raise CannotHandleAssertion(
                 f"can't find SAML request in user session: {exc}"
             ) from exc
@@ -196,7 +196,7 @@ class Processor:
         self._validate_request()
         return True
 
-    def generate_response(self) -> Dict[str, str]:
+    def generate_response(self) -> SAMLResponseParams:
         """Processes request and returns template variables suitable for a response."""
         # Build the assertion and response.
         # Only call can_handle if SP initiated Request, otherwise we have no Request
@@ -210,9 +210,9 @@ class Processor:
         self._encode_response()
 
         # Return proper template params.
-        return self._get_django_response_params()
+        return self._get_saml_response_params()
 
-    def init_deep_link(self, request: HttpRequest, url: str):
+    def init_deep_link(self, request: HttpRequest):
         """Initialize this Processor to make an IdP-initiated call to the SP's
         deep-linked URL."""
         self._http_request = request
@@ -227,4 +227,4 @@ class Processor:
             "DESTINATION": "",
             "PROVIDER_NAME": "",
         }
-        self._relay_state = url
+        self._relay_state = ""
diff --git a/passbook/providers/saml/processors/types.py b/passbook/providers/saml/processors/types.py
new file mode 100644
index 000000000..a283ba06e
--- /dev/null
+++ b/passbook/providers/saml/processors/types.py
@@ -0,0 +1,11 @@
+"""passbook saml provider types"""
+from dataclasses import dataclass
+
+
+@dataclass
+class SAMLResponseParams:
+    """Class to keep track of SAML Response Parameters"""
+
+    acs_url: str
+    saml_response: str
+    relay_state: str
diff --git a/passbook/providers/saml/templates/saml/idp/autosubmit_form.html b/passbook/providers/saml/templates/saml/idp/autosubmit_form.html
new file mode 100644
index 000000000..99166d5e9
--- /dev/null
+++ b/passbook/providers/saml/templates/saml/idp/autosubmit_form.html
@@ -0,0 +1,39 @@
+{% extends "login/base.html" %}
+
+{% load utils %}
+{% load i18n %}
+
+{% block title %}
+{% title 'Redirecting...' %}
+{% endblock %}
+
+{% block card %}
+<header class="login-pf-header">
+    <h1>{% trans 'Redirecting...' %}</h1>
+</header>
+<form method="POST" action="{{ url }}">
+    {% csrf_token %}
+    {% for key, value in attrs.items %}
+    <input type="hidden" name="{{ key }}" value="{{ value }}">
+    {% endfor %}
+    <div class="login-group">
+        <h3>
+            {% trans "Redirecting..." %}
+        </h3>
+        <p>
+            {% blocktrans with user=user %}
+            You are logged in as {{ user }}.
+            {% endblocktrans %}
+            <a href="{% url 'passbook_core:auth-logout' %}">{% trans 'Not you?' %}</a>
+        </p>
+        <input class="btn btn-primary btn-block btn-lg" type="submit" value="{% trans 'Continue' %}" />
+    </div>
+</form>
+{% endblock %}
+
+{% block scripts %}
+{{ block.super }}
+<script>
+    $('form').submit();
+</script>
+{% endblock %}
diff --git a/passbook/providers/saml/templates/saml/idp/invalid_user.html b/passbook/providers/saml/templates/saml/idp/invalid_user.html
deleted file mode 100644
index 16ba1f4e6..000000000
--- a/passbook/providers/saml/templates/saml/idp/invalid_user.html
+++ /dev/null
@@ -1,5 +0,0 @@
-{% extends "saml/idp/base.html" %}
-{% load i18n %}
-{% block content %}
-{% trans "You have logged in, but your user account is not enabled for SAML 2.0." %}
-{% endblock %}
diff --git a/passbook/providers/saml/templates/saml/idp/logged_out.html b/passbook/providers/saml/templates/saml/idp/logged_out.html
index 2d973d442..4b2a98b74 100644
--- a/passbook/providers/saml/templates/saml/idp/logged_out.html
+++ b/passbook/providers/saml/templates/saml/idp/logged_out.html
@@ -1,5 +1,9 @@
-{% extends "saml/idp/base.html" %}
+{% extends "login/base.html" %}
+
 {% load i18n %}
-{% block content %}
-{% trans "You have successfully logged out of the Identity Provider." %}
+
+{% block card %}
+<p>
+    {% trans "You have successfully logged out of the Identity Provider." %}
+</p>
 {% endblock %}
diff --git a/passbook/providers/saml/templates/saml/idp/login.html b/passbook/providers/saml/templates/saml/idp/login.html
index 13fc082b3..6c0f5e80e 100644
--- a/passbook/providers/saml/templates/saml/idp/login.html
+++ b/passbook/providers/saml/templates/saml/idp/login.html
@@ -11,15 +11,15 @@
 <header class="login-pf-header">
     <h1>{% trans 'Authorize Application' %}</h1>
 </header>
-<form method="POST" action="{{ acs_url }}">
+<form method="POST" action="{{ saml_params.acs_url }}">
     {% csrf_token %}
-    <input type="hidden" name="ACSUrl" value="{{ acs_url }}">
-    <input type="hidden" name="RelayState" value="{{ relay_state }}" />
-    <input type="hidden" name="SAMLResponse" value="{{ saml_response }}" />
+    <input type="hidden" name="ACSUrl" value="{{ saml_params.acs_url }}">
+    <input type="hidden" name="RelayState" value="{{ saml_params.relay_state }}" />
+    <input type="hidden" name="SAMLResponse" value="{{ saml_params.saml_response }}" />
     <div class="login-group">
         <h3>
-            {% blocktrans with remote=remote.application.name %}
-            You're about to sign into {{ remote }}
+            {% blocktrans with provider=provider.application.name %}
+            You're about to sign into {{ provider }}
             {% endblocktrans %}
         </h3>
         <p>
diff --git a/passbook/providers/saml/templates/saml/idp/settings.html b/passbook/providers/saml/templates/saml/idp/settings.html
deleted file mode 100644
index 336df4e9d..000000000
--- a/passbook/providers/saml/templates/saml/idp/settings.html
+++ /dev/null
@@ -1,47 +0,0 @@
-{% extends "_admin/module_default.html" %}
-
-{% load i18n %}
-{% load utils %}
-
-{% block title %}
-{% title "Overview" %}
-{% endblock %}
-
-{% block module_content %}
-<h2><clr-icon shape="application" size="32"></clr-icon>{% trans 'SAML2 IDP' %}</h2>
-<div class="row">
-  <div class="col-md-12">
-    <div class="card">
-      <div class="card-header">
-        <h2><clr-icon shape="settings" size="32"></clr-icon>{% trans 'Settings' %}</h2>
-      </div>
-      <form role="form" method="POST">
-        <div class="card-block">
-          {% include 'partials/form.html' with form=form %}
-        </div>
-        <div class="card-footer">
-          <button type="submit" class="btn btn-primary">{% trans 'Update' %}</button>
-        </div>
-      </form>
-    </div>
-  </div>
-</div>
-<div class="row">
-  <div class="col-md-12">
-    <div class="card">
-      <div class="card-header">
-        <h2><clr-icon shape="bank" size="32"></clr-icon>{% trans 'Metadata' %}</h2>
-      </div>
-      <div class="card-block">
-        <p>{% trans 'Cert Fingerprint (SHA1):' %} <pre>{{ fingerprint }}</pre></p>
-        <section class="form-block">
-          <pre lang="xml" >{{ metadata }}</pre>
-        </section>
-      </div>
-      <div class="card-footer">
-        <a href="{% url 'passbook_providers_saml:saml-metadata' %}" class="btn btn-primary"><clr-icon shape="download"></clr-icon>{% trans 'Download Metadata' %}</a>
-      </div>
-    </div>
-  </div>
-</div>
-{% endblock %}
diff --git a/passbook/providers/saml/urls.py b/passbook/providers/saml/urls.py
index a8777ba79..c48edc1df 100644
--- a/passbook/providers/saml/urls.py
+++ b/passbook/providers/saml/urls.py
@@ -4,14 +4,17 @@ from django.urls import path
 from passbook.providers.saml import views
 
 urlpatterns = [
-    path(
-        "<slug:application>/login/", views.LoginBeginView.as_view(), name="saml-login"
-    ),
+    # This view is used to initiate a Login-flow from the IDP
     path(
         "<slug:application>/login/initiate/",
         views.InitiateLoginView.as_view(),
         name="saml-login-initiate",
     ),
+    # This view is the endpoint a SP would redirect to, and saves data into the session
+    # this is required as the process view which it redirects to might have to login first.
+    path(
+        "<slug:application>/login/", views.LoginProcessView.as_view(), name="saml-login"
+    ),
     path(
         "<slug:application>/login/process/",
         views.LoginProcessView.as_view(),
diff --git a/passbook/providers/saml/views.py b/passbook/providers/saml/views.py
index 5add8e22f..17f2a2f15 100644
--- a/passbook/providers/saml/views.py
+++ b/passbook/providers/saml/views.py
@@ -5,10 +5,11 @@ from django.contrib.auth import logout
 from django.contrib.auth.mixins import AccessMixin
 from django.core.exceptions import ValidationError
 from django.core.validators import URLValidator
-from django.http import HttpRequest, HttpResponse, HttpResponseBadRequest
+from django.http import HttpRequest, HttpResponse
 from django.shortcuts import get_object_or_404, redirect, render, reverse
 from django.utils.datastructures import MultiValueDictKeyError
 from django.utils.decorators import method_decorator
+from django.utils.html import mark_safe
 from django.utils.translation import gettext as _
 from django.views import View
 from django.views.decorators.csrf import csrf_exempt
@@ -19,28 +20,16 @@ from passbook.audit.models import Event, EventAction
 from passbook.core.models import Application
 from passbook.lib.mixins import CSRFExemptMixin
 from passbook.lib.utils.template import render_to_string
+from passbook.lib.views import bad_request_message
 from passbook.policies.engine import PolicyEngine
 from passbook.providers.saml import exceptions
 from passbook.providers.saml.models import SAMLProvider
+from passbook.providers.saml.processors.types import SAMLResponseParams
 
 LOGGER = get_logger()
 URL_VALIDATOR = URLValidator(schemes=("http", "https"))
 
 
-def _generate_response(request: HttpRequest, provider: SAMLProvider) -> HttpResponse:
-    """Generate a SAML response using processor_instance and return it in the proper Django
-    response."""
-    try:
-        provider.processor.init_deep_link(request, "")
-        ctx = provider.processor.generate_response()
-        ctx["remote"] = provider
-        ctx["is_login"] = True
-    except exceptions.UserNotAuthorized:
-        return render(request, "saml/idp/invalid_user.html")
-
-    return render(request, "saml/idp/login.html", ctx)
-
-
 class AccessRequiredView(AccessMixin, View):
     """Mixin class for Views using a provider instance"""
 
@@ -97,7 +86,7 @@ class LoginBeginView(AccessRequiredView):
         try:
             request.session["SAMLRequest"] = source["SAMLRequest"]
         except (KeyError, MultiValueDictKeyError):
-            return HttpResponseBadRequest("the SAML request payload is missing")
+            return bad_request_message(request, "The SAML request payload is missing.")
 
         request.session["RelayState"] = source.get("RelayState", "")
         return redirect(
@@ -108,73 +97,84 @@ class LoginBeginView(AccessRequiredView):
         )
 
 
-class RedirectToSPView(AccessRequiredView):
-    """Return autosubmit form"""
-
-    def get(
-        self, request: HttpRequest, acs_url: str, saml_response: str, relay_state: str
-    ) -> HttpResponse:
-        """Return autosubmit form"""
-        return render(
-            request,
-            "core/autosubmit_form.html",
-            {
-                "url": acs_url,
-                "attrs": {"SAMLResponse": saml_response, "RelayState": relay_state},
-            },
-        )
-
-
 class LoginProcessView(AccessRequiredView):
     """Processor-based login continuation.
     Presents a SAML 2.0 Assertion for POSTing back to the Service Provider."""
 
+    def handle_redirect(
+        self, params: SAMLResponseParams, skipped_authorization: bool
+    ) -> HttpResponse:
+        """Handle direct redirect to SP"""
+        # Log Application Authorization
+        Event.new(
+            EventAction.AUTHORIZE_APPLICATION,
+            authorized_application=self.provider.application,
+            skipped_authorization=skipped_authorization,
+        ).from_http(self.request)
+        return render(
+            self.request,
+            "saml/idp/autosubmit_form.html",
+            {
+                "url": params.acs_url,
+                "attrs": {
+                    "SAMLResponse": params.saml_response,
+                    "RelayState": params.relay_state,
+                },
+            },
+        )
+
     # pylint: disable=unused-argument
     def get(self, request: HttpRequest, application: str) -> HttpResponse:
         """Handle get request, i.e. render form"""
         # User access gets checked in dispatch
-        if self.provider.application.skip_authorization:
-            ctx = self.provider.processor.generate_response()
-            # Log Application Authorization
-            Event.new(
-                EventAction.AUTHORIZE_APPLICATION,
-                authorized_application=self.provider.application,
-                skipped_authorization=True,
-            ).from_http(request)
-            return RedirectToSPView.as_view()(
-                request=request,
-                acs_url=ctx["acs_url"],
-                saml_response=ctx["saml_response"],
-                relay_state=ctx["relay_state"],
-            )
+
+        # Otherwise we generate the IdP initiated session
         try:
-            return _generate_response(request, self.provider)
+            # application.skip_authorization is set so we directly redirect the user
+            if self.provider.application.skip_authorization:
+                self.provider.processor.can_handle(request)
+                saml_params = self.provider.processor.generate_response()
+                return self.handle_redirect(saml_params, True)
+
+            self.provider.processor.init_deep_link(request)
+            params = self.provider.processor.generate_response()
+
+            return render(
+                request,
+                "saml/idp/login.html",
+                {
+                    "saml_params": params,
+                    "provider": self.provider,
+                    # This is only needed to for the template to render correctly
+                    "is_login": True,
+                },
+            )
+
         except exceptions.CannotHandleAssertion as exc:
-            LOGGER.debug(exc)
-        return HttpResponseBadRequest()
+            LOGGER.error(exc)
+            did_you_mean_link = request.build_absolute_uri(
+                reverse(
+                    "passbook_providers_saml:saml-login-initiate",
+                    kwargs={"application": application},
+                )
+            )
+            did_you_mean_message = (
+                f" Did you mean to go <a href='{did_you_mean_link}'>here</a>?"
+            )
+            return bad_request_message(
+                request, mark_safe(str(exc) + did_you_mean_message)
+            )
 
     # pylint: disable=unused-argument
     def post(self, request: HttpRequest, application: str) -> HttpResponse:
         """Handle post request, return back to ACS"""
         # User access gets checked in dispatch
-        if request.POST.get("ACSUrl", None):
-            # User accepted request
-            Event.new(
-                EventAction.AUTHORIZE_APPLICATION,
-                authorized_application=self.provider.application,
-                skipped_authorization=False,
-            ).from_http(request)
-            return RedirectToSPView.as_view()(
-                request=request,
-                acs_url=request.POST.get("ACSUrl"),
-                saml_response=request.POST.get("SAMLResponse"),
-                relay_state=request.POST.get("RelayState"),
-            )
-        try:
-            return _generate_response(request, self.provider)
-        except exceptions.CannotHandleAssertion as exc:
-            LOGGER.debug(exc)
-        return HttpResponseBadRequest()
+
+        # we get here when skip_authorization is False, and after the user accepted
+        # the authorization form
+        self.provider.processor.can_handle(request)
+        saml_params = self.provider.processor.generate_response()
+        return self.handle_redirect(saml_params, True)
 
 
 class LogoutView(CSRFExemptMixin, AccessRequiredView):
@@ -254,9 +254,46 @@ class DescriptorDownloadView(AccessRequiredView):
 class InitiateLoginView(AccessRequiredView):
     """IdP-initiated Login"""
 
+    def handle_redirect(
+        self, params: SAMLResponseParams, skipped_authorization: bool
+    ) -> HttpResponse:
+        """Handle direct redirect to SP"""
+        # Log Application Authorization
+        Event.new(
+            EventAction.AUTHORIZE_APPLICATION,
+            authorized_application=self.provider.application,
+            skipped_authorization=skipped_authorization,
+        ).from_http(self.request)
+        return render(
+            self.request,
+            "saml/idp/autosubmit_form.html",
+            {
+                "url": params.acs_url,
+                "attrs": {
+                    "SAMLResponse": params.saml_response,
+                    "RelayState": params.relay_state,
+                },
+            },
+        )
+
     # pylint: disable=unused-argument
     def get(self, request: HttpRequest, application: str) -> HttpResponse:
         """Initiates an IdP-initiated link to a simple SP resource/target URL."""
-        self.provider.processor.init_deep_link(request, "")
         self.provider.processor.is_idp_initiated = True
-        return _generate_response(request, self.provider)
+        self.provider.processor.init_deep_link(request)
+        params = self.provider.processor.generate_response()
+
+        # IdP-initiated Login Flow
+        if self.provider.application.skip_authorization:
+            return self.handle_redirect(params, True)
+
+        return render(
+            request,
+            "saml/idp/login.html",
+            {
+                "saml_params": params,
+                "provider": self.provider,
+                # This is only needed to for the template to render correctly
+                "is_login": True,
+            },
+        )
diff --git a/passbook/root/settings.py b/passbook/root/settings.py
index 39b066c19..d3222e3be 100644
--- a/passbook/root/settings.py
+++ b/passbook/root/settings.py
@@ -276,7 +276,7 @@ structlog.configure_once(
         structlog.stdlib.PositionalArgumentsFormatter(),
         structlog.processors.TimeStamper(),
         structlog.processors.StackInfoRenderer(),
-        # structlog.processors.format_exc_info,
+        structlog.processors.format_exc_info,
         structlog.stdlib.ProcessorFormatter.wrap_for_formatter,
     ],
     context_class=structlog.threadlocal.wrap_dict(dict),