Skip to content

Commit

Permalink
web/flows: resize captcha iframes (#12260)
Browse files Browse the repository at this point in the history
* web: Add InvalidationFlow to Radius Provider dialogues

## What

- Bugfix: adds the InvalidationFlow to the Radius Provider dialogues
  - Repairs: `{"invalidation_flow":["This field is required."]}` message, which was *not* propagated
    to the Notification.
- Nitpick: Pretties `?foo=${true}` expressions: `s/\?([^=]+)=\$\{true\}/\1/`

## Note

Yes, I know I'm going to have to do more magic when we harmonize the forms, and no, I didn't add the
Property Mappings to the wizard, and yes, I know I'm going to have pain with the *new* version of
the wizard. But this is a serious bug; you can't make Radius servers with *either* of the current
dialogues at the moment.

* web: streamline CaptchaStage

# What

This commit:

1. Replaces the mass of `if () { if() { if() } }` with two state tables:
  - One for `render()`
  - One for `renderBody()`

2. Breaks each Captcha out into "interactive" and "executive" versions
3. Creates a handler table for each Captcha type
4. Replaces the `.forEach` expression with a `for` loop.
5. Move `updated` to the end of the class.
6. Make captchDocument and captchaFrame constructed-on-demand with a cache.
7. Remove a lot of `@state` handlers
8. Give IframeMessageEvent its own type.
9. Removed `this.scriptElement`
10. Replaced `window.removeEventListener` with an `AbortController()`
# Why

1. **Replacing `if` trees with a state table.** The logic of the original was really hard to follow.
   With the state table, we can clearly see now that for the `render()` function, we care about the
   Boolean flags `[embedded, challenged, interactive]` and have appropriate effects for each. With
   `renderBody()`, we can see that we care about the Boolean flags `[hasError, challenged]`, and can
   see the appropriate effects for each one.

2. (and 3.) **Breaking each Captcha clause into separate handlers.** Again, the logic was convoluted,
   when what we really care about is "Does this captcha have a corresponding handler attached to
   `window`, and, if so, should we run the interactive or executive version of it?" By putting all
   of that into a table of `[name, challenge, execute]`, we can clearly see what's being handled
   when.

4. **Replacing `foreach()` with `for()`**: [You cannot use a `forEach()` with async
   expressions](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#:~:text=does%20not%20wait%20for%20promises).
   If you need asynchronous behavior in an ordered loop, `for()` is the safest way to handle it; if
   you need asynchronous behavior from multiple promises, `Promise.allSettled(handlers.map())` is
   the way to go.

   I tried to tell if this function *meant* to run every handler it found simultaneously, or if it
   meant to test them in order; I went with the second option, breaking and exiting the loop once a
   handler had run successfully.

5. **Reordered the code a bit**. We're trying to evolve a pattern in our source code: styles,
   properties, states, internal variables, constructor, getters & setters that are not `@property()`
   or `@state()`, DOM-related lifecycle handlers, event handlers, pre-render lifecycle handlers,
   renderers, and post-render lifecycle handlers. Helper methods (including subrenderers) go above
   the method(s) they help.

6. **Constructing Elements on demand with a cache**. It is not guaranteed that we will actually need
   either of those. Constructing them on demand with a cache is both performant and cleaner.
   Likewise, I removed these from the Lit object's `state()` table, since they're constructed once
   and never change over the lifetime of an instance of `ak-stage-captcha`.

9. **Remove `this.scriptElement`**: It was never referenced outside the one function where it was used.

10. **Remove `removeEventListener()`**: `AbortController()` is a bit more verbose for small event
    handler collections, but it's considered much more reliable and much cleaner.

* Didn't save the extracted ListenerController.
  • Loading branch information
kensternberg-authentik authored Dec 9, 2024
1 parent a117918 commit 28a2311
Show file tree
Hide file tree
Showing 2 changed files with 304 additions and 184 deletions.
41 changes: 41 additions & 0 deletions web/src/elements/utils/listenerController.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// This is a more modern way to handle disconnecting listeners on demand.

// example usage:

/*
export class MyElement extends LitElement {
this.listenerController = new ListenerController();
connectedCallback() {
super.connectedCallback();
window.addEventListener("event-1", handler1, { signal: this.listenerController.signal });
window.addEventListener("event-2", handler2, { signal: this.listenerController.signal });
window.addEventListener("event-3", handler3, { signal: this.listenerController.signal });
}
disconnectedCallback() {
// This will disconnect *all* the event listeners at once, and resets the listenerController,
// releasing the memory used for the signal as well. No more trying to map all the
// `addEventListener` to `removeEventListener` tediousness!
this.listenerController.abort();
super.disconnectedCallback();
}
}
*/

export class ListenerController {
listenerController?: AbortController;

get signal() {
if (!this.listenerController) {
this.listenerController = new AbortController();
}
return this.listenerController.signal;
}

abort() {
this.listenerController?.abort();
this.listenerController = undefined;
}
}
Loading

0 comments on commit 28a2311

Please sign in to comment.