[INNO] - Passkey Directory Endpoint#7317
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7317 +/- ##
==========================================
+ Coverage 62.36% 62.38% +0.02%
==========================================
Files 2286 2289 +3
Lines 99673 99743 +70
Branches 9003 9010 +7
==========================================
+ Hits 62157 62223 +66
- Misses 35344 35346 +2
- Partials 2172 2174 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jrmccannon
added
the
ai-review-vnext
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Passkey Directory Report feature: the Code Review DetailsNo new blocking findings. The two substantive discussion points already have open review threads:
|
# Conflicts: # src/Core/Constants.cs
# Conflicts: # src/Core/Constants.cs
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
| /// <returns>List of domains with passkey support details</returns> | ||
| [HttpGet("passkey-directory")] | ||
| [RequireFeature(FeatureFlagKeys.PasskeyDirectoryReport)] | ||
| public async Task<IEnumerable<PasskeyDirectoryResponseModel>> GetPasskeyDirectoryAsync() |
There was a problem hiding this comment.
❓ QUESTION: No AccessReports authorization check on this endpoint
Details
Every other endpoint in ReportsController gates access with _currentContext.AccessReports(orgId), but GetPasskeyDirectoryAsync only requires basic authentication (class-level [Authorize("Application")]). This means any authenticated Bitwarden user can call this endpoint, not just organization admins with report access.
The returned data originates from a public API, so there may be no confidentiality concern. However, the PR description states this powers a report for "organization administrators," and placing the endpoint in this controller without the same authorization pattern could be an intentional choice or an oversight.
Was the decision to skip AccessReports intentional given the data is publicly available, or should this endpoint require an orgId parameter with the standard permission check to stay consistent with the controller's authorization model?
|
lastbestdev
left a comment
lastbestdev
left a comment
There was a problem hiding this comment.
LGTM, no blocking comments
| /// Gets the list of domains that support passkeys from the 2FA Directory | ||
| /// </summary> | ||
| /// <returns>List of domains with passkey support details</returns> | ||
| [HttpGet("passkey-directory")] |
There was a problem hiding this comment.
Not critical: instead of placing the endpoint under /reports/, I'm wondering if we want to consolidate endpoints that call third party APIs elsewhere. They could share a controller with a base route like /external or similar.
Alternatively, we could follow what was done for HIBP and just introduce a new controller for 2FA directory API proxy requests, like /2fa-directory/.
It is worth noting that the Inactive 2FA report in the web client has direct calls to the 2FA directory API still, which should also be refactored to make requests through our API like this.
There was a problem hiding this comment.
Not critical: Where should this README live/is it appropriate to include here?
![github-code-quality[bot]](https://avatars.github.com/u/9919?s=60&v=4){kind=small}
| var entries = await cache.GetOrSetAsync( | ||
| key: _cacheKey, | ||
| factory: async _ => await FetchPasskeyDirectoryAsync(), | ||
| options: new FusionCacheEntryOptions(duration: _cacheDuration) |
There was a problem hiding this comment.
🎨 SUGGESTED: Passing a bare FusionCacheEntryOptions discards the configured fail-safe/timeout defaults for this external-API call.
Details and fix
AddReportingServices registers this cache via AddExtendedCache, which configures resilience defaults through WithDefaultEntryOptions — notably IsFailSafeEnabled = true, FactorySoftTimeout, and FactoryHardTimeout (see GlobalSettings.ExtendedCacheSettings).
Constructing new FusionCacheEntryOptions(duration: _cacheDuration) replaces those defaults for this call rather than merging, so fail-safe is disabled here. Because the factory (FetchPasskeyDirectoryAsync) calls a third-party API and does EnsureSuccessStatusCode(), a transient outage on a cold/expired cache now surfaces as a 500 to the caller, whereas fail-safe would let a slightly-stale cached copy be served.
The repo's CACHING.md documents the duration-only-override pattern, which starts from a duplicate of the default entry options:
var entries = await cache.GetOrSetAsync(
key: _cacheKey,
factory: async _ => await FetchPasskeyDirectoryAsync(),
options => options.SetDuration(_cacheDuration)
);This keeps the 24h duration while preserving fail-safe and factory timeouts. Reference: src/Core/Utilities/CACHING.md.
2 participants
🎟️ Tracking
Associated PRs
📔 Objective
This PR adds a new Passkey Directory Report endpoint (GET /reports/passkey-directory) that fetches and returns a list of domains supporting passkeys from the https://passkeys-api.2fa.directory/v1/all.json. The query layer (GetPasskeyDirectoryQuery) retrieves the external directory data, parses each entry for passwordless login support, MFA support, and documentation links, and caches results for 24 hours using FusionCache. The endpoint is gated behind the PasskeyDirectoryReport feature flag. This data powers a client-side report that helps organization administrators identify which of their members' credentials could be upgraded to passkeys. Unit tests cover the controller endpoint and the query's parsing/filtering logic.
📸 Screenshots