Skip to content

web: Normalize client-side error handling #13595

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 7, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions web/src/admin/DebugPage.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { DEFAULT_CONFIG } from "@goauthentik/common/api/config";
import { parseAPIResponseError, pluckErrorDetail } from "@goauthentik/common/errors/network";
import { MessageLevel } from "@goauthentik/common/messages";
import { AKElement } from "@goauthentik/elements/Base";
import "@goauthentik/elements/PageHeader";
Expand Down Expand Up @@ -54,10 +55,12 @@ export class DebugPage extends AKElement {
message: "Success",
});
})
.catch((exc) => {
.catch(async (error) => {
const parsedError = await parseAPIResponseError(error);

showMessage({
level: MessageLevel.error,
message: exc,
message: pluckErrorDetail(parsedError),
});
});
}}
Expand Down
23 changes: 13 additions & 10 deletions web/src/admin/admin-overview/cards/AdminStatusCard.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import { EVENT_REFRESH } from "@goauthentik/common/constants";
import { PFSize } from "@goauthentik/common/enums.js";
import {
APIError,
parseAPIResponseError,
pluckErrorDetail,
} from "@goauthentik/common/errors/network";
import { AggregateCard } from "@goauthentik/elements/cards/AggregateCard";

import { msg } from "@lit/localize";
import { PropertyValues, TemplateResult, html, nothing } from "lit";
import { state } from "lit/decorators.js";

import { ResponseError } from "@goauthentik/api";

export interface AdminStatus {
icon: string;
message?: TemplateResult;
Expand All @@ -29,7 +32,7 @@ export abstract class AdminStatusCard<T> extends AggregateCard {

// Current error state if any request fails
@state()
protected error?: string;
protected error?: APIError;

// Abstract methods to be implemented by subclasses
abstract getPrimaryValue(): Promise<T>;
Expand Down Expand Up @@ -59,9 +62,9 @@ export abstract class AdminStatusCard<T> extends AggregateCard {
this.value = value; // Triggers shouldUpdate
this.error = undefined;
})
.catch((err: ResponseError) => {
.catch(async (error: unknown) => {
this.status = undefined;
this.error = err?.response?.statusText ?? msg("Unknown error");
this.error = await parseAPIResponseError(error);
});
}

Expand All @@ -79,9 +82,9 @@ export abstract class AdminStatusCard<T> extends AggregateCard {
this.status = status;
this.error = undefined;
})
.catch((err: ResponseError) => {
.catch(async (error: unknown) => {
this.status = undefined;
this.error = err?.response?.statusText ?? msg("Unknown error");
this.error = await parseAPIResponseError(error);
});

// Prevent immediate re-render if only value changed
Expand Down Expand Up @@ -120,8 +123,8 @@ export abstract class AdminStatusCard<T> extends AggregateCard {
*/
private renderError(error: string): TemplateResult {
return html`
<p><i class="fa fa-times"></i>&nbsp;${error}</p>
<p class="subtext">${msg("Failed to fetch")}</p>
<p><i class="fa fa-times"></i>&nbsp;${msg("Failed to fetch")}</p>
<p class="subtext">${error}</p>
`;
}

Expand All @@ -146,7 +149,7 @@ export abstract class AdminStatusCard<T> extends AggregateCard {
this.status
? this.renderStatus(this.status) // Status available
: this.error
? this.renderError(this.error) // Error state
? this.renderError(pluckErrorDetail(this.error)) // Error state
: this.renderLoading() // Loading state
}
</p>
Expand Down
9 changes: 7 additions & 2 deletions web/src/admin/admin-overview/cards/RecentEventsCard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import "@goauthentik/elements/buttons/ModalButton";
import "@goauthentik/elements/buttons/SpinnerButton";
import { PaginatedResponse } from "@goauthentik/elements/table/Table";
import { Table, TableColumn } from "@goauthentik/elements/table/Table";
import { SlottedTemplateResult } from "@goauthentik/elements/types";

import { msg } from "@lit/localize";
import { CSSResult, TemplateResult, css, html } from "lit";
Expand Down Expand Up @@ -68,7 +69,7 @@ export class RecentEventsCard extends Table<Event> {
</div>`;
}

row(item: EventWithContext): TemplateResult[] {
row(item: EventWithContext): SlottedTemplateResult[] {
return [
html`<div><a href="${`#/events/log/${item.pk}`}">${actionToLabel(item.action)}</a></div>
<small>${item.app}</small>`,
Expand All @@ -81,7 +82,11 @@ export class RecentEventsCard extends Table<Event> {
];
}

renderEmpty(): TemplateResult {
renderEmpty(inner?: SlottedTemplateResult): TemplateResult {
if (this.error) {
return super.renderEmpty(inner);
}

Comment on lines -84 to +89
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes issue where errors don't render when the empty state is active. IMO this needs fixing in the base Table class but that refactor will need a PR of its own.

return super.renderEmpty(
html`<ak-empty-state header=${msg("No Events found.")}>
<div slot="body">${msg("No matching events could be found.")}</div>
Expand Down
8 changes: 4 additions & 4 deletions web/src/admin/applications/wizard/ApplicationWizardStep.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { styles } from "@goauthentik/admin/applications/wizard/ApplicationWizardFormStepStyles.css.js";
import { WizardStep } from "@goauthentik/components/ak-wizard/WizardStep.js";
import {
NavigationUpdate,
NavigationEventInit,
WizardNavigationEvent,
WizardUpdateEvent,
} from "@goauthentik/components/ak-wizard/events";
Expand All @@ -14,9 +14,9 @@ import { property, query } from "lit/decorators.js";
import { ValidationError } from "@goauthentik/api";

import {
ApplicationTransactionValidationError,
type ApplicationWizardState,
type ApplicationWizardStateUpdate,
ExtendedValidationError,
} from "./types";

export class ApplicationWizardStep extends WizardStep {
Expand Down Expand Up @@ -48,7 +48,7 @@ export class ApplicationWizardStep extends WizardStep {
}

protected removeErrors(
keyToDelete: keyof ExtendedValidationError,
keyToDelete: keyof ApplicationTransactionValidationError,
): ValidationError | undefined {
if (!this.wizard.errors) {
return undefined;
Expand All @@ -71,7 +71,7 @@ export class ApplicationWizardStep extends WizardStep {
public handleUpdate(
update?: ApplicationWizardStateUpdate,
destination?: string,
enable?: NavigationUpdate,
enable?: NavigationEventInit,
) {
// Inform ApplicationWizard of content state
if (update) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import "@goauthentik/admin/applications/wizard/ak-wizard-title.js";
import { DEFAULT_CONFIG } from "@goauthentik/common/api/config";
import { EVENT_REFRESH } from "@goauthentik/common/constants";
import { parseAPIError } from "@goauthentik/common/errors";
import { parseAPIResponseError } from "@goauthentik/common/errors/network";
import { WizardNavigationEvent } from "@goauthentik/components/ak-wizard/events.js";
import { type WizardButton } from "@goauthentik/components/ak-wizard/types";
import { showAPIErrorMessage } from "@goauthentik/elements/messages/MessageContainer";
import { CustomEmitterElement } from "@goauthentik/elements/utils/eventEmitter";
import { P, match } from "ts-pattern";

Expand All @@ -30,10 +31,11 @@ import {
type TransactionApplicationRequest,
type TransactionApplicationResponse,
type TransactionPolicyBindingRequest,
instanceOfValidationError,
} from "@goauthentik/api";

import { ApplicationWizardStep } from "../ApplicationWizardStep.js";
import { ExtendedValidationError, OneOfProvider } from "../types.js";
import { OneOfProvider, isApplicationTransactionValidationError } from "../types.js";
import { providerRenderers } from "./SubmitStepOverviewRenderers.js";

const _submitStates = ["reviewing", "running", "submitted"] as const;
Expand Down Expand Up @@ -131,39 +133,46 @@ export class ApplicationWizardSubmitStep extends CustomEmitterElement(Applicatio

this.state = "running";

return (
new CoreApi(DEFAULT_CONFIG)
.coreTransactionalApplicationsUpdate({
transactionApplicationRequest: request,
})
.then((_response: TransactionApplicationResponse) => {
this.dispatchCustomEvent(EVENT_REFRESH);
this.state = "submitted";
})

// eslint-disable-next-line @typescript-eslint/no-explicit-any
.catch(async (resolution: any) => {
const errors = (await parseAPIError(
await resolution,
)) as ExtendedValidationError;

// THIS is a really gross special case; if the user is duplicating the name of
// an existing provider, the error appears on the `app` (!) error object. We
// have to move that to the `provider.name` error field so it shows up in the
// right place.
if (Array.isArray(errors?.app?.provider)) {
const providerError = errors.app.provider;
errors.provider = errors.provider ?? {};
errors.provider.name = providerError;
delete errors.app.provider;
if (Object.keys(errors.app).length === 0) {
delete errors.app;
return new CoreApi(DEFAULT_CONFIG)
.coreTransactionalApplicationsUpdate({
transactionApplicationRequest: request,
})
.then((_response: TransactionApplicationResponse) => {
this.dispatchCustomEvent(EVENT_REFRESH);
this.state = "submitted";
})

.catch(async (error) => {
const parsedError = await parseAPIResponseError(error);

if (!instanceOfValidationError(parsedError)) {
showAPIErrorMessage(parsedError);

return;
}

if (isApplicationTransactionValidationError(parsedError)) {
// THIS is a really gross special case; if the user is duplicating the name of an existing provider, the error appears on the `app` (!) error object.
// We have to move that to the `provider.name` error field so it shows up in the right place.
if (Array.isArray(parsedError.app?.provider)) {
const providerError = parsedError.app.provider;

parsedError.provider = {
...parsedError.provider,
name: providerError,
};

delete parsedError.app.provider;

if (Object.keys(parsedError.app).length === 0) {
delete parsedError.app;
}
}
this.handleUpdate({ errors });
this.state = "reviewing";
})
);
}

this.handleUpdate({ errors: parsedError });
this.state = "reviewing";
});
}

override handleButton(button: WizardButton) {
Expand Down Expand Up @@ -225,36 +234,41 @@ export class ApplicationWizardSubmitStep extends CustomEmitterElement(Applicatio
}

renderError() {
if (Object.keys(this.wizard.errors).length === 0) {
return nothing;
}
const { errors } = this.wizard;

if (Object.keys(errors).length === 0) return nothing;

const navTo = (step: string) => () => this.dispatchEvent(new WizardNavigationEvent(step));
const errors = this.wizard.errors;
return html` <hr class="pf-c-divider" />
${match(errors as ExtendedValidationError)
${match(errors)
.with(
{ app: P.nonNullable },
() =>
html`<p>${msg("There was an error in the application.")}</p>
<p>
<a @click=${navTo("application")}
>${msg("Review the application.")}</a
>
<a @click=${WizardNavigationEvent.toListener(this, "application")}>
${msg("Review the application.")}
</a>
</p>`,
)
.with(
{ provider: P.nonNullable },
() =>
html`<p>${msg("There was an error in the provider.")}</p>
<p>
<a @click=${navTo("provider")}>${msg("Review the provider.")}</a>
<a @click=${WizardNavigationEvent.toListener(this, "provider")}
>${msg("Review the provider.")}</a
>
</p>`,
)
.with(
{ detail: P.nonNullable },
() =>
`<p>${msg("There was an error. Please go back and review the application.")}: ${errors.detail}</p>`,
html`<p>
${msg(
"There was an error. Please go back and review the application.",
)}:
${errors.detail}
</p>`,
)
.with(
{
Expand All @@ -264,7 +278,7 @@ export class ApplicationWizardSubmitStep extends CustomEmitterElement(Applicatio
html`<p>${msg("There was an error:")}:</p>
<ul>
${(errors.nonFieldErrors ?? []).map(
(e: string) => html`<li>${e}</li>`,
(reason) => html`<li>${reason}</li>`,
)}
</ul>
<p>${msg("Please go back and review the application.")}</p>`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { customElement, state } from "lit/decorators.js";
import { OAuth2ProviderRequest, SourcesApi } from "@goauthentik/api";
import { type OAuth2Provider, type PaginatedOAuthSourceList } from "@goauthentik/api";

import { ExtendedValidationError } from "../../types.js";
import { ApplicationTransactionValidationError } from "../../types.js";
import { ApplicationWizardProviderForm } from "./ApplicationWizardProviderForm.js";

@customElement("ak-application-wizard-provider-for-oauth")
Expand All @@ -34,7 +34,7 @@ export class ApplicationWizardOauth2ProviderForm extends ApplicationWizardProvid
});
}

renderForm(provider: OAuth2Provider, errors: ExtendedValidationError) {
renderForm(provider: OAuth2Provider, errors: ApplicationTransactionValidationError) {
const showClientSecretCallback = (show: boolean) => {
this.showClientSecret = show;
};
Expand Down
30 changes: 22 additions & 8 deletions web/src/admin/applications/wizard/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,30 @@ export type OneOfProvider =

export type ValidationRecord = { [key: string]: string[] };

// TODO: Elf, extend this type and apply it to every object in the wizard. Then run
// the type-checker again.

export type ExtendedValidationError = ValidationError & {
/**
* An error that occurs during the creation or modification of an application.
*
* @todo (Elf) Extend this type to include all possible errors that can occur during the creation or modification of an application.
*/
export interface ApplicationTransactionValidationError extends ValidationError {
app?: ValidationRecord;
provider?: ValidationRecord;
bindings?: ValidationRecord;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
detail?: any;
};
detail?: unknown;
}

/**
* Type-guard to determine if an API response is shaped like an {@linkcode ApplicationTransactionValidationError}.
*/
export function isApplicationTransactionValidationError(
error: ValidationError,
): error is ApplicationTransactionValidationError {
if ("app" in error) return true;
if ("provider" in error) return true;
if ("bindings" in error) return true;

return false;
}

// We use the PolicyBinding instead of the PolicyBindingRequest here, because that gives us a slot
// in which to preserve the retrieved policy, group, or user object from the SearchSelect used to
Expand All @@ -49,7 +63,7 @@ export interface ApplicationWizardState {
proxyMode: ProxyMode;
bindings: PolicyBinding[];
currentBinding: number;
errors: ExtendedValidationError;
errors: ValidationError | ApplicationTransactionValidationError;
}

export interface ApplicationWizardStateUpdate {
Expand Down
Loading
Loading