Skip to content

refactor: improve MFA challenge form logic and fix account page issues #2365

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.

Sign up for GitHub

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 3 commits into from
Sep 15, 2025
Merged

refactor: improve MFA challenge form logic and fix account page issues #2365

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f026a6d
refactor: improve MFA challenge form logic and fix account page issues
HarshMN2345 Sep 12, 2025
aeb1e1f
code suggestions
HarshMN2345 Sep 13, 2025
fee3ac3
lint issue
HarshMN2345 Sep 13, 2025
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
168 changes: 106 additions & 62 deletions src/lib/components/mfaChallengeFormList.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,39 @@
<script context="module" lang="ts">
let inputDigitFields: InputDigits;
export async function verify(challenge: Models.MfaChallenge, code: string) {
export async function verify(
challenge: Models.MfaChallenge,
code: string,
challengeType: AuthenticationFactor = AuthenticationFactor.Totp,
factors?: Models.MfaFactors & { recoveryCode: boolean }
) {
try {
if (challenge === null) {
if (!challenge) {
// Validate that the challengeType is actually enabled
if (factors) {
const factorMap = {
[AuthenticationFactor.Totp]: factors.totp,
[AuthenticationFactor.Email]: factors.email,
[AuthenticationFactor.Phone]: factors.phone,
[AuthenticationFactor.Recoverycode]: factors.recoveryCode
};
if (!factorMap[challengeType]) {
throw new Error(`Authentication factor ${challengeType} is not enabled`);
}
}
challenge = await sdk.forConsole.account.createMFAChallenge({
factor: AuthenticationFactor.Totp
factor: challengeType
});
}
await sdk.forConsole.account.updateMFAChallenge({
challengeId: challenge.$id,
otp: code
});
await invalidate(Dependencies.ACCOUNT);
trackEvent(Submit.AccountLogin, { mfa_used: true });
} catch (error) {
inputDigitFields?.clearInputsAndRefocus();
trackError(error, Submit.AccountLogin);
Copy link
Preview

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

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

The inputDigitFields?.clearInputsAndRefocus() call was removed from the error handling, but there's no corresponding functionality to clear the input fields on error. This could leave users with invalid codes still displayed.

Suggested change
trackError(error, Submit.AccountLogin);
trackError(error, Submit.AccountLogin);
code = '';

Copilot uses AI. Check for mistakes.

throw error;
}
Expand All @@ -24,32 +42,42 @@

<script lang="ts">
import { onMount } from 'svelte';
import { invalidate } from '$app/navigation';
import { Button, InputDigits, InputText } from '$lib/elements/forms';
import { sdk } from '$lib/stores/sdk';
import { invalidate } from '$app/navigation';
import { Dependencies } from '$lib/constants';
import { Submit, trackEvent, trackError } from '$lib/actions/analytics';
import { AuthenticationFactor, type Models } from '@appwrite.io/console';
import { Button, InputText, InputDigits } from '$lib/elements/forms';
import { addNotification } from '$lib/stores/notifications';
import { Icon, Typography } from '@appwrite.io/pink-svelte';
import { IconChatAlt, IconDeviceMobile, IconMail } from '@appwrite.io/pink-icons-svelte';
export let factors: Models.MfaFactors & { recoveryCode: boolean };
/** If true, the form will be submitted automatically when the code is entered. */
export let showVerifyButton: boolean = true;
export let disabled: boolean = false;
export let challenge: Models.MfaChallenge;
export let code: string;
let challengeType: AuthenticationFactor;
const enabledFactors = Object.entries(factors).filter(([_, enabled]) => enabled);
// Get enabled non-recovery factors for main factors
$: enabledMainFactors = [
factors.totp && 'totp',
factors.email && 'email',
factors.phone && 'phone'
].filter(Boolean);
async function createChallenge(factor: AuthenticationFactor) {
disabled = true;
challengeType = factor;
code = '';
try {
challenge = await sdk.forConsole.account.createMFAChallenge({ factor });
if (
factor !== AuthenticationFactor.Totp &&
factor !== AuthenticationFactor.Recoverycode
) {
challenge = await sdk.forConsole.account.createMFAChallenge({ factor });
}
} catch (error) {
Comment on lines 70 to 81
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Align challenge creation semantics with verify()

createChallenge() avoids creating a challenge for TOTP and RecoveryCode, while verify() (in the original module script) always creates one when challenge is missing. Confirm API expectations and make both paths consistent (either gate by factor or always create). I can update both places once you confirm the API contract.

🌐 Web query:

Does Appwrite’s Account.createMFAChallenge require a challenge for TOTP and RecoveryCode factors, or only for Email/Phone?

💡 Result:

You must create a challenge for all supported factors (email, phone, totp and recoveryCode). Appwrite’s createMFAChallenge accepts totp and recoveryCode as valid factors (not just email/phone) and returns a challenge ID to complete with updateMfaChallenge. [1][2]

Always create an MFA challenge for all factors (include TOTP & RecoveryCode)

Appwrite requires a challenge for all supported factors (email, phone, totp, recoveryCode). In src/lib/components/mfaChallengeFormList.svelte (lines 70–81) remove the guard that skips createMFAChallenge for AuthenticationFactor.Totp and AuthenticationFactor.Recoverycode so createChallenge always calls sdk.forConsole.account.createMFAChallenge({ factor }); align verify() and createChallenge behavior.

🤖 Prompt for AI Agents 
In src/lib/components/mfaChallengeFormList.svelte around lines 70 to 81, the
createChallenge function currently skips calling
sdk.forConsole.account.createMFAChallenge for AuthenticationFactor.Totp and
AuthenticationFactor.Recoverycode; remove that conditional guard so
createChallenge always calls sdk.forConsole.account.createMFAChallenge({ factor
}); (preserve existing disabled/challengeType/code handling and try/catch
behavior) to align createChallenge with verify() and ensure an MFA challenge is
created for all factor types.

addNotification({
type: 'error',
Expand All @@ -60,57 +88,71 @@
}
}
export async function verifyCurrent() {
try {
return await verify(challenge, code, challengeType, factors);
} catch (error) {
// Clear input fields on verification failure
code = '';
throw error;
}
}
onMount(async () => {
const enabledNonRecoveryFactors = enabledFactors.filter(
([factor, _]) => factor !== 'recoveryCode'
);
if (enabledNonRecoveryFactors.length === 1) {
if (factors.phone) {
await createChallenge(AuthenticationFactor.Phone);
} else if (factors.email) {
await createChallenge(AuthenticationFactor.Email);
} else if (factors.totp) {
challengeType = AuthenticationFactor.Totp;
}
if (enabledMainFactors.length === 0) {
return;
}
if (factors.totp) {
challengeType = AuthenticationFactor.Totp;
} else if (factors.email) {
await createChallenge(AuthenticationFactor.Email);
} else if (factors.phone) {
await createChallenge(AuthenticationFactor.Phone);
}
});
</script>

{#if challengeType === AuthenticationFactor.Recoverycode}
{#if enabledMainFactors.length === 0}
<!-- Skip MFA completely if no factors enabled -->
{:else if challengeType === AuthenticationFactor.Recoverycode}
<Typography.Text align="center">
Enter below one of the recovery codes you received when enabling MFA for this account.
Enter one of the recovery codes you received when enabling MFA.
</Typography.Text>
<InputText id="recovery-code" bind:value={code} required autofocus />
{:else}
{#if factors.totp && (challengeType === AuthenticationFactor.Totp || challengeType === null)}
<Typography.Text align="center"
>Enter below a 6-digit one-time code generated by your authentication app.</Typography.Text>
{:else if challengeType === AuthenticationFactor.Email}
<Typography.Text align="center"
>A 6-digit verification code was sent to your email, enter it below.</Typography.Text>
{:else if challengeType === AuthenticationFactor.Phone}
<Typography.Text align="center"
>A 6-digit verification code was sent to your phone, enter it below.</Typography.Text>
{/if}
{:else if challengeType === AuthenticationFactor.Totp}
<Typography.Text align="center">
Enter a 6-digit one-time code from your authenticator app.
</Typography.Text>
<InputDigits bind:value={code} required autofocus />
{:else if challengeType === AuthenticationFactor.Email}
<Typography.Text align="center">
A 6-digit verification code was sent to your email. Enter it below.
</Typography.Text>
<InputDigits bind:value={code} required autofocus />
{:else if challengeType === AuthenticationFactor.Phone}
<Typography.Text align="center">
A 6-digit verification code was sent to your phone. Enter it below.
</Typography.Text>
<InputDigits bind:value={code} required autofocus />
{/if}
{#if showVerifyButton}

{#if showVerifyButton && enabledMainFactors.length > 0}
<Button fullWidth submit {disabled}>Verify</Button>
{/if}
{#if enabledFactors.length > 1}
<span class="with-separators eyebrow-heading-3">or</span>
<div class="u-flex-vertical u-gap-8">
{#if factors.totp}
{#if (challengeType === null || challengeType === AuthenticationFactor.Totp || challengeType === AuthenticationFactor.Recoverycode) && factors.email}
<Button
secondary
fullWidth
{disabled}
on:click={() => createChallenge(AuthenticationFactor.Email)}>
<Icon icon={IconMail} slot="start" size="s" />
Email verification
</Button>
{:else if (challengeType === AuthenticationFactor.Recoverycode && !factors.email) || challengeType === AuthenticationFactor.Email}

{#if enabledMainFactors.length > 0}
{@const hasSecondaryOptions =
enabledMainFactors.length > 1 ||
(factors.recoveryCode && challengeType !== AuthenticationFactor.Recoverycode) ||
(enabledMainFactors.length === 1 && factors.recoveryCode)}

{#if hasSecondaryOptions}
<span class="with-separators eyebrow-heading-3">or</span>
<div class="u-flex-vertical u-gap-8">
<!-- Show non-active main factors -->
{#if factors.totp && challengeType !== AuthenticationFactor.Totp}
<Button
secondary
fullWidth
Expand All @@ -120,7 +162,7 @@
Authenticator app
</Button>
{/if}
{:else}

{#if factors.email && challengeType !== AuthenticationFactor.Email}
<Button
secondary
Expand All @@ -131,6 +173,7 @@
Email verification
</Button>
{/if}

{#if factors.phone && challengeType !== AuthenticationFactor.Phone}
<Button
secondary
Expand All @@ -141,15 +184,16 @@
Phone verification
</Button>
{/if}
{/if}
{#if factors.recoveryCode && challengeType !== AuthenticationFactor.Recoverycode}
<Button
text
fullWidth
{disabled}
on:click={() => createChallenge(AuthenticationFactor.Recoverycode)}>
<span class="text">Use recovery code</span>
</Button>
{/if}
</div>

{#if factors.recoveryCode && challengeType !== AuthenticationFactor.Recoverycode}
<Button
text
fullWidth
{disabled}
on:click={() => createChallenge(AuthenticationFactor.Recoverycode)}>
<span class="text">Use recovery code</span>
</Button>
{/if}
</div>
{/if}
{/if}
2 changes: 2 additions & 0 deletions src/routes/(console)/account/updateMfa.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,13 +280,15 @@
<Button
secondary
class="recovery-codes-button"
disabled={enabledMethods.length === 0}
on:click={() => (showRegenerateRecoveryCodes = true)}>
Regenerate
</Button>
{:else}
<Button
secondary
class="recovery-codes-button"
disabled={enabledMethods.length === 0}
on:click={createRecoveryCodes}>View</Button>
{/if}
</Layout.Stack>
Expand Down
Loading