fix: third-party login is broken #37707
Conversation
🦋 Changeset detectedLatest commit: e61f209 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughReplaces Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (HTTP)
participant Middleware as Auth Middleware
participant API as APIClass / Router
participant OAuth as oAuth2ServerAuth
participant DB as Token / User Store
Client->>Middleware: HTTP request -> build routeContext (request, headers, query)
Middleware->>API: pass routeContext to handler
API->>API: for each authMethod: call authMethod(routeContext)
alt authMethod is oAuth2ServerAuth
API->>OAuth: oAuth2ServerAuth({ headers, query })
OAuth->>DB: validate token / lookup access token & user
DB-->>OAuth: access token / user or invalid
OAuth-->>API: return IUser or undefined
end
alt user found
API-->>API: set routeContext.request.user and continue to handler
API-->>Client: handler response (authenticated)
else no user found
API-->>Client: unauthorized / no user
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
Looks like this PR is ready to merge! 🎉 |
d-gubert
force-pushed
the
fix/third-party-login
branch
from
b6b2a1a to
1ee1adb
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit 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.
📒 Files selected for processing (3)
.changeset/rich-dogs-wonder.md(1 hunks)apps/meteor/app/api/server/ApiClass.ts(5 hunks)apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts(3 hunks)
🧰 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/api/server/ApiClass.tsapps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/ApiClass.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-256)
⏰ 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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 📦 Meteor Build (coverage)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (9)
.changeset/rich-dogs-wonder.md (1)
1-5: LGTM!The changeset entry is properly formatted and clearly describes the fix for third-party login functionality.
apps/meteor/app/api/server/ApiClass.ts (5)
173-173: LGTM! Improved type safety.The authMethods type has been strengthened from loose any-typed functions to strongly-typed Request-based resolvers returning
Promise<IUser | undefined>. This improves type safety and enforces a consistent authentication method signature.
826-826: LGTM! Correct context binding.The call to
authenticatedRouteis properly bound to the API instance and passes the Request object as expected by the refactored authentication flow.
976-977: LGTM! Consistent header access pattern.The header retrieval uses the standard
req.headers.getmethod, which is consistent with the Web API Request interface and properly handles the nullable return type.
991-998: LGTM! Sequential authentication method evaluation.The implementation correctly iterates through authentication methods sequentially, returning the first successful result. The intentional serial execution (with justified eslint-disable) ensures authentication methods are evaluated in priority order, which is the appropriate behavior for a pluggable auth system.
255-257: No breaking change detected; signature is consistent with usage.The
addAuthMethodsignature at line 255 matches theauthMethodsarray type definition at line 173 and is used correctly in the codebase. The only usage found (oauth2-server.ts:77) implements the expected(req: Request) => Promise<IUser | undefined>pattern without issues. No evidence of a prior signature or migration concerns exists.Likely an incorrect or invalid review comment.
apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts (3)
27-33: LGTM! Improved token handling clarity.The extraction of
incomingTokenand early return improves code readability while maintaining the same logic.
46-46: LGTM! Return type aligned with new signature.The direct return of the user object is consistent with the updated function signature.
77-83: LGTM! Correct OAuth2 authentication integration.The auth method registration properly reconstructs headers and query parameters from the Request object and delegates to
oAuth2ServerAuth. The type conversions are safe, and the implementation aligns with the new Request-based authentication pattern.
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 19:43", "12/16 19:59 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 22 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 16 Dec 2025 19:59:41 GMT |
d-gubert
force-pushed
the
fix/third-party-login
branch
5 times, most recently
from
86bc3ad to
06b4036
Compare
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @d-gubert. * #37707 (comment) The following files were modified: * `apps/meteor/app/api/server/middlewares/authentication.ts` * `apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts`
d-gubert
force-pushed
the
fix/third-party-login
branch
from
06b4036 to
5001673
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/meteor/app/integrations/server/api/api.ts (1)
195-199: Type safety concern: result.content may not be a plain object.After script execution,
bodyParamsis reassigned fromresult.contentwithout validation. If the script returns non-object content, subsequent property access (e.g.,bodyParams.separateResponse) may fail.Consider validating the content:
- bodyParams = result?.content; + bodyParams = isPlainObject(result?.content) ? result.content : {};apps/meteor/lib/utils/isPlainObject.ts (1)
1-3: Consider narrower plain object check if stricter validation needed.The current implementation returns
trueforDate,Map,Set,RegExp, and other built-in objects. For the PR's use cases (validatingbodyParams/queryParams), this is acceptable since those are typically parsed JSON. If stricter validation is needed later, consider checkingObject.getPrototypeOf(value) === Object.prototype || Object.getPrototypeOf(value) === null.apps/meteor/app/api/server/helpers/parseJsonQuery.ts (1)
137-139: Redundant fallback in queryFields check.The condition
if (queryFields && !isValidQuery(query, queryFields || ['*'], ...))has a logical redundancy: ifqueryFieldsis truthy (non-empty array), thenqueryFields || ['*']will never use the['*']fallback.If the intent is to use
['*']whenqueryFieldsis empty, consider:- if (queryFields && !isValidQuery(query, queryFields || ['*'], queryOperations ?? pathAllowConf.def)) { + if (!isValidQuery(query, queryFields.length > 0 ? queryFields : ['*'], queryOperations.length > 0 ? queryOperations : pathAllowConf.def)) {Otherwise, if the current behavior is intentional (skip validation when no queryFields), the
|| ['*']can be removed.apps/meteor/app/api/server/ApiClass.ts (1)
820-830: Context initialization and user assignment.Lines 820-822 properly initialize
queryOperations,queryFields, andloggeron the route context before authentication. However, line 826 uses a non-null assertion (user!) on a value that could benullfromauthenticatedRoute.The non-null assertion is safe here because lines 832-846 handle the
!this.usercase and return early. However, for clarity, consider:- const user = await api.authenticatedRoute(this); - this.user = user!; + const user = await api.authenticatedRoute(this); + if (user) { + this.user = user; + }This makes the null-handling explicit rather than relying on downstream checks.
📜 Review details
Configuration used: CodeRabbit 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.
📒 Files selected for processing (9)
.changeset/rich-dogs-wonder.md(1 hunks)apps/meteor/app/api/server/ApiClass.ts(8 hunks)apps/meteor/app/api/server/definition.ts(2 hunks)apps/meteor/app/api/server/helpers/parseJsonQuery.ts(5 hunks)apps/meteor/app/api/server/middlewares/authentication.ts(1 hunks)apps/meteor/app/integrations/server/api/api.ts(7 hunks)apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts(4 hunks)apps/meteor/ee/server/apps/communication/endpoints/appLogsExportHandler.ts(2 hunks)apps/meteor/lib/utils/isPlainObject.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/rich-dogs-wonder.md
🧰 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/lib/utils/isPlainObject.tsapps/meteor/ee/server/apps/communication/endpoints/appLogsExportHandler.tsapps/meteor/app/api/server/middlewares/authentication.tsapps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.tsapps/meteor/app/api/server/helpers/parseJsonQuery.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/ApiClass.ts
🧠 Learnings (2)
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the permission model distinguishes between token pass-through and true impersonation: `server-endpoints.call` is required for all endpoint access, while `server-endpoints.impersonate` is only required when `info.user.id` is provided without `info.user.token` (lines 48-53), meaning the bridge needs to mint a token. When both user ID and token are provided, it's considered legitimate credential usage, not impersonation.
Applied to files:
apps/meteor/app/api/server/middlewares/authentication.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts
🧬 Code graph analysis (7)
apps/meteor/ee/server/apps/communication/endpoints/appLogsExportHandler.ts (1)
apps/meteor/app/api/server/definition.ts (1)
GenericRouteExecutionContext(145-145)
apps/meteor/app/api/server/middlewares/authentication.ts (1)
apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts (1)
oAuth2ServerAuth(22-48)
apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts (3)
packages/core-typings/src/IUser.ts (1)
IUser(186-256)apps/meteor/app/api/server/index.ts (1)
API(55-55)apps/meteor/lib/utils/isPlainObject.ts (1)
isPlainObject(1-3)
apps/meteor/app/api/server/helpers/parseJsonQuery.ts (3)
apps/meteor/app/api/server/ApiClass.ts (1)
parseJsonQuery(246-248)apps/meteor/app/api/server/definition.ts (1)
GenericRouteExecutionContext(145-145)apps/meteor/lib/utils/isPlainObject.ts (1)
isPlainObject(1-3)
apps/meteor/app/integrations/server/api/api.ts (2)
apps/meteor/app/api/server/definition.ts (1)
GenericRouteExecutionContext(145-145)apps/meteor/lib/utils/isPlainObject.ts (1)
isPlainObject(1-3)
apps/meteor/app/api/server/definition.ts (1)
packages/rest-typings/src/index.ts (2)
Method(122-122)PathPattern(120-120)
apps/meteor/app/api/server/ApiClass.ts (2)
apps/meteor/app/api/server/definition.ts (1)
GenericRouteExecutionContext(145-145)apps/meteor/app/api/server/helpers/parseJsonQuery.ts (1)
parseJsonQuery(17-153)
🔇 Additional comments (14)
apps/meteor/app/api/server/definition.ts (3)
145-151: LGTM - Well-structured route execution context types.The dual type approach is sensible:
GenericRouteExecutionContextwithanyfor flexible internal use, andRouteExecutionContextfor strongly-typed scenarios. This aligns with the route-context-based refactor throughout the PR.
153-154: LGTM - Logger added to ActionThis.Adding
readonly logger: Loggerto the execution context enables consistent logging across route handlers.
180-182: LGTM - queryFields now accessible in context.The conditional type correctly exposes
queryFieldswhen specified in options, consistent with the existingqueryOperationspattern.apps/meteor/app/integrations/server/api/api.ts (3)
43-48: LGTM - IntegrationThis properly extends GenericRouteExecutionContext.The type correctly extends
GenericRouteExecutionContextand adds the integration-specificrequest.integrationproperty along with the requiredusertype constraint.
141-142: Good defensive check with isPlainObject.Using
isPlainObjectto guard against non-objectbodyParamsprevents runtime errors when accessing properties. The fallback to{}is appropriate.
316-328: Base class already uses the parameter-based signature.The
APIClass.authenticatedRoutemethod inapps/meteor/app/api/server/ApiClass.tsis already defined asprotected async authenticatedRoute(routeContext: GenericRouteExecutionContext): Promise<IUser | null>. The override signature usingIntegrationThis(a subtype ofGenericRouteExecutionContext) is compatible with the base class and requires no changes.Likely an incorrect or invalid review comment.
apps/meteor/app/api/server/helpers/parseJsonQuery.ts (2)
17-36: Signature refactor and defensive checks look solid.The migration to
GenericRouteExecutionContextwith runtime defensive checks forparams,queryFields, andqueryOperationsimproves type safety. The mandatoryuserIdvalidation (lines 34-36) ensures authenticated context before proceeding with query parsing.
30-32: The type assertions are necessary and appropriate. InGenericRouteExecutionContext = ActionThis<any, any, any>, the conditional types onqueryOperationsandqueryFieldsresolve toany. TypeScript does not automatically narrowanyto a specific array type viaArray.isArray()checks alone, so the explicitas string[]casts are required to establish the correct type for downstream usage. The assertions are safe and justified by the guard condition.Likely an incorrect or invalid review comment.
apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts (2)
22-48: Return type simplification and early exit pattern look good.The function now correctly returns
IUser | undefineddirectly, with proper early returns for missing token (lines 30-32) and expired/invalid token scenarios. This aligns with the updated caller expectations.
78-83: The code is correct. Therequestproperty inGenericRouteExecutionContextis typed asRequest(Fetch API), which has aheadersproperty that is aHeadersobject with the.entries()method available. The implementation safely uses this API.apps/meteor/app/api/server/ApiClass.ts (4)
168-168: Auth methods signature update aligns with route-context refactor.The type change for
authMethodsto acceptGenericRouteExecutionContextis consistent with the broader refactoring effort.
246-252: Method signatures updated correctly.Both
parseJsonQueryandaddAuthMethodnow properly accept route-context-based parameters, maintaining consistency with the new authentication flow.
972-998: Core fix for third-party login authentication.The refactored
authenticatedRoutecorrectly:
- Attempts standard header-based auth first (x-user-id + x-auth-token)
- Falls back to iterating registered
authMethodswith therouteContext- Returns the first successful user match
The serial execution pattern (lines 988-995) is appropriate here as auth methods should be tried in order until one succeeds. This addresses the third-party login issue by ensuring OAuth2 and other auth methods receive the properly typed route context.
920-920: parseJsonQuery binding updated correctly.The closure now properly passes
this(the route context) toapi.parseJsonQuery, maintaining consistency with the new signature.
apps/meteor/ee/server/apps/communication/endpoints/appLogsExportHandler.ts
Show resolved
Hide resolved
d-gubert
force-pushed
the
fix/third-party-login
branch
from
66a0917 to
a79d389
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37707 +/- ##
===========================================
- Coverage 67.63% 67.63% -0.01%
===========================================
Files 3457 3457
Lines 113662 113661 -1
Branches 20903 20903
===========================================
- Hits 76874 76872 -2
- Misses 34657 34659 +2
+ Partials 2131 2130 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@copilot evaluate the code changes in this PR and write end-to-end tests that guarantee oauth2 apps authentication works in the v1/ endpoints |
d-gubert
force-pushed
the
fix/third-party-login
branch
from
a79d389 to
15e4f11
Compare
d-gubert
force-pushed
the
fix/third-party-login
branch
from
15e4f11 to
2f82a50
Compare
d-gubert
force-pushed
the
fix/third-party-login
branch
from
2f82a50 to
e61f209
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/api/server/ApiClass.ts (1)
972-997: Core fix for third-party login - proper route context propagation.This refactor correctly passes the route context to each auth method, enabling OAuth providers to access request headers (like the Bearer token). The serial execution with early return on first successful auth is appropriate.
Minor type inconsistency:
authenticatedRoutereturnsIUser | nullbutauthMethodsreturnIUser | undefined. This works due to JavaScript's truthiness check at line 992, but consider aligning the types for clarity:- protected async authenticatedRoute(routeContext: GenericRouteExecutionContext): Promise<IUser | null> { + protected async authenticatedRoute(routeContext: GenericRouteExecutionContext): Promise<IUser | null | undefined> {Or update
authMethodsto returnIUser | null.
📜 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.
📒 Files selected for processing (10)
.changeset/rich-dogs-wonder.md(1 hunks)apps/meteor/app/api/server/ApiClass.ts(8 hunks)apps/meteor/app/api/server/definition.ts(2 hunks)apps/meteor/app/api/server/helpers/parseJsonQuery.ts(5 hunks)apps/meteor/app/api/server/middlewares/authentication.ts(1 hunks)apps/meteor/app/integrations/server/api/api.ts(7 hunks)apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts(4 hunks)apps/meteor/ee/server/apps/communication/endpoints/appLogsExportHandler.ts(2 hunks)apps/meteor/lib/utils/isPlainObject.ts(1 hunks)apps/meteor/tests/end-to-end/api/oauth-server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/app/oauth2-server-config/server/oauth/oauth2-server.ts
- apps/meteor/lib/utils/isPlainObject.ts
- apps/meteor/app/api/server/middlewares/authentication.ts
- apps/meteor/ee/server/apps/communication/endpoints/appLogsExportHandler.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/tests/end-to-end/api/oauth-server.tsapps/meteor/app/api/server/helpers/parseJsonQuery.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/ApiClass.ts
🧠 Learnings (6)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
🧬 Code graph analysis (4)
apps/meteor/tests/end-to-end/api/oauth-server.ts (1)
apps/meteor/tests/data/api-data.ts (1)
request(10-10)
apps/meteor/app/integrations/server/api/api.ts (2)
apps/meteor/app/api/server/definition.ts (1)
GenericRouteExecutionContext(145-145)apps/meteor/lib/utils/isPlainObject.ts (1)
isPlainObject(1-3)
apps/meteor/app/api/server/definition.ts (1)
packages/rest-typings/src/index.ts (2)
Method(122-122)PathPattern(120-120)
apps/meteor/app/api/server/ApiClass.ts (4)
apps/meteor/app/api/server/definition.ts (1)
GenericRouteExecutionContext(145-145)packages/core-typings/src/IUser.ts (1)
IUser(186-256)apps/meteor/app/api/server/helpers/parseJsonQuery.ts (1)
parseJsonQuery(17-149)ee/packages/media-calls/src/logger.ts (1)
logger(3-3)
🔇 Additional comments (12)
apps/meteor/tests/end-to-end/api/oauth-server.ts (1)
146-177: LGTM! These tests verify the core OAuth fix.The new test suite correctly validates that OAuth bearer tokens work with v1 API endpoints, which directly addresses the third-party login issue. The test structure follows existing patterns in this file.
One minor observation: The local
accessTokenvariable at line 147 shadows the outer scope variable at line 13, but this appears intentional for test isolation and clarity..changeset/rich-dogs-wonder.md (1)
1-5: LGTM!Appropriate patch-level version bump for a bug fix with a clear, user-facing description.
apps/meteor/app/api/server/definition.ts (2)
145-151: LGTM!The new
GenericRouteExecutionContextandRouteExecutionContexttypes provide clean abstractions for route-context-based authentication. UsingActionThis<any, any, any>for the generic version appropriately allows flexibility while maintaining the underlying type structure.
153-154: LGTM!Adding
loggerandqueryFieldsas readonly properties toActionThiscorrectly exposes these to the route context, enablingparseJsonQueryto access them without relying on implicitthisbindings.Also applies to: 181-181
apps/meteor/app/api/server/helpers/parseJsonQuery.ts (2)
17-32: LGTM! Improved type safety for route context.The refactored function signature with
GenericRouteExecutionContextand the runtime guards usingisPlainObjectandArray.isArrayprovide better safety. The fallback defaults ({}for params,[]for queryFields/queryOperations) ensure the function handles edge cases gracefully.
35-35: Good defensive checks usingtypeof.Using
typeof params?.sort === 'string'is more explicit than truthy checks and correctly handles edge cases likeparams.sort = 0or empty strings.Also applies to: 58-58, 108-108
apps/meteor/app/integrations/server/api/api.ts (3)
141-142: LGTM! Proper defensive handling of bodyParams.Using
isPlainObjectto guardthis.bodyParamsand extractingseparateResponselocally is a clean pattern that prevents runtime errors ifbodyParamsis unexpectedly not an object.
195-199: Verify handling of undefinedresult.content.If
result?.contentisundefinedor not a plain object, subsequent code at lines 197-199 accessesbodyParams.separateResponseand may fail. Consider adding a guard:- bodyParams = result?.content; + bodyParams = isPlainObject(result?.content) ? result.content : {};
316-328: LGTM! Clean refactor to explicit route context parameter.The
authenticatedRoutemethod now acceptsrouteContextexplicitly instead of relying onthis, which aligns with the broader route-context pattern. The logic for finding the integration and returning the user remains correct.apps/meteor/app/api/server/ApiClass.ts (3)
168-168: LGTM! Core authentication method refactored to route-context pattern.The
authMethodsarray andaddAuthMethodnow properly useGenericRouteExecutionContext, enabling OAuth and other third-party auth providers to access request headers and query parameters through the route context. This is the key change that fixes the third-party login issue.Also applies to: 250-252
820-830: LGTM! Route context properly populated before authentication.Setting
queryOperations,queryFields, andloggeron the context before callingauthenticatedRouteensures all necessary properties are available for auth methods (including OAuth) to function correctly.
246-248: LGTM!The
parseJsonQuerymethod correctly delegates to the imported function with the route context.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Current auth for ApiClass does not run extra auth methods registered with
addAuthMethod, causing third-party login to not work.Issue(s)
SUP-943
#35419
Steps to test or reproduce
Using the Zapier app from the Marketplace is the most straightforward way to test this.
Without the changes here, you can authenticate your Zap with your Rocket.Chat workspace, but the zap can't perform any actions on your behalf.
With the changes applied, zaps are able to perform actions on your behalf.
Further comments
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.