Skip to content

refactor(lightspeed): add middleware for resolving user identity#3227

Open
Jdubrick wants to merge 6 commits into
redhat-developer:mainfrom
Jdubrick:refactor-middleware-lightspeed
Open

refactor(lightspeed): add middleware for resolving user identity#3227
Jdubrick wants to merge 6 commits into
redhat-developer:mainfrom
Jdubrick:refactor-middleware-lightspeed

Conversation

@Jdubrick
Copy link
Copy Markdown
Contributor

@Jdubrick Jdubrick commented May 21, 2026

Hey, I just made a Pull Request!

Moves the resolution of the user identity to a middleware step, this reduces a lot of the duplicated resolving code and makes it easier to extend for tasks in 2.1.0. This adds the user identity to the individual request by extending the Express Request type, so it is easy to reference.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge Bot commented May 21, 2026

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Remediation recommended

1. Identity errors misclassified ✓ Resolved 🐞 Bug ☼ Reliability
Description
createIdentityMiddleware catches all exceptions from httpAuth/userInfo and always returns 401 with a
generic log line, even when the failure is an internal error (e.g., userInfo
outage/misconfiguration). This masks non-auth failures as “Unauthorized” and loses stack traces,
making production diagnosis and alerting significantly harder.
Code

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.ts[R43-52]

Relevance

⭐⭐⭐ High

Team recently fixed incorrect status mapping to preserve upstream codes; masking all errors as 401
likely flagged similarly.

PR-#3070
PR-#1786

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The middleware’s catch block always returns 401 and does not log the thrown error, whereas other
parts of the backend preserve error details and distinguish forbidden vs internal failures; this
inconsistency will hide real failures behind an auth error and remove stack traces.

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.ts[38-52]
workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts[239-245]
workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/utils.ts[49-73]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`createIdentityMiddleware` currently catches *all* errors during identity resolution and responds with `401 Unauthorized`, while also logging without including the thrown error object. This makes internal failures indistinguishable from auth failures and drops stack traces.

## Issue Context
The middleware runs early for most routes (and for notebooks under `/v1`) so its error handling becomes the system-of-record for identity failures.

## Fix Focus Areas
- workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.ts[38-52]

## What to change
1. Log the actual error object (so stack traces are preserved), and include basic request context (method + path).
2. Return status codes based on error type:
  - For expected auth failures (e.g., `NotAllowedError` / auth-specific errors thrown by Backstage auth), respond `401` (or `403` if that matches your auth model).
  - For unexpected/internal failures, respond `500` (or `next(error)` to let the global Backstage error middleware format and log it consistently).
3. If returning JSON directly, consider aligning the error shape with the rest of this backend’s handlers (optional), but do not lose the underlying error in logs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@rhdh-gh-app
Copy link
Copy Markdown

rhdh-gh-app Bot commented May 21, 2026

Changed Packages

Package Name Package Path Changeset Bump Current Version
@red-hat-developer-hub/backstage-plugin-lightspeed-backend workspaces/lightspeed/plugins/lightspeed-backend patch v2.8.5

@rhdh-qodo-merge
Copy link
Copy Markdown

Review Summary by Qodo

Refactor user identity resolution into reusable middleware

✨ Enhancement 🧪 Tests

Grey Divider

Walkthroughs

Description
• Centralizes user identity resolution via middleware to eliminate code duplication
• Extends Express Request type with credentials and userEntityRef fields
• Implements identity middleware in both lightspeed and notebooks routers
• Adds comprehensive tests for middleware, permission denial, and identity deduplication
Diagram
flowchart LR
  A["Express Request"] -->|"createIdentityMiddleware"| B["Resolve Credentials & UserEntityRef"]
  B -->|"Attach to req"| C["Request with Identity"]
  C -->|"getIdentity"| D["Extract & Validate Identity"]
  D -->|"Return typed object"| E["Route Handlers"]
  E -->|"Use credentials & userEntityRef"| F["Authorization & Operations"]

Loading

File Changes

1. workspaces/lightspeed/plugins/lightspeed-backend/src/service/express.d.ts ✨ Enhancement +25/-0

Extend Express Request type with identity fields

• Extends Express Request interface with credentials and userEntityRef optional fields
• Enables type-safe access to identity information across route handlers
• Populated by the identity middleware during request processing

workspaces/lightspeed/plugins/lightspeed-backend/src/service/express.d.ts


2. workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.ts ✨ Enhancement +70/-0

Add middleware for resolving user identity

• Creates createIdentityMiddleware that resolves user identity once per request
• Implements guard check to skip resolution if identity already populated
• Provides getIdentity utility function to extract and validate identity from request
• Returns 401 Unauthorized on identity resolution failure

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.ts


3. workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.test.ts 🧪 Tests +55/-0

Add tests for identity middleware

• Tests successful identity extraction when both credentials and userEntityRef are present
• Validates error handling when credentials or userEntityRef are missing
• Ensures proper error messages for middleware validation failures

workspaces/lightspeed/plugins/lightspeed-backend/src/service/middleware/getIdentity.test.ts


View more (4)
4. workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts ✨ Enhancement +13/-11

Integrate identity middleware in notebooks router

• Integrates createIdentityMiddleware into notebooks router at /v1 path
• Replaces inline getUserId function calls with getIdentity utility
• Removes duplicate credential resolution logic from permission and ownership checks
• Simplifies route handlers by using pre-resolved identity from middleware

workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouters.ts


5. workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouter.test.ts 🧪 Tests +108/-1

Add permission denial and deduplication tests

• Adds test suite for 403 Permission Denied responses across all notebook endpoints
• Implements identity deduplication test to verify middleware runs only once
• Refactors test setup to store httpAuth reference for spy verification
• Tests permission denial for POST/GET sessions and document operations

workspaces/lightspeed/plugins/lightspeed-backend/src/service/notebooks/notebooksRouter.test.ts


6. workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts ✨ Enhancement +43/-41

Integrate identity middleware in main router

• Integrates createIdentityMiddleware into main router with path-based guard for notebooks
• Replaces all inline httpAuth.credentials() and userInfo.getUserInfo() calls with
 getIdentity()
• Eliminates duplicate identity resolution across MCP server, feedback, query, and conversation
 endpoints
• Simplifies code by removing intermediate variable assignments for user identity

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.ts


7. workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts 🧪 Tests +80/-0

Add notebook conversation IDs endpoint tests

• Adds tests for /notebook-conversation-ids endpoint filtering by authenticated user
• Tests successful retrieval of conversation IDs for user's notebook sessions
• Tests empty array response when user has no notebook sessions
• Validates filtering excludes other users' sessions and sessions without conversation IDs

workspaces/lightspeed/plugins/lightspeed-backend/src/service/router.test.ts


Grey Divider

Qodo Logo

@rhdh-qodo-merge rhdh-qodo-merge Bot added enhancement New feature or request Tests labels May 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 21, 2026

Codecov Report

❌ Patch coverage is 89.09091% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.83%. Comparing base (fb32b28) to head (eb4e4fb).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3227      +/-   ##
==========================================
+ Coverage   53.82%   53.83%   +0.01%     
==========================================
  Files        2362     2363       +1     
  Lines       84815    84824       +9     
  Branches    23508    23509       +1     
==========================================
+ Hits        45650    45667      +17     
+ Misses      37714    37706       -8     
  Partials     1451     1451              
Flag Coverage Δ *Carryforward flag
adoption-insights 83.58% <ø> (ø) Carriedforward from e5744e4
ai-integrations 70.03% <ø> (ø) Carriedforward from e5744e4
app-defaults 69.60% <ø> (ø) Carriedforward from e5744e4
augment 47.62% <ø> (ø) Carriedforward from e5744e4
bulk-import 72.86% <ø> (ø) Carriedforward from e5744e4
cost-management 16.49% <ø> (ø) Carriedforward from e5744e4
dcm 32.85% <ø> (ø) Carriedforward from e5744e4
extensions 61.79% <ø> (ø) Carriedforward from e5744e4
global-floating-action-button 74.30% <ø> (ø) Carriedforward from e5744e4
global-header 61.68% <ø> (ø) Carriedforward from e5744e4
homepage 50.99% <ø> (ø) Carriedforward from e5744e4
konflux 91.01% <ø> (ø) Carriedforward from e5744e4
lightspeed 68.53% <89.09%> (+0.19%) ⬆️
mcp-integrations 81.59% <ø> (ø) Carriedforward from e5744e4
orchestrator 36.36% <ø> (ø) Carriedforward from e5744e4
quickstart 62.88% <ø> (ø) Carriedforward from e5744e4
sandbox 79.49% <ø> (ø) Carriedforward from e5744e4
scorecard 83.84% <ø> (ø) Carriedforward from e5744e4
theme 64.54% <ø> (ø) Carriedforward from e5744e4
translations 8.49% <ø> (ø) Carriedforward from e5744e4
x2a 78.59% <ø> (ø) Carriedforward from e5744e4

*This pull request uses carry forward flags. Click here to find out more.


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb32b28...eb4e4fb. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jdubrick Jdubrick force-pushed the refactor-middleware-lightspeed branch from 231d37a to e5744e4 Compare May 21, 2026 20:22
Signed-off-by: Jordan Dubrick <[email protected]>
@sonarqubecloud
Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant