From 610b6c7f708747b654195943d9a8f3ea1326666a Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 11 Oct 2020 19:26:20 +0200 Subject: [PATCH] policies: add PolicyAccessView, which does complete access checking --- passbook/policies/{mixins.py => views.py} | 57 ++++++++------- passbook/providers/oauth2/views/authorize.py | 32 +++------ passbook/providers/saml/views.py | 75 ++++++++++---------- swagger.yaml | 11 +-- 4 files changed, 85 insertions(+), 90 deletions(-) rename passbook/policies/{mixins.py => views.py} (60%) diff --git a/passbook/policies/mixins.py b/passbook/policies/views.py similarity index 60% rename from passbook/policies/mixins.py rename to passbook/policies/views.py index 8de3d1bba..a3f8480b2 100644 --- a/passbook/policies/mixins.py +++ b/passbook/policies/views.py @@ -1,11 +1,12 @@ """passbook access helper classes""" -from typing import Optional +from typing import Any, Optional from django.contrib import messages from django.contrib.auth.mixins import AccessMixin from django.contrib.auth.views import redirect_to_login from django.http import HttpRequest, HttpResponse from django.utils.translation import gettext as _ +from django.views.generic.base import View from structlog import get_logger from passbook.core.models import Application, Provider, User @@ -23,16 +24,40 @@ class BaseMixin: request: HttpRequest -class PolicyAccessMixin(BaseMixin, AccessMixin): +class PolicyAccessView(AccessMixin, View): """Mixin class for usage in Authorization views. Provider functions to check application access, etc""" - def handle_no_permission(self, application: Optional[Application] = None): + provider: Provider + application: Application + + def resolve_provider_application(self): + """Resolve self.provider and self.application. *.DoesNotExist Exceptions cause a normal + AccessDenied view to be shown. An Http404 exception + is not caught, and will return directly""" + raise NotImplementedError + + def dispatch(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse: + try: + self.resolve_provider_application() + except (Application.DoesNotExist, Provider.DoesNotExist): + return self.handle_no_permission_authenticated() + # Check if user is unauthenticated, so we pass the application + # for the identification stage + if not request.user.is_authenticated: + return self.handle_no_permission() + # Check permissions + result = self.user_has_access() + if not result.passing: + return self.handle_no_permission_authenticated(result) + return super().dispatch(request, *args, **kwargs) + + def handle_no_permission(self) -> HttpResponse: """User has no access and is not authenticated, so we remember the application they try to access and redirect to the login URL. The application is saved to show a hint on the Identification Stage what the user should login for.""" - if application: - self.request.session[SESSION_KEY_APPLICATION_PRE] = application + if self.application: + self.request.session[SESSION_KEY_APPLICATION_PRE] = self.application return redirect_to_login( self.request.get_full_path(), self.get_login_url(), @@ -48,34 +73,18 @@ class PolicyAccessMixin(BaseMixin, AccessMixin): response.policy_result = result return response - def provider_to_application(self, provider: Provider) -> Application: - """Lookup application assigned to provider, throw error if no application assigned""" - try: - return provider.application - except Application.DoesNotExist as exc: - messages.error( - self.request, - _( - 'Provider "%(name)s" has no application assigned' - % {"name": provider} - ), - ) - raise exc - - def user_has_access( - self, application: Application, user: Optional[User] = None - ) -> PolicyResult: + def user_has_access(self, user: Optional[User] = None) -> PolicyResult: """Check if user has access to application.""" user = user or self.request.user policy_engine = PolicyEngine( - application, user or self.request.user, self.request + self.application, user or self.request.user, self.request ) policy_engine.build() result = policy_engine.result LOGGER.debug( "AccessMixin user_has_access", user=user, - app=application, + app=self.application, result=result, ) if not result.passing: diff --git a/passbook/providers/oauth2/views/authorize.py b/passbook/providers/oauth2/views/authorize.py index 312864d4e..4a482985d 100644 --- a/passbook/providers/oauth2/views/authorize.py +++ b/passbook/providers/oauth2/views/authorize.py @@ -7,7 +7,6 @@ from uuid import uuid4 from django.http import HttpRequest, HttpResponse from django.shortcuts import get_object_or_404, redirect from django.utils import timezone -from django.views import View from structlog import get_logger from passbook.audit.models import Event, EventAction @@ -24,7 +23,7 @@ from passbook.flows.views import SESSION_KEY_PLAN from passbook.lib.utils.time import timedelta_from_string from passbook.lib.utils.urls import redirect_with_qs from passbook.lib.views import bad_request_message -from passbook.policies.mixins import PolicyAccessMixin +from passbook.policies.views import PolicyAccessView from passbook.providers.oauth2.constants import ( PROMPT_CONSNET, PROMPT_NONE, @@ -329,28 +328,17 @@ class OAuthFulfillmentStage(StageView): return urlunsplit(uri) -class AuthorizationFlowInitView(PolicyAccessMixin, View): +class AuthorizationFlowInitView(PolicyAccessView): """OAuth2 Flow initializer, checks access to application and starts flow""" + def resolve_provider_application(self): + client_id = self.request.GET.get("client_id") + self.provider = get_object_or_404(OAuth2Provider, client_id=client_id) + self.application = self.provider.application + # pylint: disable=unused-argument def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: """Check access to application, start FlowPLanner, return to flow executor shell""" - client_id = request.GET.get("client_id") - # TODO: This whole block should be moved to a base class - provider = get_object_or_404(OAuth2Provider, client_id=client_id) - try: - application = self.provider_to_application(provider) - except Application.DoesNotExist: - return self.handle_no_permission_authenticated() - # Check if user is unauthenticated, so we pass the application - # for the identification stage - if not request.user.is_authenticated: - return self.handle_no_permission(application) - # Check permissions - result = self.user_has_access(application) - if not result.passing: - return self.handle_no_permission_authenticated(result) - # TODO: End block # Extract params so we can save them in the plan context try: params = OAuthAuthorizationParams.from_request(request) @@ -358,14 +346,14 @@ class AuthorizationFlowInitView(PolicyAccessMixin, View): # pylint: disable=no-member return bad_request_message(request, error.description, title=error.error) # Regardless, we start the planner and return to it - planner = FlowPlanner(provider.authorization_flow) + planner = FlowPlanner(self.provider.authorization_flow) # planner.use_cache = False planner.allow_empty_flows = True plan: FlowPlan = planner.plan( self.request, { PLAN_CONTEXT_SSO: True, - PLAN_CONTEXT_APPLICATION: application, + PLAN_CONTEXT_APPLICATION: self.application, # OAuth2 related params PLAN_CONTEXT_PARAMS: params, PLAN_CONTEXT_SCOPE_DESCRIPTIONS: UserInfoView().get_scope_descriptions( @@ -390,5 +378,5 @@ class AuthorizationFlowInitView(PolicyAccessMixin, View): return redirect_with_qs( "passbook_flows:flow-executor-shell", self.request.GET, - flow_slug=provider.authorization_flow.slug, + flow_slug=self.provider.authorization_flow.slug, ) diff --git a/passbook/providers/saml/views.py b/passbook/providers/saml/views.py index 37450b623..9f7ae4658 100644 --- a/passbook/providers/saml/views.py +++ b/passbook/providers/saml/views.py @@ -23,7 +23,7 @@ from passbook.flows.stage import StageView from passbook.flows.views import SESSION_KEY_PLAN from passbook.lib.utils.urls import redirect_with_qs from passbook.lib.views import bad_request_message -from passbook.policies.mixins import PolicyAccessMixin +from passbook.policies.views import PolicyAccessView from passbook.providers.saml.exceptions import CannotHandleAssertion from passbook.providers.saml.models import SAMLBindings, SAMLProvider from passbook.providers.saml.processors.assertion import AssertionProcessor @@ -46,34 +46,35 @@ REQUEST_KEY_RELAY_STATE = "RelayState" SESSION_KEY_AUTH_N_REQUEST = "authn_request" -class SAMLSSOView(PolicyAccessMixin, View): +class SAMLSSOView(PolicyAccessView): """ "SAML SSO Base View, which plans a flow and injects our final stage. Calls get/post handler.""" - application: Application - provider: SAMLProvider - - def dispatch( - self, request: HttpRequest, *args, application_slug: str, **kwargs - ) -> HttpResponse: - self.application = get_object_or_404(Application, slug=application_slug) + def resolve_provider_application(self): + self.application = get_object_or_404( + Application, slug=self.kwargs["application_slug"] + ) self.provider: SAMLProvider = get_object_or_404( SAMLProvider, pk=self.application.provider_id ) - if not request.user.is_authenticated: - return self.handle_no_permission(self.application) - has_access = self.user_has_access(self.application) - if not has_access.passing: - return self.handle_no_permission_authenticated(has_access) - # Call the method handler, which checks the SAML Request - method_response = super().dispatch(request, *args, application_slug, **kwargs) + + def check_saml_request(self) -> Optional[HttpRequest]: + """Handler to verify the SAML Request. Must be implemented by a subclass""" + raise NotImplementedError + + # pylint: disable=unused-argument + def get(self, request: HttpRequest, application_slug: str) -> HttpResponse: + """Verify the SAML Request, and if valid initiate the FlowPlanner for the application""" + # Call the method handler, which checks the SAML + # Request and returns a HTTP Response on error + method_response = self.check_saml_request() if method_response: return method_response # Regardless, we start the planner and return to it planner = FlowPlanner(self.provider.authorization_flow) planner.allow_empty_flows = True plan = planner.plan( - self.request, + request, { PLAN_CONTEXT_SSO: True, PLAN_CONTEXT_APPLICATION: self.application, @@ -81,23 +82,25 @@ class SAMLSSOView(PolicyAccessMixin, View): }, ) plan.append(in_memory_stage(SAMLFlowFinalView)) - self.request.session[SESSION_KEY_PLAN] = plan + request.session[SESSION_KEY_PLAN] = plan return redirect_with_qs( "passbook_flows:flow-executor-shell", - self.request.GET, + request.GET, flow_slug=self.provider.authorization_flow.slug, ) + def post(self, request: HttpRequest, application_slug: str) -> HttpResponse: + """GET and POST use the same handler, but we can't + override .dispatch easily because PolicyAccessView's dispatch""" + return self.get(request, application_slug) + class SAMLSSOBindingRedirectView(SAMLSSOView): """SAML Handler for SSO/Redirect bindings, which are sent via GET""" - # pylint: disable=unused-argument - def get( # lgtm [py/similar-function] - self, request: HttpRequest, application_slug: str - ) -> Optional[HttpResponse]: + def check_saml_request(self) -> Optional[HttpRequest]: """Handle REDIRECT bindings""" - if REQUEST_KEY_SAML_REQUEST not in request.GET: + if REQUEST_KEY_SAML_REQUEST not in self.request.GET: LOGGER.info("handle_saml_request: SAML payload missing") return bad_request_message( self.request, "The SAML request payload is missing." @@ -105,10 +108,10 @@ class SAMLSSOBindingRedirectView(SAMLSSOView): try: auth_n_request = AuthNRequestParser(self.provider).parse_detached( - request.GET[REQUEST_KEY_SAML_REQUEST], - request.GET.get(REQUEST_KEY_RELAY_STATE), - request.GET.get(REQUEST_KEY_SAML_SIGNATURE), - request.GET.get(REQUEST_KEY_SAML_SIG_ALG), + self.request.GET[REQUEST_KEY_SAML_REQUEST], + self.request.GET.get(REQUEST_KEY_RELAY_STATE), + self.request.GET.get(REQUEST_KEY_SAML_SIGNATURE), + self.request.GET.get(REQUEST_KEY_SAML_SIG_ALG), ) self.request.session[SESSION_KEY_AUTH_N_REQUEST] = auth_n_request except CannotHandleAssertion as exc: @@ -121,12 +124,9 @@ class SAMLSSOBindingRedirectView(SAMLSSOView): class SAMLSSOBindingPOSTView(SAMLSSOView): """SAML Handler for SSO/POST bindings""" - # pylint: disable=unused-argument - def post( - self, request: HttpRequest, application_slug: str - ) -> Optional[HttpResponse]: + def check_saml_request(self) -> Optional[HttpRequest]: """Handle POST bindings""" - if REQUEST_KEY_SAML_REQUEST not in request.POST: + if REQUEST_KEY_SAML_REQUEST not in self.request.POST: LOGGER.info("handle_saml_request: SAML payload missing") return bad_request_message( self.request, "The SAML request payload is missing." @@ -134,8 +134,8 @@ class SAMLSSOBindingPOSTView(SAMLSSOView): try: auth_n_request = AuthNRequestParser(self.provider).parse( - request.POST[REQUEST_KEY_SAML_REQUEST], - request.POST.get(REQUEST_KEY_RELAY_STATE), + self.request.POST[REQUEST_KEY_SAML_REQUEST], + self.request.POST.get(REQUEST_KEY_RELAY_STATE), ) self.request.session[SESSION_KEY_AUTH_N_REQUEST] = auth_n_request except CannotHandleAssertion as exc: @@ -147,10 +147,7 @@ class SAMLSSOBindingPOSTView(SAMLSSOView): class SAMLSSOBindingInitView(SAMLSSOView): """SAML Handler for for IdP Initiated login flows""" - # pylint: disable=unused-argument - def get( - self, request: HttpRequest, application_slug: str - ) -> Optional[HttpResponse]: + def check_saml_request(self) -> Optional[HttpRequest]: """Create SAML Response from scratch""" LOGGER.debug( "handle_saml_no_request: No SAML Request, using IdP-initiated flow." diff --git a/swagger.yaml b/swagger.yaml index 6bfd6a28c..66f0eafd0 100755 --- a/swagger.yaml +++ b/swagger.yaml @@ -122,7 +122,7 @@ paths: /core/applications/: get: operationId: core_applications_list - description: Application Viewset + description: Custom list method that checks Policy based access instead of guardian parameters: - name: ordering in: query @@ -186,7 +186,7 @@ paths: tags: - core parameters: [] - /core/applications/{pbm_uuid}/: + /core/applications/{slug}/: get: operationId: core_applications_read description: Application Viewset @@ -240,12 +240,13 @@ paths: tags: - core parameters: - - name: pbm_uuid + - name: slug in: path - description: A UUID string identifying this Application. + description: Internal application name, used in URLs. required: true type: string - format: uuid + format: slug + pattern: ^[-a-zA-Z0-9_]+$ /core/groups/: get: operationId: core_groups_list