chore: Passport Github Enterprise OAuth#40608
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 PR adds GitHub Enterprise as a new OAuth provider to Rocket.Chat. A new ChangesGitHub Enterprise OAuth Provider
🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Git: Failed to clone repository. Please run the 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts`:
- Around line 17-24: Normalize the GitHub Enterprise base URL before composing
endpoints to avoid double slashes: inside the service === 'github_enterprise'
branch in createOAuthServiceConfig.ts, read API_GitHub_Enterprise_URL into a
local variable (e.g., base) and trim trailing slashes (replace /\/+$/ with empty
string) then use that normalized base when constructing authorizationURL,
tokenURL, and userProfileURL so the endpoints become
`${base}/login/oauth/authorize`, `${base}/login/oauth/access_token`, and
`${base}/api/v3/user`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c02e67d4-9191-48a8-b7a5-abc5ca9c4810
📒 Files selected for processing (3)
apps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.tsapps/meteor/server/lib/oauth/oauthConfigs.ts
📜 Review details
🧰 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/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
🧠 Learnings (3)
📚 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/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.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/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
🔇 Additional comments (2)
apps/meteor/server/lib/oauth/oauthConfigs.ts (1)
24-27: LGTM!apps/meteor/server/lib/oauth/configureOAuthServices.ts (1)
23-31: LGTM!
| if (service === 'github_enterprise') { | ||
| return { | ||
| provider: service, | ||
| clientId: settings.get<string>('Accounts_OAuth_GitHub_Enterprise_id'), | ||
| clientSecret: settings.get<string>('Accounts_OAuth_GitHub_Enterprise_secret'), | ||
| authorizationURL: `${settings.get<string>('API_GitHub_Enterprise_URL')}/login/oauth/authorize`, | ||
| tokenURL: `${settings.get<string>('API_GitHub_Enterprise_URL')}/login/oauth/access_token`, | ||
| userProfileURL: `${settings.get<string>('API_GitHub_Enterprise_URL')}/api/v3/user`, |
There was a problem hiding this comment.
Normalize the GitHub Enterprise base URL once.
If API_GitHub_Enterprise_URL ends with /, these template strings generate //login/oauth/... and //api/v3/user. Strip trailing slashes before composing the endpoints.
Proposed fix
if (service === 'github_enterprise') {
+ const baseUrl = settings.get<string>('API_GitHub_Enterprise_URL').replace(/\/+$/, '');
return {
provider: service,
clientId: settings.get<string>('Accounts_OAuth_GitHub_Enterprise_id'),
clientSecret: settings.get<string>('Accounts_OAuth_GitHub_Enterprise_secret'),
- authorizationURL: `${settings.get<string>('API_GitHub_Enterprise_URL')}/login/oauth/authorize`,
- tokenURL: `${settings.get<string>('API_GitHub_Enterprise_URL')}/login/oauth/access_token`,
- userProfileURL: `${settings.get<string>('API_GitHub_Enterprise_URL')}/api/v3/user`,
+ authorizationURL: `${baseUrl}/login/oauth/authorize`,
+ tokenURL: `${baseUrl}/login/oauth/access_token`,
+ userProfileURL: `${baseUrl}/api/v3/user`,
strategy: OAuthConfigs.github_enterprise.strategy,
scope: OAuthConfigs.github_enterprise.scope,
};
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts` around lines 17 -
24, Normalize the GitHub Enterprise base URL before composing endpoints to avoid
double slashes: inside the service === 'github_enterprise' branch in
createOAuthServiceConfig.ts, read API_GitHub_Enterprise_URL into a local
variable (e.g., base) and trim trailing slashes (replace /\/+$/ with empty
string) then use that normalized base when constructing authorizationURL,
tokenURL, and userProfileURL so the endpoints become
`${base}/login/oauth/authorize`, `${base}/login/oauth/access_token`, and
`${base}/api/v3/user`.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/phishing-resistant-mfa #40608 +/- ##
============================================================
Coverage 69.67% 69.68%
============================================================
Files 3320 3320
Lines 122705 122705
Branches 21905 21881 -24
============================================================
+ Hits 85499 85506 +7
+ Misses 33864 33857 -7
Partials 3342 3342
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yash-rajpal
merged commit afda09b
into
feat/phishing-resistant-mfa
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
PRM-41
Summary by CodeRabbit