feat: added a new enum for grant types#39441
feat: added a new enum for grant types#39441youssefesmail6 wants to merge 1 commit 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 |
|
|
|
WalkthroughThis pull request introduces a GrantType enum to centralize OAuth grant type constants, replacing hardcoded string literals ('authorization_code', 'refresh_token') with typed enum values across the codebase for consistency and maintainability. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/custom-oauth/server/custom_oauth_server.js">
<violation number="1" location="apps/meteor/app/custom-oauth/server/custom_oauth_server.js:125">
P1: `GrantType` is referenced without import/declaration, causing OAuth token request construction to fail at runtime.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| code: query.code, | ||
| redirect_uri: OAuth._redirectUri(this.name, config), | ||
| grant_type: 'authorization_code', | ||
| grant_type: GrantType.AuthorizationCode, |
There was a problem hiding this comment.
P1: GrantType is referenced without import/declaration, causing OAuth token request construction to fail at runtime.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/custom-oauth/server/custom_oauth_server.js, line 125:
<comment>`GrantType` is referenced without import/declaration, causing OAuth token request construction to fail at runtime.</comment>
<file context>
@@ -122,7 +122,7 @@ export class CustomOAuth {
code: query.code,
redirect_uri: OAuth._redirectUri(this.name, config),
- grant_type: 'authorization_code',
+ grant_type: GrantType.AuthorizationCode,
state: query.state,
});
</file context>
| grant_type: GrantType.AuthorizationCode, | |
| grant_type: 'authorization_code', |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/server/oauth2-server/model.ts (1)
16-19: MoveGrantTypeinto a lightweight shared module.Exporting a runtime enum from
model.tsmakes every caller load the whole OAuth model module, including@rocket.chat/models, just to read two constants. A small sharedgrantTypes/constantsmodule would keep the enum reusable without coupling unrelated OAuth flows to this server implementation.Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/oauth2-server/model.ts` around lines 16 - 19, Move the runtime enum GrantType out of the heavy OAuth model into a tiny shared module (e.g., grantTypes or constants) so callers don't import the whole OAuth model; create a new module that exports the two values (as a const enum-like object or union type + const) and update usages to import GrantType from that new module instead of model.ts (ensure you update the export/import in both the original model where GrantType was defined and any other references that import GrantType).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js`:
- Around line 122-126: The code references GrantType.AuthorizationCode inside
getAccessToken() but never imports GrantType; add an import for GrantType from
the library that defines it (the same OAuth/OAuth2 library used elsewhere in the
project) so getAccessToken() can use GrantType.AuthorizationCode without
throwing a ReferenceError; update the top of custom_oauth_server.js to import
GrantType (e.g., import { GrantType } from '<oauth2-lib>'; or the correct module
used in your repo).
In `@apps/meteor/tests/end-to-end/api/oauth-server.ts`:
- Line 6: Test still uses the literal 'refresh_token' instead of the enum;
update the refresh grant usages to use GrantType.RefreshToken (replace the
string 'refresh_token' in the OAuth request bodies with GrantType.RefreshToken)
wherever the test constructs a refresh flow request (the same change applies to
the other occurrence that currently uses the hardcoded literal), keeping the
rest of the request shape intact so the test uses the enum from import {
GrantType }.
---
Nitpick comments:
In `@apps/meteor/server/oauth2-server/model.ts`:
- Around line 16-19: Move the runtime enum GrantType out of the heavy OAuth
model into a tiny shared module (e.g., grantTypes or constants) so callers don't
import the whole OAuth model; create a new module that exports the two values
(as a const enum-like object or union type + const) and update usages to import
GrantType from that new module instead of model.ts (ensure you update the
export/import in both the original model where GrantType was defined and any
other references that import GrantType).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95b60d30-331e-4cb7-8444-504431cce168
📒 Files selected for processing (5)
apps/meteor/app/cloud/server/functions/finishOAuthAuthorization.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/packages/linkedin-oauth/linkedin-server.jsapps/meteor/server/oauth2-server/model.tsapps/meteor/tests/end-to-end/api/oauth-server.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
🧰 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/cloud/server/functions/finishOAuthAuthorization.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/tests/end-to-end/api/oauth-server.tsapps/meteor/server/oauth2-server/model.tsapps/meteor/packages/linkedin-oauth/linkedin-server.js
🧠 Learnings (5)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/cloud/server/functions/finishOAuthAuthorization.tsapps/meteor/tests/end-to-end/api/oauth-server.tsapps/meteor/server/oauth2-server/model.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/cloud/server/functions/finishOAuthAuthorization.tsapps/meteor/tests/end-to-end/api/oauth-server.tsapps/meteor/server/oauth2-server/model.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/oauth-server.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/server/oauth2-server/model.ts
🪛 Biome (2.4.4)
apps/meteor/packages/linkedin-oauth/linkedin-server.js
[error] 5-5: Illegal use of an import declaration outside of a module
(parse)
🔇 Additional comments (2)
apps/meteor/app/cloud/server/functions/finishOAuthAuthorization.ts (1)
9-9: Nice cleanup.Using
GrantType.AuthorizationCodehere removes another magic string and keeps the token exchange aligned with the shared grant definitions.Also applies to: 31-31
apps/meteor/packages/linkedin-oauth/linkedin-server.js (1)
5-5: Nice cleanup.Using
GrantType.AuthorizationCodehere keeps the LinkedIn OAuth flow aligned with the shared grant definitions and removes another hardcoded grant string.Also applies to: 19-19
| const params = new URLSearchParams({ | ||
| code: query.code, | ||
| redirect_uri: OAuth._redirectUri(this.name, config), | ||
| grant_type: 'authorization_code', | ||
| grant_type: GrantType.AuthorizationCode, | ||
| state: query.state, |
There was a problem hiding this comment.
Add the missing GrantType import.
GrantType.AuthorizationCode is referenced here, but GrantType is never defined in this module. That will throw a ReferenceError the first time getAccessToken() runs and break custom OAuth logins.
🛠️ Suggested fix
import { settings } from '../../settings/server';
+import { GrantType } from '/server/oauth2-server/model';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/custom-oauth/server/custom_oauth_server.js` around lines 122
- 126, The code references GrantType.AuthorizationCode inside getAccessToken()
but never imports GrantType; add an import for GrantType from the library that
defines it (the same OAuth/OAuth2 library used elsewhere in the project) so
getAccessToken() can use GrantType.AuthorizationCode without throwing a
ReferenceError; update the top of custom_oauth_server.js to import GrantType
(e.g., import { GrantType } from '<oauth2-lib>'; or the correct module used in
your repo).
| import type { Response } from 'supertest'; | ||
|
|
||
| import { getCredentials, api, request, credentials } from '../../data/api-data'; | ||
| import { GrantType } from '/server/oauth2-server/model'; |
There was a problem hiding this comment.
Use the enum for the refresh flow too.
This file still hardcodes 'refresh_token' at Line 115, so the migration is only half done here. Switching that request to GrantType.RefreshToken keeps the test aligned with the new contract and avoids another typo-prone literal.
Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/tests/end-to-end/api/oauth-server.ts` at line 6, Test still uses
the literal 'refresh_token' instead of the enum; update the refresh grant usages
to use GrantType.RefreshToken (replace the string 'refresh_token' in the OAuth
request bodies with GrantType.RefreshToken) wherever the test constructs a
refresh flow request (the same change applies to the other occurrence that
currently uses the hardcoded literal), keeping the rest of the request shape
intact so the test uses the enum from import { GrantType }.
2 participants
Added an
enumforGrantTypeto replace previously hardcoded strings in the authentication flow.GrantType.AuthorizationCodereplaces"authorization_code"GrantType.RefreshTokenreplaces"refresh_token"This improves type safety, readability, and reduces chance of typos.
Summary by CodeRabbit