From 2fa6cf855d2e88b22685958b64d4867d3b2fdf38 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Sun, 7 Aug 2022 13:42:23 +0200 Subject: [PATCH] stages/consent: simplify logic, correctly update existing consent Signed-off-by: Jens Langhammer --- authentik/flows/challenge.py | 2 +- authentik/stages/consent/stage.py | 41 ++++++++++++-------- authentik/stages/consent/tests.py | 19 +++++++-- web/src/flows/stages/consent/ConsentStage.ts | 2 +- 4 files changed, 41 insertions(+), 23 deletions(-) diff --git a/authentik/flows/challenge.py b/authentik/flows/challenge.py index 225da0a39..a28150f63 100644 --- a/authentik/flows/challenge.py +++ b/authentik/flows/challenge.py @@ -107,7 +107,7 @@ class PermissionDict(TypedDict): class PermissionSerializer(PassiveSerializer): """Permission used for consent""" - name = CharField() + name = CharField(allow_blank=True) id = CharField() diff --git a/authentik/stages/consent/stage.py b/authentik/stages/consent/stage.py index e91e7c8bf..c207a8a9c 100644 --- a/authentik/stages/consent/stage.py +++ b/authentik/stages/consent/stage.py @@ -18,6 +18,7 @@ from authentik.flows.stage import ChallengeStageView from authentik.lib.utils.time import timedelta_from_string from authentik.stages.consent.models import ConsentMode, ConsentStage, UserConsent +PLAN_CONTEXT_CONSENT = "consent" PLAN_CONTEXT_CONSENT_TITLE = "consent_title" PLAN_CONTEXT_CONSENT_HEADER = "consent_header" PLAN_CONTEXT_CONSENT_PERMISSIONS = "consent_permissions" @@ -65,21 +66,28 @@ class ConsentStageView(ChallengeStageView): challenge = ConsentChallenge(data=data) return challenge - def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: + def should_always_prompt(self) -> bool: + """Check if the current request should require a prompt for non consent reasons, + i.e. this stage injected from another stage, mode is always requireed or no application + is set.""" current_stage: ConsentStage = self.executor.current_stage # Make this StageView work when injected, in which case `current_stage` is an instance # of the base class, and we don't save any consent, as it is assumed to be a one-time # prompt if not isinstance(current_stage, ConsentStage): - return super().get(request, *args, **kwargs) + return True # For always require, we always return the challenge if current_stage.mode == ConsentMode.ALWAYS_REQUIRE: - return super().get(request, *args, **kwargs) + return True # at this point we need to check consent from database if PLAN_CONTEXT_APPLICATION not in self.executor.plan.context: # No application in this plan, hence we can't check DB and require user consent - return super().get(request, *args, **kwargs) + return True + return None + def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: + if self.should_always_prompt(): + return super().get(request, *args, **kwargs) application = self.executor.plan.context[PLAN_CONTEXT_APPLICATION] user = self.request.user @@ -89,6 +97,7 @@ class ConsentStageView(ChallengeStageView): consent: Optional[UserConsent] = UserConsent.filter_not_expired( user=user, application=application ).first() + self.executor.plan.context[PLAN_CONTEXT_CONSENT] = consent if consent: perms = self.executor.plan.context.get(PLAN_CONTEXT_CONSENT_PERMISSIONS, []) @@ -110,26 +119,24 @@ class ConsentStageView(ChallengeStageView): def challenge_valid(self, response: ChallengeResponse) -> HttpResponse: if response.data["token"] != self.request.session[SESSION_KEY_CONSENT_TOKEN]: + self.logger.info("Invalid consent token, re-showing prompt") return self.get(self.request) - current_stage: ConsentStage = self.executor.current_stage - if PLAN_CONTEXT_APPLICATION not in self.executor.plan.context: + if self.should_always_prompt(): return self.executor.stage_ok() + current_stage: ConsentStage = self.executor.current_stage application = self.executor.plan.context[PLAN_CONTEXT_APPLICATION] permissions = self.executor.plan.context.get( PLAN_CONTEXT_CONSENT_PERMISSIONS, [] ) + self.executor.plan.context.get(PLAN_CONTEXT_CONSENT_EXTRA_PERMISSIONS, []) permissions_string = " ".join(x["id"] for x in permissions) - # Make this StageView work when injected, in which case `current_stage` is an instance - # of the base class, and we don't save any consent, as it is assumed to be a one-time - # prompt - if not isinstance(current_stage, ConsentStage): - return self.executor.stage_ok() - # Since we only get here when no consent exists, we can create it without update - consent = UserConsent( - user=self.request.user, - application=application, - permissions=permissions_string, - ) + + if not self.executor.plan.context.get(PLAN_CONTEXT_CONSENT, None): + self.executor.plan.context[PLAN_CONTEXT_CONSENT] = UserConsent( + user=self.request.user, + application=application, + ) + consent: UserConsent = self.executor.plan.context[PLAN_CONTEXT_CONSENT] + consent.permissions = permissions_string if current_stage.mode == ConsentMode.PERMANENT: consent.expiring = False if current_stage.mode == ConsentMode.EXPIRING: diff --git a/authentik/stages/consent/tests.py b/authentik/stages/consent/tests.py index f15d2372c..35b508a5a 100644 --- a/authentik/stages/consent/tests.py +++ b/authentik/stages/consent/tests.py @@ -1,6 +1,5 @@ """consent tests""" from time import sleep -from uuid import uuid4 from django.urls import reverse @@ -41,8 +40,13 @@ class TestConsentStage(FlowTestCase): plan = FlowPlan(flow_pk=flow.pk.hex, bindings=[binding], markers=[StageMarker()]) session = self.client.session session[SESSION_KEY_PLAN] = plan - session[SESSION_KEY_CONSENT_TOKEN] = str(uuid4()) session.save() + response = self.client.get( + reverse("authentik_api:flow-executor", kwargs={"flow_slug": flow.slug}), + ) + self.assertEqual(response.status_code, 200) + + session = self.client.session response = self.client.post( reverse("authentik_api:flow-executor", kwargs={"flow_slug": flow.slug}), { @@ -65,12 +69,19 @@ class TestConsentStage(FlowTestCase): flow_pk=flow.pk.hex, bindings=[binding], markers=[StageMarker()], - context={PLAN_CONTEXT_APPLICATION: self.application}, + context={ + PLAN_CONTEXT_APPLICATION: self.application, + }, ) session = self.client.session session[SESSION_KEY_PLAN] = plan - session[SESSION_KEY_CONSENT_TOKEN] = str(uuid4()) session.save() + response = self.client.get( + reverse("authentik_api:flow-executor", kwargs={"flow_slug": flow.slug}), + ) + self.assertEqual(response.status_code, 200) + + session = self.client.session response = self.client.post( reverse("authentik_api:flow-executor", kwargs={"flow_slug": flow.slug}), { diff --git a/web/src/flows/stages/consent/ConsentStage.ts b/web/src/flows/stages/consent/ConsentStage.ts index d9e9b6b44..7352bdfed 100644 --- a/web/src/flows/stages/consent/ConsentStage.ts +++ b/web/src/flows/stages/consent/ConsentStage.ts @@ -86,7 +86,7 @@ export class ConsentStage extends BaseStage ${this.challenge.additionalPermissions.map((permission) => { return html`
  • - ${permission.name} + ${permission.name === "" ? permission.id : permission.name}
    • `; })}