Upgrade passport/saml to 5.0 #3247
Conversation
This addresses the breaking changes in 5.0 listed here https://github.com/node-saml/node-saml/blob/v5.0.0/CHANGELOG.md#-major-changes Todo: test with existing saml workflow
WalkthroughThis pull request introduces updates to the SAML integration in the project. The dependency Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
backend/src/ee/routes/v1/kmip-router.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/ee/routes/v1/ssh-certificate-router.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. backend/src/ee/routes/v1/dynamic-secret-lease-router.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "@typescript-eslint/eslint-plugin". (The package "@typescript-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/backend".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@typescript-eslint/eslint-plugin" was referenced from the config file in "backend/.eslintrc.js". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team.
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (3)
backend/src/lib/ms/index.ts (3)
1-15: Add explicit return type and documentation for better maintainabilityThis is a good wrapper implementation for the ms library that adds proper validation and error handling. To improve maintainability, consider adding:
- An explicit return type to the function
- JSDoc comments explaining what the function does and providing examples
import msFn, { StringValue } from "ms"; import { BadRequestError } from "../errors"; +/** + * Convert a time string (e.g. '1d', '10h', '2.5y') to milliseconds + * Provides validation and consistent error handling + * @param val - The string to convert (e.g. '1d', '10h', '2.5y') + * @returns The number of milliseconds + * @throws {BadRequestError} If the input is not a string or the format is invalid + * @example + * ms('2 days') // 172800000 + * ms('1d') // 86400000 + * ms('10h') // 36000000 + */ -export const ms = (val: string) => { +export const ms = (val: string): number => { if (typeof val !== "string") { throw new BadRequestError({ message: `Date must be string` }); } try { return msFn(val as StringValue); } catch { throw new BadRequestError({ message: `Invalid date format string: ${val}` }); } };
11-11: Avoid type assertion when possibleThe type assertion
as StringValuemight not be necessary sincevalis already validated as a string, and themslibrary should handle compatible string inputs correctly.- return msFn(val as StringValue); + return msFn(val);
12-13: Consider catching specific error typesThe current implementation catches all errors without discriminating. If the
mslibrary throws specific error types, catching them explicitly would provide better error handling.try { return msFn(val as StringValue); - } catch { + } catch (error) { + // If there's specific error type information, you could use it here throw new BadRequestError({ message: `Invalid date format string: ${val}` }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
backend/src/ee/routes/v1/dynamic-secret-lease-router.ts(1 hunks)backend/src/ee/routes/v1/dynamic-secret-router.ts(1 hunks)backend/src/ee/routes/v1/identity-project-additional-privilege-router.ts(1 hunks)backend/src/ee/routes/v1/kmip-router.ts(1 hunks)backend/src/ee/routes/v1/ssh-certificate-router.ts(1 hunks)backend/src/ee/routes/v1/ssh-certificate-template-router.ts(1 hunks)backend/src/ee/routes/v1/user-additional-privilege-router.ts(1 hunks)backend/src/ee/routes/v2/identity-project-additional-privilege-router.ts(1 hunks)backend/src/ee/services/access-approval-request/access-approval-request-service.ts(2 hunks)backend/src/ee/services/dynamic-secret-lease/dynamic-secret-lease-service.ts(1 hunks)backend/src/ee/services/identity-project-additional-privilege-v2/identity-project-additional-privilege-v2-service.ts(1 hunks)backend/src/ee/services/identity-project-additional-privilege/identity-project-additional-privilege-service.ts(1 hunks)backend/src/ee/services/kmip/kmip-service.ts(1 hunks)backend/src/ee/services/project-user-additional-privilege/project-user-additional-privilege-service.ts(1 hunks)backend/src/ee/services/ssh-certificate-template/ssh-certificate-template-service.ts(1 hunks)backend/src/ee/services/ssh/ssh-certificate-authority-fns.ts(1 hunks)backend/src/lib/ms/index.ts(1 hunks)backend/src/server/routes/v1/certificate-authority-router.ts(1 hunks)backend/src/server/routes/v1/certificate-router.ts(1 hunks)backend/src/server/routes/v1/certificate-template-router.ts(1 hunks)backend/src/server/routes/v1/project-membership-router.ts(1 hunks)backend/src/server/routes/v2/group-project-router.ts(1 hunks)backend/src/server/routes/v2/identity-project-router.ts(1 hunks)backend/src/services/certificate-authority/certificate-authority-service.ts(1 hunks)backend/src/services/certificate-template/certificate-template-fns.ts(1 hunks)backend/src/services/group-project/group-project-service.ts(1 hunks)backend/src/services/identity-project/identity-project-service.ts(1 hunks)backend/src/services/identity/identity-dal.ts(1 hunks)backend/src/services/project-membership/project-membership-service.ts(1 hunks)backend/src/services/super-admin/super-admin-service.ts(1 hunks)
✅ Files skipped from review due to trivial changes (29)
- backend/src/server/routes/v1/certificate-router.ts
- backend/src/ee/routes/v1/dynamic-secret-router.ts
- backend/src/server/routes/v1/certificate-template-router.ts
- backend/src/ee/routes/v2/identity-project-additional-privilege-router.ts
- backend/src/server/routes/v1/project-membership-router.ts
- backend/src/ee/routes/v1/user-additional-privilege-router.ts
- backend/src/server/routes/v1/certificate-authority-router.ts
- backend/src/services/identity/identity-dal.ts
- backend/src/ee/routes/v1/dynamic-secret-lease-router.ts
- backend/src/server/routes/v2/group-project-router.ts
- backend/src/ee/services/ssh/ssh-certificate-authority-fns.ts
- backend/src/ee/services/ssh-certificate-template/ssh-certificate-template-service.ts
- backend/src/ee/services/dynamic-secret-lease/dynamic-secret-lease-service.ts
- backend/src/services/identity-project/identity-project-service.ts
- backend/src/services/certificate-template/certificate-template-fns.ts
- backend/src/ee/routes/v1/kmip-router.ts
- backend/src/server/routes/v2/identity-project-router.ts
- backend/src/ee/services/project-user-additional-privilege/project-user-additional-privilege-service.ts
- backend/src/ee/routes/v1/ssh-certificate-template-router.ts
- backend/src/ee/services/access-approval-request/access-approval-request-service.ts
- backend/src/ee/services/identity-project-additional-privilege/identity-project-additional-privilege-service.ts
- backend/src/services/project-membership/project-membership-service.ts
- backend/src/ee/routes/v1/identity-project-additional-privilege-router.ts
- backend/src/ee/services/kmip/kmip-service.ts
- backend/src/services/group-project/group-project-service.ts
- backend/src/services/super-admin/super-admin-service.ts
- backend/src/ee/routes/v1/ssh-certificate-router.ts
- backend/src/services/certificate-authority/certificate-authority-service.ts
- backend/src/ee/services/identity-project-additional-privilege-v2/identity-project-additional-privilege-v2-service.ts
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check TS and Lint
- GitHub Check: Run integration test
- GitHub Check: Check API Changes
🔇 Additional comments (1)
backend/src/lib/ms/index.ts (1)
1-15: Nice implementation of a wrapper function with proper error handlingGood job implementing this wrapper function that provides robust error handling around the
mslibrary. This centralized approach will make working with time strings more consistent across the application and supports the SAML integration upgrade properly.
This addresses the breaking changes in 5.0 listed here https://github.com/node-saml/node-saml/blob/v5.0.0/CHANGELOG.md#-major-changes
Todo: test with existing saml workflow
Summary by CodeRabbit
Chores
Refactor
msutility to utilize a local module, improving code organization.