From 70aaf70b2ef4b06bfb62e0f79441f61daafe4ede Mon Sep 17 00:00:00 2001 From: FoxxMD Date: Thu, 13 Aug 2026 15:36:47 +0000 Subject: [PATCH] refactor: Strongly type and simplify auth error logic * Use separate errors for init auth check throws and root auth error throws (to be implemented in each component) * Consolidate auth issue discovery to one reusable function * rethrow on testAuth based on auth error type and unrecoverable flag --- src/backend/common/AbstractInitializable.ts | 65 ++++++++++++--------- src/backend/common/errors/MSErrors.ts | 46 ++++++++++++++- 2 files changed, 80 insertions(+), 31 deletions(-) diff --git a/src/backend/common/AbstractInitializable.ts b/src/backend/common/AbstractInitializable.ts index 3a0591f6..ac2513c1 100644 --- a/src/backend/common/AbstractInitializable.ts +++ b/src/backend/common/AbstractInitializable.ts @@ -3,12 +3,10 @@ import {truncateStringToLength } from "../../core/StringUtils.ts"; import { hasNodeNetworkException } from "./errors/NodeErrors.ts"; import { hasUpstreamError } from "./errors/UpstreamError.ts"; import type {WebhookPayload} from "./infrastructure/config/health/webhooks.ts"; -import { AuthCheckError, BuildDataError, ConnectionCheckError, ParseCacheError, PostInitError, StageError } from "./errors/MSErrors.ts"; +import { AuthCheckError, AuthError, BuildDataError, ConnectionCheckError, findAuthIssue, ParseCacheError, PostInitError, StageError, type AuthErrorMap } from "./errors/MSErrors.ts"; import { messageWithCausesTruncatedDefault } from "../../core/ErrorUtils.ts"; import { spawn } from 'abort-controller-x'; import { COMPONENT_AUTH_TYPE, type ComponentAuthType } from "../../core/Atomic.ts"; -import { findCauseByFunc } from "../utils/ErrorUtils.ts"; - export default abstract class AbstractInitializable { requiresAuth: boolean = false; requiresAuthInteraction: boolean = false; @@ -290,26 +288,19 @@ export default abstract class AbstractInitializable { canTryAuth = () => this.isUsable() && this.authGated() && !this.hasUnrecoverableAuthFailure() - findAuthFailure = () => this.errors.find(x => findCauseByFunc(x, (e) => e.name === 'Authentication Check' || e instanceof AuthCheckError)); - - hasAuthFailure = () => this.findAuthFailure !== undefined; - - findUnrecoverableAuthFailure = (): AuthCheckError | undefined => { - const err = this.findAuthFailure(); - if(err !== undefined) { - const authErr = findCauseByFunc(err, (e) => e.name === 'Authentication Check' || e instanceof AuthCheckError); - if(authErr === undefined) { - // this shouldn't happen? - return undefined; - } - if((authErr as AuthCheckError).unrecoverable) { - return authErr as AuthCheckError; + findAuthIssue = (opts: Parameters>[1] = {}): AuthErrorMap[T] | undefined => { + for(const err of this.errors) { + const authError = findAuthIssue(err, opts); + if(authError !== undefined) { + return authError; } } return undefined; } - hasUnrecoverableAuthFailure = () => this.findUnrecoverableAuthFailure() !== undefined; + hasAuthIssue = () => this.findAuthIssue() !== undefined; + + hasUnrecoverableAuthFailure = () => this.findAuthIssue({unrecoverable: true}) !== undefined; canAuthUnattended = () => !this.authGated() || this.authType === COMPONENT_AUTH_TYPE.unattended || (this.authType === COMPONENT_AUTH_TYPE.interactive && !this.hasUnrecoverableAuthFailure()) ; @@ -327,18 +318,26 @@ export default abstract class AbstractInitializable { this.logger.debug('Auth OK but step was forced'); } - const unrecoverableAuth = this.findUnrecoverableAuthFailure(); + // only throw *before* testing if we have previously tested and the error was unrecoverable + // there is no reason to constantly retry auth when we already know it won't succeed + // + // test retries should be forced by api calls and auth callback flows *after* we have made changes to component credentials + const unrecoverableAuth = this.findAuthIssue({unrecoverable: true}); if(unrecoverableAuth !== undefined) { + let unrecoverable: boolean | undefined, + cause: Error; if(!force) { - if(unrecoverableAuth.cause === undefined || !(unrecoverableAuth.cause instanceof AuthCheckError)) { - if(this.requiresAuthInteraction) { - throw new AuthCheckError('Authentication failure: Will not retry auth because user interaction is required for authentication', {cause: unrecoverableAuth, unrecoverable: true}); - } - throw new AuthCheckError('Authentication failure: Will not retry auth because authentication previously failed and must be reauthenticated', {cause: unrecoverableAuth, unrecoverable: true}); + if(unrecoverableAuth instanceof AuthCheckError) { + // we threw a fallback AuthCheckError + unrecoverable = true; + cause = unrecoverableAuth.cause as Error; } else { - // this is one of the errors above and we retrying n+1 times so just keep throwing the same error - throw unrecoverableAuth; + cause = unrecoverableAuth; + } + if(this.authType === COMPONENT_AUTH_TYPE.interactive) { + throw new AuthCheckError('Authentication failure: Will not retry auth because user interaction is required for authentication', {cause, unrecoverable}); } + throw new AuthCheckError('Authentication failure: Will not retry auth because authentication previously failed and must be reauthenticated', {cause, unrecoverable}); } this.logger.debug('Auth previously failed for non upstream/network reasons but retry is being forced'); } @@ -347,10 +346,18 @@ export default abstract class AbstractInitializable { await this.doAuthentication(); this.authed = true; } catch (e) { - // only signal as auth failure if error was NOT either a node network error or a non-showstopping upstream error - const unrecoverable = !(hasNodeNetworkException(e) || hasUpstreamError(e, false)); + let unrecoverableMsg: boolean, + unrecoverable: boolean | undefined; + if(e instanceof AuthError) { + unrecoverableMsg = e.unrecoverable; + } else { + // if component auth test didn't throw AuthError (it should have!) + // then fallback teo determining by these conditions + unrecoverable = !(hasNodeNetworkException(e) || hasUpstreamError(e, false)); + unrecoverableMsg = unrecoverable; + } this.authed = false; - throw new AuthCheckError(`Authentication test failed!${unrecoverable === false ? ' Due to a network issue. Will retry authentication on next heartbeat.' : ''}`, {cause: e, unrecoverable}); + throw new AuthCheckError(`Authentication test failed!${unrecoverableMsg === false ? ' Due to a network issue. Will retry authentication on next heartbeat.' : ''}`, {cause: e, unrecoverable}); } } diff --git a/src/backend/common/errors/MSErrors.ts b/src/backend/common/errors/MSErrors.ts index cb21532d..b10f72c7 100644 --- a/src/backend/common/errors/MSErrors.ts +++ b/src/backend/common/errors/MSErrors.ts @@ -33,8 +33,8 @@ export class ConnectionCheckError extends StageError { } addKnownErrorConstructor(ConnectionCheckError); -export class AuthCheckError extends StageError { - override name = 'Authentication Check'; +export class AuthError extends NamedError { + override name = 'Authentication Error'; unrecoverable: boolean = false; public constructor(msg?: string, opts: ErrorOptions & {unrecoverable?: boolean} = {}) { @@ -42,7 +42,19 @@ export class AuthCheckError extends StageError { this.unrecoverable = opts.unrecoverable ?? false; } } +addKnownErrorConstructor(AuthError); +export const isAuthError = (e: Error): e is AuthError => e.name === 'Authentication Error' || e instanceof AuthError; +export class AuthCheckError extends StageError { + override name = 'Authentication Check'; + unrecoverable?: boolean; + + public constructor(msg?: string, opts: ErrorOptions & {unrecoverable?: boolean} = {}) { + super(msg, opts); + this.unrecoverable = opts.unrecoverable; + } +} addKnownErrorConstructor(AuthCheckError); +export const isAuthCheckError = (e: Error): e is AuthCheckError => e.name === 'Authentication Check' || e instanceof AuthCheckError; export class PostInitError extends StageError { name = 'Post Initialization'; @@ -206,3 +218,33 @@ export class InvalidRegexError extends SimpleError { } } addKnownErrorConstructor(InvalidRegexError, () => new InvalidRegexError(new RegExp(/1/))); + +export type AuthErrorMap = { + root: AuthError + check: AuthCheckError + auth: AuthError | AuthCheckError + parent: Error +} + +export const findAuthIssue = (err: Error, opts: { type?: T, unrecoverable?: boolean } = {}): AuthErrorMap[T] | undefined => { + const { + type = 'auth', + unrecoverable + } = opts; + const authError = findCauseByFunc(err, (e) => { + if (['check','auth'].includes(type) && isAuthCheckError(e)) { + return unrecoverable === undefined || e.unrecoverable === unrecoverable; + } + if (['root','auth'].includes(type) && isAuthError(e)) { + return unrecoverable === undefined || e.unrecoverable === unrecoverable; + } + return false; + }); + if (authError !== undefined) { + if (type === 'parent') { + return err as AuthErrorMap[T]; + } + return authError as AuthErrorMap[T]; + } + return undefined; +} \ No newline at end of file -- 2.51.2