Skip to content

Conversation

@asithade
Copy link
Contributor

@asithade asithade commented Dec 16, 2025

Summary

Updated auth middleware to always attempt token refresh on optional authentication routes (like /meetings/), providing better UX for authenticated users while maintaining safe fallback behavior.

Changes Made

  • Modified extractBearerToken to always attempt token refresh regardless of route type
  • Implemented graceful failure handling: optional routes log WARNING and continue, required routes log ERROR and force logout
  • Updated route classification to pass isOptionalRoute parameter instead of attemptRefresh
  • Simplified auth decision logic by removing redundant checks

Benefits

  • ✅ Authenticated users on optional routes get enhanced features when token refresh succeeds
  • ✅ No interruption to meeting access if refresh fails
  • ✅ Better logging differentiation between route types (WARNING vs ERROR)
  • ✅ No redirect loops on optional routes
  • ✅ Maintains backward compatibility

Technical Details

  • File Modified: apps/lfx-one/src/server/middleware/auth.middleware.ts
  • Lines Changed: 82-158, 372-383, 176-188
  • All Tests Passing: Lint + Build + Type Check verified

JIRA

LFXV2-913

Generated with Claude Code

- Always attempt token refresh on optional auth routes (e.g., /meetings/)
- Implement graceful failure handling based on route type
- Optional routes: log warning and continue without token (no logout)
- Required routes: log error and force logout for re-authentication
- Replace attemptRefresh parameter with isOptionalRoute for clearer intent
- Simplify auth decision logic by removing redundant checks

Benefits:
- Authenticated users get enhanced features when possible
- No interruption to meeting access if refresh fails
- Better logging differentiation (WARNING vs ERROR)
- No redirect loops on optional routes
- Maintains backward compatibility

LFXV2-913

Generated with [Claude Code](https://claude.ai/code)

Signed-off-by: Asitha de Silva <[email protected]>
@asithade asithade requested a review from jordane as a code owner December 16, 2025 22:58
Copilot AI review requested due to automatic review settings December 16, 2025 22:58
@coderabbitai
Copy link

coderabbitai bot commented Dec 16, 2025

Walkthrough

Token extraction logic in authentication middleware is refactored to always attempt token refresh when expired, with refined failure behavior based on route type: optional routes log warnings and continue without tokens, while required routes trigger re-authentication.

Changes

Cohort / File(s) Summary
Authentication Middleware Token Refresh Logic
apps/lfx-one/src/server/middleware/auth.middleware.ts
Updated token extraction function to unconditionally attempt refresh on expiration. Added isOptionalRoute parameter to differentiate failure handling: optional routes log warnings and proceed; required routes enforce logout and re-authentication. Call sites modified to pass route type flag.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Areas requiring extra attention:
    • Token refresh logic branching: verify that optional vs. required route differentiation is correctly applied across all call sites
    • Error handling flow: ensure logout/re-authentication paths are only triggered for required routes and not inadvertently for optional routes
    • Function signature changes: confirm all callers properly pass the isOptionalRoute flag and no call sites are missed

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: improving token refresh on optional routes for better UX, which aligns with the primary objective in the changeset.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing the modifications to auth middleware, graceful failure handling, and benefits of the changes.
Linked Issues check ✅ Passed The PR implementation addresses all key objectives from LFXV2-913: always attempting token refresh on optional routes, implementing route-type-dependent failure handling, replacing attemptRefresh with isOptionalRoute parameter, and simplifying auth logic.
Out of Scope Changes check ✅ Passed All changes are directly related to the authentication middleware objectives. The modifications to extractBearerToken, failure handling, and route classification are all within scope of LFXV2-913.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/LFXV2-913

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a3c51a0 and d09e759.

📒 Files selected for processing (1)
  • apps/lfx-one/src/server/middleware/auth.middleware.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,css,scss}

📄 CodeRabbit inference engine (CLAUDE.md)

Always include license headers on all source files - run ./check-headers.sh to verify

Files:

  • apps/lfx-one/src/server/middleware/auth.middleware.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.{ts,tsx}: Always use async/await for promises instead of .then() chains in TypeScript services
Use TypeScript interfaces instead of union types for better maintainability

Files:

  • apps/lfx-one/src/server/middleware/auth.middleware.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (CLAUDE.md)

Do not nest ternary expressions

Files:

  • apps/lfx-one/src/server/middleware/auth.middleware.ts
apps/lfx-one/src/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

Shared package uses direct source imports during development for hot reloading

Files:

  • apps/lfx-one/src/server/middleware/auth.middleware.ts
🧬 Code graph analysis (1)
apps/lfx-one/src/server/middleware/auth.middleware.ts (2)
packages/shared/src/interfaces/auth.interface.ts (1)
  • TokenExtractionResult (139-144)
apps/lfx-one/src/server/services/logger.service.ts (1)
  • logger (485-485)
⏰ 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: Agent
🔇 Additional comments (5)
apps/lfx-one/src/server/middleware/auth.middleware.ts (5)

82-96: LGTM! Parameter rename improves clarity.

The function signature change from attemptRefresh to isOptionalRoute better expresses intent - it indicates the route's authentication requirement rather than an action to take. The parameter is properly documented and included in logging context for debugging.


103-110: LGTM! Unconditional token refresh improves UX.

Always attempting token refresh when expired is correct - authenticated users on optional routes benefit from enhanced features when refresh succeeds, and required routes need the refresh attempt regardless. This change aligns perfectly with the PR objectives.


112-132: LGTM! Error handling properly differentiates by route type.

The implementation correctly handles token refresh failures based on route type:

  • Optional routes: log WARNING and return needsLogout: false (graceful degradation)
  • Required routes: log ERROR and return needsLogout: true (force re-authentication)

This ensures optional routes remain accessible even when token refresh fails, while required routes maintain security by forcing logout.


176-188: LGTM! Comments accurately reflect the new behavior.

The updated comments correctly describe that optional routes always allow access regardless of token refresh outcome, with needsLogout guaranteed to be false for optional routes. This matches the implementation in extractBearerToken.


365-372: LGTM! Token extraction logic correctly handles route types.

The implementation properly determines when to attempt token extraction:

  • Optional routes: always attempt (for UX enhancement when tokens available)
  • API routes with tokenRequired: always attempt (needed for bearer token auth)
  • SSR routes without tokenRequired: skip (use OIDC session instead)

The isOptionalRoute flag is correctly derived from routeConfig.auth and passed to extractBearerToken, ensuring the right failure handling behavior.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enhances the authentication middleware to improve user experience by always attempting token refresh on optional authentication routes (like /meetings/), while maintaining different failure handling based on route type. The change enables authenticated users on optional routes to access enhanced features when token refresh succeeds, without disrupting access when it fails.

Key Changes

  • Modified token refresh behavior to always attempt refresh regardless of route type, improving UX for authenticated users on optional routes
  • Implemented differentiated error handling: optional routes log warnings and continue without forced logout, while required routes log errors and force re-authentication
  • Simplified authentication decision logic by removing redundant checks, since needsLogout is now always false for optional routes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@asithade asithade merged commit fee90b1 into main Dec 16, 2025
13 checks passed
@asithade asithade deleted the feat/LFXV2-913 branch December 16, 2025 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants