Conversation
…hentication. Added support for Keycloak integration and improved project service URL handling. Refactored API context to utilize shared authentication and streamlined project element fetching logic.
…d-auth path and update various package versions. Removed extraneous dependencies and improved dependency resolution for better project structure.
WalkthroughAdds JWT-based auth/authorization middleware with Keycloak JWKS verification, extends backend config, updates dependencies, guards backend routes, introduces a frontend authenticated app shell with role-based routing, creates an authenticated API client, and refactors ApiContext to a minimal, token-enabled client provider. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant FE as Frontend (AuthenticatedApp)
participant Auth as Auth Provider
participant API as AuthenticatedApiClient
participant BE as Backend (Express)
participant KC as Keycloak JWKS
U->>FE: Navigate to app
FE->>Auth: Initialize (autoLogin)
Auth-->>FE: isAuthenticated? token?
FE->>API: setAuthTokenGetter(getAccessToken)
U->>FE: Request protected route
FE->>API: GET /projects (with Bearer token)
API->>BE: /projects
BE->>BE: authMiddleware verifies JWT
BE->>KC: Fetch signing key (JWKS) if needed
KC-->>BE: Public key
BE->>BE: Validate issuer/audience, extract roles
BE->>BE: requireReadPermission
BE-->>API: 200 OK (projects)
API-->>FE: Projects data
FE-->>U: Render view
sequenceDiagram
autonumber
actor U as User
participant FE as Frontend
participant API as AuthenticatedApiClient
participant BE as Backend
participant PS as Project Service
U->>FE: Open project elements
FE->>API: GET /project-elements/:projectId
API->>BE: /project-elements/:projectId (Bearer token)
BE->>BE: authMiddleware + requireProjectAccess('read')
alt Non-admin user
BE->>PS: GET /users/me/projects (Bearer token)
PS-->>BE: User projects + roles
BE->>BE: Check action roles for project
alt Authorized
BE-->>API: 200 OK
else Unauthorized
BE-->>API: 403 Forbidden
end
else Admin user
BE-->>API: 200 OK
end
API-->>FE: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/server.ts (5)
216-233: Guard get-kennwerte with read + project access.Currently only global auth applies; users with generic plugin access can fetch any project’s kennwerte. Enforce read permission and project-level read.
app.get("/get-kennwerte/:projectName", param('projectName').trim().notEmpty().withMessage('Project name is required'), handleValidationErrors, haltOnTimedout, + requireReadPermission, + requireProjectAccess('read'), async (req: Request, res: Response) => {
235-266: Enforce per-project write access on save-kennwerte.Without project-level checks, a user with write role could write to any project.
app.post("/save-kennwerte", strictLimiter, // Apply strict rate limiting for write operations body('projectName').trim().notEmpty().withMessage('Project name is required'), body('kennwerte').isObject().withMessage('Kennwerte must be an object'), handleValidationErrors, haltOnTimedout, - requireWritePermission, + requireWritePermission, + requireProjectAccess('write'), async (req: Request, res: Response) => {
268-280: Enforce per-project write access on reapply-costs.Same security gap as above.
app.post("/reapply-costs", strictLimiter, body('projectName').trim().notEmpty().withMessage('Project name is required'), handleValidationErrors, haltOnTimedout, - requireWritePermission, + requireWritePermission, + requireProjectAccess('write'), async (req: Request, res: Response) => {
293-301: Require project and enforce per-project write on confirm-costs.Project is optional and there is no project access check. This enables sending costs for arbitrary/unknown projects.
app.post("/confirm-costs", strictLimiter, body('data').isArray().withMessage('Data must be an array'), body('data.*.id').notEmpty().withMessage('Each element must have an id'), - body('project').optional().trim().notEmpty(), + body('project').trim().notEmpty().withMessage('Project is required'), handleValidationErrors, haltOnTimedout, - requireWritePermission, + requireWritePermission, + requireProjectAccess('write'), async (req: Request<{}, {}, KafkaMessageBody>, res: Response) => {
159-165: IncludeprojectNamein access middlewareIn
backend/auth-middleware.ts, update the param extraction to also read the route’sprojectName:- const projectId = req.params.projectId || req.params.project_name; + const projectId = req.params.projectId + || req.params.project_name + || req.params.projectName;
🧹 Nitpick comments (12)
backend/package.json (1)
23-25: Enforce JWKS rate limiting and clock toleranceJWKS client is configured with caching (cacheMaxEntries/cacheMaxAge) but missing rate limiting—add
rateLimit: trueandjwksRequestsPerMinutein thejwksClient()options (backend/auth-middleware.ts).jwt.verifyalready enforcesexp/nbf; consider adding a smallclockTolerance(e.g., 5s) to accommodate clock skew.package.json (1)
18-24: Validate auth UI deps and workspace path.
- @nhmzh/shared-auth via file: path: confirm this resolves in CI and publish workflows; consider using workspaces ("workspace:*") if you’re on npm/pnpm workspaces.
- oidc-client-ts/react-oidc-context: check peer deps align with React 18 and React Router v7 usage in the app shell.
If you want, I can add a quick script to verify peer-dep compatibility.backend/config.ts (1)
21-28: Prod safety: avoid http default authority and require https unless explicitly disabled.Defaulting to http for KEYCLOAK_AUTHORITY is risky in prod. Recommend:
- Fail fast (or log a loud warning) if NODE_ENV=production and authority is http.
- Add KEYCLOAK_REQUIRE_HTTPS (default true) and enforce it.
I can add a small guard if you want.
src/api/AuthenticatedApiClient.ts (5)
36-39: Gate request logging to dev.Avoid noisy logs in prod and possible URL leakage.
- console.debug(`Making authenticated request to: ${config.url}`); + if (import.meta.env.DEV) console.debug(`[cost-api] -> ${config.method?.toUpperCase()} ${config.url}`);
47-61: Surface 401/403 to the auth layer with a callback.Consider accepting an optional onAuthError callback (set alongside the token getter) to centralize re-login/refresh and user messaging.
I can add a small hook-based notifier if desired.
121-136: Strongly type payloads and responses.Replace any with typed DTOs and use Axios generics:
- async saveCostCalculations(projectId: string, calculations: any): Promise<any> { + async saveCostCalculations(projectId: string, calculations: CostCalculationsDto): Promise<SaveResult> { const response = await this.axiosInstance.post<SaveResult>(I can sketch DTOs if you share backend response shapes.
172-190: Type the blob response explicitly.Small improvement:
- const response = await this.axiosInstance.get( + const response = await this.axiosInstance.get<Blob>( `/projects/${encodeURIComponent(projectId)}/costs/export`,
279-281: Consider lazy/factory init for testability and token injection order.Export a factory or lazy singleton to avoid constructing the client before setAuthTokenGetter is registered and to ease testing.
I can provide a minimal refactor if wanted.
src/AuthenticatedApp.tsx (2)
49-51: Avoid indefinite spinner when auth fails.If auto-login fails or the session is invalid, this returns LoadingScreen forever. Consider handling an error state from useAuth() (if exposed) to redirect to /unauthorized or surface a retry/login action.
54-70: Optional: add a catch-all route to prevent accidental blank screens.Redirect unknown paths to /plugin-cost or show a 404.
<Route path="/unauthorized" element={<UnauthorizedScreen />} /> <Route path="/" element={<Navigate to="/plugin-cost" replace />} /> + <Route path="*" element={<Navigate to="/plugin-cost" replace />} />src/contexts/ApiContext.tsx (1)
1-1: Stabilize context value to avoid unnecessary re-renders.Memoize the provider value so downstream consumers don’t re-render on every parent render.
-import React, { createContext, useContext, useEffect } from 'react'; +import React, { createContext, useContext, useEffect, useMemo } from 'react'; @@ - return ( - <ApiContext.Provider value={{ apiClient: authenticatedCostApiClient }}> + const value = useMemo(() => ({ apiClient: authenticatedCostApiClient }), []); + return ( + <ApiContext.Provider value={value}> {children} </ApiContext.Provider> );Also applies to: 20-24
backend/auth-middleware.ts (1)
38-49: Single source of truth for roles across FE/BE.These role lists are duplicated in the frontend (ProtectedRoute). Publish shared role constants (e.g., a small shared package or generated types) to prevent drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
backend/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
backend/auth-middleware.ts(1 hunks)backend/config.ts(1 hunks)backend/package.json(1 hunks)backend/server.ts(7 hunks)package.json(1 hunks)src/AuthenticatedApp.tsx(1 hunks)src/StandaloneApp.tsx(1 hunks)src/api/AuthenticatedApiClient.ts(1 hunks)src/contexts/ApiContext.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/auth-middleware.ts (1)
backend/config.ts (1)
config(13-46)
backend/server.ts (2)
backend/auth-middleware.ts (5)
authMiddleware(144-202)requireReadPermission(205-220)User(22-29)requireProjectAccess(302-323)requireWritePermission(222-237)backend/mongodb.ts (1)
getAllProjects(1264-1287)
src/contexts/ApiContext.tsx (1)
src/api/AuthenticatedApiClient.ts (1)
setAuthTokenGetter(10-12)
🔇 Additional comments (3)
backend/package.json (1)
32-32: Good: add typings for jsonwebtoken.This prevents ad-hoc any usage around token claims.
src/StandaloneApp.tsx (1)
2-5: LGTM: delegate to AuthenticatedApp.Clean handoff; reduces routing/auth duplication.
src/contexts/ApiContext.tsx (1)
15-19: Confirm timing: token getter set before first API call.setAuthTokenGetter runs in useEffect (post-mount). If any child fires a request on initial render, it could miss the getter. Mounting ApiProvider under AuthProvider (as suggested) helps, but please confirm no immediate API calls occur before effects run.
| let jwksClientInstance: jwksClient.JwksClient | null = null; | ||
|
|
||
| const getJwksClient = (): jwksClient.JwksClient => { | ||
| if (!jwksClientInstance) { | ||
| const authority = config.keycloak.authority; | ||
| jwksClientInstance = jwksClient({ | ||
| jwksUri: `${authority}/protocol/openid-connect/certs`, | ||
| cache: true, | ||
| cacheMaxEntries: 5, | ||
| cacheMaxAge: 10 * 60 * 1000, // 10 minutes | ||
| }); | ||
| } | ||
| return jwksClientInstance; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden JWKS client configuration.
Enable rate limiting to protect your JWKS endpoint and rely on the client’s internal cache (you can drop the custom keyCache if desired).
- jwksClientInstance = jwksClient({
+ jwksClientInstance = jwksClient({
jwksUri: `${authority}/protocol/openid-connect/certs`,
- cache: true,
+ cache: true,
+ rateLimit: true,
cacheMaxEntries: 5,
cacheMaxAge: 10 * 60 * 1000, // 10 minutes
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let jwksClientInstance: jwksClient.JwksClient | null = null; | |
| const getJwksClient = (): jwksClient.JwksClient => { | |
| if (!jwksClientInstance) { | |
| const authority = config.keycloak.authority; | |
| jwksClientInstance = jwksClient({ | |
| jwksUri: `${authority}/protocol/openid-connect/certs`, | |
| cache: true, | |
| cacheMaxEntries: 5, | |
| cacheMaxAge: 10 * 60 * 1000, // 10 minutes | |
| }); | |
| } | |
| return jwksClientInstance; | |
| let jwksClientInstance: jwksClient.JwksClient | null = null; | |
| const getJwksClient = (): jwksClient.JwksClient => { | |
| if (!jwksClientInstance) { | |
| const authority = config.keycloak.authority; | |
| jwksClientInstance = jwksClient({ | |
| jwksUri: `${authority}/protocol/openid-connect/certs`, | |
| cache: true, | |
| rateLimit: true, | |
| cacheMaxEntries: 5, | |
| cacheMaxAge: 10 * 60 * 1000, // 10 minutes | |
| }); | |
| } | |
| return jwksClientInstance; | |
| }; |
🤖 Prompt for AI Agents
In backend/auth-middleware.ts around lines 55 to 67, the jwksClient constructor
should be hardened by enabling the built-in rate limiter and relying on the
client's internal cache instead of any external/custom keyCache; update the
options to include rateLimit: true (and optionally a reasonable
jwksRequestsPerMinute limit if supported by your jwks client version) and remove
or stop using any external keyCache logic so the client uses its own cache and
rate limiting to protect the JWKS endpoint.
| // Project service integration | ||
| export const checkProjectAccess = async ( | ||
| projectId: string, | ||
| action: 'read' | 'write' | 'delete', | ||
| user: User, | ||
| token: string | ||
| ): Promise<boolean> => { | ||
| // Admin users have access to all projects | ||
| if (user.roles.includes('Admin')) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| // Fetch user's projects from the project service | ||
| const projectServiceUrl = config.projectService?.url || 'http://localhost:3001'; | ||
| const response = await axios.get(`${projectServiceUrl}/api/projects/my-projects`, { | ||
| headers: { | ||
| 'Authorization': `Bearer ${token}` | ||
| } | ||
| }); | ||
|
|
||
| const projects = response.data.projects || []; | ||
|
|
||
| // Check if user has access to this project | ||
| for (const project of projects) { | ||
| if (project.projectId === projectId || project.erzProjectCode === projectId) { | ||
| const projectRoles = project.roles || []; | ||
|
|
||
| if (action === 'read') { | ||
| return projectRoles.some((role: string) => PLUGIN_ROLES.read.includes(role)); | ||
| } else if (action === 'write') { | ||
| return projectRoles.some((role: string) => PLUGIN_ROLES.write.includes(role)); | ||
| } else if (action === 'delete') { | ||
| return projectRoles.some((role: string) => PLUGIN_ROLES.delete.includes(role)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } catch (error) { | ||
| logger.error('Error checking project access:', error); | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
checkProjectAccess: add timeout and accept additional identifiers.
- External call lacks a timeout; a slow project service will stall requests.
- If routes pass projectName today, also match by project.name to avoid false denials (until the API standardizes on projectId/erzProjectCode).
- const response = await axios.get(`${projectServiceUrl}/api/projects/my-projects`, {
- headers: {
- 'Authorization': `Bearer ${token}`
- }
- });
+ const response = await axios.get(`${projectServiceUrl}/api/projects/my-projects`, {
+ headers: { 'Authorization': `Bearer ${token}` },
+ timeout: 3000,
+ });
@@
- if (project.projectId === projectId || project.erzProjectCode === projectId) {
+ if (
+ project.projectId === projectId ||
+ project.erzProjectCode === projectId ||
+ project.name === projectId
+ ) {Longer-term: standardize all backend routes and frontend clients to use a canonical project identifier (projectId or erzProjectCode) and remove the name fallback.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Project service integration | |
| export const checkProjectAccess = async ( | |
| projectId: string, | |
| action: 'read' | 'write' | 'delete', | |
| user: User, | |
| token: string | |
| ): Promise<boolean> => { | |
| // Admin users have access to all projects | |
| if (user.roles.includes('Admin')) { | |
| return true; | |
| } | |
| try { | |
| // Fetch user's projects from the project service | |
| const projectServiceUrl = config.projectService?.url || 'http://localhost:3001'; | |
| const response = await axios.get(`${projectServiceUrl}/api/projects/my-projects`, { | |
| headers: { | |
| 'Authorization': `Bearer ${token}` | |
| } | |
| }); | |
| const projects = response.data.projects || []; | |
| // Check if user has access to this project | |
| for (const project of projects) { | |
| if (project.projectId === projectId || project.erzProjectCode === projectId) { | |
| const projectRoles = project.roles || []; | |
| if (action === 'read') { | |
| return projectRoles.some((role: string) => PLUGIN_ROLES.read.includes(role)); | |
| } else if (action === 'write') { | |
| return projectRoles.some((role: string) => PLUGIN_ROLES.write.includes(role)); | |
| } else if (action === 'delete') { | |
| return projectRoles.some((role: string) => PLUGIN_ROLES.delete.includes(role)); | |
| } | |
| } | |
| } | |
| return false; | |
| } catch (error) { | |
| logger.error('Error checking project access:', error); | |
| return false; | |
| } | |
| }; | |
| // Project service integration | |
| export const checkProjectAccess = async ( | |
| projectId: string, | |
| action: 'read' | 'write' | 'delete', | |
| user: User, | |
| token: string | |
| ): Promise<boolean> => { | |
| // Admin users have access to all projects | |
| if (user.roles.includes('Admin')) { | |
| return true; | |
| } | |
| try { | |
| // Fetch user's projects from the project service | |
| const projectServiceUrl = config.projectService?.url || 'http://localhost:3001'; | |
| const response = await axios.get(`${projectServiceUrl}/api/projects/my-projects`, { | |
| headers: { 'Authorization': `Bearer ${token}` }, | |
| timeout: 3000, | |
| }); | |
| const projects = response.data.projects || []; | |
| // Check if user has access to this project | |
| for (const project of projects) { | |
| if ( | |
| project.projectId === projectId || | |
| project.erzProjectCode === projectId || | |
| project.name === projectId | |
| ) { | |
| const projectRoles = project.roles || []; | |
| if (action === 'read') { | |
| return projectRoles.some((role: string) => PLUGIN_ROLES.read.includes(role)); | |
| } else if (action === 'write') { | |
| return projectRoles.some((role: string) => PLUGIN_ROLES.write.includes(role)); | |
| } else if (action === 'delete') { | |
| return projectRoles.some((role: string) => PLUGIN_ROLES.delete.includes(role)); | |
| } | |
| } | |
| } | |
| return false; | |
| } catch (error) { | |
| logger.error('Error checking project access:', error); | |
| return false; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In backend/auth-middleware.ts around lines 256 to 299, the external
project-service GET call has no timeout and the project matching only checks
projectId/erzProjectCode; update the axios request to include a reasonable
timeout (e.g., 3–5s) in the options object and keep the Authorization header,
and extend the matching logic so a project also matches when project.name ===
projectId (to accept routes that pass a projectName until the API standardizes);
preserve existing role checks and error handling when applying these changes.
| // Middleware to check project-specific access | ||
| export const requireProjectAccess = (action: 'read' | 'write' | 'delete' = 'read') => { | ||
| return async (req: Request, res: Response, next: NextFunction) => { | ||
| if (!req.user || !req.token) { | ||
| return res.status(401).json({ error: 'User not authenticated' }); | ||
| } | ||
|
|
||
| const projectId = req.params.projectId || req.params.project_name; | ||
| if (!projectId) { | ||
| return res.status(400).json({ error: 'Project ID is required' }); | ||
| } | ||
|
|
||
| const hasAccess = await checkProjectAccess(projectId, action, req.user, req.token); | ||
|
|
||
| if (!hasAccess) { | ||
| return res.status(403).json({ | ||
| error: `You don't have ${action} permission for project: ${projectId}` | ||
| }); | ||
| } | ||
|
|
||
| next(); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
requireProjectAccess: read common parameter names and body/query fallbacks.
Routes currently pass projectName; this middleware won’t find it. Until the API is standardized, support multiple keys.
-export const requireProjectAccess = (action: 'read' | 'write' | 'delete' = 'read') => {
+export const requireProjectAccess = (action: 'read' | 'write' | 'delete' = 'read') => {
return async (req: Request, res: Response, next: NextFunction) => {
@@
- const projectId = req.params.projectId || req.params.project_name;
- if (!projectId) {
+ const projectId =
+ (req.params as any).projectId ||
+ (req.params as any).project_name ||
+ (req.params as any).projectName ||
+ (req.body as any)?.project ||
+ (req.body as any)?.projectName ||
+ (typeof (req.query as any)?.project === 'string' ? (req.query as any).project : undefined);
+ if (!projectId) {
return res.status(400).json({ error: 'Project ID is required' });
}
- const hasAccess = await checkProjectAccess(projectId, action, req.user, req.token);
+ const hasAccess = await checkProjectAccess(projectId, action, req.user, req.token);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Middleware to check project-specific access | |
| export const requireProjectAccess = (action: 'read' | 'write' | 'delete' = 'read') => { | |
| return async (req: Request, res: Response, next: NextFunction) => { | |
| if (!req.user || !req.token) { | |
| return res.status(401).json({ error: 'User not authenticated' }); | |
| } | |
| const projectId = req.params.projectId || req.params.project_name; | |
| if (!projectId) { | |
| return res.status(400).json({ error: 'Project ID is required' }); | |
| } | |
| const hasAccess = await checkProjectAccess(projectId, action, req.user, req.token); | |
| if (!hasAccess) { | |
| return res.status(403).json({ | |
| error: `You don't have ${action} permission for project: ${projectId}` | |
| }); | |
| } | |
| next(); | |
| }; | |
| }; | |
| // Middleware to check project-specific access | |
| export const requireProjectAccess = (action: 'read' | 'write' | 'delete' = 'read') => { | |
| return async (req: Request, res: Response, next: NextFunction) => { | |
| if (!req.user || !req.token) { | |
| return res.status(401).json({ error: 'User not authenticated' }); | |
| } | |
| const projectId = | |
| (req.params as any).projectId || | |
| (req.params as any).project_name || | |
| (req.params as any).projectName || | |
| (req.body as any)?.project || | |
| (req.body as any)?.projectName || | |
| (typeof (req.query as any)?.project === 'string' ? (req.query as any).project : undefined); | |
| if (!projectId) { | |
| return res.status(400).json({ error: 'Project ID is required' }); | |
| } | |
| const hasAccess = await checkProjectAccess(projectId, action, req.user, req.token); | |
| if (!hasAccess) { | |
| return res.status(403).json({ | |
| error: `You don't have ${action} permission for project: ${projectId}` | |
| }); | |
| } | |
| next(); | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In backend/auth-middleware.ts around lines 301 to 323, the middleware only reads
req.params.projectId or req.params.project_name so it will miss routes that send
projectName or put the identifier in body/query; update the projectId resolution
to check common keys in params, body and query (e.g. projectId, project_name,
projectName, project) and normalize the value into a single projectId variable
before validation, making sure to coerce to string and handle missing/empty
values the same way as now; then pass that normalized projectId to
checkProjectAccess.
| keycloak: { | ||
| authority: process.env.KEYCLOAK_AUTHORITY || 'http://localhost:8080/realms/nhmzh', | ||
| clientId: process.env.KEYCLOAK_CLIENT_ID || 'plugin-cost', | ||
| clientSecret: process.env.KEYCLOAK_CLIENT_SECRET, | ||
| }, | ||
| projectService: { | ||
| url: process.env.PROJECT_SERVICE_URL || 'http://localhost:3001', | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit issuer/audience and operational knobs.
Right now only authority/clientId are exposed. For robust verification and ops, include:
- issuer (often same as authority, but explicit)
- audience (expected aud/azp)
- optional jwksUri override
- small clock tolerance
- projectService timeout
Apply this patch:
keycloak: {
- authority: process.env.KEYCLOAK_AUTHORITY || 'http://localhost:8080/realms/nhmzh',
- clientId: process.env.KEYCLOAK_CLIENT_ID || 'plugin-cost',
- clientSecret: process.env.KEYCLOAK_CLIENT_SECRET,
+ authority: process.env.KEYCLOAK_AUTHORITY || 'http://localhost:8080/realms/nhmzh',
+ issuer: process.env.KEYCLOAK_ISSUER || process.env.KEYCLOAK_AUTHORITY || 'http://localhost:8080/realms/nhmzh',
+ audience: process.env.KEYCLOAK_AUDIENCE || process.env.KEYCLOAK_CLIENT_ID || 'plugin-cost',
+ jwksUri: process.env.KEYCLOAK_JWKS_URI, // optional override
+ clockToleranceSec: parseInt(process.env.KEYCLOAK_CLOCK_TOLERANCE_SEC || '60'),
+ clientId: process.env.KEYCLOAK_CLIENT_ID || 'plugin-cost',
+ clientSecret: process.env.KEYCLOAK_CLIENT_SECRET, // likely unused for RS256 verify; keep if introspection is needed
},
projectService: {
- url: process.env.PROJECT_SERVICE_URL || 'http://localhost:3001',
+ url: process.env.PROJECT_SERVICE_URL || 'http://localhost:3001',
+ timeoutMs: parseInt(process.env.PROJECT_SERVICE_TIMEOUT_MS || '5000'),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| keycloak: { | |
| authority: process.env.KEYCLOAK_AUTHORITY || 'http://localhost:8080/realms/nhmzh', | |
| clientId: process.env.KEYCLOAK_CLIENT_ID || 'plugin-cost', | |
| clientSecret: process.env.KEYCLOAK_CLIENT_SECRET, | |
| }, | |
| projectService: { | |
| url: process.env.PROJECT_SERVICE_URL || 'http://localhost:3001', | |
| }, | |
| keycloak: { | |
| authority: process.env.KEYCLOAK_AUTHORITY || 'http://localhost:8080/realms/nhmzh', | |
| issuer: process.env.KEYCLOAK_ISSUER || process.env.KEYCLOAK_AUTHORITY || 'http://localhost:8080/realms/nhmzh', | |
| audience: process.env.KEYCLOAK_AUDIENCE || process.env.KEYCLOAK_CLIENT_ID || 'plugin-cost', | |
| jwksUri: process.env.KEYCLOAK_JWKS_URI, // optional override | |
| clockToleranceSec: parseInt(process.env.KEYCLOAK_CLOCK_TOLERANCE_SEC || '60'), | |
| clientId: process.env.KEYCLOAK_CLIENT_ID || 'plugin-cost', | |
| clientSecret: process.env.KEYCLOAK_CLIENT_SECRET, // likely unused for RS256 verify; keep if introspection is needed | |
| }, | |
| projectService: { | |
| url: process.env.PROJECT_SERVICE_URL || 'http://localhost:3001', | |
| timeoutMs: parseInt(process.env.PROJECT_SERVICE_TIMEOUT_MS || '5000'), | |
| }, |
🤖 Prompt for AI Agents
In backend/config.ts around lines 21 to 28, the Keycloak and projectService
config only expose authority/clientId/clientSecret and url; add explicit issuer
(default to authority), audience (default to clientId), an optional jwksUri
override, and a small clockToleranceSeconds (e.g. 5) to support token
verification tolerance, all wired to env vars (KEYCLOAK_ISSUER,
KEYCLOAK_AUDIENCE, KEYCLOAK_JWKS_URI, KEYCLOAK_CLOCK_TOLERANCE). Also add a
projectService.timeoutSecs (env PROJECT_SERVICE_TIMEOUT_SECS with a sensible
default like 5) so callers can use a request timeout. Ensure types/defaults use
existing pattern (process.env... || fallback) and keep keys under keycloak and
projectService objects.
| // --- Authentication Middleware --- | ||
| // Apply authentication to all routes except health check | ||
| app.use(authMiddleware({ optional: false })); | ||
| // Skip authentication for health endpoint | ||
| app.use("/health", authMiddleware({ optional: true })); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
/health is still protected due to middleware order.
The global auth middleware runs for /health before the optional one, so health checks return 401/403. Skip /health in the global gate.
-// --- Authentication Middleware ---
-// Apply authentication to all routes except health check
-app.use(authMiddleware({ optional: false }));
-// Skip authentication for health endpoint
-app.use("/health", authMiddleware({ optional: true }));
+// --- Authentication Middleware ---
+// Apply authentication to all routes, but skip /health
+app.use((req, res, next) =>
+ req.path === '/health'
+ ? next()
+ : authMiddleware({ optional: false })(req, res, next)
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // --- Authentication Middleware --- | |
| // Apply authentication to all routes except health check | |
| app.use(authMiddleware({ optional: false })); | |
| // Skip authentication for health endpoint | |
| app.use("/health", authMiddleware({ optional: true })); | |
| // --- Authentication Middleware --- | |
| // Apply authentication to all routes, but skip /health | |
| app.use((req, res, next) => | |
| req.path === '/health' | |
| ? next() | |
| : authMiddleware({ optional: false })(req, res, next) | |
| ); |
🤖 Prompt for AI Agents
In backend/server.ts around lines 100 to 105, the global auth middleware is
registered before the health-specific optional middleware so /health is still
protected; change the order or exclude the /health path from the global gate:
register the /health route (or its authMiddleware({ optional: true })) before
app.use(authMiddleware({ optional: false })) OR update the global middleware to
skip the /health path (e.g., check req.path and call next() for "/health") so
the health endpoint is not authenticated.
| baseURL: process.env.NODE_ENV === 'production' | ||
| ? '/api' | ||
| : 'http://localhost:3004', | ||
| timeout: 30000, | ||
| }); |
There was a problem hiding this comment.
Fix: process.env is not reliable in Vite browser builds.
Vite uses import.meta.env.PROD/DEV. Using process.env.NODE_ENV can throw at runtime in the browser.
Apply:
- baseURL: process.env.NODE_ENV === 'production'
+ baseURL: import.meta.env.PROD
? '/api'
: 'http://localhost:3004',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| baseURL: process.env.NODE_ENV === 'production' | |
| ? '/api' | |
| : 'http://localhost:3004', | |
| timeout: 30000, | |
| }); | |
| baseURL: import.meta.env.PROD | |
| ? '/api' | |
| : 'http://localhost:3004', | |
| timeout: 30000, | |
| }); |
🤖 Prompt for AI Agents
In src/api/AuthenticatedApiClient.ts around lines 19 to 23, the code uses
process.env.NODE_ENV to choose the baseURL which is unreliable in Vite browser
builds; replace that check with Vite's import.meta.env (use import.meta.env.PROD
or import.meta.env.DEV) to determine production vs development and set baseURL
accordingly, ensuring any TypeScript typings for import.meta.env are present in
the project (or cast/guard as needed) so the code won't reference process.env at
runtime in the browser.
| async importCostData(projectId: string, file: File): Promise<any> { | ||
| try { | ||
| const formData = new FormData(); | ||
| formData.append('file', file); | ||
|
|
||
| const response = await this.axiosInstance.post( | ||
| `/projects/${encodeURIComponent(projectId)}/costs/import`, | ||
| formData, | ||
| { | ||
| headers: { | ||
| 'Content-Type': 'multipart/form-data' | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
Bug: don’t set multipart Content-Type manually (boundary gets lost).
Browsers must set the boundary; setting Content-Type breaks uploads.
- const response = await this.axiosInstance.post(
+ const response = await this.axiosInstance.post(
`/projects/${encodeURIComponent(projectId)}/costs/import`,
formData,
{
- headers: {
- 'Content-Type': 'multipart/form-data'
- }
+ // Let the browser set multipart boundary automatically
}
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async importCostData(projectId: string, file: File): Promise<any> { | |
| try { | |
| const formData = new FormData(); | |
| formData.append('file', file); | |
| const response = await this.axiosInstance.post( | |
| `/projects/${encodeURIComponent(projectId)}/costs/import`, | |
| formData, | |
| { | |
| headers: { | |
| 'Content-Type': 'multipart/form-data' | |
| } | |
| } | |
| ); | |
| async importCostData(projectId: string, file: File): Promise<any> { | |
| try { | |
| const formData = new FormData(); | |
| formData.append('file', file); | |
| const response = await this.axiosInstance.post( | |
| `/projects/${encodeURIComponent(projectId)}/costs/import`, | |
| formData, | |
| { | |
| // Let the browser set multipart boundary automatically | |
| } | |
| ); |
🤖 Prompt for AI Agents
In src/api/AuthenticatedApiClient.ts around lines 198 to 211, the code manually
sets 'Content-Type: multipart/form-data' which removes the required boundary and
breaks uploads; remove that manual header so the browser (or axios when provided
FormData) can set the Content-Type with the correct boundary automatically —
simply omit the headers object (or at least the Content-Type entry) when posting
the FormData.
| <Typography variant="h4" color="error"> | ||
| Access Denied | ||
| </Typography> | ||
| <Typography variant="body1" sx={{ mt: 2 }}> | ||
| You don't have permission to access this application. | ||
| </Typography> | ||
| <Typography variant="body2" sx={{ mt: 1, color: 'text.secondary' }}> | ||
| Required roles: Admin, Fachplanung_Kosten, or Viewer | ||
| </Typography> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix role list drift between UI text and gate; centralize READ_ROLES.
UnauthorizedScreen shows only three roles, but ProtectedRoute requires eight. This will confuse users and drift over time. Define a single READ_ROLES constant and reuse it for both the text and the gate.
@@
-import { CircularProgress, Box, Typography } from '@mui/material';
+import { CircularProgress, Box, Typography } from '@mui/material';
+
+// Single source of truth for read roles (keep in sync with backend PLUGIN_ROLES.read)
+const READ_ROLES = [
+ 'Admin',
+ 'Fachplanung_Kosten',
+ 'Projektleitung_Architektur',
+ 'Projektleitung_Statik',
+ 'Projektleitung_Gebaudetechnik',
+ 'Fachplanung_Oekobilanz',
+ 'Fachplanung_Gebaudetechnik',
+ 'Viewer',
+];
@@
- <Typography variant="body2" sx={{ mt: 1, color: 'text.secondary' }}>
- Required roles: Admin, Fachplanung_Kosten, or Viewer
- </Typography>
+ <Typography variant="body2" sx={{ mt: 1, color: 'text.secondary' }}>
+ Required roles: {READ_ROLES.join(', ')}
+ </Typography>
@@
- <ProtectedRoute
- requiredRoles={['Admin', 'Fachplanung_Kosten', 'Projektleitung_Architektur',
- 'Projektleitung_Statik', 'Projektleitung_Gebaudetechnik',
- 'Fachplanung_Oekobilanz', 'Fachplanung_Gebaudetechnik', 'Viewer']}
+ <ProtectedRoute
+ requiredRoles={READ_ROLES}
fallbackPath="/unauthorized"
>Also applies to: 58-66, 6-7
| <BrowserRouter> | ||
| <AuthProvider appName="plugin-cost" autoLogin={true}> | ||
| <AuthenticatedContent /> | ||
| </AuthProvider> | ||
| </BrowserRouter> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mount ApiProvider under AuthProvider so API calls always have a token.
Ensure the authenticated API client is available app-wide and receives the token getter before children run.
@@
-import { FC } from 'react';
+import { FC } from 'react';
import { BrowserRouter, Routes, Route, Navigate } from 'react-router-dom';
import { AuthProvider, useAuth, ProtectedRoute } from '@nhmzh/shared-auth';
import App from './App';
import { CircularProgress, Box, Typography } from '@mui/material';
+import { ApiProvider } from './contexts/ApiContext';
@@
return (
<BrowserRouter>
<AuthProvider appName="plugin-cost" autoLogin={true}>
- <AuthenticatedContent />
+ <ApiProvider>
+ <AuthenticatedContent />
+ </ApiProvider>
</AuthProvider>
</BrowserRouter>
);Also applies to: 1-5
🤖 Prompt for AI Agents
In src/AuthenticatedApp.tsx around lines 76 to 80, the ApiProvider must be
mounted under AuthProvider so that the authenticated API client always has
access to the token getter before any children run; move or insert ApiProvider
as a child of AuthProvider (i.e., BrowserRouter > AuthProvider > ApiProvider >
AuthenticatedContent) and pass the token getter/function provided by
AuthProvider into ApiProvider so all downstream components receive an API client
initialized with the token.
1 participant
Summary by CodeRabbit
New Features
Changes
Configuration
Chores