feat(meetings): add join URL endpoint and simplify auth middleware #79
Conversation
- Add GET /api/meetings/:uid/join-url endpoint for fetching meeting join URLs - Create MeetingJoinURL interface for type safety - Update public meeting controller to fetch join URLs for public meetings - Simplify authentication middleware by removing redundant authContext - Create unified authMiddleware replacing dual middleware system - Use req.oidc directly from OIDC middleware - Add comprehensive debug logging for authentication troubleshooting - Remove obsolete auth-token.middleware.ts and protected-routes.middleware.ts LFXV2-417 LFXV2-452 Signed-off-by: Asitha de Silva <asithade@gmail.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a global, configurable route-based OIDC auth middleware replacing legacy per-route middleware; exposes GET /meetings/:uid/join-url with service and shared-interface support; updates public/public-password meeting flows to optionally attach join_url and call services with access=false; refactors microservice proxy and API client, removes legacy auth-token/protected-routes middleware, tweaks client auth interceptor and meeting UI, and updates VSCode settings. MeetingController contains a duplicated getMeetingJoinUrl insertion. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant B as Browser / Client
participant S as Server
participant A as Auth Middleware
participant O as OIDC Provider
Note over S,A: Global auth middleware applied to incoming requests
B->>S: HTTP request (SSR or API)
S->>A: classify route & check req.oidc
A->>A: extract token (if present) & decide (public/optional/required)
alt SSR required & unauthenticated
A-->>B: 302 Redirect to login
B->>O: Login flow
else API required & unauthenticated
A-->>B: 401 AuthenticationError
else allowed
A-->>S: next() (req.bearerToken maybe populated)
S->>S: route handler executes
S-->>B: response
end
sequenceDiagram
autonumber
participant C as Client
participant S as Server
participant M as MeetingService
participant P as Microservice Proxy
C->>S: GET /meetings/:uid/join-url
S->>M: getMeetingJoinUrl(req, uid)
M->>P: proxy request -> /meetings/{uid}/join_url
P-->>M: { join_url } or error
M-->>S: join_url (optional)
S-->>C: 200 { join_url } or 200 with meeting without join_url
sequenceDiagram
autonumber
participant C as Client
participant S as Server
participant Pub as PublicMeetingController
participant MS as MeetingService
participant PS as ProjectService
participant Proxy as Microservice Proxy
C->>S: GET /public/api/meetings/:id
S->>Pub: handler
Pub->>MS: getMeetingById(req, id, 'meeting', false)
MS->>Proxy: fetch meeting
Proxy-->>MS: meeting
MS-->>Pub: meeting (no access fields)
Pub->>PS: getProjectById(req, meeting.project_uid, false)
PS->>Proxy: fetch project
Proxy-->>PS: project
PS-->>Pub: project (trimmed: name, slug, logo_url)
Pub->>MS: getMeetingJoinUrl(req, id)
MS->>Proxy: fetch join_url
Proxy-->>MS: join_url or error
MS-->>Pub: attach optional join_url
Pub-->>C: 200 { meeting (+/- join_url), project }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60–90 minutes Possibly related PRs
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
github-actions
bot
requested review from
andrest50,
audigregorie,
dealako and
mauriciozanettisalomao
There was a problem hiding this comment.
Pull Request Overview
This PR implements a meeting join URL endpoint and simplifies authentication middleware for better maintainability. The changes focus on providing join URLs for meetings while consolidating redundant authentication layers.
Key changes:
- Adds a new API endpoint
GET /api/meetings/:uid/join-urlfor fetching meeting join URLs - Replaces multiple authentication middleware files with a unified
auth.middleware.ts - Updates public meeting controller to automatically fetch join URLs with graceful error handling
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/meeting.interface.ts | Adds MeetingJoinURL interface for type safety |
| packages/shared/src/interfaces/auth.interface.ts | Adds comprehensive auth middleware interfaces and types |
| apps/lfx-pcc/src/server/services/meeting.service.ts | Adds getMeetingJoinUrl service method and optional access parameter |
| apps/lfx-pcc/src/server/services/microservice-proxy.service.ts | Simplifies constructor and removes redundant executeRequest method |
| apps/lfx-pcc/src/server/middleware/auth.middleware.ts | Creates unified authentication middleware with comprehensive routing logic |
| apps/lfx-pcc/src/server/controllers/meeting.controller.ts | Implements getMeetingJoinUrl controller method with validation |
| apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts | Updates to fetch join URLs for public meetings with error handling |
| apps/lfx-pcc/src/server/routes/meetings.route.ts | Adds new join-url endpoint route |
| apps/lfx-pcc/src/server/server.ts | Replaces old middleware with unified auth middleware |
| apps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.ts | Extends authentication to include public API routes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.ts (1)
19-31: TheUserServicesignal is never set in this codebase (noauthenticated.set(…)calls or login methods), souserService.authenticated()always returnsfalse—meaning cookies are never forwarded for any/api/or/public/api/request. You’ll need to implement and call a method that updatesauthenticated(e.g. after SSR reads cookies) before this interceptor will forward credentials.apps/lfx-pcc/src/server/services/meeting.service.ts (1)
42-64: Avoid logging full meeting objects; potential PII/secret leakage (e.g., passwords, join_url).
req.log.debug({ meetings }, 'Meetings')can leak sensitive fields. Sanitize or log counts/keys only.Use:
- req.log.debug({ meetings }, 'Meetings'); + req.log.debug({ meeting_count: meetings.length }, 'Meetings fetched');Alternatively:
Logger.sanitizebefore logging.
🧹 Nitpick comments (12)
apps/lfx-pcc/src/app/modules/meeting/meeting.component.html (1)
260-268: Guard join button when join_url is missing and add data-testid.
- Disable button if the backend didn’t attach a join_url.
- Add data-testid per guidelines for new UI elements.
Apply:
- <div class="flex items-center justify-end pt-3"> - <lfx-button + <div class="flex items-center justify-end pt-3"> + <lfx-button + [attr.data-testid]="'join-section-join-button'" size="small" - [href]="meeting().join_url" + [href]="meeting().join_url" severity="primary" label="Join Meeting" icon="fa-light fa-sign-in" - [disabled]="joinForm.invalid"></lfx-button> + [disabled]="joinForm.invalid || !meeting()?.join_url"></lfx-button>apps/lfx-pcc/src/server/services/microservice-proxy.service.ts (1)
30-39: Consider M2M token use for public flows.Per team learnings, server-side calls from public endpoints should use M2M tokens. Expose a tokenStrategy option on proxy methods to select user vs M2M token and source M2M from your token service.
apps/lfx-pcc/src/server/services/project.service.ts (1)
46-80: Backwards-compatible flag looks good; small naming nit.Param name access could be clearer (e.g., augmentAccess or includeAccess) to convey intent.
- public async getProjectById(req: Request, projectId: string, access: boolean = true): Promise<Project> { + public async getProjectById(req: Request, projectId: string, augmentAccess: boolean = true): Promise<Project> { @@ - if (access) { + if (augmentAccess) {apps/lfx-pcc/src/server/controllers/meeting.controller.ts (1)
666-667: Mark join URL responses as non-cacheable.Join URLs are sensitive/short-lived; prevent intermediary/browser caching.
- // Send the join URL data to the client - res.json(joinUrlData); + // Send the join URL data to the client (prevent caching) + res.set('Cache-Control', 'no-store, private, max-age=0'); + res.json(joinUrlData);apps/lfx-pcc/src/server/server.ts (1)
203-205: Redundant error handler mount.You already have a global error handler delegating to
apiErrorHandler(Lines 273-283). Keeping the path-specificapp.use('/api/*', apiErrorHandler)adds complexity without clear benefit.Apply:
-// Add API error handler middleware -app.use('/api/*', apiErrorHandler); +// Handled by the global error handler belowapps/lfx-pcc/src/server/services/meeting.service.ts (1)
69-107: Simplify single-meeting flow to reduce indexing mistakes and improve readability.Operate on a single object instead of an array-of-one; keeps types clean and avoids repeated
[0].- let meeting = resources.map((resource) => resource.data); - - if (meeting[0].committees && meeting[0].committees.length > 0) { - meeting = await this.getMeetingCommittees(req, meeting); - } - - if (access) { - // Add writer access field to the meeting - return await this.accessCheckService.addAccessToResource(req, meeting[0], 'meeting', 'organizer'); - } - - return meeting[0]; + let [meeting] = resources.map((resource) => resource.data); + + if (meeting.committees && meeting.committees.length > 0) { + [meeting] = await this.getMeetingCommittees(req, [meeting]); + } + + if (access) { + return await this.accessCheckService.addAccessToResource(req, meeting, 'meeting', 'organizer'); + } + + return meeting;apps/lfx-pcc/src/server/middleware/auth.middleware.ts (4)
249-256: Use the computed redirect URL when invoking OIDC login.Leverage
decision.redirectUrlto keep execution aligned with decision output.- // Use OIDC login method which handles the redirect properly - res.oidc.login({ returnTo: req.originalUrl }); + // Use OIDC login method which handles the redirect properly + res.oidc.login({ returnTo: decision.redirectUrl ?? req.originalUrl });
79-83: Avoid fetching user info on every request; prefer session user first.
fetchUserInfo()incurs a network call. Usereq.oidc.userif present, fetch only as fallback.- const user = (await req.oidc.fetchUserInfo()) ?? (req.oidc?.user as User); + const user = (req.oidc?.user as User) ?? (await req.oidc.fetchUserInfo());
258-267: Propagate statusCode from decision into the error (or remove field).
AuthDecision.statusCodeis computed but not used here. Either include it in the error metadata/constructor or drop the field from the interface.- const error = new AuthenticationError( + const error = new AuthenticationError( decision.errorType === 'authorization' ? 'Insufficient permissions to access this resource' : 'Authentication required to access this resource', { operation: 'auth_middleware', service: 'authentication', path: req.path, + statusCode: decision.statusCode, } );
74-76: Trim cookie metadata logging.Even cookie names can be noisy and sensitive. Consider removing unless needed for debugging.
- cookies: Object.keys(req.cookies || {}),packages/shared/src/interfaces/auth.interface.ts (2)
115-124: Align AuthDecision.statusCode with executor.
statusCodeis defined here but not acted upon inexecuteAuthDecision. Either document it as informational, have the executor apply it, or remove to avoid drift.
69-76: Remove unused BearerTokenOptions interface (packages/shared/src/interfaces/auth.interface.ts:69–76). No references outside its own declaration; drop it to trim the public API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
.vscode/settings.json(1 hunks)apps/lfx-pcc/src/app/modules/meeting/meeting.component.html(1 hunks)apps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.ts(1 hunks)apps/lfx-pcc/src/server/controllers/meeting.controller.ts(1 hunks)apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts(3 hunks)apps/lfx-pcc/src/server/middleware/auth-token.middleware.ts(0 hunks)apps/lfx-pcc/src/server/middleware/auth.middleware.ts(1 hunks)apps/lfx-pcc/src/server/middleware/protected-routes.middleware.ts(0 hunks)apps/lfx-pcc/src/server/routes/meetings.route.ts(1 hunks)apps/lfx-pcc/src/server/server.ts(2 hunks)apps/lfx-pcc/src/server/services/meeting.service.ts(5 hunks)apps/lfx-pcc/src/server/services/microservice-proxy.service.ts(3 hunks)apps/lfx-pcc/src/server/services/project.service.ts(2 hunks)packages/shared/src/interfaces/auth.interface.ts(1 hunks)packages/shared/src/interfaces/meeting.interface.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/lfx-pcc/src/server/middleware/protected-routes.middleware.ts
- apps/lfx-pcc/src/server/middleware/auth-token.middleware.ts
🧰 Additional context used
📓 Path-based instructions (7)
apps/lfx-pcc/src/**/*.{ts,html}
📄 CodeRabbit inference engine (CLAUDE.md)
Use LFX wrapper components instead of importing PrimeNG components directly
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.component.htmlapps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.tsapps/lfx-pcc/src/server/middleware/auth.middleware.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tsapps/lfx-pcc/src/server/routes/meetings.route.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/server/services/microservice-proxy.service.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
apps/lfx-pcc/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.html: Add data-testid attributes to new components for reliable test targeting
Follow data-testid naming convention: [section]-[component]-[element]
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.component.html
**/*.{ts,tsx,js,jsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-pcc/src/app/modules/meeting/meeting.component.htmlapps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.tsapps/lfx-pcc/src/server/middleware/auth.middleware.tspackages/shared/src/interfaces/meeting.interface.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tspackages/shared/src/interfaces/auth.interface.tsapps/lfx-pcc/src/server/routes/meetings.route.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/server/services/microservice-proxy.service.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer TypeScript interfaces over union types when modeling shapes and contracts
Files:
apps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.tsapps/lfx-pcc/src/server/middleware/auth.middleware.tspackages/shared/src/interfaces/meeting.interface.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tspackages/shared/src/interfaces/auth.interface.tsapps/lfx-pcc/src/server/routes/meetings.route.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/server/services/microservice-proxy.service.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.tsapps/lfx-pcc/src/server/middleware/auth.middleware.tspackages/shared/src/interfaces/meeting.interface.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tspackages/shared/src/interfaces/auth.interface.tsapps/lfx-pcc/src/server/routes/meetings.route.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/server/services/microservice-proxy.service.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.ts: In the Angular app, use direct imports for standalone components (no barrel imports)
When defining types for PrimeNG usage, reference PrimeNG’s component interfaces
Files:
apps/lfx-pcc/src/app/shared/interceptors/authentication.interceptor.tsapps/lfx-pcc/src/server/middleware/auth.middleware.tsapps/lfx-pcc/src/server/services/project.service.tsapps/lfx-pcc/src/server/controllers/meeting.controller.tsapps/lfx-pcc/src/server/routes/meetings.route.tsapps/lfx-pcc/src/server/services/meeting.service.tsapps/lfx-pcc/src/server/services/microservice-proxy.service.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all shared interfaces in the shared package
Files:
packages/shared/src/interfaces/meeting.interface.tspackages/shared/src/interfaces/auth.interface.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints
📚 Learning: 2025-09-05T18:09:48.792Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.792Z
Learning: Applies to {apps/lfx-pcc/src,packages/shared/src}/**/index.ts : Avoid barrel exports; use direct imports instead of index.ts barrels
Applied to files:
apps/lfx-pcc/src/server/server.ts
📚 Learning: 2025-09-05T18:09:48.791Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints
Applied to files:
apps/lfx-pcc/src/server/server.ts
🧬 Code graph analysis (7)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (2)
packages/shared/src/interfaces/auth.interface.ts (5)
RouteAuthConfig(100-109)AuthConfig(145-152)User(8-41)AuthMiddlewareResult(130-139)AuthDecision(115-124)apps/lfx-pcc/src/server/helpers/logger.ts (1)
error(52-65)
apps/lfx-pcc/src/server/services/project.service.ts (1)
packages/shared/src/interfaces/project.interface.ts (1)
Project(54-103)
apps/lfx-pcc/src/server/controllers/meeting.controller.ts (2)
apps/lfx-pcc/src/server/helpers/logger.ts (2)
Logger(10-129)error(52-65)apps/lfx-pcc/src/server/helpers/validation.helper.ts (1)
validateUidParameter(30-49)
apps/lfx-pcc/src/server/services/meeting.service.ts (1)
packages/shared/src/interfaces/meeting.interface.ts (2)
Meeting(75-142)MeetingJoinURL(427-430)
apps/lfx-pcc/src/server/services/microservice-proxy.service.ts (1)
apps/lfx-pcc/src/server/services/api-client.service.ts (1)
ApiClientService(9-123)
apps/lfx-pcc/src/server/server.ts (1)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (1)
authMiddleware(351-351)
apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts (1)
apps/lfx-pcc/src/server/helpers/logger.ts (1)
error(52-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (10)
.vscode/settings.json (1)
64-66: LGTM — editor-only setting.No runtime impact; fine to keep in repo-scoped settings.
packages/shared/src/interfaces/meeting.interface.ts (1)
422-430: LGTM — concise response contract for join URL.Name aligns with existing join_url field; docs included.
apps/lfx-pcc/src/server/routes/meetings.route.ts (1)
24-26: LGTM — route wiring for join URL.Placement after /:uid is fine; no path shadowing.
apps/lfx-pcc/src/server/server.ts (1)
190-196: No action needed:authMiddlewarealready exempts/public/apiand/meetingsas optional routes.apps/lfx-pcc/src/server/services/meeting.service.ts (1)
384-390: LGTM: join URL proxy method.Straight-through proxy aligns with the shared
MeetingJoinURLinterface and defers errors to caller.apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts (1)
45-47: LGTM: explicitaccess=falsefor public fetch.Keeps access augmentation off for public responses as intended.
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (1)
22-24: No singular/meetingSSR route detected—no update needed.packages/shared/src/interfaces/auth.interface.ts (3)
100-109: RouteAuthConfig looks good.Clear, minimal shape with optional token flag for APIs.
130-139: AuthMiddlewareResult is appropriate.Carries only what the decision engine needs; no leaked authContext.
145-152: AuthConfig is sufficient and future-proof.Defaults plus route array match the new middleware.
apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
Outdated
Show resolved
Hide resolved
- Fix critical security vulnerability where non-GET requests to protected SSR routes bypassed authentication - Add proper 401 error handling for non-GET requests to protected SSR routes - Fix Authorization header being set to 'Bearer undefined' when bearerToken is not provided - Make Authorization header conditional in ApiClientService based on bearerToken presence LFXV2-417 Signed-off-by: Asitha de Silva <asithade@gmail.com>
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (2)
171-206: SSR non-GET unauth bypass fixed.Non-GET SSR now returns 401 when unauthenticated. Good catch.
66-77: Express type augmentation reminder (oidc, log, id, bearerToken).If not already added, extend Express types so these fields compile cleanly.
Also applies to: 267-270, 339-347
🧹 Nitpick comments (8)
apps/lfx-pcc/src/server/services/api-client.service.ts (3)
89-97: Handle empty/204/304 responses before JSON.parse.Avoid parsing empty body which throws.
- const data = await response.text(); + const text = await response.text(); + const isEmpty = response.status === 204 || response.status === 304 || !text; const apiResponse: ApiResponse<T> = { - data: data ? JSON.parse(data) : null, + data: isEmpty ? null : JSON.parse(text), status: response.status, statusText: response.statusText, headers: Object.fromEntries(response.headers.entries()), };
125-128: Support array query params.
new URLSearchParams(query)stringifies arrays as comma-joined. Expand arrays to repeated keys.- const queryString = query ? new URLSearchParams(query).toString() : ''; + const queryString = query ? this.buildSearchParams(query) : '';Add this helper inside the class:
private buildSearchParams(query: Record<string, any>): string { const params = new URLSearchParams(); for (const [k, v] of Object.entries(query)) { if (Array.isArray(v)) v.forEach(x => params.append(k, String(x))); else if (v != null) params.append(k, String(v)); } return params.toString(); }
10-18: Retry config is unused.
retryAttempts/retryDelayare never applied. Implement retry/backoff (idempotent methods) or drop the fields.apps/lfx-pcc/src/server/middleware/auth.middleware.ts (5)
25-27: Guest access for join URL endpoint?Current
/apirule requires auth+token, which blocks unauthenticated guests. If guests may retrieve join URLs for public meetings, add a specific override above the generic/apirule, or expose it under/public/api.+ // Join URL endpoint – allow optional auth; controller enforces meeting visibility + { pattern: /^\/api\/meetings\/[^/]+\/join-url$/, type: 'api', auth: 'optional', tokenRequired: false },Confirm intended behavior vs LFXV2-417.
66-83: Avoid fetching user info on every request.Use
req.oidc.userwhen present; callfetchUserInfo()only if missing.- if (req.oidc?.isAuthenticated()) { - const user = (await req.oidc.fetchUserInfo()) ?? (req.oidc?.user as User); + if (req.oidc?.isAuthenticated()) { + const user = (req.oidc?.user as User) ?? (await req.oidc.fetchUserInfo()); req.log.debug({ path: req.path, userId: user?.sub }, 'Authentication check successful'); return { authenticated: true, user }; }
68-76: Don’t log cookie names.Reduce log verbosity and risk by logging count only.
- cookies: Object.keys(req.cookies || {}), + cookieCount: Object.keys(req.cookies || {}).length,
101-121: Consider token refresh before failing token checks.If supported by your OIDC lib, attempt refresh when
isExpired()is true.
266-273: Guardres.oidc.loginto avoid runtime error.Fall back to plain redirect if OIDC object is unavailable.
- if (decision.redirectUrl) { - // Use OIDC login method which handles the redirect properly - res.oidc.login({ returnTo: req.originalUrl }); - } else { - res.redirect('/'); - } + if (res.oidc?.login) { + res.oidc.login({ returnTo: req.originalUrl }); + } else { + res.redirect(`/login?returnTo=${encodeURIComponent(req.originalUrl)}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts(1 hunks)apps/lfx-pcc/src/server/services/api-client.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer TypeScript interfaces over union types when modeling shapes and contracts
Files:
apps/lfx-pcc/src/server/services/api-client.service.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/server/services/api-client.service.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
apps/lfx-pcc/src/**/*.{ts,html}
📄 CodeRabbit inference engine (CLAUDE.md)
Use LFX wrapper components instead of importing PrimeNG components directly
Files:
apps/lfx-pcc/src/server/services/api-client.service.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
**/*.{ts,tsx,js,jsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-pcc/src/server/services/api-client.service.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.ts: In the Angular app, use direct imports for standalone components (no barrel imports)
When defining types for PrimeNG usage, reference PrimeNG’s component interfaces
Files:
apps/lfx-pcc/src/server/services/api-client.service.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints
🧬 Code graph analysis (1)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (2)
packages/shared/src/interfaces/auth.interface.ts (5)
RouteAuthConfig(100-109)AuthConfig(145-152)User(8-41)AuthMiddlewareResult(130-139)AuthDecision(115-124)apps/lfx-pcc/src/server/helpers/logger.ts (1)
error(52-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (2)
apps/lfx-pcc/src/server/services/api-client.service.ts (1)
53-54: Confirm Node.js runtime version supports AbortSignal.timeout
AbortSignal.timeout is available in Node.js v16.14.0+ and v17.3.0+ (nodejs.org). Verify yourpackage.jsonengines.nodeconstraint (or your CI/runtime) meets this requirement, or include an appropriate polyfill (e.g. via undici) if targeting older Node versions.apps/lfx-pcc/src/server/middleware/auth.middleware.ts (1)
294-363: No changes needed: OIDC middleware precedes custom auth middleware and routes mount correctly.
app.use(auth(authConfig))at line 168 runs beforeapp.use(authMiddleware)at line 191, and subsequent routes are mounted in the intended order.
Signed-off-by: Asitha de Silva <asithade@gmail.com>
Signed-off-by: Asitha de Silva <asithade@gmail.com>
Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (2)
198-212: SSR non-GET auth bypass fixed — nice.The 401 branch for SSR non-GET closes the gap flagged earlier.
Also applies to: 214-229, 231-246
101-159: Token refresh path is fine; ensure typings exist for req.bearerToken/req.oidc/req.log.Prior feedback noted only bearerToken was declared. Make sure Request/Response are augmented for oidc, log, and id.
#!/bin/bash # Verify Express module augmentation for oidc/log/id/bearerToken rg -nP -C2 'declare module .express.|namespace\s+Express' --type-add 'dts:*.d.ts' --type dts rg -nP -C3 'interface\s+Request\s*{[^}]*\boidc\b' --type-add 'dts:*.d.ts' --type dts rg -nP -C3 'interface\s+Request\s*{[^}]*\blog\b' --type-add 'dts:*.d.ts' --type dts rg -nP -C3 'interface\s+Request\s*{[^}]*\bid\b' --type-add 'dts:*.d.ts' --type dts rg -nP -C3 'interface\s+Response\s*{[^}]*\boidc\b' --type-add 'dts:*.d.ts' --type dts
🧹 Nitpick comments (8)
apps/lfx-pcc/src/app/shared/guards/auth.guard.ts (1)
25-31: Comment/code mismatch and minor redirect nit.The comment says “using Router for SSR compatibility,” but the code uses window.location. Either update the comment or switch to a Router UrlTree. Also prefer location.replace to avoid polluting browser history with the transient /login hop.
- // Redirect to login with returnTo parameter using Router for SSR compatibility + // Redirect to server-side /login with returnTo parameter (browser only). + // On non-browser/SSR, return false and let server middleware handle redirects. @@ - if (isPlatformBrowser(platformId)) { - window.location.href = `/login?returnTo=${returnTo}`; - } + if (isPlatformBrowser(platformId)) { + window.location.replace(`/login?returnTo=${returnTo}`); + }apps/lfx-pcc/src/server/server.ts (3)
75-93: Fix Pino redaction path for Set-Cookie.Use dot-notation (res.headers.set-cookie). The bracket form won’t be recognized by Pino.
- : ['access_token', 'refresh_token', 'authorization', 'cookie', 'req.headers.authorization', 'req.headers.cookie', 'res.headers["set-cookie"]'], + : ['access_token', 'refresh_token', 'authorization', 'cookie', 'req.headers.authorization', 'req.headers.cookie', 'res.headers.set-cookie'],Apply the same change in httpLogger redact config.
205-207: Prefer prefix mount for API error handler.Use '/api' instead of '/api/*' for the error handler mount to catch all API errors consistently.
-app.use('/api/*', apiErrorHandler); +app.use('/api', apiErrorHandler);
125-132: Logger ignore lists a non-existent '/api/health'.Either add an /api/health endpoint or remove it from the ignore predicate.
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (4)
35-39: Make defaults consistent with the catch-all.With the above change, leaving defaultAuth: 'required' is correct. If you keep a catch-all '/' of 'optional', defaultAuth will never be used—avoid that inconsistency.
290-297: Use the computed redirectUrl to hit /login (and its returnTo validation).Currently you ignore decision.redirectUrl and call res.oidc.login directly. Redirecting to /login centralizes sanitization and keeps behavior consistent.
- case 'redirect': - if (decision.redirectUrl) { - // Use OIDC login method which handles the redirect properly - res.oidc.login({ returnTo: req.originalUrl }); - } else { - res.redirect('/'); - } + case 'redirect': + if (decision.redirectUrl) { + return res.redirect(decision.redirectUrl); + } + res.redirect('/'); break;
66-96: Avoid remote userinfo call on every request.fetchUserInfo can be an extra network hop. Prefer req.oidc.user for middleware decisions; defer userinfo fetch to places that need fresh profile.
- if (req.oidc?.isAuthenticated()) { - const user = (await req.oidc.fetchUserInfo()) ?? (req.oidc?.user as User); + if (req.oidc?.isAuthenticated()) { + const user = (req.oidc?.user as User) ?? undefined;
340-345: Skip token extraction for SSR optional routes to reduce overhead.Refreshing/extracting tokens for every optional SSR hit isn’t needed. Restrict to API routes (and required SSR if desired).
- if (routeConfig.tokenRequired || routeConfig.auth === 'optional') { + if (routeConfig.type === 'api' && (routeConfig.tokenRequired || routeConfig.auth === 'optional')) { hasToken = await extractBearerToken(req); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
apps/lfx-pcc/src/app/app.routes.ts(2 hunks)apps/lfx-pcc/src/app/shared/guards/auth.guard.ts(1 hunks)apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts(3 hunks)apps/lfx-pcc/src/server/middleware/auth.middleware.ts(1 hunks)apps/lfx-pcc/src/server/server.ts(2 hunks)apps/lfx-pcc/src/server/services/meeting.service.ts(5 hunks)apps/lfx-pcc/src/server/utils/security.util.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/lfx-pcc/src/server/utils/security.util.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/lfx-pcc/src/server/services/meeting.service.ts
- apps/lfx-pcc/src/server/controllers/public-meeting.controller.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer TypeScript interfaces over union types when modeling shapes and contracts
Files:
apps/lfx-pcc/src/app/shared/guards/auth.guard.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/app/shared/guards/auth.guard.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
apps/lfx-pcc/src/**/*.{ts,html}
📄 CodeRabbit inference engine (CLAUDE.md)
Use LFX wrapper components instead of importing PrimeNG components directly
Files:
apps/lfx-pcc/src/app/shared/guards/auth.guard.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
**/*.{ts,tsx,js,jsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-pcc/src/app/shared/guards/auth.guard.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.ts: In the Angular app, use direct imports for standalone components (no barrel imports)
When defining types for PrimeNG usage, reference PrimeNG’s component interfaces
Files:
apps/lfx-pcc/src/app/shared/guards/auth.guard.tsapps/lfx-pcc/src/app/app.routes.tsapps/lfx-pcc/src/server/server.tsapps/lfx-pcc/src/server/middleware/auth.middleware.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints
📚 Learning: 2025-09-05T18:09:48.791Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints
Applied to files:
apps/lfx-pcc/src/server/server.ts
🧬 Code graph analysis (3)
apps/lfx-pcc/src/app/app.routes.ts (1)
apps/lfx-pcc/src/app/shared/guards/auth.guard.ts (1)
authGuard(16-32)
apps/lfx-pcc/src/server/server.ts (1)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (1)
authMiddleware(392-392)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (1)
packages/shared/src/interfaces/auth.interface.ts (5)
RouteAuthConfig(100-109)AuthConfig(145-152)User(8-41)AuthMiddlewareResult(130-139)AuthDecision(115-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (2)
apps/lfx-pcc/src/app/shared/guards/auth.guard.ts (1)
20-23: Guard check looks good.Synchronous check against the signal is clear and cheap.
apps/lfx-pcc/src/server/server.ts (1)
191-193: Good placement of the unified auth middleware.Mounted after OIDC and before routers; aligns with design.
- Remove async checkAuthentication function to simplify auth flow - Remove user from AuthMiddlewareResult interface for cleaner separation - Optimize user info fetching in server.ts with fallback logic - Remove unnecessary build step from e2e-tests workflow - Clean up imports and debug logging in meeting service - Add debug logging to proxy service for better observability LFXV2-417 Signed-off-by: Asitha de Silva <asithade@gmail.com>
Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/lfx-pcc/src/server/server.ts (1)
222-226: Prefer token claims; fallback to userinfo — add minor type/robustness guardCurrent logic is sound. To avoid overwriting with a sparse userinfo payload and to satisfy TS types, guard and cast:
- auth.user = req.oidc?.user as User; - if (!auth.user?.email) { - auth.user = await req.oidc.fetchUserInfo(); - } + auth.user = req.oidc?.user as User; + if (!auth.user?.email) { + const info = (await req.oidc.fetchUserInfo()) as unknown as User; + if (info?.email) auth.user = info; + }Optional: before passing
auth.userinto SSR, project to a minimal, known-safe subset of fields to avoid leaking extraneous claims. I can provide a small sanitizer util if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/e2e-tests.yml(0 hunks)apps/lfx-pcc/src/server/middleware/auth.middleware.ts(1 hunks)apps/lfx-pcc/src/server/server.ts(3 hunks)apps/lfx-pcc/src/server/services/meeting.service.ts(5 hunks)apps/lfx-pcc/src/server/services/microservice-proxy.service.ts(3 hunks)packages/shared/src/interfaces/auth.interface.ts(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/e2e-tests.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/lfx-pcc/src/server/services/microservice-proxy.service.ts
- apps/lfx-pcc/src/server/middleware/auth.middleware.ts
- packages/shared/src/interfaces/auth.interface.ts
- apps/lfx-pcc/src/server/services/meeting.service.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer TypeScript interfaces over union types when modeling shapes and contracts
Files:
apps/lfx-pcc/src/server/server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-pcc/src/server/server.ts
apps/lfx-pcc/src/**/*.{ts,html}
📄 CodeRabbit inference engine (CLAUDE.md)
Use LFX wrapper components instead of importing PrimeNG components directly
Files:
apps/lfx-pcc/src/server/server.ts
**/*.{ts,tsx,js,jsx,html,scss,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-pcc/src/server/server.ts
apps/lfx-pcc/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-pcc/src/**/*.ts: In the Angular app, use direct imports for standalone components (no barrel imports)
When defining types for PrimeNG usage, reference PrimeNG’s component interfaces
Files:
apps/lfx-pcc/src/server/server.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints
📚 Learning: 2025-09-05T18:09:48.791Z
Learnt from: CR
PR: linuxfoundation/lfx-v2-ui#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-05T18:09:48.791Z
Learning: Public routes include /meeting and /public/api; use M2M tokens for server-side API calls from public endpoints
Applied to files:
apps/lfx-pcc/src/server/server.ts
🧬 Code graph analysis (1)
apps/lfx-pcc/src/server/server.ts (2)
apps/lfx-pcc/src/server/middleware/auth.middleware.ts (1)
authMiddleware(376-376)packages/shared/src/interfaces/auth.interface.ts (1)
User(8-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: E2E Tests / Playwright E2E Tests
🔇 Additional comments (2)
apps/lfx-pcc/src/server/server.ts (2)
18-18: Unified auth middleware import — LGTMImporting the consolidated auth middleware here aligns with the new OIDC-driven flow.
191-197: Confirm public route allowlist and API behaviors
- auth.middleware.ts (lines 17–26) defines
{ pattern: '/public/api', type: 'api', auth: 'optional', tokenRequired: false }and{ pattern: '/meetings', type: 'ssr', auth: 'optional' }, confirming public endpoints pass through without redirect.- Verify OPTIONS/HEAD (preflight) requests bypass auth as intended.
- Verify unauthorized API calls return JSON 401/403 (no HTML redirects).
Optional: mount the API error handler on public routes for symmetry:
+ app.use('/public/api/*', apiErrorHandler);
🧹 Deployment RemovedThe deployment for PR #79 has been removed. |
Summary
This PR implements two key features:
Meeting Join URL Endpoint (LFXV2-417):
GET /api/meetings/:uid/join-urlendpoint for fetching meeting join URLsAuthentication Middleware Simplification (LFXV2-452):
authContextcreation in favor of usingreq.oidcdirectlyChanges Made
Meeting Join URL Feature
MeetingJoinURLinterface in shared package for type safetygetMeetingJoinUrl()service method using microservice proxygetMeetingJoinUrl()controller method with validation and loggingAuthentication Middleware Simplification
authContextcreation from unified auth middlewareAuthMiddlewareResultinterface to removeauthContextpropertyRequestWithAuthinterface and switched to standard ExpressRequestauth-token.middleware.tsandprotected-routes.middleware.tsauth.middleware.tsfocused on routing decisions onlyTechnical Details
API Endpoint
Authentication Flow
req.oidcwith authentication statereq.oidcauthobject for Angular dependency injection onlyDebug Logging
Enhanced logging now shows:
Test Plan
Related Issues