Web/update provider forms for invalidation #11856
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Pull the OAuth2 Provider Form `render()` method out into a standalone function.
- Why: So it can be shared by both the Wizard and the Provider function. The renderer is (or at
least, can be) a pure function: you give it input and it produces HTML, *and then it stops*.
- Provide a test harness that can test the OAuth2 provider form.
* main: (44 commits) web/admin: add strict dompurify config for diagram (#11783) core: bump cryptography from 43.0.1 to 43.0.3 (#11750) web: bump API Client version (#11781) sources: add Kerberos (#10815) root: rework CSRF middleware to set secure flag (#11753) web/admin: improve invalidation flow default & field grouping (#11769) providers/scim: add comparison with existing group on update and delta update users (#11414) website: bump mermaid from 10.6.0 to 10.9.3 in /website (#11766) web/flows: use dompurify for footer links (#11773) core, web: update translations (#11775) core: bump goauthentik.io/api/v3 from 3.2024083.10 to 3.2024083.11 (#11776) website: bump @types/react from 18.3.11 to 18.3.12 in /website (#11777) website: bump http-proxy-middleware from 2.0.6 to 2.0.7 in /website (#11771) web: bump API Client version (#11770) stages: authenticator_endpoint_gdtc (#10477) core: add prompt_data to auth flow (#11702) tests/e2e: fix dex tests failing (#11761) web/rac: disable DPI scaling (#11757) web/admin: update flow background (#11758) website/docs: fix some broken links (#11742) ...
* 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) ...
…s and ./admin/applications/wizard
## What
- For LDAP, OAuth2, Radius, SAML, SCIM, and Proxy providers, extract the literal form rendering
component of each provider into a function. After all, that's what they are: they take input (the
render state) and produce output (HTML with event handlers).
- Rip out all of the forms in the wizard and replace them with ☝️
- Write E2E tests that exercise *all* of the components in *all* of the forms mentioned. See test
results. These tests come in two flavors, "simple" (minimum amount needed to make the provider
"pass" the backend's parsers) and "complete" (touches every legal field in the form according to
the authentik `./schema.yml` file). As a result, every field is validated against the schema
(although the schema is currently ported into the test by hand.
- Fixed some serious bugginess in the way the wizard `commit` phase handles errors.
## Details
### Providers
In some cases, I broke up the forms into smaller units:
- Proxy, especially, with standalone units now for `renderHttpBasic`, `renderModeSelector`,
`renderSettings`, and the differing modes)
- SAML now has a `renderHasSigningKp` object, which makes that part of the code much more readable.
I also extracted a few of static `options` collections into static const objects, so that the form
object itself would be a bit more readable.
### Wizard
Just ripped out all of the Provider forms. All of them. They weren't going to be needed in our
glorious new future.
Using the information provided by the `providerTypes` object, it was easy to extract all of the
information that had once been in `ak-application-wizard-authentication-method-choice.choices`. The
only thing left now is the renderers, one for each of the forms ripped out. Everything else is just
gone.
As a result, though, that's no longer a static list. It has to be derived from information sent via
the API. So now it's in a context that's built when the wizard is initialized, and accessed by the
`createTypes` pass as well as the specific provider.
The error handling in the `commit` pass was just broken. I have improved it quite a bit, and now it
actually displays helpful messages when things go wrong.
### Tests
Wrote a simple test runner that iterates through a collection of fields, setting their values via
field-type instructions contained in each line. For example, the "simple" OAuth2 Provider test looks
like this:
```
export const simpleOAuth2ProviderForm: TestProvider = () => [
[setTypeCreate, "selectProviderType", "OAuth2/OpenID Provider"],
[clickButton, "Next"],
[setTextInput, "name", newObjectName("New Oauth2 Provider")],
[setSearchSelect, "authorizationFlow", /default-provider-authorization-explicit-consent/],
];
```
Each control checks for the existence of the object, and in most cases its current `display`.
(SearchSelect only checks existence, due to the oddness of the portaled popup.) Where a field can't
reasonably be modified and still pass, we at least verify that the name provided in `schema.yml`
corresponds to an existing, available control on the form or wizard panel.
Combined with a routine for logging in and navigating to the Provider page, and another one to
validate that a new and uniqute "Successfully Created Provider" notification appeared, this makes
testing each provider a simple message of filling out the table of fields you want populated.
Equally simple: these *exact same tests* can be incorporated into a wrapper for logging in,
navigating to the Application page, and filling out an Application, and then a new and unique
Provider for that Application, by Provider Type.
As a special case, the Wizard variant checks the `TestSequence` object returned by the
`TestProvider` function and removes the `name` field, since the Wizard pre-populates that
automatically.
As a result of this, the contents of `./web/src` has lost 1,504 lines of code. And results like
these, where the behavior has been cross-checked three ways (the forms, the tests (and so the
back-end), *and the schema* all agree on field names and behaviors, gives me much more confidence
that the refactor works as expected:
```
[chrome 130.0.6723.70 mac #0-1] Running: chrome (v130.0.6723.70) on mac
[chrome 130.0.6723.70 mac #0-1] Session ID: 039c70690eebc83ffbc2eef97043c774
[chrome 130.0.6723.70 mac #0-1]
[chrome 130.0.6723.70 mac #0-1] » /tests/specs/providers.ts
[chrome 130.0.6723.70 mac #0-1] Configuring Providers
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple LDAP provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple OAuth2 provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple Radius provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple SAML provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple SCIM provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple Proxy provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple Forward Auth (single application) provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Simple Forward Auth (domain level) provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete OAuth2 provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete LDAP provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete Radius provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete SAML provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete SCIM provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete Proxy provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete Forward Auth (single application) provider
[chrome 130.0.6723.70 mac #0-1] ✓ Should successfully configure a Complete Forward Auth (domain level) provider
[chrome 130.0.6723.70 mac #0-1]
[chrome 130.0.6723.70 mac #0-1] 16 passing (1m 48.5s)
------------------------------------------------------------------
[chrome 130.0.6723.70 mac #0-2] Running: chrome (v130.0.6723.70) on mac
[chrome 130.0.6723.70 mac #0-2] Session ID: 5a3ae12c851eff8fffd2686096759146
[chrome 130.0.6723.70 mac #0-2]
[chrome 130.0.6723.70 mac #0-2] » /tests/specs/new-application-by-wizard.ts
[chrome 130.0.6723.70 mac #0-2] Configuring Applications Via the Wizard
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple LDAP provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple OAuth2 provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple Radius provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple SAML provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple SCIM provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple Proxy provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple Forward Auth (single) provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Simple Forward Auth (domain) provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete OAuth2 provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete LDAP provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete Radius provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete SAML provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete SCIM provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete Proxy provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete Forward Auth (single) provider
[chrome 130.0.6723.70 mac #0-2] ✓ Should successfully configure an application with a Complete Forward Auth (domain) provider
[chrome 130.0.6723.70 mac #0-2]
[chrome 130.0.6723.70 mac #0-2] 16 passing (2m 3s)
```
🎉
✅ Deploy Preview for authentik-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* main: web/admin: fix code-based MFA toggle not working in wizard (#11854) sources/kerberos: add kiprop to ignored system principals (#11852) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh_CN (#11846) translate: Updates for file locale/en/LC_MESSAGES/django.po in it (#11845) translate: Updates for file web/xliff/en.xlf in zh_CN (#11847) translate: Updates for file web/xliff/en.xlf in zh-Hans (#11848) translate: Updates for file locale/en/LC_MESSAGES/django.po in zh-Hans (#11849) translate: Updates for file web/xliff/en.xlf in it (#11850) website: 2024.10 Release Notes (#11839) translate: Updates for file web/xliff/en.xlf in zh-Hans (#11814) core, web: update translations (#11821) core: bump goauthentik.io/api/v3 from 3.2024083.13 to 3.2024083.14 (#11830) core: bump service-identity from 24.1.0 to 24.2.0 (#11831) core: bump twilio from 9.3.5 to 9.3.6 (#11832) core: bump pytest-randomly from 3.15.0 to 3.16.0 (#11833) website/docs: Update social-logins github (#11822) website/docs: remove � (#11823) lifecycle: fix kdc5-config missing (#11826) website/docs: update preview status of different features (#11817)
❌ 1 Tests Failed:
View the full list of 1 ❄️ flaky tests
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
| @@ -56,6 +66,21 @@ export class ApplicationWizard extends CustomListenerElement( | |||
| */ | |||
| providerCache: Map<string, OneOfProvider> = new Map(); | |||
|
|
|||
| connectedCallback() { | |||
| super.connectedCallback(); | |||
| new ProvidersApi(DEFAULT_CONFIG).providersAllTypesList().then((providerTypes) => { | |||
There was a problem hiding this comment.
By removing all the static provider choices, I can now rely on the ProvidersApi to give me that list instead.
| render() { | ||
| const selectedTypes = providerModelsList.filter( | ||
| (t) => t.formName === this.wizard.providerModel, | ||
| const selectedTypes = this.providerModelsList.filter( |
There was a problem hiding this comment.
Since I'm using the form as provided by the ProxyProviderForm, I no longer need this distinction. They're just modelNames now.
| type State = { | ||
| state: "idle" | "running" | "error" | "success"; | ||
| label: string | TemplateResult; | ||
| icon: string[]; | ||
| }; | ||
|
|
||
| const providerMap: Map<string, string> = Object.values(ProviderModelEnum) |
There was a problem hiding this comment.
On the other hoof, now I need to do some type and string weirdness to map the short-form and long-form of the model names as sent to the back end.
| @@ -133,23 +152,19 @@ export class ApplicationWizardCommitApplication extends BasePanel { | |||
| }) | |||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |||
| .catch(async (resolution: any) => { | |||
| const errors = await parseAPIError(resolution); | |||
| this.errors = await parseAPIError(resolution); | |||
There was a problem hiding this comment.
I wasn't saving my errors. Dammit. This was the big bug in error handling.
| @@ -58,6 +62,7 @@ export class ProviderWizard extends AKElement { | |||
| }} | |||
| > | |||
| <ak-wizard-page-type-create | |||
| name="selectProviderType" | |||
There was a problem hiding this comment.
Makes it a lot easier for the tests to find! And hey, this is actually a control: ak-radio-card. We should abstract that.
| @@ -55,6 +56,79 @@ export default class Page { | |||
| await browser.keys(Key.Tab); | |||
| } | |||
|
|
|||
| async setSearchSelect(name: string, value: string) { | |||
There was a problem hiding this comment.
This can probably be deleted soon. Although I have the thought that it might make sense to move all of the controls into a JS Object so that they can all have a local context object; that way, the $ WebdriverIO context can be localized to, say, this form or this wizard panel, and we wouldn't have to struggle with "stale" (really, fragmented-by-Lit) objects on the page sharing fieldname identifiers. That's a thought for the future, though.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
web/admin: Unify the forms for providers between the ./admin/providers and ./admin/applications/wizard
What
./schema.ymlfile). As a result, every field is validated against the schema (although the schema is currently ported into the test by hand.commitphase handles errors.Details
Providers
In some cases, I broke up the forms into smaller units:
renderHttpBasic,renderModeSelector,renderSettings, and the differing modes)renderHasSigningKpobject, which makes that part of the code much more readable.I also extracted a few of static
optionscollections into static const objects, so that the form object itself would be a bit more readable.Wizard
Just ripped out all of the Provider forms. All of them. They weren’t going to be needed in our glorious new future.
Using the information provided by the
providerTypesobject, it was easy to extract all of the information that had once been inak-application-wizard-authentication-method-choice.choices. The only thing left now is the renderers, one for each of the forms ripped out. Everything else is just gone.As a result, though, that’s no longer a static list. It has to be derived from information sent via the API. So now it’s in a context that’s built when the wizard is initialized, and accessed by the
createTypespass as well as the specific provider.The error handling in the
commitpass was just broken. I have improved it quite a bit, and now it actually displays helpful messages when things go wrong.Tests
Wrote a simple test runner that iterates through a collection of fields, setting their values via field-type instructions contained in each line. For example, the “simple” OAuth2 Provider test looks like this:
Each control checks for the existence of the object, and in most cases its current
display. (SearchSelect only checks existence, due to the oddness of the portaled popup.) Where a field can’t reasonably be modified and still pass, we at least verify that the name provided inschema.ymlcorresponds to an existing, available control on the form or wizard panel.Combined with a routine for logging in and navigating to the Provider page, and another one to validate that a new and uniqute “Successfully Created Provider” notification appeared, this makes testing each provider a simple message of filling out the table of fields you want populated.
Equally simple: these exact same tests can be incorporated into a wrapper for logging in, navigating to the Application page, and filling out an Application, and then a new and unique Provider for that Application, by Provider Type.
As a special case, the Wizard variant checks the
TestSequenceobject returned by theTestProviderfunction and removes thenamefield, since the Wizard pre-populates that automatically.As a result of this, the contents of
./web/srchas lost 1,504 lines of code. And results like these, where the behavior has been cross-checked three ways (the forms, the tests (and so the back-end), and the schema all agree on field names and behaviors, gives me much more confidence that the refactor works as expected:🎉
Checklist
make web)npm test:e2e:watch)