From 70075e6f0a47bf81f21888500527e2fdaf93fb94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simonyi=20Gerg=C5=91?= <28359278+gergosimonyi@users.noreply.github.com> Date: Thu, 24 Oct 2024 09:04:40 +0200 Subject: [PATCH] stages/authenticator_validate: autoselect last used 2fa device (#11087) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * authenticator_validate: autoselect last used device class * improve usability of `AuthenticatorValidationStage` * don't automatically offer the recovery key authenticator validation I believe this could confuse users more than help them * web: move mutator block into the `willUpdate` override Removed the section of code from the renderer that updates the state of the component; Mutating in the middle of a render is strongly discouraged. This block contains an algorithm for determining if the selectedDeviceChallenge should be set and how; since `selectedDeviceChallenge` is a state, we don't want to be changing it outside of those lifecycle methods that do not trigger a rerender. * web: move styles() to top of class, extract custom CSS to a named block. * lint: collapse multiple early returns, missing curly brace. * autoselect device only once even if the user only has 1 device * make `DeviceChallenge.last_used` nullable instead of optional * clarify button text * fix typo * add docs for automatic device selection * update docs Co-authored-by: Tana M Berry Signed-off-by: Simonyi Gergő <28359278+gergosimonyi@users.noreply.github.com> * fix punctuation --------- Signed-off-by: Simonyi Gergő <28359278+gergosimonyi@users.noreply.github.com> Co-authored-by: Ken Sternberg Co-authored-by: Tana M Berry --- .../authenticator_validate/challenge.py | 3 +- .../stages/authenticator_validate/stage.py | 2 + .../authenticator_validate/tests/test_sms.py | 1 + .../tests/test_stage.py | 2 + .../tests/test_webauthn.py | 3 + schema.yml | 10 ++ .../AuthenticatorValidateStage.ts | 170 ++++++++++-------- .../AuthenticatorValidateStageCode.ts | 43 +++-- .../stages/authenticator_validate/base.ts | 2 +- .../stages/authenticator_validate/index.md | 10 +- 10 files changed, 155 insertions(+), 91 deletions(-) diff --git a/authentik/stages/authenticator_validate/challenge.py b/authentik/stages/authenticator_validate/challenge.py index c11439684f..1f9a656a38 100644 --- a/authentik/stages/authenticator_validate/challenge.py +++ b/authentik/stages/authenticator_validate/challenge.py @@ -8,7 +8,7 @@ from django.http.response import Http404 from django.shortcuts import get_object_or_404 from django.utils.translation import gettext as __ from django.utils.translation import gettext_lazy as _ -from rest_framework.fields import CharField +from rest_framework.fields import CharField, DateTimeField from rest_framework.serializers import ValidationError from structlog.stdlib import get_logger from webauthn import options_to_json @@ -45,6 +45,7 @@ class DeviceChallenge(PassiveSerializer): device_class = CharField() device_uid = CharField() challenge = JSONDictField() + last_used = DateTimeField(allow_null=True) def get_challenge_for_device( diff --git a/authentik/stages/authenticator_validate/stage.py b/authentik/stages/authenticator_validate/stage.py index 96ae7e6215..bde76d37db 100644 --- a/authentik/stages/authenticator_validate/stage.py +++ b/authentik/stages/authenticator_validate/stage.py @@ -217,6 +217,7 @@ class AuthenticatorValidateStageView(ChallengeStageView): "device_class": device_class, "device_uid": device.pk, "challenge": get_challenge_for_device(self.request, stage, device), + "last_used": device.last_used, } ) challenge.is_valid() @@ -237,6 +238,7 @@ class AuthenticatorValidateStageView(ChallengeStageView): self.request, self.executor.current_stage, ), + "last_used": None, } ) challenge.is_valid() diff --git a/authentik/stages/authenticator_validate/tests/test_sms.py b/authentik/stages/authenticator_validate/tests/test_sms.py index 5cce796207..850854a892 100644 --- a/authentik/stages/authenticator_validate/tests/test_sms.py +++ b/authentik/stages/authenticator_validate/tests/test_sms.py @@ -107,6 +107,7 @@ class AuthenticatorValidateStageSMSTests(FlowTestCase): "device_class": "sms", "device_uid": str(device.pk), "challenge": {}, + "last_used": None, }, }, ) diff --git a/authentik/stages/authenticator_validate/tests/test_stage.py b/authentik/stages/authenticator_validate/tests/test_stage.py index 98fe5d2fe4..82a2f51322 100644 --- a/authentik/stages/authenticator_validate/tests/test_stage.py +++ b/authentik/stages/authenticator_validate/tests/test_stage.py @@ -169,6 +169,7 @@ class AuthenticatorValidateStageTests(FlowTestCase): "device_class": "baz", "device_uid": "quox", "challenge": {}, + "last_used": None, } }, ) @@ -188,6 +189,7 @@ class AuthenticatorValidateStageTests(FlowTestCase): "device_class": "static", "device_uid": "1", "challenge": {}, + "last_used": None, }, }, ) diff --git a/authentik/stages/authenticator_validate/tests/test_webauthn.py b/authentik/stages/authenticator_validate/tests/test_webauthn.py index e4247b221a..05fe216081 100644 --- a/authentik/stages/authenticator_validate/tests/test_webauthn.py +++ b/authentik/stages/authenticator_validate/tests/test_webauthn.py @@ -274,6 +274,7 @@ class AuthenticatorValidateStageWebAuthnTests(FlowTestCase): "device_class": device.__class__.__name__.lower().replace("device", ""), "device_uid": device.pk, "challenge": {}, + "last_used": None, } ] session[SESSION_KEY_PLAN] = plan @@ -352,6 +353,7 @@ class AuthenticatorValidateStageWebAuthnTests(FlowTestCase): "device_class": device.__class__.__name__.lower().replace("device", ""), "device_uid": device.pk, "challenge": {}, + "last_used": None, } ] session[SESSION_KEY_PLAN] = plan @@ -432,6 +434,7 @@ class AuthenticatorValidateStageWebAuthnTests(FlowTestCase): "device_class": device.__class__.__name__.lower().replace("device", ""), "device_uid": device.pk, "challenge": {}, + "last_used": None, } ] session[SESSION_KEY_PLAN] = plan diff --git a/schema.yml b/schema.yml index d4f3eb78ac..f0c8447abb 100644 --- a/schema.yml +++ b/schema.yml @@ -40204,10 +40204,15 @@ components: challenge: type: object additionalProperties: {} + last_used: + type: string + format: date-time + nullable: true required: - challenge - device_class - device_uid + - last_used DeviceChallengeRequest: type: object description: Single device challenge @@ -40221,10 +40226,15 @@ components: challenge: type: object additionalProperties: {} + last_used: + type: string + format: date-time + nullable: true required: - challenge - device_class - device_uid + - last_used DeviceClassesEnum: enum: - static diff --git a/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStage.ts b/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStage.ts index a80ec94022..3bfad7def0 100644 --- a/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStage.ts +++ b/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStage.ts @@ -6,7 +6,7 @@ import { BaseStage, StageHost, SubmitOptions } from "@goauthentik/flow/stages/ba import { PasswordManagerPrefill } from "@goauthentik/flow/stages/identification/IdentificationStage"; import { msg } from "@lit/localize"; -import { CSSResult, TemplateResult, css, html, nothing } from "lit"; +import { CSSResult, PropertyValues, TemplateResult, css, html, nothing } from "lit"; import { customElement, state } from "lit/decorators.js"; import PFButton from "@patternfly/patternfly/components/Button/button.css"; @@ -25,6 +25,37 @@ import { FlowsApi, } from "@goauthentik/api"; +const customCSS = css` + ul { + padding-top: 1rem; + } + ul > li:not(:last-child) { + padding-bottom: 1rem; + } + .authenticator-button { + display: flex; + align-items: center; + } + :host([theme="dark"]) .authenticator-button { + color: var(--ak-dark-foreground) !important; + } + i { + font-size: 1.5rem; + padding: 1rem 0; + width: 3rem; + } + .right { + display: flex; + flex-direction: column; + justify-content: space-between; + height: 100%; + text-align: left; + } + .right > * { + height: 50%; + } +`; + @customElement("ak-stage-authenticator-validate") export class AuthenticatorValidateStage extends BaseStage< @@ -33,6 +64,10 @@ export class AuthenticatorValidateStage > implements StageHost { + static get styles(): CSSResult[] { + return [PFBase, PFLogin, PFForm, PFFormControl, PFTitle, PFButton, customCSS]; + } + flowSlug = ""; set loading(value: boolean) { @@ -47,14 +82,18 @@ export class AuthenticatorValidateStage return this.host.brand; } + @state() + _firstInitialized: boolean = false; + @state() _selectedDeviceChallenge?: DeviceChallenge; set selectedDeviceChallenge(value: DeviceChallenge | undefined) { const previousChallenge = this._selectedDeviceChallenge; this._selectedDeviceChallenge = value; - if (!value) return; - if (value === previousChallenge) return; + if (value === undefined || value === previousChallenge) { + return; + } // We don't use this.submit here, as we don't want to advance the flow. // We just want to notify the backend which challenge has been selected. new FlowsApi(DEFAULT_CONFIG).flowsExecutorSolve({ @@ -79,37 +118,39 @@ export class AuthenticatorValidateStage return this.host?.submit(payload, options) || Promise.resolve(); } - static get styles(): CSSResult[] { - return [PFBase, PFLogin, PFForm, PFFormControl, PFTitle, PFButton].concat(css` - ul { - padding-top: 1rem; - } - ul > li:not(:last-child) { - padding-bottom: 1rem; - } - .authenticator-button { - display: flex; - align-items: center; - } - :host([theme="dark"]) .authenticator-button { - color: var(--ak-dark-foreground) !important; - } - i { - font-size: 1.5rem; - padding: 1rem 0; - width: 3rem; - } - .right { - display: flex; - flex-direction: column; - justify-content: space-between; - height: 100%; - text-align: left; - } - .right > * { - height: 50%; - } - `); + willUpdate(_changed: PropertyValues) { + if (this._firstInitialized || !this.challenge) { + return; + } + + this._firstInitialized = true; + + // If user only has a single device, autoselect that device. + if (this.challenge.deviceChallenges.length === 1) { + this.selectedDeviceChallenge = this.challenge.deviceChallenges[0]; + return; + } + + // If TOTP is allowed from the backend and we have a pre-filled value + // from the password manager, autoselect TOTP. + const totpChallenge = this.challenge.deviceChallenges.find( + (challenge) => challenge.deviceClass === DeviceClassesEnum.Totp, + ); + if (PasswordManagerPrefill.totp && totpChallenge) { + console.debug( + "authentik/stages/authenticator_validate: found prefill totp code, selecting totp challenge", + ); + this.selectedDeviceChallenge = totpChallenge; + return; + } + + // If the last used device is not Static, autoselect that device. + const lastUsedChallenge = this.challenge.deviceChallenges + .filter((deviceChallenge) => deviceChallenge.lastUsed) + .sort((a, b) => b.lastUsed!.valueOf() - a.lastUsed!.valueOf())[0]; + if (lastUsedChallenge && lastUsedChallenge.deviceClass !== DeviceClassesEnum.Static) { + this.selectedDeviceChallenge = lastUsedChallenge; + } } renderDevicePickerSingle(deviceChallenge: DeviceChallenge) { @@ -228,45 +269,28 @@ export class AuthenticatorValidateStage } render(): TemplateResult { - if (!this.challenge) { - return html` `; - } - // User only has a single device class, so we don't show a picker - if (this.challenge?.deviceChallenges.length === 1) { - this.selectedDeviceChallenge = this.challenge.deviceChallenges[0]; - } - // TOTP is a bit special, assuming that TOTP is allowed from the backend, - // and we have a pre-filled value from the password manager, - // directly set the the TOTP device Challenge as active. - const totpChallenge = this.challenge.deviceChallenges.find( - (challenge) => challenge.deviceClass === DeviceClassesEnum.Totp, - ); - if (PasswordManagerPrefill.totp && totpChallenge) { - console.debug( - "authentik/stages/authenticator_validate: found prefill totp code, selecting totp challenge", - ); - this.selectedDeviceChallenge = totpChallenge; - } - return html` - ${this.selectedDeviceChallenge - ? this.renderDeviceChallenge() - : html` -
- -
`}`; + return this.challenge + ? html` + ${this.selectedDeviceChallenge + ? this.renderDeviceChallenge() + : html` +
+ +
`}` + : html` `; } } diff --git a/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStageCode.ts b/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStageCode.ts index 9065f2e4d8..01fcd4ef68 100644 --- a/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStageCode.ts +++ b/web/src/flow/stages/authenticator_validate/AuthenticatorValidateStageCode.ts @@ -31,6 +31,34 @@ export class AuthenticatorValidateStageWebCode extends BaseDeviceStage< `); } + deviceMessage(): string { + switch (this.deviceChallenge?.deviceClass) { + case DeviceClassesEnum.Sms: + return msg("A code has been sent to you via SMS."); + case DeviceClassesEnum.Totp: + return msg( + "Open your two-factor authenticator app to view your authentication code.", + ); + case DeviceClassesEnum.Static: + return msg("Enter a one-time recovery code for this user."); + } + + return msg("Enter the code from your authenticator device."); + } + + deviceIcon(): string { + switch (this.deviceChallenge?.deviceClass) { + case DeviceClassesEnum.Sms: + return "fa-key"; + case DeviceClassesEnum.Totp: + return "fa-mobile-alt"; + case DeviceClassesEnum.Static: + return "fa-sticky-note"; + } + + return "fa-mobile-alt"; + } + render(): TemplateResult { if (!this.challenge) { return html` `; @@ -44,19 +72,8 @@ export class AuthenticatorValidateStageWebCode extends BaseDeviceStage< > ${this.renderUserInfo()}
- - ${this.deviceChallenge?.deviceClass == DeviceClassesEnum.Sms - ? html`

${msg("A code has been sent to you via SMS.")}

` - : html`

- ${msg( - "Open your two-factor authenticator app to view your authentication code.", - )} -

`} + +

${this.deviceMessage()}

`; } } diff --git a/website/docs/add-secure-apps/flows-stages/stages/authenticator_validate/index.md b/website/docs/add-secure-apps/flows-stages/stages/authenticator_validate/index.md index 907a967e19..bbe7a1e7bf 100644 --- a/website/docs/add-secure-apps/flows-stages/stages/authenticator_validate/index.md +++ b/website/docs/add-secure-apps/flows-stages/stages/authenticator_validate/index.md @@ -5,10 +5,10 @@ title: Authenticator validation stage This stage validates an already configured Authenticator Device. This device has to be configured using any of the other authenticator stages: - [Duo authenticator stage](../authenticator_duo/index.md) -- [SMS authenticator stage](../authenticator_sms/index.md). -- [Static authenticator stage](../authenticator_static/index.md). +- [SMS authenticator stage](../authenticator_sms/index.md) +- [Static authenticator stage](../authenticator_static/index.md) - [TOTP authenticator stage](../authenticator_totp/index.md) -- [WebAuth authenticator stage](../authenticator_webauthn/index.md). +- [WebAuthn authenticator stage](../authenticator_webauthn/index.md) You can select which type of device classes are allowed. @@ -75,3 +75,7 @@ Optionally restrict which WebAuthn device types can be used to authenticate. When no restriction is set, all WebAuthn devices a user has registered are allowed. These restrictions only apply to WebAuthn devices created with authentik 2024.4 or later. + +#### Automatic device selection + +If the user has more than one device, the user is prompted to select which device they want to use for validation. After the user successfully authenticates with a certain device, that device is marked as "last used". In subsequent prompts by the Authenticator validation stage, the last used device is automatically selected for the user. Should they wish to use another device, the user can return to the device selection screen.