Feature/merge verifier components #230
Merged
Conversation
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
Phase 1 of verifier component merge (ADR-06): - Add Session and SessionCollection to db package (OIDC session management) - Add Client and ClientCollection to db package (dynamic client registration) - Add middleware package with RateLimiter (token bucket rate limiting) - Update db Service to include Sessions and Clients collections These additions prepare the verifier for OIDC Provider functionality that will be migrated from verifier-proxy.
Phase 2 of verifier component merge (ADR-06): - Add OAuthError type with RFC 6749/6750 compliant error codes - Add pre-defined error variables for common OAuth/OIDC errors - Add utils/validation.go with URI validation and SSRF prevention - Add PKCE validation wrapper These additions prepare for OIDC handler migration.
- Add handler_oidc.go with Authorize and Token handlers - Update Client struct with OIDC provider fields (tracer, signing key, caches) - Add helper methods: generateSessionID, generateAuthorizationCode, generateAccessToken, generateRefreshToken, generateSubjectIdentifier - Add DCQL query building methods for OpenID4VP - Update main.go to pass tracer to apiv1.New() - All packages compile successfully
- Add handler_openid4vp.go with CreateRequestObject, GetRequestObject, HandleDirectPost, GetPollStatus methods - Add handler_client_registration.go with RegisterClient, GetClientInformation, DeleteClient methods (RFC 7591/7592) - Add ErrNotFound error to errors.go - All handlers compile and build successfully
Migrated handlers: - handler_api.go: GetDiscoveryMetadata, GetJWKS, ProcessDirectPost, ProcessCallback, GetQRCode, PollSession, GetUserInfo - handler_session_preference.go: UpdateSessionPreference, ConfirmCredentialDisplay, GetCredentialDisplayData - Added UpdateClient (RFC 7592 PUT) to handler_client_registration.go - Added extractAndMapClaims with ClaimsExtractor support - Added testing.go helpers Migrated httpserver: - endpoints_oidc.go: OIDC Discovery, JWKS, Authorize, Token, UserInfo - endpoints_openid4vp.go: Request object, Direct post, Callback, QR, Poll - endpoints_client_registration.go: RFC 7591/7592 registration - endpoints_credential_display.go: Session preferences, credential display - service.go: Rate limiting, all route wiring Migrated static files: - authorize.html, authorize_enhanced.html - credential_display.html - digital-credentials.js All verifier-proxy functionality now available in verifier. ADR-06 Phase 2 complete.
…ation Phase 3 of ADR-06 verifier components merge - Cleanup: Removed redundant code: - internal/verifier_proxy/ - All code now in internal/verifier/ - cmd/verifier-proxy/ - Entry point merged into cmd/verifier/ - bootstrapping/verifier-proxy/ - No longer needed Updated build system: - Makefile: Removed verifier-proxy from SERVICES, test, build, docker-build targets - docker-compose.yaml: Removed verifier-proxy and mongo-init-verifier-proxy services - Added presentation_requests volume mount to verifier service Consolidated documentation: - Created comprehensive docs/verifier/README.md covering both OpenID4VP and OIDC Provider functionality - Moved IMPROVEMENT_ANALYSIS.md, MULTI_TENANCY_ANALYSIS.md, OIDC_CONFORMANCE_TESTING.md to docs/verifier/ - Removed docs/verifier-proxy/ directory Configuration note: - The verifier_proxy config section is kept for backward compatibility - Existing deployments can continue using their current config.yaml files This completes the Phase 2 migration work and removes all redundant code, resulting in a single unified verifier service as specified in ADR-06.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR merges the verifier and verifier-proxy components into a single unified verifier service. The main changes involve consolidating functionality and updating the UI with new static HTML/JavaScript files.
Key Changes:
- Deletion of integration tests and test infrastructure from verifier-proxy
- Removal of middleware and HTTP server components
- Addition of new static UI files supporting W3C Digital Credentials API
- New HTML templates for authorization flows with enhanced UX
Reviewed changes
Copilot reviewed 62 out of 72 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/verifier_proxy/middleware/rate_limit_test.go | Deleted rate limiting tests |
| internal/verifier_proxy/integration/* | Deleted integration test suite and helpers |
| internal/verifier_proxy/apiv1/*_test.go | Deleted unit tests for API handlers |
| internal/verifier_proxy/httpserver/* | Deleted HTTP server implementation |
| internal/verifier_proxy/db/* | Deleted database service and models |
| internal/verifier/static/digital-credentials.js | Added W3C Digital Credentials API client |
| internal/verifier/static/authorize.html | Added basic authorization page |
| internal/verifier/static/authorize_enhanced.html | Added enhanced authorization page with DC API support |
| internal/verifier/static/credential_display.html | Added credential review/display page |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Test Coverage:
- apiv1: 7.3% (client functions, errors, discovery metadata)
- utils: 61% (validation functions: redirect URI, scopes, PKCE, HTTPS)
- Target: >75% (requires database integration tests)
Test Files Added:
- client_test.go: Token generation, subject identifiers, signing methods
- errors_test.go: OAuth error types, HTTP status mapping
- handler_api_metadata_test.go: Discovery metadata, JWKS endpoints
- handler_oidc_test.go: Request/response validation, PKCE tests
- helpers_test.go: Test fixture helpers (RSA keys, sessions)
- mock_db_test.go: Mock database collections for unit tests
- utils/validation_test.go: URI, scope, PKCE validation tests
Note: Handler integration tests require MongoDB and are not included.
Consider adding integration test suite with testcontainers.
- Add SessionStore and ClientStore interfaces to db/interfaces.go - Update db.Service to use interfaces for Sessions and Clients fields - Add NewServiceWithMocks() for creating Service with mock implementations - Update mock_db_test.go with compile-time interface satisfaction checks - Add CreateTestClientWithMock() helper that properly wires mocks to Client - Add handler integration tests that use mock database: - TestAuthorize_ClientValidation tests client/redirect/scope validation - TestAuthorize_PKCEValidation tests PKCE enforcement - TestMockClientCollection tests mock CRUD operations - TestMockSessionCollection tests mock session operations This infrastructure enables unit testing handlers without MongoDB dependency. Coverage improved from 7.3% to 9.5% for apiv1 package.
Added tests for: - hmacEqual constant-time comparison (100% coverage) - Client registration helpers: generateClientID, generateClientSecret, hashClientSecret, generateRegistrationAccessToken, hashRegistrationAccessToken, verifyRegistrationAccessToken (75-100% coverage) - validateRegistrationRequest comprehensive edge cases (45% → 82.5%) - authenticateClient with mock database (0% → 90%) - ValidateURI, validateIPAddress, checkSSRF in utils (0% → 77-100%) Coverage improvements: - apiv1 package: 9.5% → 18.1% - utils package: 61% → 91.5% - Total: 12.5% → 22.4%
Add coverage.out to .gitignore to prevent accidental commits of coverage reports. New tests added: - GetQRCode: 0% → 75% (tests QR code generation and error cases) - PollSession: 0% → 88.9% (tests session polling with different statuses) - GetUserInfo: 0% → 87.5% (tests UserInfo endpoint with verified claims) - generateNonce: 0% → 100% (tests nonce generation uniqueness) - GetJWKS: improved with ECDSA key tests (added generateTestECDSAKey helper) Coverage improvements: - apiv1: 19.1% → 23.5% - utils: 91.5% (stable) - total: 23.4% → 27.5%
Add extensive test coverage for the Token endpoint (authorization_code grant): - Token(): 0% → 100% - handleAuthorizationCodeGrant(): 0% → 79.2% - generateIDToken(): 0% → 92.3% - authenticateOIDCClient(): 0% → 100% - handleRefreshTokenGrant(): 0% → 100% (stub test for unimplemented feature) Test cases cover: - Successful token exchange with client authentication - Invalid grant types - Invalid/expired/used authorization codes - Client credential validation (bcrypt hashing) - Client ID and redirect URI matching - PKCE validation (S256 and plain methods) - Public clients (no authentication required) Coverage improvements: - apiv1: 23.5% → 30.5% (+7%) - total: 27.5% → 33.9% (+6.4%) Note: Fixed bcrypt hash issue by using hashPassword() helper function that generates proper hashes at test time instead of hardcoded values.
Add extensive test coverage for the Authorize endpoint: - Authorize(): 32.8% → 82.0% (+49.2%) Test cases cover: - Successful authorization request flow - Client validation (not found, invalid) - Redirect URI validation (matching against allowed URIs) - Response type validation (code vs unsupported types) - Scope validation (allowed vs unauthorized scopes) - PKCE enforcement (required but missing, successful validation) - Session creation with OIDC request parameters - Digital Credentials API configuration - Authorization page CSS customization Helper additions: - createSimplePresentationTemplate(): Creates basic DCQL templates for testing with configurable scopes, credential queries, and claim mappings Coverage improvements: - apiv1: 30.5% → 34.6% (+4.1%) - total: 33.9% → 37.7% (+3.8%) The tests validate the complete authorization flow including session creation, configuration propagation, and response structure.
Add comprehensive tests for Dynamic Client Registration endpoints: - TestRegisterClient: 8 test cases covering minimal/full registration, PKCE, validation errors (missing redirect URIs, invalid auth method, invalid grant type, invalid response type, URI with fragment) - TestGetClientInformation: 3 test cases (successful, not found, invalid token) - TestDeleteClient: 3 test cases (successful, not found, invalid token) - TestUpdateClient: 4 test cases (successful, not found, invalid token, invalid request) Coverage improvement: apiv1 34.6% -> 44.0%, total 37.7% -> 46.8% Part of ADR-06 Phase 3 (Testing)
Add comprehensive tests for OpenID4VP presentation flow: - TestCreateRequestObject: 4 test cases covering basic, DC API enabled, custom response mode, and preferred formats - TestBuildVPFormats: 4 test cases covering format construction - TestGetRequestObject: 2 test cases (successful, not found) - TestHandleDirectPost: 2 test cases (successful, session not found) - TestGetPollStatus: 3 test cases (pending, code issued, not found) Update CreateTestClientWithMock to initialize ttlcache instances for request object and encryption key caches. Coverage improvement: apiv1 44.0% -> 51.1%, total 46.8% -> 53.5% OpenID4VP handler coverage: - CreateRequestObject: 0% -> 88.9% - buildVPFormats: 0% -> 100% - GetRequestObject: 0% -> 100% - HandleDirectPost: 0% -> 66.7% - GetPollStatus: 0% -> 85.7% Part of ADR-06 Phase 3 (Testing)
Add comprehensive tests for session preference and credential display: - TestUpdateSessionPreference: 3 test cases (success true/false, not found) - TestConfirmCredentialDisplay: 4 test cases (confirmed, cancelled, wrong status, not found) - TestGetCredentialDisplayData: 3 test cases (with VP token, without VP token, not found) Coverage improvement: apiv1 51.1% -> 56.0%, total 53.5% -> 58.1% Session preference handler coverage: - UpdateSessionPreference: 0% -> 70.0% - ConfirmCredentialDisplay: 0% -> 89.7% - GetCredentialDisplayData: 0% -> 92.9% Part of ADR-06 Phase 3 (Testing)
Add tests for utility functions and UI handlers: - TestGetKID: 6 test cases covering valid JWT, missing KID, malformed base64, malformed JSON, non-string KID - TestUIMetadata: 3 test cases covering credentials with wallets, empty config, credentials only (verifies sensitive fields are cleared) Coverage improvement: apiv1 56.0% -> 58.4%, total 58.1% -> 60.4% New coverage: - GetKID: 0% -> 100% - UIMetadata: 0% -> 100% Part of ADR-06 Phase 3 (Testing)
- Add TestClient_buildLegacyDCQLQuery with 5 test cases - Add TestGetJWKS_KeyTypes covering RSA, EC P-256/384/521, and no key scenarios - Coverage improvements for buildLegacyDCQLQuery (86.7%) and GetJWKS (100%)
- Add TestVerificationCallback with cached credential and not-found scenarios - Add credentialCache initialization to CreateTestClientWithMock - VerificationCallback coverage: 0% → 100%
- Add test for expired access token - Add test for session without sub claim - Add test for session with non-string sub claim - GetUserInfo coverage: 87.5% → 93.8%
- Add TestClient_createDCQLQuery covering nil presentationBuilder fallback - Add non-existent hostname test case for checkSSRF DNS lookup error - createDCQLQuery coverage: 80% → 100% - checkSSRF coverage: 77.8% → 88.9% - utils package coverage: 91.5% → 93.2%
…ationRequest, and DigitalCredentialsDisabled tests - Add TestExtractClaimsFromVPToken for nil claims extractor case - Add tests for client_uri, policy_uri, and tos_uri validation errors - validateRegistrationRequest now at 100% coverage - Add TestAuthorize_DigitalCredentialsDisabled for DC API disabled branch - Authorize improved from 82% to 90.2% Coverage increased from 65.2% to 66.0%
- Add test case for updating all optional fields (JWKSUri, ClientURI, LogoURI, etc.) - Add test case for updating with JWKS instead of JWKSUri - UpdateClient improved from 76% to 96% Coverage increased from 66.0% to 67.0%
- Add test case for scope with VCTM containing claims - buildLegacyDCQLQuery now at 100% coverage Coverage increased from 67.0% to 67.2%
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 72 out of 84 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Update VERIFIER_MERGE_PLAN.md to reflect completed migration - Mark all 5 phases as complete with actual timeline and results - Document final test coverage (67.2% overall, 93.2% utils) - List all test files created during migration - Add success criteria with final status - Mark VERIFIER_MIGRATION_PLAN.md as superseded The verifier and verifier-proxy services have been successfully merged into a single unified verifier component.
- Fix bug where extractAndMapClaims called method on nil claimsExtractor - Add comprehensive tests for ProcessDirectPost handler - Add tests for HandleDirectPost, extractClaimsFromVPToken - Add tests for generateSessionID, containsOIDC helpers Coverage improved from 67.2% to 71.6%, exceeding 70% target
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This attempts to merge verifier and verifier-proxy into a single verifier service that combines both functions.