stages/identification: add captcha to identification stage #11711

gergosimonyi · 2024-10-17T16:05:40Z

Details

Adds an optional captcha to an Identification stage to run in the background while the user inputs their information.

Checklist

Local tests pass (ak test authentik/)
The code has been formatted (make lint-fix)

If an API change has been made

The API schema has been updated (make gen-build)

If changes to the frontend have been made

The code has been formatted (make web)

If applicable

The documentation has been updated
The documentation has been formatted (make website)

netlify · 2024-10-17T16:05:57Z

✅ Deploy Preview for authentik-storybook ready!

Name	Link
🔨 Latest commit	`bf2ae13`
🔍 Latest deploy log	https://app.netlify.com/sites/authentik-storybook/deploys/671a53b075ba21000813b932
😎 Deploy Preview	https://deploy-preview-11711--authentik-storybook.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-10-17T16:06:03Z

✅ Deploy Preview for authentik-docs canceled.

Name	Link
🔨 Latest commit	`bf2ae13`
🔍 Latest deploy log	https://app.netlify.com/sites/authentik-docs/deploys/671a53b0b8bfc4000880c3ad

codecov · 2024-10-17T16:16:21Z

Codecov Report

Attention: Patch coverage is 97.26027% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.57%. Comparing base (2fa50de) to head (bf2ae13).
Report is 31 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
authentik/stages/captcha/stage.py	96.42%	1 Missing ⚠️
authentik/stages/identification/stage.py	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11711      +/-   ##
==========================================
- Coverage   92.74%   92.57%   -0.17%     
==========================================
  Files         740      760      +20     
  Lines       36783    37788    +1005     
==========================================
+ Hits        34113    34981     +868     
- Misses       2670     2807     +137

Flag	Coverage Δ
e2e	`49.13% <15.06%> (-0.12%)`	⬇️
integration	`24.93% <12.32%> (-0.01%)`	⬇️
unit	`90.15% <97.26%> (-0.12%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-10-17T16:48:20Z

authentik PR Installation instructions

Instructions for docker-compose

Add the following block to your .env file:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-bf2ae13c52de89306b7babc609b99aa2068ddea2
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

For arm64, use these values:

AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-bf2ae13c52de89306b7babc609b99aa2068ddea2-arm64
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s

Afterwards, run the upgrade commands from the latest release notes.

Instructions for Kubernetes

Add the following block to your values.yml file:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
global:
    image:
        repository: ghcr.io/goauthentik/dev-server
        tag: gh-bf2ae13c52de89306b7babc609b99aa2068ddea2

For arm64, use these values:

authentik:
    outposts:
        container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
global:
    image:
        repository: ghcr.io/goauthentik/dev-server
        tag: gh-bf2ae13c52de89306b7babc609b99aa2068ddea2-arm64

Afterwards, run the upgrade commands from the latest release notes.

kensternberg-authentik

I made some comments and requested two changes. I've marked comment where code change suggestions are just that. I hope that's helpful. Two of the suggestions are about bad habits, and one is just a pointer to a different way to use SearchSelect.

kensternberg-authentik · 2024-10-17T21:39:51Z

web/src/admin/stages/identification/IdentificationStageForm.ts

+                            .selected=${(stage: Stage): boolean => {
+                                return stage.pk === this.instance?.captchaStage;
+                            }}
+                            ?blankable=${true}


This is not a change request. It is only a suggestion.

Not a bad habit, but there is a wrapper for SearchSelect called SearchSelectEz with a slightly more compact API. You don't have to use it, but I like to; it includes the externally-supplied machinery as a single syntactical and semantic unit, and I think it's a bit more readable. This example also shows the arrow functions as function expressions, and includes blankable in its pure-attribute form.

<ak-search-select-ez .config=${{ fetchObjects: async (query?: string): Promise<Stage[]> => { const args: StagesCaptchaListRequest = { ordering: "name", }; if (query !== undefined) { args.search = query; } const stages = await new StagesApi(DEFAULT_CONFIG).stagesCaptchaList(args); return stages.results; }, groupBy: (items: Stage[]) => groupBy(items, (stage) => stage.verboseNamePlural), renderElement: (stage: Stage): string => stage.name, value: (stage: Stage | undefined): string | undefined => stage?.pk, selected: (stage: Stage): boolean => stage.pk === this.instance?.captchaStage, }} blankable></ak-search-select-ez>

I know the ${{ }} looks a bit React-y, but it is, because it's doing something that's much more common in React than in Lit: passing a whole object to a property. The outer ${ } creates a template scope, and the inner { } creates a new object to be passed via the template scope.

web/src/admin/stages/identification/IdentificationStageForm.ts

kensternberg-authentik · 2024-10-17T21:45:52Z

web/src/admin/stages/identification/IdentificationStageForm.ts

+                            }}
+                            .selected=${(stage: Stage): boolean => {
+                                return stage.pk === this.instance?.captchaStage;
+                            }}


This is a change request.

The second bad habit: ?blankable=${true} is better written as just blankable; the Lit cruft here adds nothing. Lit does the right thing with boolean attributes. Only use the Lit form for a boolean attribute if blankable could change dynamically.

kensternberg-authentik · 2024-10-17T22:01:45Z

web/src/flow/stages/captcha/CaptchaStage.ts

+    embedded = false;
+
+    @property()
+    onTokenChange: (token: string) => void = (token) => {


This is a change request.

I don't think the default value you set here does what you think it should do. onTokenChange is a way for the client code to pass a mutator, a function that reaches up into the client and sets a value there, as you're doing in the IdentificationStage.ts changes below.

You have two choices. The first is

onTokenChange: (token: string) => void = (_token: string) => undefined;

... which will swallow any errors until you realize that you forgot to supply onTokenChange in your client code.

The better choice is:

onTokenChange: (token: string) => void = (_token: string) => { throw new Error("Client failed to supply a handling function to CaptchaStage"); };

... which will crash early and crash informatively if you do forget. (Regarding my comment about arrow functions, a throw is always a statement, so it has to be wrapped in scope delimiters.)

In either case, the function declaration's argument signature must begin with an underscore (the _token), because it is a declaration and eslint will complain that all declared variables are not being used, but it is a convention that eslint should not make that complaint about variable names that begin with an underscore. (The names in the definition's signature are pure documentation, except when they're keys for a defined record object being passed in. TypeScript does have its little annoyances.)

This is not a change request. It is just a suggestion.

Also? That type-and-default pairing makes my head hurt. Maybe you could, somewhere above the class declaration, put:

type TokenHandler = (token: string) => void;

... and then:

onTokenChange: TokenHandler = (_token: string) => { throw new Error("Client failed to supply a handling function to CaptchaStage"); };

web/src/flow/stages/identification/IdentificationStage.ts

kensternberg-authentik · 2024-10-17T22:17:08Z

web/src/flow/stages/captcha/CaptchaStage.ts

@@ -157,10 +155,16 @@ export class CaptchaStage extends BaseStage<CaptchaChallenge, CaptchaChallengeRe
        if (this.captchaInteractive) {
            return html`${this.captchaContainer}`;
        }
+        if (this.embedded) {
+            return html``;


This is a change request.

Please return nothing instead of an empty html() call. Lit will create empty DOM-state tracking tags for an empty html(), which in the small is irrelevant, but it's still messy.

You can add nothing in the Lit base import:

import { CSSResult, PropertyValues, TemplateResult, html, nothing } from "lit";

web/src/flow/stages/captcha/CaptchaStage.ts

kensternberg-authentik · 2024-10-17T22:34:54Z

website/docs/add-secure-apps/flows-stages/stages/identification/index.md

+
+## Captcha stage
+
+To run a captcha in the background while the user is inputting their identification, a Captcha stage can be selected here. If a Captcha stage is selected in the Identification stage, the Captcha stage should not be bound to the flow.


Suggestion (and maybe @tanberry can chime in): "entering" instead of "inputting". "Inputting" is such an ugly phrase. Either that or "... while the user inputs their identification, ..." but that's still mechanizing the human, and I like my humans to stay human.

gergosimonyi · 2024-10-18T08:10:13Z

@kensternberg-authentik

Thank you for the extensive review! I've implemented everything, except

ak-search-select-ez: I'll skip this for now as it doesn't seem to be used anywhere, I'd rather not have its maiden flight be so close to a release
nothing instead of html``: because nothing doesn't fit the function's return type, TemplateResult, and this is prettier (in my opinion) than changing 2 function signatures like

@@ -147,7 +147,7 @@ export class CaptchaStage extends BaseStage<CaptchaChallenge, CaptchaChallengeRe
         return true;
     }
 
-    renderBody(): TemplateResult {
+    renderBody(): TemplateResult | typeof nothing {
         if (this.error) {
             return html`<ak-empty-state icon="fa-times" header=${this.error}> </ak-empty-state>`;
         }
@@ -160,7 +160,7 @@ export class CaptchaStage extends BaseStage<CaptchaChallenge, CaptchaChallengeRe
         return html`<ak-empty-state loading header=${msg("Verifying...")}></ak-empty-state>`;
     }
 
-    render(): TemplateResult {
+    render(): TemplateResult | typeof nothing {
         if (this.embedded) {
             return this.renderBody();
         }

Let me know if that's acceptable or if you have any other insights.

authentik/stages/identification/stage.py

(In Captcha stage contexts the name `token` seems well-scoped.)

tanberry

A couple of edits... and questions... but I'm approving now so that I am not the bottleneck. ;)

tanberry · 2024-10-18T16:25:38Z

website/docs/add-secure-apps/flows-stages/stages/identification/index.md

-To prompt users for their password on the same step as identifying themselves, a password stage can be selected here. If a password stage is selected in the Identification stage, the password stage should not be bound to the flow.
+To prompt users for their password on the same step as identifying themselves, a Password stage can be selected here. If a Password stage is selected in the Identification stage, the Password stage should not be bound to the flow.
+
+## Captcha stage


Suggested change

## Captcha stage

## CAPTCHA stage

tanberry · 2024-10-18T16:26:54Z

website/docs/add-secure-apps/flows-stages/stages/identification/index.md

+
+## Captcha stage
+
+To run a captcha in the background while the user is entering their identification, a Captcha stage can be selected here. If a Captcha stage is selected in the Identification stage, the Captcha stage should not be bound to the flow.


Suggested change

To run a captcha in the background while the user is entering their identification, a Captcha stage can be selected here. If a Captcha stage is selected in the Identification stage, the Captcha stage should not be bound to the flow.

To run a CAPTCHA process in the background while the user is entering their identification, a CAPTCHA stage can be selected here. If a CAPTCHA stage is selected in the Identification stage, the CAPTCHA stage should not be bound to the flow.

I do feel like we could explain more about when the CAPTCHA prompt is shown to the user... what do we mean by running CAPTCHA in the background? And when is that process brought to the foreground, for user to see?

And I want to look to see if we have docs on "binding a stage binding to a stage". Your docs cover enough to get it doen, but would be great if we could link to more info. I think you gave me a lot more details when we first spoke about this... and I cannot find my notes, whah.

Thanks @gergosimonyi for explaining about he background.. that CATCHA is being run in the background if CATCHA has been configured. Makes sesne, haha. For the second paragraph, though, I do still want to look at how we can better explain this... that can happen later though.

kensternberg-authentik

I approve of the changes you made at my request, but please review @tanberry's suggestion and requests before committing. At this point, the code should work as expected and is not unmaintainable.

…n stages Note: this doesn't remove the captcha itself, if interactive, only the loading indicator.

BeryJu

Just 2 small things this time around

BeryJu · 2024-10-22T21:03:52Z

authentik/stages/captcha/stage.py

@@ -27,6 +27,60 @@ class CaptchaChallenge(WithUserInfoChallenge):
    component = CharField(default="ak-stage-captcha")


+def get_friendly_captcha_error(error: str) -> str:


Hmm I'm not sure if these error codes are the same for all the Captcha implementations we support. Also things like invalid/missing secret are probably not something we should disclose to end-users
I think we can probably fall back to Invalid captcha response as a generic error message

Yeah, fair enough. I was caught up on how to help admins, but this is not the way, I'll add some logging for that.

I think it's reasonable to keep the ones where we advise end users to try again, and I'll make them even more friendly. I'll include all "recoverable-by-retry" error codes from reCAPTCHA, hCaptcha and Turnstile.

BeryJu · 2024-10-22T21:07:01Z

web/src/flow/stages/captcha/CaptchaStage.ts

@@ -45,6 +46,11 @@ export class CaptchaStage extends BaseStage<CaptchaChallenge, CaptchaChallengeRe
    @state()
    scriptElement?: HTMLScriptElement;

+    @property()
+    onTokenChange: TokenHandler = (_token: string) => {
+        throw new Error("Client failed to supply a handling function to CaptchaStage");


Shouldn't the default for this be

this.host?.submit({ token: token, });

Ken and I don't think so, see some discussion above.

(By the rules of refactoring, you're right. I'm altering component contract here and I changed every callsite to match.)

I'm mainly asking because when the stage is rendered as default, there's no default handler there, which doesn't seem correct? Or did I miss it

This is the point I realize I'm probably missing something. What does "rendered as default" mean here?

My thinking is: this component will always need to be passed challenge and host. Whoever's supplying that (e.g. the FlowExecutor) should also supply onTokenChange.

Ah yeah I must've overlooked https://github.com/goauthentik/authentik/pull/11711/files#diff-4ed6c41b430eb15195ace2424e99a073db6c9e588f4627cf0f899ef9fad0f084R311-R312, I'd prefer that from the aspect of the flow executor all stages have the same signature so imo it should be the default callback handler on the stage and then it can be overridden when rendered embedded

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

* main: (22 commits) lifecycle: fix missing krb5 deps for full testing in image (#11815) translate: Updates for file web/xliff/en.xlf in zh-Hans (#11810) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh-Hans (#11809) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh_CN (#11808) web: bump API Client version (#11807) core: bump goauthentik.io/api/v3 from 3.2024083.12 to 3.2024083.13 (#11806) core: bump ruff from 0.7.0 to 0.7.1 (#11805) core: bump twilio from 9.3.4 to 9.3.5 (#11804) core, web: update translations (#11803) providers/scim: handle no members in group in consistency check (#11801) stages/identification: add captcha to identification stage (#11711) website/docs: improve root page and redirect (#11798) providers/scim: clamp batch size for patch requests (#11797) web/admin: fix missing div in wizard forms (#11794) providers/proxy: fix handling of AUTHENTIK_HOST_BROWSER (#11722) core, web: update translations (#11789) core: bump goauthentik.io/api/v3 from 3.2024083.11 to 3.2024083.12 (#11790) core: bump gssapi from 1.8.3 to 1.9.0 (#11791) web: bump API Client version (#11792) stages/authenticator_validate: autoselect last used 2fa device (#11087) ...

add captcha to identification stage

465010e

gergosimonyi force-pushed the stages/identification/add-captcha branch from 9593192 to 465010e Compare October 17, 2024 16:09

gergosimonyi marked this pull request as ready for review October 17, 2024 16:49

gergosimonyi requested review from a team as code owners October 17, 2024 16:49

BeryJu changed the title ~~add captcha to identification stage~~ stages/identification: add captcha to identification stage Oct 17, 2024

kensternberg-authentik requested changes Oct 17, 2024

View reviewed changes

gergosimonyi added 3 commits October 18, 2024 09:52

simplify component invocations

47f5a85

fail fast on onTokenChange default behavior

c223047

reword docs

e523618

gergosimonyi requested a review from kensternberg-authentik October 18, 2024 08:11

rissson requested changes Oct 18, 2024

View reviewed changes

authentik/stages/identification/stage.py Outdated Show resolved Hide resolved

gergosimonyi added 2 commits October 18, 2024 16:23

rename token to captcha_token in Identification stage contexts

069e63d

(In Captcha stage contexts the name `token` seems well-scoped.)

use nothing instead of ` html ```

e21c5ec

rissson approved these changes Oct 18, 2024

View reviewed changes

tanberry approved these changes Oct 21, 2024

View reviewed changes

kensternberg-authentik approved these changes Oct 21, 2024

View reviewed changes

gergosimonyi added 6 commits October 22, 2024 16:01

Merge branch 'main' into stages/identification/add-captcha

c2f3f1a

remove rendered Captcha component from document flow on Identificatio…

ef0604f

…n stages Note: this doesn't remove the captcha itself, if interactive, only the loading indicator.

add invisible requirement to captcha on Identification stage

78dca3a

stylize docs

1a0ef46

add friendlier error messages to Captcha stage

2327931

fix tests

f58c740

BeryJu reviewed Oct 22, 2024

View reviewed changes

gergosimonyi added 2 commits October 23, 2024 10:23

make captcha error messages even friendlier

51055c1

add test case to retriable captcha

a450324

gergosimonyi requested a review from BeryJu October 23, 2024 08:34

use default

bf2ae13

Signed-off-by: Jens Langhammer <jens@goauthentik.io>

BeryJu approved these changes Oct 24, 2024

View reviewed changes

gergosimonyi merged commit 9ee0ba1 into main Oct 25, 2024
66 checks passed

gergosimonyi deleted the stages/identification/add-captcha branch October 25, 2024 06:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stages/identification: add captcha to identification stage #11711

stages/identification: add captcha to identification stage #11711

gergosimonyi commented Oct 17, 2024 •

edited

Loading

netlify bot commented Oct 17, 2024 •

edited

Loading

netlify bot commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

kensternberg-authentik left a comment •

edited

Loading

kensternberg-authentik Oct 17, 2024

kensternberg-authentik Oct 17, 2024

kensternberg-authentik Oct 17, 2024

kensternberg-authentik Oct 17, 2024

kensternberg-authentik Oct 17, 2024

kensternberg-authentik Oct 17, 2024

gergosimonyi commented Oct 18, 2024

tanberry left a comment

tanberry Oct 18, 2024

tanberry Oct 18, 2024

tanberry Oct 18, 2024

tanberry Oct 21, 2024

kensternberg-authentik left a comment

BeryJu left a comment

BeryJu Oct 22, 2024

gergosimonyi Oct 23, 2024

BeryJu Oct 22, 2024

gergosimonyi Oct 23, 2024

BeryJu Oct 23, 2024

gergosimonyi Oct 24, 2024

BeryJu Oct 24, 2024


		## Captcha stage

		To run a captcha in the background while the user is inputting their identification, a Captcha stage can be selected here. If a Captcha stage is selected in the Identification stage, the Captcha stage should not be bound to the flow.

	To run a captcha in the background while the user is entering their identification, a Captcha stage can be selected here. If a Captcha stage is selected in the Identification stage, the Captcha stage should not be bound to the flow.
	To run a CAPTCHA process in the background while the user is entering their identification, a CAPTCHA stage can be selected here. If a CAPTCHA stage is selected in the Identification stage, the CAPTCHA stage should not be bound to the flow.

		@@ -27,6 +27,60 @@ class CaptchaChallenge(WithUserInfoChallenge):
		component = CharField(default="ak-stage-captcha")


		def get_friendly_captcha_error(error: str) -> str:

stages/identification: add captcha to identification stage #11711

stages/identification: add captcha to identification stage #11711

Conversation

gergosimonyi commented Oct 17, 2024 • edited Loading

Details

Checklist

netlify bot commented Oct 17, 2024 • edited Loading

✅ Deploy Preview for authentik-storybook ready!

netlify bot commented Oct 17, 2024 • edited Loading

✅ Deploy Preview for authentik-docs canceled.

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 17, 2024 • edited Loading

kensternberg-authentik left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gergosimonyi commented Oct 18, 2024

tanberry left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kensternberg-authentik left a comment

Choose a reason for hiding this comment

BeryJu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gergosimonyi commented Oct 17, 2024 •

edited

Loading

netlify bot commented Oct 17, 2024 •

edited

Loading

netlify bot commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

github-actions bot commented Oct 17, 2024 •

edited

Loading

kensternberg-authentik left a comment •

edited

Loading