From 22838e66fe20b264dac3e14cdc037fc26739a213 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Mon, 24 Feb 2020 14:40:12 +0100 Subject: [PATCH] providers/saml: fix users being able to authenticate without audit logs being created --- passbook/core/forms/applications.py | 3 +- passbook/core/views/authentication.py | 3 +- passbook/lib/sentry.py | 2 +- passbook/providers/saml/urls.py | 6 +-- passbook/providers/saml/views.py | 55 ++++++--------------------- 5 files changed, 18 insertions(+), 51 deletions(-) diff --git a/passbook/core/forms/applications.py b/passbook/core/forms/applications.py index 8ea1b9d58..71df296e1 100644 --- a/passbook/core/forms/applications.py +++ b/passbook/core/forms/applications.py @@ -10,7 +10,8 @@ class ApplicationForm(forms.ModelForm): """Application Form""" provider = forms.ModelChoiceField( - queryset=Provider.objects.all().order_by('pk').select_subclasses(), required=False + queryset=Provider.objects.all().order_by("pk").select_subclasses(), + required=False, ) class Meta: diff --git a/passbook/core/views/authentication.py b/passbook/core/views/authentication.py index 523cce48b..4749beeda 100644 --- a/passbook/core/views/authentication.py +++ b/passbook/core/views/authentication.py @@ -231,7 +231,6 @@ class PasswordResetView(View): login(request, nonce.user) nonce.delete() messages.success( - request, - _(("Temporarily authenticated with Nonce, " "please change your password")), + request, _(("Temporarily authenticated, please change your password")), ) return redirect("passbook_core:user-change-password") diff --git a/passbook/lib/sentry.py b/passbook/lib/sentry.py index f50b612aa..d803ef729 100644 --- a/passbook/lib/sentry.py +++ b/passbook/lib/sentry.py @@ -12,7 +12,7 @@ LOGGER = get_logger() class SentryIgnoredException(Exception): - """Base Class for all errors that are supressed, and not sent to sentry.""" + """Base Class for all errors that are suppressed, and not sent to sentry.""" def before_send(event, hint): diff --git a/passbook/providers/saml/urls.py b/passbook/providers/saml/urls.py index bddf07571..5fe3b87dc 100644 --- a/passbook/providers/saml/urls.py +++ b/passbook/providers/saml/urls.py @@ -16,9 +16,9 @@ urlpatterns = [ "/login/", views.LoginBeginView.as_view(), name="saml-login" ), path( - "/login/process/", - views.LoginProcessView.as_view(), - name="saml-login-process", + "/login/authorize/", + views.AuthorizeView.as_view(), + name="saml-login-authorize", ), path("/logout/", views.LogoutView.as_view(), name="saml-logout"), path( diff --git a/passbook/providers/saml/views.py b/passbook/providers/saml/views.py index 871e88b9e..653c9f9de 100644 --- a/passbook/providers/saml/views.py +++ b/passbook/providers/saml/views.py @@ -91,14 +91,14 @@ class LoginBeginView(AccessRequiredView): request.session["RelayState"] = source.get("RelayState", "") return redirect( reverse( - "passbook_providers_saml:saml-login-process", + "passbook_providers_saml:saml-login-authorize", kwargs={"application": application}, ) ) -class LoginProcessView(AccessRequiredView): - """Processor-based login continuation. +class AuthorizeView(AccessRequiredView): + """Ask the user for authorization to continue to the SP. Presents a SAML 2.0 Assertion for POSTing back to the Service Provider.""" def handle_redirect( @@ -131,9 +131,10 @@ class LoginProcessView(AccessRequiredView): try: # application.skip_authorization is set so we directly redirect the user if self.provider.application.skip_authorization: + LOGGER.debug("skipping authz", application=self.provider.application) return self.post(request, application) - self.provider.processor.init_deep_link(request) + self.provider.processor.can_handle(request) params = self.provider.processor.generate_response() return render( @@ -166,7 +167,7 @@ class LoginProcessView(AccessRequiredView): """Handle post request, return back to ACS""" # User access gets checked in dispatch - # we get here when skip_authorization is False, and after the user accepted + # we get here when skip_authorization is True, and after the user accepted # the authorization form self.provider.processor.can_handle(request) saml_params = self.provider.processor.generate_response() @@ -268,45 +269,11 @@ 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.is_idp_initiated = True - 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, - "title": "Authorize Application", - }, + return redirect( + reverse( + "passbook_providers_saml:saml-login-authorize", + kwargs={"application": application}, + ) )