Identity Provider Authentication Integration - #95
Conversation
…ncryption options
…om DB_* variables and enhance documentation
… and error reporting in migrations
…al overrides in documentation
…th error management
…nd detailed error reporting
…ing and batch SQL execution
- Updated SKILL.md to reflect changes in code structure and specifications. - Modified .vscode/settings.json for database connection settings. - Revised AGENTS.md to clarify UI change documentation. - Adjusted CONTRIBUTING.md to align with the new SQL Server workflow. - Updated README.md to reflect the current tech stack and development setup. - Revised architecture documentation (arkitekturbeskrivning-kravhantering.md) for SQL Server integration. - Updated database schema documentation (database-schema.md) to reflect TypeORM usage. - Revised GitHub Codespaces documentation for SQL Server setup. - Updated MCP server documentation (mcp-server-contributor-guide.md and mcp-server-user-guide.md) for SQL Server integration. - Adjusted reference data documentation (reference-data-and-ai.md) for TypeORM changes. - Updated SQL Server developer workflow documentation (sql-server-developer-workflow.md) for new commands and environment variables. - Revised quality testing documentation (QUALITY.md) to reflect the current implementation.
- Deleted the requirements DAL lifecycle test file due to redundancy. - Updated database mock implementations in various test files to use `getRequestSqlServerDataSource` instead of `getRequestDatabase`. - Removed the requirements route test file as it was no longer needed. - Adjusted the risk levels and taxonomy routes tests to reflect the new database mock. - Refactored the UI settings tests to use a mock SQL Server database and updated terminology loading and saving logic.
… to exclude unnecessary paths
…DP management feat: add new dependencies for session management and OpenID Connect support
- Added centralized auth configuration to manage environment variables and authentication settings. - Introduced login state management using signed cookies for PKCE verifier and OIDC state. - Created functions to verify MCP bearer tokens and handle JWT verification. - Implemented OIDC discovery and configuration caching for efficient token handling. - Enhanced role parsing from OIDC claims to support various formats and legacy shapes. - Integrated session management with iron-session to store minimal validated OIDC claims. - Updated navigation component to include authentication menu for both desktop and mobile views. - Added Docker Compose configuration for local OIDC IdP development using Keycloak. - Updated internationalization messages to include authentication-related strings. - Enhanced middleware to enforce authentication for API routes and handle unauthenticated requests. - Added tests and setup for authentication-related functionalities.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a complete OIDC-based authentication stack: dev Keycloak IdP and devcontainer forwarding; server/browser PKCE login, callback, logout; short-lived login-state and encrypted session cookies; MCP Bearer-token JWT verification with JWKS caching; centralized auth libraries (config/oidc/session/mcp/roles/hsa-id/csrf/audit); proxy enforcement and header stripping; many client components migrated to apiFetch; extensive tests and docs/i18n updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Proxy as rgba(70,130,180,0.5) Proxy
participant Login as rgba(34,139,34,0.5) /api/auth/login
participant IdP as rgba(128,0,128,0.5) Keycloak
participant Callback as rgba(34,139,34,0.5) /api/auth/callback
participant Session as rgba(210,180,140,0.5) Encrypted Session Cookie
Browser->>Proxy: Request protected page
Proxy->>Proxy: Check session & auth config
alt not signed in
Proxy-->>Browser: 302 -> /api/auth/login
Browser->>Login: GET /api/auth/login (generate PKCE/state/nonce)
Login->>Session: Save short-lived login-state cookie
Login-->>Browser: 302 -> IdP authorize URL
Browser->>IdP: Authorization request (code_challenge,state,nonce)
IdP-->>Browser: 302 -> /api/auth/callback?code=...
Browser->>Callback: GET /api/auth/callback?code=...
Callback->>Session: Load login-state, validate state/nonce
Callback->>IdP: Token exchange (code + code_verifier)
IdP-->>Callback: tokens + id_token
Callback->>Callback: Validate ID token claims (sub, names, employeeHsaId, roles)
Callback->>Session: Persist main encrypted session cookie
Callback-->>Browser: 302 -> returnTo
end
Browser->>Proxy: Subsequent request with session cookie
Proxy->>Session: Validate session
Proxy->>Browser: Forward to app
sequenceDiagram
participant MCPClient as MCP Client
participant Proxy as rgba(70,130,180,0.5) Proxy
participant MCPRoute as rgba(34,139,34,0.5) /api/mcp
participant Verifier as rgba(255,140,0,0.5) verifyMcpBearerToken
participant JWKS as rgba(128,0,128,0.5) IdP JWKS
participant MCPServer as rgba(210,180,140,0.5) MCP Server
MCPClient->>Proxy: POST /api/mcp with Authorization: Bearer <JWT>
Proxy->>Proxy: Enforce Bearer requirement for /api/mcp
Proxy->>MCPRoute: Forward request
MCPRoute->>Verifier: verifyMcpBearerToken(request)
Verifier->>JWKS: Fetch or use cached JWKS
Verifier->>Verifier: jwtVerify(... issuer/audience/tolerance)
alt token valid
Verifier-->>MCPRoute: Verified actor
MCPRoute->>MCPServer: createRequestContext(request,'mcp') (await)
MCPServer->>MCPServer: Read attached actor from WeakMap
MCPServer-->>MCPRoute: Process request as verified actor
else invalid
Verifier-->>MCPRoute: Throw McpAuthError (401)
MCPRoute-->>MCPClient: 401 + WWW-Authenticate: Bearer
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
|
❌ Quality checks failed! Please fix the following issues:
You can also run |
|
❌ Integration tests failed! Please check the test results and fix any issues. You can run integration tests locally with:
Check the artifacts for detailed test reports. |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/auth/audit.ts (1)
107-129: Duck-typedRequestdetection is fragile — considerinstanceof Request.
typeof (input as Request).headers?.get === 'function'will also match anySecurityEventRequest-shaped object that happens to carry aheadersfield with agetmethod (e.g., a future caller passing a plain object with a Map-likeheaders). Since this module runs in a modern runtime whereRequestis globally available,input instanceof Requestis both safer and clearer. The currentSecurityEventRequesthas noheaders, so risk today is low — flagging as defensive hygiene.♻️ Proposed tightening
- if (typeof (input as Request).headers?.get === 'function') { - const req = input as Request + if (input instanceof Request) { + const req = input🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/auth/audit.ts` around lines 107 - 129, The duck-typing in normalizeRequest that checks typeof (input as Request).headers?.get === 'function' can misidentify non-Request objects; replace that check with a proper runtime Request check using instanceof Request (i.e., treat input as a Request only if input instanceof Request) so the Request-specific logic (new URL(req.url).pathname, req.method, headers.get(...)) runs only for real Request instances and otherwise return input as SecurityEventRequest; update references to req and the returned SecurityEventRequest accordingly.lib/ai/requirement-prompt.ts (1)
243-253: Consider validating locale parity for all prompt keys.Only
ai.prompt.defaultInstructionis resolved at module load; the other required keys (ai.prompt.system.intro,ai.prompt.system.taxonomyIntro,ai.prompt.system.headings.*,ai.prompt.system.outputRules,ai.prompt.noUsageScenariosAvailable,ai.prompt.userHeader) are fetched lazily. Ifsv.jsonis missing a key thaten.jsonhas (or vice-versa), the throw only surfaces for the affected locale at call time — and thenoUsageScenariosAvailablepath only fires whentaxonomy.scenariosis empty, which may not be exercised in tests.Consider adding a unit test (or a single startup validator) that iterates a declared list of required prompt paths across both
'en'and'sv', so any drift fails CI rather than a specific user's request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ai/requirement-prompt.ts` around lines 243 - 253, DEFAULT_INSTRUCTION_EN/DEFAULT_INSTRUCTION_SV call getPromptMessage at module load for only one key, but other required prompt keys (e.g. ai.prompt.system.intro, ai.prompt.system.taxonomyIntro, ai.prompt.system.headings.*, ai.prompt.system.outputRules, ai.prompt.noUsageScenariosAvailable, ai.prompt.userHeader) are fetched lazily which can hide missing keys until runtime; add a startup validator or unit test that lists all required prompt paths and iterates locales 'en' and 'sv', calling getPromptMessage for each path and failing fast (throw/assert) when any key is missing so key parity between locales is enforced in CI rather than at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ai/requirement-prompt.ts`:
- Around line 243-253: DEFAULT_INSTRUCTION_EN/DEFAULT_INSTRUCTION_SV call
getPromptMessage at module load for only one key, but other required prompt keys
(e.g. ai.prompt.system.intro, ai.prompt.system.taxonomyIntro,
ai.prompt.system.headings.*, ai.prompt.system.outputRules,
ai.prompt.noUsageScenariosAvailable, ai.prompt.userHeader) are fetched lazily
which can hide missing keys until runtime; add a startup validator or unit test
that lists all required prompt paths and iterates locales 'en' and 'sv', calling
getPromptMessage for each path and failing fast (throw/assert) when any key is
missing so key parity between locales is enforced in CI rather than at runtime.
In `@lib/auth/audit.ts`:
- Around line 107-129: The duck-typing in normalizeRequest that checks typeof
(input as Request).headers?.get === 'function' can misidentify non-Request
objects; replace that check with a proper runtime Request check using instanceof
Request (i.e., treat input as a Request only if input instanceof Request) so the
Request-specific logic (new URL(req.url).pathname, req.method, headers.get(...))
runs only for real Request instances and otherwise return input as
SecurityEventRequest; update references to req and the returned
SecurityEventRequest accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45b8f9fa-b4c3-443c-8a61-a2a8246de3b3
📒 Files selected for processing (4)
docs/auth-how-it-works.mdlib/ai/requirement-prompt.tslib/auth/audit.tstests/unit/auth-audit.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/auth-audit.test.ts
- docs/auth-how-it-works.md
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/auth/audit.ts`:
- Around line 107-129: normalizeRequest currently returns a SecurityEventRequest
input unchanged, which can leak query strings; update normalizeRequest to
sanitize input.path by stripping query and fragment (e.g., replace path with
input.path.split(/[?#]/,1)[0] ?? ''), then return a new SecurityEventRequest
object preserving other fields like method, userAgent, and requestId (use the
same field names as in normalizeRequest) so pre-shaped requests get the same
query-stripping behavior as native Request objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: deebb3ce-4cd2-4bf8-b1da-13635c6d9143
📒 Files selected for processing (3)
lib/auth/audit.tstests/unit/auth-audit.test.tstests/unit/requirement-prompt.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/auth-audit.test.ts
- tests/unit/requirement-prompt.test.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/auth-audit.test.ts (2)
219-239: Avoid coupling to V8's exact circular-reference wording.The assertion on Line 237 pins the test to V8's current phrasing
"Converting circular structure to JSON". Node has reworded this message in the past (e.g., to include the offending property path), and Vitest environments can run under different runtimes via aliases. A short, stable substring (or just asserting the 4-arg shape with a non-empty string) keeps the intent without the brittleness. Also, thecircular as Record<string, never>cast on Line 229 is semantically wrong —Record<string, never>means “object with no values” — and should beRecord<string, unknown>to match the public API.♻️ Proposed tightening
- // Force a circular reference into detail to provoke JSON.stringify. - detail: circular as Record<string, never>, + // Force a circular reference into detail to provoke JSON.stringify. + detail: circular as Record<string, unknown>, }), ).not.toThrow() expect(errorSpy).toHaveBeenCalledTimes(1) - expect(errorSpy.mock.calls[0]).toEqual([ - '[security-audit] failed to record event', - 'auth.token.rejected', - 'mcp', - expect.stringContaining('Converting circular structure to JSON'), - ]) + expect(errorSpy.mock.calls[0]).toEqual([ + '[security-audit] failed to record event', + 'auth.token.rejected', + 'mcp', + expect.stringMatching(/circular/i), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth-audit.test.ts` around lines 219 - 239, The test pins to V8's exact error wording and uses an incorrect cast; change the circular test so the circular object is typed as Record<string, unknown> (not Record<string, never>) and relax the assertion on errorSpy.mock.calls[0] to check the 4-arg shape with the last argument being a non-empty string (or contains a small stable substring like "circular") instead of asserting the full V8 message; update references to recordSecurityEvent and errorSpy in the assertions accordingly so the test validates that recordSecurityEvent does not throw and that errorSpy was called once with the event name, actor source, and a non-empty error string.
63-73: Tighten the ISO-8601 assertion (optional nit).
new Date(ev.ts).toISOString()only rejects unparseable strings; any parseable date (including non-ISO like"April 22 2026") would pass, so this doesn't verify the contract thattsis ISO-8601 UTC. A regex or round-trip equality gives more signal for what the comment says is being tested.♻️ Suggested strengthening
- const ev = emittedEvents()[0] - expect(typeof ev.ts).toBe('string') - expect(() => new Date(ev.ts as string).toISOString()).not.toThrow() + const ev = emittedEvents()[0] + expect(typeof ev.ts).toBe('string') + expect(ev.ts).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?Z$/) + // Round-trip to guarantee canonical UTC form + expect(new Date(ev.ts as string).toISOString()).toBe(ev.ts)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth-audit.test.ts` around lines 63 - 73, The test currently only checks that ev.ts parses as a Date; tighten it by asserting the timestamp is in ISO-8601 UTC format — either assert new Date(ev.ts as string).toISOString() === ev.ts as string (round-trip equality) or match ev.ts against an ISO-8601 UTC regex; update the assertion in the test that uses recordSecurityEvent and emittedEvents() (referenced symbols: recordSecurityEvent, emittedEvents, ev.ts) accordingly so the test verifies ISO-8601 Z-formatted timestamps rather than any parseable date string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/auth-audit.test.ts`:
- Around line 219-239: The test pins to V8's exact error wording and uses an
incorrect cast; change the circular test so the circular object is typed as
Record<string, unknown> (not Record<string, never>) and relax the assertion on
errorSpy.mock.calls[0] to check the 4-arg shape with the last argument being a
non-empty string (or contains a small stable substring like "circular") instead
of asserting the full V8 message; update references to recordSecurityEvent and
errorSpy in the assertions accordingly so the test validates that
recordSecurityEvent does not throw and that errorSpy was called once with the
event name, actor source, and a non-empty error string.
- Around line 63-73: The test currently only checks that ev.ts parses as a Date;
tighten it by asserting the timestamp is in ISO-8601 UTC format — either assert
new Date(ev.ts as string).toISOString() === ev.ts as string (round-trip
equality) or match ev.ts against an ISO-8601 UTC regex; update the assertion in
the test that uses recordSecurityEvent and emittedEvents() (referenced symbols:
recordSecurityEvent, emittedEvents, ev.ts) accordingly so the test verifies
ISO-8601 Z-formatted timestamps rather than any parseable date string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0da7cff6-c145-4aeb-910a-0bc573071452
📒 Files selected for processing (4)
docs/auth-how-it-works.mdlib/auth/audit.tstests/unit/auth-audit.test.tstests/unit/requirement-prompt.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/auth-how-it-works.md
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/auth/audit.ts
- tests/unit/requirement-prompt.test.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/auth-audit.test.ts (2)
156-206: Consider asserting the breadcrumbtsmatches the primary eventts.
recordRedactionBreadcrumbis invoked withnormalized.ts(snippet 1, line ~205), so every breadcrumb should share the primary event's timestamp. The current assertions coverauditEvent,actorSource,breadcrumb, andchannelbut notts, leaving a small gap around the per-event correlation guarantee.🧪 Optional assertion
expect(emittedRedactionBreadcrumbs()[0]).toMatchObject({ auditEvent: 'auth.login.failed', actorSource: 'oidc', breadcrumb: 'detail-key-redacted', channel: 'security-audit', + ts: ev.ts, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth-audit.test.ts` around lines 156 - 206, The test omits asserting that each redaction breadcrumb uses the same timestamp as the primary audit event; update the test after calling recordSecurityEvent(...) to capture the primary event (ev = emittedEvents()[0]) and then assert that each breadcrumb from emittedRedactionBreadcrumbs() has its ts equal to ev.ts (or at least assert emittedRedactionBreadcrumbs()[0].ts === ev.ts), since recordRedactionBreadcrumb is called with normalized.ts; reference emittedEvents(), emittedRedactionBreadcrumbs(), recordRedactionBreadcrumb, normalized.ts and recordSecurityEvent when making the assertion.
220-244: Minor: assertion oferrorCalllength relies on implementation detail.
expect(errorCall).toHaveLength(4)locks in the exact positional shape of theconsole.errorcall (message, event, actorSource, err-message). This is fine as a regression guard, but if the audit module later appends context (e.g., a request id or correlation token), this test will fail even though the behavior is still correct. Consider a looser assertion (e.g.,toHaveLength(expect.any(Number))is not possible; instead useexpect(errorCall?.length).toBeGreaterThanOrEqual(4)) if you want the test to remain resilient to added diagnostic fields. Optional — current strictness is defensible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth-audit.test.ts` around lines 220 - 244, Update the brittle assertion that checks the exact number of args passed to console.error by relaxing the length check on the captured call; instead of asserting expect(errorCall).toHaveLength(4) use a numeric-range assertion against errorCall.length (e.g., expect(errorCall?.length).toBeGreaterThanOrEqual(4)) so the test still verifies the required first 4 fields while allowing additional diagnostic fields to be appended; locate the assertion around the variable errorCall derived from errorSpy.mock.calls[0] in the test case "does not throw when JSON.stringify throws (circular ref)" and replace the strict length check accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/auth-audit.test.ts`:
- Around line 156-206: The test omits asserting that each redaction breadcrumb
uses the same timestamp as the primary audit event; update the test after
calling recordSecurityEvent(...) to capture the primary event (ev =
emittedEvents()[0]) and then assert that each breadcrumb from
emittedRedactionBreadcrumbs() has its ts equal to ev.ts (or at least assert
emittedRedactionBreadcrumbs()[0].ts === ev.ts), since recordRedactionBreadcrumb
is called with normalized.ts; reference emittedEvents(),
emittedRedactionBreadcrumbs(), recordRedactionBreadcrumb, normalized.ts and
recordSecurityEvent when making the assertion.
- Around line 220-244: Update the brittle assertion that checks the exact number
of args passed to console.error by relaxing the length check on the captured
call; instead of asserting expect(errorCall).toHaveLength(4) use a numeric-range
assertion against errorCall.length (e.g.,
expect(errorCall?.length).toBeGreaterThanOrEqual(4)) so the test still verifies
the required first 4 fields while allowing additional diagnostic fields to be
appended; locate the assertion around the variable errorCall derived from
errorSpy.mock.calls[0] in the test case "does not throw when JSON.stringify
throws (circular ref)" and replace the strict length check accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a011b943-4ed4-48d4-89f3-f0d0437bc5a6
📒 Files selected for processing (1)
tests/unit/auth-audit.test.ts
…ons and error call validation
1 participant
Description
This branch is based on the branch for the PR #92. We can merge this PR in instead of PR #92.
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is