refactor(saml): convert ServiceProvider deflate flow to async/await#38742
refactor(saml): convert ServiceProvider deflate flow to async/await#38742ATHARVA279 wants to merge 2 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughThe SAML ServiceProvider refactors internal callback-based async flows to async/await. A module-scoped Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts (1)
110-154:⚠️ Potential issue | 🟡 Minor
deflateRawAsyncis now outside thetry/catch— intentional?The
deflateRawAsynccall on Line 111 is outside thetryblock (Lines 112–153). If deflation fails, the raw error propagates instead of going through theerror instanceof Error ? error : String(error)normalization on Line 152. This is benign sincedeflateRawAsyncwill only throwErrorinstances, but it's a subtle behavioral change from the original code worth being aware of.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts` around lines 110 - 154, The call to deflateRawAsync in requestToUrl is currently outside the try/catch so its errors bypass the existing normalization; move the await deflateRawAsync(request) inside the existing try block (or wrap it in a small try/catch that rethrows using the same normalization: throw error instanceof Error ? error : String(error)) so all exceptions from deflation are handled consistently by the requestToUrl error path.
🧹 Nitpick comments (2)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts (2)
167-168: Remove code comment from implementation.Line 167 introduces a code comment in new code. As per coding guidelines, "Avoid code comments in the implementation".
Proposed fix
- // TBD. We should really include a proper RelayState here const relayState = Meteor.absoluteUrl();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts` around lines 167 - 168, Remove the inline implementation comment preceding relayState and keep only the code; delete the comment "// TBD. We should really include a proper RelayState here" so the function uses const relayState = Meteor.absoluteUrl() without implementation comments, leaving any design notes to PR description or separate documentation; locate the declaration of relayState in ServiceProvider.ts to remove that comment.
96-105: The async IIFE bridge is correct but consider making the method itselfasync.The
void (async () => { ... })()pattern properly bridges async logic into the callback-based signature. If the public contract permits it, convertinglogoutResponseToUrlto return aPromise<string>(likerequestToUrlalready does) and deprecating the callback overload would complete the modernization goal from issue#38741. That said, if callers still rely on the callback shape, this intermediate step is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts` around lines 96 - 105, The method logoutResponseToUrl currently wraps an async IIFE to call buildLogoutResponseUrl and invoke a callback; change logoutResponseToUrl into an async function that returns Promise<string> (remove the callback parameter), simply await this.buildLogoutResponseUrl(response) and return the target, and mark the old callback-based usage as deprecated (update or remove any callback callers to use the returned Promise). Ensure references to logoutResponseToUrl and buildLogoutResponseUrl are updated accordingly so callers now await logoutResponseToUrl(response).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts
🧬 Code graph analysis (1)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts (1)
apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts (3)
error(142-146)relayState(45-47)relayState(49-51)
🔇 Additional comments (2)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts (2)
28-29: LGTM — module-scoped promisified helper.Clean extraction avoids repeated
util.promisifycalls across methods.
156-178: Clean extraction of the logout response URL building logic.The method correctly mirrors the pattern from
requestToUrlfor logout responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts`:
- Around line 110-154: The call to deflateRawAsync in requestToUrl is currently
outside the try/catch so its errors bypass the existing normalization; move the
await deflateRawAsync(request) inside the existing try block (or wrap it in a
small try/catch that rethrows using the same normalization: throw error
instanceof Error ? error : String(error)) so all exceptions from deflation are
handled consistently by the requestToUrl error path.
---
Nitpick comments:
In `@apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts`:
- Around line 167-168: Remove the inline implementation comment preceding
relayState and keep only the code; delete the comment "// TBD. We should really
include a proper RelayState here" so the function uses const relayState =
Meteor.absoluteUrl() without implementation comments, leaving any design notes
to PR description or separate documentation; locate the declaration of
relayState in ServiceProvider.ts to remove that comment.
- Around line 96-105: The method logoutResponseToUrl currently wraps an async
IIFE to call buildLogoutResponseUrl and invoke a callback; change
logoutResponseToUrl into an async function that returns Promise<string> (remove
the callback parameter), simply await this.buildLogoutResponseUrl(response) and
return the target, and mark the old callback-based usage as deprecated (update
or remove any callback callers to use the returned Promise). Ensure references
to logoutResponseToUrl and buildLogoutResponseUrl are updated accordingly so
callers now await logoutResponseToUrl(response).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts (1)
156-177: Consider extracting shared URL-building logic to reduce duplication.
buildLogoutResponseUrlandrequestToUrlshare nearly identical steps: deflate → base64 → target URL selection → separator → relay state → sign → stringify. Extracting a common private helper (e.g.,buildRedirectUrl(payload, target, relayState)) would reduce duplication and make future changes less error-prone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts` around lines 156 - 177, Both buildLogoutResponseUrl and requestToUrl duplicate the same URL-building steps (deflate → base64 → choose target → append separator → set RelayState → maybeSignRequest → querystring.stringify); extract that into a private helper (e.g., buildRedirectUrl or buildSignedRedirect) and have buildLogoutResponseUrl and requestToUrl call it. The helper should accept the payload (SAMLResponse or SAMLRequest), target URL (use serviceProviderOptions.idpSLORedirectURL or caller-provided target), and relayState (default to Meteor.absoluteUrl()), perform deflateRawAsync + base64, call maybeSignRequest, and return target + querystring.stringify(signedPayload) so both methods keep minimal logic and no duplicated steps.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts
🔇 Additional comments (4)
apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts (4)
28-29: Clean extraction ofdeflateRawAsync.Good use of
util.promisifyat module scope to avoid repeated promisification.
96-105: LGTM — async IIFE withvoidis a clean bridge from callback to async.The
voidoperator correctly suppresses the floating-promise lint, and the try/catch properly funnels errors back into the callback.
112-112: Clean swap todeflateRawAsync.
156-177: No action needed. TheidpSLORedirectURLproperty is a requiredstringfield in theIServiceProviderOptionsinterface, not optional. The code at line 159 correctly assigns this property totargetwithout a guard, and calling.indexOf('?')is safe. The pattern inrequestToUrlthat checksif (this.serviceProviderOptions.idpSLORedirectURL)before using it is defensive coding despite the property being required by the type.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/meteor-accounts-saml/server/lib/ServiceProvider.ts`:
- Around line 156-177: Both buildLogoutResponseUrl and requestToUrl duplicate
the same URL-building steps (deflate → base64 → choose target → append separator
→ set RelayState → maybeSignRequest → querystring.stringify); extract that into
a private helper (e.g., buildRedirectUrl or buildSignedRedirect) and have
buildLogoutResponseUrl and requestToUrl call it. The helper should accept the
payload (SAMLResponse or SAMLRequest), target URL (use
serviceProviderOptions.idpSLORedirectURL or caller-provided target), and
relayState (default to Meteor.absoluteUrl()), perform deflateRawAsync + base64,
call maybeSignRequest, and return target + querystring.stringify(signedPayload)
so both methods keep minimal logic and no duplicated steps.
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
fix #38741
Summary by CodeRabbit