From 5b8b9733450531b346e9932eb122ae67457ea696 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Tue, 23 Mar 2021 22:18:24 +0100 Subject: [PATCH] flows: revert to sever-side redirects for security, pass querystring from client during flow plan Signed-off-by: Jens Langhammer --- authentik/flows/views.py | 17 +++++++++++- authentik/stages/email/apps.py | 1 - authentik/stages/email/stage.py | 6 ++--- authentik/stages/email/tests/test_stage.py | 12 +++------ authentik/stages/email/urls.py | 8 ------ authentik/stages/email/views.py | 31 ---------------------- swagger.yaml | 7 ++++- web/src/flows/FlowExecutor.ts | 25 +++-------------- web/src/flows/utils.ts | 10 ------- 9 files changed, 32 insertions(+), 85 deletions(-) delete mode 100644 authentik/stages/email/urls.py delete mode 100644 authentik/stages/email/views.py delete mode 100644 web/src/flows/utils.ts diff --git a/authentik/flows/views.py b/authentik/flows/views.py index 5cd4370dc..cdda12c4d 100644 --- a/authentik/flows/views.py +++ b/authentik/flows/views.py @@ -4,11 +4,13 @@ from typing import Any, Optional from django.contrib.auth.mixins import LoginRequiredMixin from django.http import Http404, HttpRequest, HttpResponse, HttpResponseRedirect +from django.http.request import QueryDict from django.shortcuts import get_object_or_404, redirect from django.template.response import TemplateResponse from django.utils.decorators import method_decorator from django.views.decorators.clickjacking import xframe_options_sameorigin from django.views.generic import View +from drf_yasg2 import openapi from drf_yasg2.utils import no_body, swagger_auto_schema from rest_framework.permissions import AllowAny from rest_framework.views import APIView @@ -101,6 +103,8 @@ class FlowExecutorView(APIView): # To match behaviour with loading an empty flow plan from cache, # we don't show an error message here, but rather call _flow_done() return self._flow_done() + # Initial flow request, check if we have an upstream query string passed in + request.session[SESSION_KEY_GET] = QueryDict(request.GET.get("query", "")) # We don't save the Plan after getting the next stage # as it hasn't been successfully passed yet next_stage = self.plan.next(self.request) @@ -121,8 +125,19 @@ class FlowExecutorView(APIView): return super().dispatch(request) @swagger_auto_schema( - responses={200: Challenge()}, + responses={ + 200: Challenge(), + }, request_body=no_body, + manual_parameters=[ + openapi.Parameter( + "query", + openapi.IN_QUERY, + required=True, + description="Querystring as received", + type=openapi.TYPE_STRING, + ) + ], operation_id="flows_executor_get", ) def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: diff --git a/authentik/stages/email/apps.py b/authentik/stages/email/apps.py index 68847e1b8..499cc600e 100644 --- a/authentik/stages/email/apps.py +++ b/authentik/stages/email/apps.py @@ -16,7 +16,6 @@ class AuthentikStageEmailConfig(AppConfig): name = "authentik.stages.email" label = "authentik_stages_email" verbose_name = "authentik Stages.Email" - mountpoint = "stages/email/" def ready(self): import_module("authentik.stages.email.tasks") diff --git a/authentik/stages/email/stage.py b/authentik/stages/email/stage.py index 3724a1e49..162e14ee1 100644 --- a/authentik/stages/email/stage.py +++ b/authentik/stages/email/stage.py @@ -45,7 +45,7 @@ class EmailStageView(ChallengeStageView): def get_full_url(self, **kwargs) -> str: """Get full URL to be used in template""" base_url = reverse( - "authentik_stages_email:from-email", + "authentik_core:if-flow", kwargs={"flow_slug": self.executor.flow.slug}, ) relative_url = f"{base_url}?{urlencode(kwargs)}" @@ -66,7 +66,7 @@ class EmailStageView(ChallengeStageView): template_name=current_stage.template, to=[pending_user.email], template_context={ - "url": self.get_full_url(**{QS_KEY_TOKEN: token.pk.hex}), + "url": self.get_full_url(**{QS_KEY_TOKEN: token.key}), "user": pending_user, "expires": token.expires, }, @@ -77,7 +77,7 @@ class EmailStageView(ChallengeStageView): # Check if the user came back from the email link to verify if QS_KEY_TOKEN in request.session.get(SESSION_KEY_GET, {}): token = get_object_or_404( - Token, pk=request.session[SESSION_KEY_GET][QS_KEY_TOKEN] + Token, key=request.session[SESSION_KEY_GET][QS_KEY_TOKEN] ) self.executor.plan.context[PLAN_CONTEXT_PENDING_USER] = token.user token.delete() diff --git a/authentik/stages/email/tests/test_stage.py b/authentik/stages/email/tests/test_stage.py index a071736f8..42c998abc 100644 --- a/authentik/stages/email/tests/test_stage.py +++ b/authentik/stages/email/tests/test_stage.py @@ -10,7 +10,7 @@ from authentik.core.models import Token, User from authentik.flows.markers import StageMarker from authentik.flows.models import Flow, FlowDesignation, FlowStageBinding from authentik.flows.planner import PLAN_CONTEXT_PENDING_USER, FlowPlan -from authentik.flows.views import SESSION_KEY_PLAN +from authentik.flows.views import SESSION_KEY_GET, SESSION_KEY_PLAN from authentik.stages.email.models import EmailStage from authentik.stages.email.stage import QS_KEY_TOKEN @@ -104,17 +104,11 @@ class TestEmailStage(TestCase): ) session = self.client.session session[SESSION_KEY_PLAN] = plan + token: Token = Token.objects.get(user=self.user) + session[SESSION_KEY_GET] = {QS_KEY_TOKEN: token.key} session.save() with patch("authentik.flows.views.FlowExecutorView.cancel", MagicMock()): - # Call the executor shell to preseed the session - url = reverse( - "authentik_stages_email:from-email", - kwargs={"flow_slug": self.flow.slug}, - ) - token = Token.objects.get(user=self.user) - url += f"?{QS_KEY_TOKEN}={token.pk.hex}" - self.client.get(url) # Call the actual executor to get the JSON Response response = self.client.get( reverse( diff --git a/authentik/stages/email/urls.py b/authentik/stages/email/urls.py deleted file mode 100644 index 9c31d2a6b..000000000 --- a/authentik/stages/email/urls.py +++ /dev/null @@ -1,8 +0,0 @@ -"""Email stage url patterns""" -from django.urls import path - -from authentik.stages.email.views import FromEmailView - -urlpatterns = [ - path("from-email//", FromEmailView.as_view(), name="from-email"), -] diff --git a/authentik/stages/email/views.py b/authentik/stages/email/views.py deleted file mode 100644 index 5b533da5b..000000000 --- a/authentik/stages/email/views.py +++ /dev/null @@ -1,31 +0,0 @@ -"""Email stage views""" -from django.http.request import HttpRequest -from django.http.response import HttpResponse, HttpResponseBadRequest -from django.shortcuts import get_object_or_404, redirect -from django.views import View -from structlog.stdlib import get_logger - -from authentik.core.models import Token -from authentik.flows.views import SESSION_KEY_GET -from authentik.stages.email.stage import QS_KEY_TOKEN - -LOGGER = get_logger() - - -class FromEmailView(View): - """FromEmailView, this view is linked in the email confirmation link. - It is required because the flow executor does not pass query args to the API, - so this view gets called, checks for a Querystring and updates the plan - if everything is valid.""" - - def get(self, request: HttpRequest, flow_slug: str) -> HttpResponse: - """Check for ?token param and validate it.""" - if QS_KEY_TOKEN not in request.GET: - LOGGER.debug("No token set") - return HttpResponseBadRequest() - # Lookup token here to quickly fail for invalid input - get_object_or_404(Token, pk=request.GET[QS_KEY_TOKEN]) - if SESSION_KEY_GET not in request.session: - request.session[SESSION_KEY_GET] = {} - request.session[SESSION_KEY_GET][QS_KEY_TOKEN] = request.GET[QS_KEY_TOKEN] - return redirect("authentik_core:if-flow", flow_slug=flow_slug) diff --git a/swagger.yaml b/swagger.yaml index 493b34f4a..f79e6acf7 100755 --- a/swagger.yaml +++ b/swagger.yaml @@ -2744,7 +2744,12 @@ paths: get: operationId: flows_executor_get description: Get the next pending challenge from the currently active flow. - parameters: [] + parameters: + - name: query + in: query + description: Querystring as received + required: true + type: string responses: '200': description: Challenge that gets sent to the client based on which stage diff --git a/web/src/flows/FlowExecutor.ts b/web/src/flows/FlowExecutor.ts index 6def9c535..3d291d5da 100644 --- a/web/src/flows/FlowExecutor.ts +++ b/web/src/flows/FlowExecutor.ts @@ -40,11 +40,8 @@ import { ifDefined } from "lit-html/directives/if-defined"; import { until } from "lit-html/directives/until"; import { TITLE_SUFFIX } from "../elements/router/RouterOutlet"; import { AccessDeniedChallenge } from "./access_denied/FlowAccessDenied"; -import { getQueryVariables } from "./utils"; import { SpinnerSize } from "../elements/Spinner"; -export const NEXT_ARG = "next"; - @customElement("ak-flow-executor") export class FlowExecutor extends LitElement implements StageHost { @@ -129,7 +126,8 @@ export class FlowExecutor extends LitElement implements StageHost { }); this.loading = true; new FlowsApi(DEFAULT_CONFIG).flowsExecutorGetRaw({ - flowSlug: this.flowSlug + flowSlug: this.flowSlug, + query: window.location.search.substring(1), }).then((challengeRaw) => { return challengeRaw.raw.json(); }).then((challenge) => { @@ -168,29 +166,14 @@ export class FlowExecutor extends LitElement implements StageHost { `; } - private redirect(challenge: RedirectChallenge): void { - // Check if there is a ?next arg and save it - // this is used for deep linking, if a user tries to access an application, - // but needs to authenticate first - const queryVars = getQueryVariables(); - localStorage.clear(); - if (NEXT_ARG in queryVars) { - const next = queryVars[NEXT_ARG]; - console.debug("authentik/flows: redirecting to saved url", next); - window.location.assign(next); - return; - } - console.debug("authentik/flows: redirecting to url from server", challenge.to); - window.location.assign(challenge.to); - } - renderChallenge(): TemplateResult { if (!this.challenge) { return html``; } switch (this.challenge.type) { case ChallengeTypeEnum.Redirect: - this.redirect(this.challenge as RedirectChallenge); + console.debug("authentik/flows: redirecting to url from server", (this.challenge as RedirectChallenge).to); + window.location.assign((this.challenge as RedirectChallenge).to); return html` diff --git a/web/src/flows/utils.ts b/web/src/flows/utils.ts deleted file mode 100644 index 360560a45..000000000 --- a/web/src/flows/utils.ts +++ /dev/null @@ -1,10 +0,0 @@ -export function getQueryVariables(): Record { - const query = window.location.search.substring(1); - const vars = query.split("&"); - const entries: Record = {}; - for (let i = 0; i < vars.length; i++) { - const pair = vars[i].split("="); - entries[decodeURIComponent(pair[0])] = decodeURIComponent(pair[1]); - } - return entries; -}