From 73dbbdcb280f6b1d845f60d992571d218e89a63d Mon Sep 17 00:00:00 2001 From: NGPixel Date: Sat, 29 Aug 2026 22:11:08 -0400 Subject: [PATCH] fix: login race condition + login UI improvements --- backend/api/schemas/authentication.ts | 5 + backend/api/users.ts | 2 +- backend/locales/en.json | 1 - backend/models/users.ts | 4 +- frontend/public/_assets/icons/fluent-ai.svg | 184 +++++++++++++++++++- frontend/src/boot/api.js | 22 ++- frontend/src/components/AuthLoginPanel.vue | 153 +++++++++++++++- frontend/src/components/shared/WBtn.vue | 34 +++- frontend/src/css/_base.scss | 13 +- frontend/src/pages/AdminAuth.vue | 5 - 10 files changed, 404 insertions(+), 19 deletions(-) diff --git a/backend/api/schemas/authentication.ts b/backend/api/schemas/authentication.ts index e56a05461..40e6b5ae9 100644 --- a/backend/api/schemas/authentication.ts +++ b/backend/api/schemas/authentication.ts @@ -30,6 +30,11 @@ export async function registerSchemas(app: FastifyInstance): Promise { description: 'For `setupTfa` only: the `otpauth://` URI as an SVG QR code, to be rendered as-is.' }, + tfaSecret: { + type: 'string', + description: + 'For `setupTfa` only: the base32 secret that QR code encodes, for a user typing it into an authenticator app rather than scanning it.' + }, redirect: { type: 'string', description: 'Where to send the user once logged in. A path within this wiki, or a URL.' diff --git a/backend/api/users.ts b/backend/api/users.ts index 7aadf29cd..be1f157af 100644 --- a/backend/api/users.ts +++ b/backend/api/users.ts @@ -832,7 +832,7 @@ async function routes(app: FastifyInstance) { tfaSecret: { type: 'string', description: - 'The base32 secret the QR code encodes, for a user who would rather type it into an authenticator app than scan it. Only ever returned here, to the user setting 2FA up on their own account.' + 'The base32 secret the QR code encodes, for a user who would rather type it into an authenticator app than scan it. Returned only to the user setting 2FA up on their own account — here, and from the login flow that enforces it.' } } } diff --git a/backend/locales/en.json b/backend/locales/en.json index 6900b497d..f979a80d2 100644 --- a/backend/locales/en.json +++ b/backend/locales/en.json @@ -157,7 +157,6 @@ "admin.auth.registration": "Registration", "admin.auth.registrationHint": "Allow any user successfully authorized by the strategy to access the wiki.", "admin.auth.registrationLocalHint": "Whether to allow guests to register new accounts.", - "admin.auth.registrationNotEnforced": "Saved but not enforced yet: self-registration is not implemented.", "admin.auth.saveFailed": "Failed to save {strategy}.", "admin.auth.saveSuccess": "Authentication configuration saved successfully.", "admin.auth.security": "Security", diff --git a/backend/models/users.ts b/backend/models/users.ts index df5d915ee..15e824d30 100644 --- a/backend/models/users.ts +++ b/backend/models/users.ts @@ -226,6 +226,7 @@ export interface AfterLoginResult { nextAction: string continuationToken?: string tfaQRImage?: string + tfaSecret?: string redirect: string } @@ -1386,7 +1387,7 @@ class Users { */ } else if (str.conf?.enforceTfa || authStr.tfaRequired) { try { - const { tfaQRImage } = await this.startTfaSetup(user, strategyId, context.siteId) + const { secret, tfaQRImage } = await this.startTfaSetup(user, strategyId, context.siteId) const tfaToken = await this.generateToken({ kind: 'tfaSetup', userId: user.id, @@ -1401,6 +1402,7 @@ class Users { nextAction: 'setupTfa', continuationToken: tfaToken, tfaQRImage, + tfaSecret: secret, redirect } } catch (errc) { diff --git a/frontend/public/_assets/icons/fluent-ai.svg b/frontend/public/_assets/icons/fluent-ai.svg index 14a151717..41ae37795 100644 --- a/frontend/public/_assets/icons/fluent-ai.svg +++ b/frontend/public/_assets/icons/fluent-ai.svg @@ -1 +1,183 @@ - \ No newline at end of file + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/frontend/src/boot/api.js b/frontend/src/boot/api.js index 8eea3a11e..30663401a 100644 --- a/frontend/src/boot/api.js +++ b/frontend/src/boot/api.js @@ -14,7 +14,27 @@ export function initializeApi() { const client = ky.create({ prefix: '/_api', credentials: 'same-origin', - throwHttpErrors: (statusNumber) => statusNumber > 400 // Don't throw for 400 + throwHttpErrors: (statusNumber) => statusNumber > 400, // Don't throw for 400 + /* + ky retries by default, and both halves of that default are wrong for this API. + + `methods` includes `put` and `delete`, so a write that the server answered 500 or 503 to is + sent again, twice, with no way for the caller to know -- and every write here is a page save, + an upload or a login rather than something safe to repeat. Only GET and HEAD are. + + `statusCodes` includes 429, and 429 is in `afterStatusCodes`, so ky reads `Retry-After` and + SLEEPS for it before trying again -- uncapped, since `maxRetryAfter` defaults to Infinity. This + wiki's auth limiter answers a banned client with the remaining ban, up to 900 seconds + (`helpers/rateLimit.ts`), so one rate-limited login sat silently under the "Signing in..." + overlay for half an hour: two waits of fifteen minutes, no error, no redirect, and two further + attempts spent against the very limit that refused it. A 429 from this API is a decision with a + duration attached, not a blip -- it belongs in front of the user, not in a sleep. + */ + retry: { + methods: ['get', 'head'], + statusCodes: [408, 500, 502, 503, 504], + afterStatusCodes: [] + } }) if (import.meta.env.SSR) { diff --git a/frontend/src/components/AuthLoginPanel.vue b/frontend/src/components/AuthLoginPanel.vue index a8218fe9f..69ed8fd66 100644 --- a/frontend/src/components/AuthLoginPanel.vue +++ b/frontend/src/components/AuthLoginPanel.vue @@ -116,6 +116,7 @@ class="acrylic-btn w-full" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.passkeys.signin`)" no-caps icon="la:key" @@ -134,6 +135,7 @@ :key="str.id" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.actions.loginWith`, { provider: str.activeStrategy.displayName })" no-caps :icon="`img:` + str.activeStrategy.strategy.icon" @@ -147,6 +149,7 @@ v-if="selectedStrategy.activeStrategy.registration" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.switchToRegister.link`)" no-caps icon="la:user-plus" @@ -158,6 +161,7 @@ v-if="selectedStrategy.activeStrategy.allowForgotPassword" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.forgotPasswordLink`)" no-caps icon="la:life-ring" @@ -195,6 +199,7 @@ class="acrylic-btn w-full" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.forgotPasswordCancel`)" no-caps icon="la:arrow-circle-left" @@ -224,6 +229,7 @@ class="acrylic-btn w-full" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.switchToLogin.link`)" no-caps icon="la:arrow-circle-left" @@ -279,6 +285,7 @@ class="acrylic-btn w-full" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.switchToLogin.link`)" no-caps icon="la:arrow-circle-left" @@ -357,6 +364,7 @@ class="acrylic-btn w-full" flat color="primary" + :text-color="acrylicBtnTextColor" :label="t(`auth.switchToLogin.link`)" no-caps icon="la:arrow-circle-left" @@ -455,6 +463,36 @@
+ +
+ {{ t('auth.tfaSetupInstrManual') }} +
+
+ + {{ + groupedTfaSecret + }} + +

{{ t('auth.tfaSetupInstrSecond') }}

{ return browserSupportsWebAuthn() }) +/** The 2FA secret in groups of four, which is how a 32-character string stays readable to type. */ +const groupedTfaSecret = computed(() => state.tfaSecret.replace(/.{4}(?=.)/g, '$& ')) + +/* + What the labels on the panel's flat "acrylic" buttons are coloured -- the passkey and provider + buttons, and every link-like one at the foot of a screen. + + `primary` is a mid-tone picked to read on white, and this panel sits straight on the page rather + than in a card: white in light mode, `dark-6` in dark, where the same blue is the ~2.7:1 that + `--color-primary-light` exists for. Only the text: the buttons keep their `primary` fill, which is + a 10% tint that a lighter colour would wash out. + + Bound rather than written in the stylesheet because `WBtn` resolves both colours to inline styles, + which no rule outside it can beat without `!important`. +*/ +const acrylicBtnTextColor = computed(() => (dark.isActive ? 'primary-light' : 'primary')) + // VALIDATION RULES const loginUsernameValidation = [(val) => val.length > 0 || t('auth.errors.missingUsername')] @@ -803,6 +873,7 @@ async function handleLoginResponse(resp) { state.securityCode = '' state.screen = 'tfasetup' state.tfaQRImage = resp.tfaQRImage + state.tfaSecret = resp.tfaSecret loading.hide() break } @@ -854,6 +925,10 @@ async function handleLoginResponse(resp) { * LOGIN */ async function login() { + if (state.isSubmitting) { + return + } + state.isSubmitting = true clearError() loading.show({ message: t('auth.signingIn') @@ -881,6 +956,8 @@ async function login() { console.warn(err) loading.hide() showError(localizeError(apiErrorMessage(err), t)) + } finally { + state.isSubmitting = false } } @@ -888,6 +965,10 @@ async function login() { * LOGIN WITH PASSKEY */ async function loginWithPasskey() { + if (state.isSubmitting) { + return + } + state.isSubmitting = true clearError() loading.show({ message: t('auth.signingIn') @@ -919,6 +1000,8 @@ async function loginWithPasskey() { return } showError(localizeError(apiErrorMessage(err), t)) + } finally { + state.isSubmitting = false } } @@ -926,6 +1009,10 @@ async function loginWithPasskey() { * FORGOT PASSWORD */ async function forgotPassword() { + if (state.isSubmitting) { + return + } + state.isSubmitting = true clearError() loading.show({ message: t('auth.forgotPasswordLoading') @@ -955,6 +1042,8 @@ async function forgotPassword() { } catch (err) { loading.hide() showError(localizeError(apiErrorMessage(err), t)) + } finally { + state.isSubmitting = false } } @@ -966,6 +1055,10 @@ async function forgotPassword() { * success screen, and the login form is a link away from it, with the new password to type into it. */ async function resetPassword() { + if (state.isSubmitting) { + return + } + state.isSubmitting = true clearError() loading.show({ message: t('auth.changePwd.loading') @@ -994,6 +1087,8 @@ async function resetPassword() { } catch (err) { loading.hide() showError(localizeError(apiErrorMessage(err), t)) + } finally { + state.isSubmitting = false } } @@ -1001,6 +1096,10 @@ async function resetPassword() { * REGISTER */ async function register() { + if (state.isSubmitting) { + return + } + state.isSubmitting = true clearError() try { const isFormValid = await registerForm.value.validate(true) @@ -1031,6 +1130,8 @@ async function register() { } catch (err) { loading.hide() showError(localizeError(apiErrorMessage(err), t)) + } finally { + state.isSubmitting = false } } @@ -1038,6 +1139,10 @@ async function register() { * CHANGE PASSWORD */ async function changePwd() { + if (state.isSubmitting) { + return + } + state.isSubmitting = true clearError() try { const isFormValid = await changePwdForm.value.validate(true) @@ -1064,6 +1169,8 @@ async function changePwd() { } } catch (err) { showError(localizeError(apiErrorMessage(err), t)) + } finally { + state.isSubmitting = false } } @@ -1115,6 +1222,15 @@ async function handleTFAError(err) { } async function verifyTFA() { + /* + `continuationToken` as well as the flag: `submitTFA` spends the token on the way out, and the OTP + boxes keep their digits when the model behind them is cleared -- so an autofill that writes them + a second time re-emits a code the login it belonged to has already finished with. + */ + if (state.isSubmitting || !state.continuationToken) { + return + } + state.isSubmitting = true clearError() loading.show({ message: t('auth.signingIn') @@ -1123,6 +1239,28 @@ async function verifyTFA() { await handleLoginResponse(await submitTFA(false)) } catch (err) { await handleTFAError(err) + } finally { + state.isSubmitting = false + } +} + +/** + * Copy the setup key shown under the QR code, for typing it into an authenticator app by hand. + */ +async function copyTfaSecret() { + try { + // -> Without the display grouping: a space is harmless in most authenticator apps, but not all + await copyToClipboard(state.tfaSecret) + notify({ + type: 'positive', + message: t('auth.tfaSetupKeyCopied') + }) + } catch (err) { + notify({ + type: 'negative', + message: t('auth.tfaSetupKeyCopyFailed'), + caption: err.message + }) } } @@ -1130,6 +1268,11 @@ async function verifyTFA() { * FINISH TFA SETUP */ async function finishSetupTFA() { + // -> As `verifyTFA`: no token left means the code in the boxes belongs to a finished login + if (state.isSubmitting || !state.continuationToken) { + return + } + state.isSubmitting = true clearError() loading.show({ message: t('auth.tfaSetupVerifying') @@ -1143,6 +1286,8 @@ async function finishSetupTFA() { await handleLoginResponse(resp) } catch (err) { await handleTFAError(err) + } finally { + state.isSubmitting = false } } @@ -1155,6 +1300,10 @@ async function finishSetupTFA() { * the paths through `cancelVerifyEmail()`. */ async function confirmEmail() { + if (state.isSubmitting) { + return + } + state.isSubmitting = true clearError() loading.show({ message: t('auth.verifyEmail.loading') @@ -1179,6 +1328,8 @@ async function confirmEmail() { cancelVerifyEmail() // -> After the screen change, which clears the box this is about to fill showError(localizeError(apiErrorMessage(err), t)) + } finally { + state.isSubmitting = false } } diff --git a/frontend/src/components/shared/WBtn.vue b/frontend/src/components/shared/WBtn.vue index 1353658ea..e3f56e16c 100644 --- a/frontend/src/components/shared/WBtn.vue +++ b/frontend/src/components/shared/WBtn.vue @@ -46,13 +46,19 @@ const props = defineProps({ }, /** * Theme or palette color name (`primary`, `negative`, `grey-7`, ...), resolved against the - * Tailwind color variables. Drives the background for solid variants, the text for flat ones. + * Tailwind color variables. The button's own color: the background for solid variants, and the + * hover tint for unfilled ones (`acrylic-btn` builds its resting fill from it too). */ color: { type: String, default: null }, - /** Overrides the foreground color independently of `color`. */ + /** + * The label and icon color, when it is not the button's. Defaults to `color` on an unfilled + * button and to white on a solid one, which is what every caller that names only `color` gets -- + * so this is only ever set where the two genuinely differ, as on the login screen, where the + * flat buttons keep a `primary` fill under a lighter `primary-light` label. + */ textColor: { type: String, default: null @@ -222,8 +228,12 @@ const classes = computed(() => [ isSolid.value && !props.unelevated ? 'shadow-card' : '', props.outline ? 'border border-current' : '', isDisabled.value ? 'pointer-events-none opacity-60' : 'cursor-pointer', - // -> Flat buttons have no background of their own, so hover tints with the current text color - isSolid.value ? 'hover:brightness-110' : 'hover:bg-current/10', + /* + Flat buttons have no background of their own, so hover tints with the button's own color rather + than with `currentcolor` -- which is the label, and on a button whose `textColor` differs from + its `color` those are two different answers. + */ + isSolid.value ? 'hover:brightness-110' : 'hover:bg-(--w-btn-tint)', props.glossy ? 'w-glossy' : '' ]) @@ -251,6 +261,22 @@ const styles = computed(() => { out.padding = `${SIZES[v] ?? v} ${SIZES[h] ?? h}` } + /* + The button's own color, for the tints that are drawn from it: the hover fill above, and + `acrylic-btn`'s resting one in `css/_base.scss`. Always set, so neither has to name a fallback, + and `currentcolor` where there is no `color` -- which is what both used unconditionally before + text and fill could differ. + */ + out['--w-btn-color'] = props.color ? `var(--color-${props.color})` : 'currentcolor' + + /* + The hover tint, as a variable rather than as `bg-[color-mix(...)]` in the class list: Tailwind + reads an arbitrary `color-mix()` as a color it cannot resolve and emits `background-color: + var(--w-btn-color)` ahead of it as the pre-`color-mix` fallback -- an opaque fill under a label + of the same color, on a button whose whole point is that it has no fill until it is hovered. + */ + out['--w-btn-tint'] = 'color-mix(in srgb, var(--w-btn-color) 10%, transparent)' + if (isSolid.value && props.color) { out.backgroundColor = `var(--color-${props.color})` // -> Solid buttons default to white text, matching the palette's intended contrast diff --git a/frontend/src/css/_base.scss b/frontend/src/css/_base.scss index 74975c464..d8576be74 100644 --- a/frontend/src/css/_base.scss +++ b/frontend/src/css/_base.scss @@ -152,15 +152,20 @@ body::-webkit-scrollbar-thumb { // own hover state -- so the tint is a background on the button itself. Kept alongside the rule // above rather than replacing it, because both kinds of button are in play until Quasar is gone. // -// Unlayered (this file is plain CSS, not a Tailwind layer), so it beats the `hover:bg-current/10` -// utility WBtn carries for its flat variant, which would otherwise cancel the hover step out. +// Unlayered (this file is plain CSS, not a Tailwind layer), so it beats the hover-tint utility WBtn +// carries for its flat variant, which would otherwise cancel the hover step out. +// +// `--w-btn-color` rather than `currentcolor`: that is the button's `color` prop, which is the label +// too unless a `text-color` says otherwise -- and where it does, the fill is meant to stay with the +// button rather than follow the text. WBtn always sets it, falling back to `currentcolor` itself, +// so a Quasar-era button that somehow reaches this rule still tints. .w-btn.acrylic-btn { - background-color: color-mix(in srgb, currentcolor 10%, transparent); + background-color: color-mix(in srgb, var(--w-btn-color, currentcolor) 10%, transparent); } @media (hover: hover) { .w-btn.acrylic-btn:hover { - background-color: color-mix(in srgb, currentcolor 30%, transparent); + background-color: color-mix(in srgb, var(--w-btn-color, currentcolor) 30%, transparent); } } diff --git a/frontend/src/pages/AdminAuth.vue b/frontend/src/pages/AdminAuth.vue index a6355f4ad..b2f9ab56f 100644 --- a/frontend/src/pages/AdminAuth.vue +++ b/frontend/src/pages/AdminAuth.vue @@ -165,11 +165,6 @@ ? t(`admin.auth.registrationLocalHint`) : t(`admin.auth.registrationHint`) }} - - - {{ - t(`admin.auth.registrationNotEnforced`) - }}