fix(web): respect passkey availability in secure verification - #6155
Open
Imzl-zl wants to merge 2 commits into
Open
fix(web): respect passkey availability in secure verification#6155Imzl-zl wants to merge 2 commits into
Imzl-zl wants to merge 2 commits into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Passkey status API now distinguishes credential binding from system enablement. The frontend propagates that state to profile registration and secure verification, selects usable methods with fallback logic, and adds localized system-disabled messaging. ChangesPasskey availability and verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/default/src/features/auth/passkey/hooks/use-passkey-management.ts (1)
175-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency:
systemEnabledskipsuseMemoused by sibling derived values.
enabledandlastUsedboth wrap derivation inuseMemo, butsystemEnableddoes not. Harmless since it's a cheap read, but inconsistent style.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/auth/passkey/hooks/use-passkey-management.ts` at line 175, Update the systemEnabled derivation in the passkey management hook to use useMemo consistently with the sibling enabled and lastUsed values, preserving the existing status?.system_enabled ?? null result.web/default/src/features/auth/secure-verification/hooks/use-secure-verification.ts (1)
211-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
selectVerificationMethodforrecommendedMethodto eliminate duplicated logic.
recommendedMethodduplicates the same fallback logic asselectVerificationMethodwithout a preferred method. Reusing the helper ensures a single source of truth for method selection.♻️ Proposed refactor
const recommendedMethod = useMemo<VerificationMethod | null>(() => { - if (methods.hasPasskey && methods.passkeySupported) return 'passkey' - if (methods.has2FA) return '2fa' - return null + return selectVerificationMethod(methods) }, [methods])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/auth/secure-verification/hooks/use-secure-verification.ts` around lines 211 - 215, Update the recommendedMethod useMemo to call selectVerificationMethod without a preferred method instead of duplicating the passkey, 2FA, and null fallback checks. Preserve the existing memoization and methods dependency.web/default/src/features/auth/secure-verification/lib/select-verification-method.test.ts (1)
19-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse Vitest instead of
node:testfor this test file.As per coding guidelines,
web/default/**/*.test.tsfiles should use Vitest. This file usesnode:testandnode:assert/strict, which may not be picked up by the project's Vitest test runner or CI pipeline. Consider migrating to Vitest'sdescribe,test, andexpectAPIs.Additionally, consider adding table-style tests covering these missing invariants:
- No
preferredMethodwith passkey available → returns'passkey'(default fallback)- No
preferredMethodwith only 2FA → returns'2fa'- Preferred
'2fa'with only passkey available → falls back to'passkey'♻️ Proposed migration to Vitest with table tests
-import assert from 'node:assert/strict' -import { describe, test } from 'node:test' - -import { selectVerificationMethod } from './select-verification-method' - -describe('selectVerificationMethod', () => { - test('falls back to 2FA when the preferred Passkey is unavailable', () => { - assert.equal( - selectVerificationMethod( - { has2FA: true, hasPasskey: false, passkeySupported: true }, - 'passkey' - ), - '2fa' - ) - }) - - test('falls back to 2FA when a bound Passkey is unsupported by the device', () => { - assert.equal( - selectVerificationMethod( - { has2FA: true, hasPasskey: true, passkeySupported: false }, - 'passkey' - ), - '2fa' - ) - }) - - test('uses an available preferred method', () => { - assert.equal( - selectVerificationMethod( - { has2FA: true, hasPasskey: true, passkeySupported: true }, - '2fa' - ), - '2fa' - ) - assert.equal( - selectVerificationMethod( - { has2FA: true, hasPasskey: true, passkeySupported: true }, - 'passkey' - ), - 'passkey' - ) - }) - - test('returns null when no usable method is available', () => { - assert.equal( - selectVerificationMethod({ - has2FA: false, - hasPasskey: true, - passkeySupported: false, - }), - null - ) - }) -}) +import { describe, expect, it } from 'vitest' + +import type { VerificationMethod, VerificationMethods } from '../types' +import { selectVerificationMethod } from './select-verification-method' + +describe('selectVerificationMethod', () => { + const cases: Array<{ + name: string + methods: VerificationMethods + preferred?: VerificationMethod + expected: VerificationMethod | null + }> = [ + { + name: 'falls back to 2FA when the preferred Passkey is unavailable', + methods: { has2FA: true, hasPasskey: false, passkeySupported: true }, + preferred: 'passkey', + expected: '2fa', + }, + { + name: 'falls back to 2FA when a bound Passkey is unsupported by the device', + methods: { has2FA: true, hasPasskey: true, passkeySupported: false }, + preferred: 'passkey', + expected: '2fa', + }, + { + name: 'uses an available preferred 2FA', + methods: { has2FA: true, hasPasskey: true, passkeySupported: true }, + preferred: '2fa', + expected: '2fa', + }, + { + name: 'uses an available preferred Passkey', + methods: { has2FA: true, hasPasskey: true, passkeySupported: true }, + preferred: 'passkey', + expected: 'passkey', + }, + { + name: 'defaults to Passkey when no preference and Passkey is available', + methods: { has2FA: true, hasPasskey: true, passkeySupported: true }, + expected: 'passkey', + }, + { + name: 'defaults to 2FA when no preference and only 2FA is available', + methods: { has2FA: true, hasPasskey: false, passkeySupported: true }, + expected: '2fa', + }, + { + name: 'falls back to Passkey when preferred 2FA is unavailable', + methods: { has2FA: false, hasPasskey: true, passkeySupported: true }, + preferred: '2fa', + expected: 'passkey', + }, + { + name: 'returns null when no usable method is available', + methods: { has2FA: false, hasPasskey: true, passkeySupported: false }, + expected: null, + }, + ] + + for (const { name, methods, preferred, expected } of cases) { + it(name, () => { + expect(selectVerificationMethod(methods, preferred)).toBe(expected) + }) + } +})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/default/src/features/auth/secure-verification/lib/select-verification-method.test.ts` around lines 19 - 72, Replace the node:test and node:assert/strict imports in selectVerificationMethod tests with Vitest’s describe, test, and expect APIs. Extend the selectVerificationMethod coverage with table-style cases for default passkey fallback, default 2FA fallback, and preferred-2FA fallback to passkey when only passkey is usable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/default/src/features/auth/passkey/hooks/use-passkey-management.ts`:
- Line 175: Update the systemEnabled derivation in the passkey management hook
to use useMemo consistently with the sibling enabled and lastUsed values,
preserving the existing status?.system_enabled ?? null result.
In
`@web/default/src/features/auth/secure-verification/hooks/use-secure-verification.ts`:
- Around line 211-215: Update the recommendedMethod useMemo to call
selectVerificationMethod without a preferred method instead of duplicating the
passkey, 2FA, and null fallback checks. Preserve the existing memoization and
methods dependency.
In
`@web/default/src/features/auth/secure-verification/lib/select-verification-method.test.ts`:
- Around line 19-72: Replace the node:test and node:assert/strict imports in
selectVerificationMethod tests with Vitest’s describe, test, and expect APIs.
Extend the selectVerificationMethod coverage with table-style cases for default
passkey fallback, default 2FA fallback, and preferred-2FA fallback to passkey
when only passkey is usable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 49bdf770-61c6-4621-b5cc-392854fa29ab
📒 Files selected for processing (17)
controller/passkey.gocontroller/passkey_test.goweb/default/src/features/auth/passkey/hooks/use-passkey-management.tsweb/default/src/features/auth/passkey/types.tsweb/default/src/features/auth/secure-verification/api.tsweb/default/src/features/auth/secure-verification/components/secure-verification-dialog.tsxweb/default/src/features/auth/secure-verification/hooks/use-secure-verification.tsweb/default/src/features/auth/secure-verification/lib/select-verification-method.test.tsweb/default/src/features/auth/secure-verification/lib/select-verification-method.tsweb/default/src/features/profile/components/passkey-card.tsxweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh-TW.jsonweb/default/src/i18n/locales/zh.json
Author
|
已复核 CodeRabbit 的 3 条 nitpick，并在
验证结果：
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
📝 变更描述 / Description
修复默认前端在 Passkey 不可用时仍将其选为首选安全验证方式，以及系统全局关闭 Passkey 后个人资料仍允许启动注册的问题。
system_enabled，明确区分个人凭证绑定状态与系统全局可用状态。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
bun test src/features/auth/secure-verification/lib/select-verification-method.test.ts：4 项通过。go test ./controller -run TestPasskeyStatusSeparatesCredentialAndSystemState -count=1：通过。bun run typecheck：通过。oxlint与oxfmt --check：通过。bun run i18n:sync：en、zh、zh-TW、fr、ja、ru、vi 均为 0 missing / 0 extras / 0 untranslated。bun run build：生产构建通过。git diff --check：通过。Summary by CodeRabbit