From cc781cad00e8fd04ead9217c2af8fa0576ed5944 Mon Sep 17 00:00:00 2001 From: Ken Sternberg <133134217+kensternberg-authentik@users.noreply.github.com> Date: Fri, 20 Oct 2023 10:01:18 -0700 Subject: [PATCH 1/2] web/admin: improve user email button labels (#7233) * web: isolate clipboard handling We would like to use the clipboard for more than just the token copy button. This commit enables that by separating the "Write to Clipboard" and "Write to Notifications" routines into separate functions, putting "writeToClipboard" into the utilities collection, and clarifying what happens when a custom presses the TokenCopy button. * web: break out the recovery link logic into a standalone function UserViewPage and UserLinkPage have the same functionality to request to view a link with which a user may access an account recovery flow. The language and error messages were different on both of those pages. This commit harmonizes the language by making the request a standalone function. It also exploits the breakout of the "write to clipboard" commit to write the link to the clipboard, and to inform the user that the clipboard has been written to, when possible. * web: parity between UserViewPage and UserListPage Since the UserListPage's "accordion" view has an offer to "Email the recovery link" to the user, it seemed appropriate to grant the same capability to the UserListPage. * web: harmonize the CSS. After a bit of messing around, I have also ensured that the gap between the buttons is the same in all cases, that in the columnar display the buttons are of uniform width, and that the buttons have the same next: - "Set Password" - "View Recovery Link" - "Email Recovery Link" NOTE: This commit is contingent upon the PR for [isolate clipboard handling](https://github.com/goauthentik/authentik/pull/7229) to be accepted, as it relies on the clipboard handler for the "write link to clipboard" feature. * web: ensure the existence of the user Every `...render()` method in the UserViewPage class has a preamble guard clause: ``` if (!this.user) { return html``; } ``` With this clause, it should not be necessary to repeatedly check the type of `this.user` throughout the rest of the method, but the nominal type is `User?`, which means that functions called from within the method need to be protected against `undefined` failure. By creating a new variable with the type after the guard clause, we ensure the type is `User` (no question!) and can safely use it without those checks. Along the way, I replaced the empty html with `nothing` and corrected (mostly by removing) the return types. References: - [Lit-HTML: Prefer `nothing` over empty html or other falsey walues](https://lit.dev/docs/api/templates/#nothing) - [TypeScript: Type annotations on return types are rarely necessary](https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#:~:text=Return%20Type%20Annotations&text=Much%20like%20variable%20type%20annotations,example%20doesn't%20change%20anything.) * web: accepting suggested label change --- web/src/admin/users/UserListPage.ts | 115 ++++++++++++++++------------ web/src/admin/users/UserViewPage.ts | 108 ++++++++++++-------------- 2 files changed, 115 insertions(+), 108 deletions(-) diff --git a/web/src/admin/users/UserListPage.ts b/web/src/admin/users/UserListPage.ts index 7b3e3075c..17eddae14 100644 --- a/web/src/admin/users/UserListPage.ts +++ b/web/src/admin/users/UserListPage.ts @@ -21,10 +21,11 @@ import { getURLParam, updateURLParams } from "@goauthentik/elements/router/Route import { PaginatedResponse } from "@goauthentik/elements/table/Table"; import { TableColumn } from "@goauthentik/elements/table/Table"; import { TablePage } from "@goauthentik/elements/table/TablePage"; +import { writeToClipboard } from "@goauthentik/elements/utils/writeToClipboard"; import "@patternfly/elements/pf-tooltip/pf-tooltip.js"; import { msg, str } from "@lit/localize"; -import { CSSResult, TemplateResult, html } from "lit"; +import { CSSResult, TemplateResult, css, html } from "lit"; import { customElement, property, state } from "lit/decorators.js"; import PFAlert from "@patternfly/patternfly/components/Alert/alert.css"; @@ -40,6 +41,56 @@ import { UserPath, } from "@goauthentik/api"; +export const requestRecoveryLink = (user: User) => + new CoreApi(DEFAULT_CONFIG) + .coreUsersRecoveryRetrieve({ + id: user.pk, + }) + .then((rec) => + writeToClipboard(rec.link).then((wroteToClipboard) => + showMessage({ + level: MessageLevel.success, + message: rec.link, + description: wroteToClipboard + ? msg("A copy of this recovery link has been placed in your clipboard") + : "", + }), + ), + ) + .catch((ex: ResponseError) => + ex.response.json().then(() => + showMessage({ + level: MessageLevel.error, + message: msg( + "The current tenant must have a recovery flow configured to use a recovery link", + ), + }), + ), + ); + +export const renderRecoveryEmailRequest = (user: User) => + html` + ${msg("Send link")} + ${msg("Send recovery link to user")} + + + `; + +const recoveryButtonStyles = css` + #recovery-request-buttons { + display: flex; + flex-direction: row; + flex-wrap: wrap; + gap: 0.375rem; + } + #recovery-request-buttons > *, + #update-password-request .pf-c-button { + margin: 0; + } +`; + @customElement("ak-user-list") export class UserListPage extends TablePage { expandable = true; @@ -74,7 +125,7 @@ export class UserListPage extends TablePage { me?: SessionUser; static get styles(): CSSResult[] { - return super.styles.concat(PFDescriptionList, PFCard, PFAlert); + return [...super.styles, PFDescriptionList, PFCard, PFAlert, recoveryButtonStyles]; } constructor() { @@ -287,8 +338,14 @@ export class UserListPage extends TablePage { ${msg("Recovery")}
-
- +
+ ${msg("Update password")} ${msg("Update password")} { ? html` { - return new CoreApi(DEFAULT_CONFIG) - .coreUsersRecoveryRetrieve({ - id: item.pk, - }) - .then((rec) => { - showMessage({ - level: MessageLevel.success, - message: msg( - "Successfully generated recovery link", - ), - description: rec.link, - }); - }) - .catch((ex: ResponseError) => { - ex.response.json().then(() => { - showMessage({ - level: MessageLevel.error, - message: msg( - "No recovery flow is configured.", - ), - }); - }); - }); - }} + .apiRequest=${() => requestRecoveryLink(item)} > - ${msg("Copy recovery link")} + ${msg("Create recovery link")} ${item.email - ? html` - - ${msg("Send link")} - - - ${msg("Send recovery link to user")} - - - - - ` + ? renderRecoveryEmailRequest(item) : html`${msg( "Recovery link cannot be emailed, user has no email address saved.", diff --git a/web/src/admin/users/UserViewPage.ts b/web/src/admin/users/UserViewPage.ts index c97a20298..64a917c9d 100644 --- a/web/src/admin/users/UserViewPage.ts +++ b/web/src/admin/users/UserViewPage.ts @@ -5,11 +5,14 @@ import "@goauthentik/admin/users/UserForm"; import "@goauthentik/admin/users/UserPasswordForm"; import "@goauthentik/app/admin/users/UserAssignedGlobalPermissionsTable"; import "@goauthentik/app/admin/users/UserAssignedObjectPermissionsTable"; +import { + renderRecoveryEmailRequest, + requestRecoveryLink, +} from "@goauthentik/app/admin/users/UserListPage"; import { me } from "@goauthentik/app/common/users"; import "@goauthentik/app/elements/rbac/ObjectPermissionsPage"; import { DEFAULT_CONFIG } from "@goauthentik/common/api/config"; import { EVENT_REFRESH } from "@goauthentik/common/constants"; -import { MessageLevel } from "@goauthentik/common/messages"; import "@goauthentik/components/events/ObjectChangelog"; import "@goauthentik/components/events/UserEvents"; import { AKElement, rootInterface } from "@goauthentik/elements/Base"; @@ -21,13 +24,12 @@ import "@goauthentik/elements/Tabs"; import "@goauthentik/elements/buttons/ActionButton"; import "@goauthentik/elements/buttons/SpinnerButton"; import "@goauthentik/elements/forms/ModalForm"; -import { showMessage } from "@goauthentik/elements/messages/MessageContainer"; import "@goauthentik/elements/oauth/UserRefreshList"; import "@goauthentik/elements/user/SessionList"; import "@goauthentik/elements/user/UserConsentList"; import { msg, str } from "@lit/localize"; -import { CSSResult, TemplateResult, css, html } from "lit"; +import { css, html, nothing } from "lit"; import { customElement, property, state } from "lit/decorators.js"; import PFButton from "@patternfly/patternfly/components/Button/button.css"; @@ -72,7 +74,7 @@ export class UserViewPage extends AKElement { @state() me?: SessionUser; - static get styles(): CSSResult[] { + static get styles() { return [ PFBase, PFPage, @@ -84,12 +86,24 @@ export class UserViewPage extends AKElement { PFDescriptionList, PFSizing, css` - .pf-c-description-list__description ak-action-button { - margin-right: 6px; - margin-bottom: 6px; - } .ak-button-collection { - max-width: 12em; + display: flex; + flex-direction: column; + gap: 0.375rem; + max-width: 12rem; + } + .ak-button-collection > * { + flex: 1 0 100%; + } + #reset-password-button { + margin-right: 0; + } + + #ak-email-recovery-request, + #update-password-request .pf-c-button, + #ak-email-recovery-request .pf-c-button { + margin: 0; + width: 100%; } `, ]; @@ -103,7 +117,7 @@ export class UserViewPage extends AKElement { }); } - render(): TemplateResult { + render() { return html`${msg("User Info")}
@@ -129,7 +147,7 @@ export class UserViewPage extends AKElement { ${msg("Username")}
-
${this.user.username}
+
${user.username}
@@ -137,7 +155,7 @@ export class UserViewPage extends AKElement { ${msg("Name")}
-
${this.user.name}
+
${user.name}
@@ -145,7 +163,7 @@ export class UserViewPage extends AKElement { ${msg("Email")}
-
${this.user.email || "-"}
+
${user.email || "-"}
@@ -154,7 +172,7 @@ export class UserViewPage extends AKElement {
- ${this.user.lastLogin?.toLocaleString()} + ${user.lastLogin?.toLocaleString()}
@@ -165,7 +183,7 @@ export class UserViewPage extends AKElement {
@@ -177,7 +195,7 @@ export class UserViewPage extends AKElement {
@@ -191,7 +209,7 @@ export class UserViewPage extends AKElement { ${msg("Update")} ${msg("Update User")} - + @@ -238,7 +254,7 @@ export class UserViewPage extends AKElement { .apiRequest=${() => { return new CoreApi(DEFAULT_CONFIG) .coreUsersImpersonateCreate({ - id: this.user?.pk || 0, + id: user.pk, }) .then(() => { window.location.href = "/"; @@ -255,7 +271,7 @@ export class UserViewPage extends AKElement { ` - : html``} + : nothing} @@ -265,12 +281,12 @@ export class UserViewPage extends AKElement {
- + ${msg("Update password")} ${msg("Update password")}
@@ -329,9 +323,9 @@ export class UserViewPage extends AKElement { `; } - renderBody(): TemplateResult { + renderBody() { if (!this.user) { - return html``; + return nothing; } return html`
Date: Fri, 20 Oct 2023 20:37:52 +0200 Subject: [PATCH 2/2] sources/oauth: fix oidc well-known parsing (#7248) --- authentik/sources/oauth/api/source.py | 15 ++++++--------- authentik/sources/oauth/tasks.py | 2 +- authentik/sources/oauth/tests/test_views.py | 1 + 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/authentik/sources/oauth/api/source.py b/authentik/sources/oauth/api/source.py index 62941f863..1a350bd43 100644 --- a/authentik/sources/oauth/api/source.py +++ b/authentik/sources/oauth/api/source.py @@ -71,15 +71,12 @@ class OAuthSourceSerializer(SourceSerializer): text = exc.response.text if exc.response else str(exc) raise ValidationError({"oidc_well_known_url": text}) config = well_known_config.json() - try: - attrs["authorization_url"] = config["authorization_endpoint"] - attrs["access_token_url"] = config["token_endpoint"] - attrs["profile_url"] = config["userinfo_endpoint"] - inferred_oidc_jwks_url = config["jwks_uri"] - except (IndexError, KeyError) as exc: - raise ValidationError( - {"oidc_well_known_url": f"Invalid well-known configuration: {exc}"} - ) + if "issuer" not in config: + raise ValidationError({"oidc_well_known_url": "Invalid well-known configuration"}) + attrs["authorization_url"] = config.get("authorization_endpoint", "") + attrs["access_token_url"] = config.get("token_endpoint", "") + attrs["profile_url"] = config.get("userinfo_endpoint", "") + inferred_oidc_jwks_url = config.get("jwks_uri", "") # Prefer user-entered URL to inferred URL to default URL jwks_url = attrs.get("oidc_jwks_url") or inferred_oidc_jwks_url or source_type.oidc_jwks_url diff --git a/authentik/sources/oauth/tasks.py b/authentik/sources/oauth/tasks.py index 1588117b9..6197df512 100644 --- a/authentik/sources/oauth/tasks.py +++ b/authentik/sources/oauth/tasks.py @@ -38,7 +38,7 @@ def update_well_known_jwks(self: MonitoredTask): for source_attr, config_key in source_attr_key: # Check if we're actually changing anything to only # save when something has changed - if getattr(source, source_attr) != config[config_key]: + if getattr(source, source_attr, "") != config[config_key]: dirty = True setattr(source, source_attr, config[config_key]) except (IndexError, KeyError) as exc: diff --git a/authentik/sources/oauth/tests/test_views.py b/authentik/sources/oauth/tests/test_views.py index 2e1919c17..16e57c057 100644 --- a/authentik/sources/oauth/tests/test_views.py +++ b/authentik/sources/oauth/tests/test_views.py @@ -50,6 +50,7 @@ class TestOAuthSource(TestCase): def test_api_validate_openid_connect(self): """Test API validation (with OIDC endpoints)""" openid_config = { + "issuer": "foo", "authorization_endpoint": "http://mock/oauth/authorize", "token_endpoint": "http://mock/oauth/token", "userinfo_endpoint": "http://mock/oauth/userinfo",