Conversation
- Add ApiVersionService to fetch and cache backend API version from /status - Add ApiVersionContext to provide API version throughout the app - Add DiscoverAndTrustService for combined discovery + trust evaluation - Auto-detect backend capabilities via api_version field - Backwards compatible: defaults to API version 1 for older backends - discover-and-trust feature available when api_version >= 2
- Test ApiVersionService: fetchApiVersion, getApiFeatures, caching behavior - Test DiscoverAndTrustService: discoverAndTrust, convenience wrappers - Test feature availability checks - Test error handling and type definitions
- Add discover-and-trust service import to OpenID4VCIHelper - When discover-and-trust API is available (API version >= 2), use it for getCredentialIssuerMetadata instead of the HTTP proxy - Fall back to legacy proxy approach if discover-and-trust fails or is not available (backwards compatible with older backends) - This eliminates the console errors from unauthenticated proxy requests to /.well-known/openid-credential-issuer endpoints
- Import discover-and-trust service in OpenID4VP - Add verifier trust check after certificate validation in handleAuthorizationRequest - Use discoverAndTrustVerifier() to verify if verifier is trusted - Reject authorization requests from untrusted verifiers - Fall back to certificate-based trust if discover-and-trust fails or is unavailable - This enables policy-based verifier trust evaluation in addition to X.509 validation
| cachedApiVersion = await fetchApiVersion(); | ||
| cacheTimestamp = now; | ||
| return cachedApiVersion; | ||
| } | ||
|
|
||
| /** | ||
| * Forces a fresh fetch of the API version, updating the cache. | ||
| */ | ||
| export async function refreshApiVersion(): Promise<number> { | ||
| cachedApiVersion = await fetchApiVersion(); | ||
| cacheTimestamp = Date.now(); | ||
| return cachedApiVersion; | ||
| } |
There was a problem hiding this comment.
| cachedApiVersion = await fetchApiVersion(); | |
| cacheTimestamp = now; | |
| return cachedApiVersion; | |
| } | |
| /** | |
| * Forces a fresh fetch of the API version, updating the cache. | |
| */ | |
| export async function refreshApiVersion(): Promise<number> { | |
| cachedApiVersion = await fetchApiVersion(); | |
| cacheTimestamp = Date.now(); | |
| return cachedApiVersion; | |
| } | |
| refreshApiVersion(now); | |
| } | |
| /** | |
| * Forces a fresh fetch of the API version, updating the cache. | |
| */ | |
| export async function refreshApiVersion(now?: Date): Promise<number> { | |
| cachedApiVersion = await fetchApiVersion(); | |
| cacheTimestamp = now || Date.now(); | |
| return cachedApiVersion; | |
| } |
| if (cachedApiVersion === null) { | ||
| return DEFAULT_API_VERSION >= minVersion; | ||
| } | ||
| return cachedApiVersion >= minVersion; |
There was a problem hiding this comment.
| if (cachedApiVersion === null) { | |
| return DEFAULT_API_VERSION >= minVersion; | |
| } | |
| return cachedApiVersion >= minVersion; | |
| return getCachedApiVersion() >= minVersion; |
| * Checks if a specific feature is available based on the cached API version. | ||
| * If the version hasn't been fetched yet, assumes DEFAULT_API_VERSION. | ||
| */ | ||
| export function isFeatureAvailable(minVersion: number): boolean { |
There was a problem hiding this comment.
isFeatureAvailable is confusing because it does not accept a feature (name) as a parameter. I would suggest something like: isApiVersionHigher, if this logic requires a function, since it can be reduced to a one-liner.
| const apiVersion = response.data?.api_version; | ||
| if (typeof apiVersion === 'string') { | ||
| const parsed = parseInt(apiVersion, 10); | ||
| return isNaN(parsed) ? DEFAULT_API_VERSION : parsed; | ||
| } | ||
| if (typeof apiVersion === 'number') { | ||
| return apiVersion; | ||
| } | ||
|
|
||
| return DEFAULT_API_VERSION; |
There was a problem hiding this comment.
| const apiVersion = response.data?.api_version; | |
| if (typeof apiVersion === 'string') { | |
| const parsed = parseInt(apiVersion, 10); | |
| return isNaN(parsed) ? DEFAULT_API_VERSION : parsed; | |
| } | |
| if (typeof apiVersion === 'number') { | |
| return apiVersion; | |
| } | |
| return DEFAULT_API_VERSION; | |
| const apiVersion = Number(response.data?.api_version) | |
| return isNan(apiVersion) ? DEFAULT_API_VERSION : apiVersion; |
| * Call ensureApiVersionLoaded() first if you need a guaranteed check. | ||
| */ | ||
| export function isDiscoverAndTrustAvailable(): boolean { | ||
| return getCachedApiVersion() >= API_VERSION_DISCOVER_AND_TRUST; |
There was a problem hiding this comment.
| return getCachedApiVersion() >= API_VERSION_DISCOVER_AND_TRUST; | |
| return isFeatureAvailable(API_VERSION_DISCOVER_AND_TRUST); |
| const apiVersion = await getApiVersion(); | ||
|
|
||
| if (apiVersion < API_VERSION_DISCOVER_AND_TRUST) { |
There was a problem hiding this comment.
| const apiVersion = await getApiVersion(); | |
| if (apiVersion < API_VERSION_DISCOVER_AND_TRUST) { | |
| await ensureApiVersionLoaded(); | |
| if (!isFeatureAvailable(API_VERSION_DISCOVER_AND_TRUST)) { |
| export async function discoverAndTrust( | ||
| request: DiscoverAndTrustRequest, | ||
| authToken: string | ||
| ): Promise<DiscoverAndTrustResponse> { | ||
| const apiVersion = await getApiVersion(); | ||
|
|
||
| if (apiVersion < API_VERSION_DISCOVER_AND_TRUST) { | ||
| throw new Error( | ||
| `discover-and-trust requires API version ${API_VERSION_DISCOVER_AND_TRUST}, ` + | ||
| `but backend reports version ${apiVersion}` | ||
| ); | ||
| } | ||
|
|
||
| const response = await axios.post<DiscoverAndTrustResponse>( | ||
| `${BACKEND_URL}/api/discover-and-trust`, | ||
| request, | ||
| { | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| Authorization: `Bearer ${authToken}`, | ||
| }, | ||
| timeout: 30000, // 30 second timeout for discovery + trust evaluation | ||
| } | ||
| ); | ||
|
|
||
| return response.data; | ||
| } | ||
|
|
||
| /** | ||
| * Discovers and evaluates trust for an issuer. | ||
| * Convenience wrapper around discoverAndTrust. | ||
| */ | ||
| export async function discoverAndTrustIssuer( | ||
| issuerIdentifier: string, | ||
| authToken: string, | ||
| credentialType?: string | ||
| ): Promise<DiscoverAndTrustResponse> { | ||
| return discoverAndTrust( | ||
| { | ||
| entity_identifier: issuerIdentifier, | ||
| role: 'issuer', | ||
| credential_type: credentialType, | ||
| }, | ||
| authToken | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Discovers and evaluates trust for a verifier. | ||
| * Convenience wrapper around discoverAndTrust. | ||
| */ | ||
| export async function discoverAndTrustVerifier( | ||
| verifierIdentifier: string, | ||
| authToken: string, | ||
| credentialType?: string | ||
| ): Promise<DiscoverAndTrustResponse> { | ||
| return discoverAndTrust( | ||
| { | ||
| entity_identifier: verifierIdentifier, | ||
| role: 'verifier', | ||
| credential_type: credentialType, | ||
| }, | ||
| authToken | ||
| ); | ||
| } |
There was a problem hiding this comment.
passing the auth token to every method of a server is not completely clean. I think it would be cleaner to use a DiscoverAndTrustRepository with a method discoverAndTrust() in which the fetch takes place. Let the repository be responsible for getting the authToken. Then, the service will have two methods: discoverAndTrustVerifier and discoverAndTrustIssuer in which the repository method is called (without the authToken parameter).
src/lib/services/OpenID4VCIHelper.ts
Outdated
| const publicKey = await importX509(getPublicKeyFromB64Cert(parsedHeader.x5c[0]), parsedHeader.alg); | ||
| const { payload } = await jwtVerify(metadata.signed_metadata, publicKey); | ||
| return { metadata: payload as OpenidCredentialIssuerMetadata }; |
There was a problem hiding this comment.
Nesting this many levels deep greatly reduces the readability and predictability of the code. Consider refactoring to multiple functions with early returns.
| // Evaluate verifier trust via discover-and-trust API if available | ||
| if (isDiscoverAndTrustAvailable()) { | ||
| const appToken = api.getAppToken(); | ||
| if (appToken) { | ||
| try { | ||
| const verifierUrl = response_uri ? new URL(response_uri).origin : client_id; | ||
| const trustResult = await discoverAndTrustVerifier(verifierUrl, appToken); | ||
| if (!trustResult.trusted) { | ||
| console.warn('Verifier not trusted:', trustResult.reason); | ||
| return { error: HandleAuthorizationRequestError.NONTRUSTED_VERIFIER }; | ||
| } | ||
| console.log('Verifier trust verified:', trustResult.reason); | ||
| } catch (err) { | ||
| // Log but don't fail - fall back to certificate-based trust | ||
| console.warn('discover-and-trust verifier check failed, using certificate-based trust:', err); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| // Evaluate verifier trust via discover-and-trust API if available | |
| if (isDiscoverAndTrustAvailable()) { | |
| const appToken = api.getAppToken(); | |
| if (appToken) { | |
| try { | |
| const verifierUrl = response_uri ? new URL(response_uri).origin : client_id; | |
| const trustResult = await discoverAndTrustVerifier(verifierUrl, appToken); | |
| if (!trustResult.trusted) { | |
| console.warn('Verifier not trusted:', trustResult.reason); | |
| return { error: HandleAuthorizationRequestError.NONTRUSTED_VERIFIER }; | |
| } | |
| console.log('Verifier trust verified:', trustResult.reason); | |
| } catch (err) { | |
| // Log but don't fail - fall back to certificate-based trust | |
| console.warn('discover-and-trust verifier check failed, using certificate-based trust:', err); | |
| } | |
| } | |
| } | |
| async function evaluateVerifierTrust(): Promise<Boolean | undefined> { | |
| if (!isDiscoverAndTrustAvailable()) return; | |
| const appToken = api.getAppToken(); // the responsibility of getting the appToken could be delegated to a a repository | |
| if (!appToken) return; | |
| try { | |
| const verifierUrl = response_uri ? new URL(response_uri).origin : client_id; | |
| const trustResult = await discoverAndTrustVerifier(verifierUrl, appToken); | |
| if (trustResult.trusted) { | |
| console.log('Verifier trust verified:', trustResult.reason); | |
| return; | |
| } | |
| console.warn('Verifier not trusted:', trustResult.reason); | |
| return false; | |
| } catch (err) { | |
| // Log but don't fail - fall back to certificate-based trust | |
| console.warn('discover-and-trust verifier check failed, using certificate-based trust:', err); | |
| } | |
| } | |
| const isVerifierTrusted = await evaluateVerifierTrust(); | |
| if (isVerifierTrusted === false) { | |
| return { error: HandleAuthorizationRequestError.NONTRUSTED_VERIFIER }; | |
| } |
jessevanmuijden
left a comment
There was a problem hiding this comment.
There are some small refactors possible, but especially the deeply nested logical structures need some love in my opinion.
|
excellent review @jessevanmuijden ! |
- Simplify fetchApiVersion type checking using Number() + isNaN() - Rename isFeatureAvailable to supportsApiVersion for clarity - Refactor getApiVersion to call refreshApiVersion (DRY) - Use supportsApiVersion consistently in DiscoverAndTrustService - Extract verifySignedMetadata and tryDiscoverAndTrust helpers to reduce nesting - Extract evaluateVerifierTrust function with early returns in OpenID4VP - Update tests to match renamed functions and new mocks
|
@jessevanmuijden can you see if we managed to cover the comments in that last push |
This is an implementation of pluggable trust for issue #994. The implementation depends on the API in sirosfoundation/go-wallet-backend#6:
A new backend API (cf pr 6 to go-wallet-backend above) is introduced that handles issuer and verifier metadata discovery and trust. This API removes the need for using the http proxy for metadata discovery and also uses go-trust (or similar solution) to provide pluggable trust decisions. The change to the frontend is minimal and backwards compatibility is maintained via api versioning which is also added as a feature in this PR.