chore: Passport Wordpress OAuth#40593
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 |
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughMigrate WordPress OAuth registration to use addPassportCustomOAuth with a cleared Passport strategy; change config typing to Partial, update a typed identityTokenSentVia assignment for wordpress-com, and preserve ServiceConfiguration upsert/remove keyed by the WordPress service. ChangesWordPress OAuth Configuration Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
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.
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/wordpress/server/lib.ts (1)
9-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the OAuth config on each refresh.
configis now a long-livedPartial, and this refactor no longer clears mode-specific fields. If an admin switches fromcustomorwordpress-comback to another server type, staletokenPath,authorizePath,scope, oridentityTokenSentViavalues remain on the shared object and are written back through$set, so the next login can keep using the previous provider's endpoints.Suggested direction
-const config: Partial<OAuthConfiguration> = { +const baseConfig: Partial<OAuthConfiguration> = { serverURL: '', identityPath: '/oauth/me', addAutopublishFields: { forLoggedInUser: ['services.wordpress'], forOtherUsers: ['services.wordpress.user_login'], }, accessTokenParam: 'access_token', }; const fillSettings = _.debounce(async (): Promise<void> => { - config.serverURL = settings.get('API_Wordpress_URL'); - if (!config.serverURL) { - if (config.serverURL === undefined) { + const serverURL = settings.get('API_Wordpress_URL'); + if (!serverURL) { + if (serverURL === undefined) { return fillSettings(); } return; } + + const config: Partial<OAuthConfiguration> = { + ...baseConfig, + serverURL, + };You'll also want the persistence step to replace or explicitly unset keys that are no longer applicable; otherwise omitted fields can survive in
ServiceConfiguration.Also applies to: 20-64, 68-74
🤖 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/app/wordpress/server/lib.ts` around lines 9 - 18, The current module-level `config: Partial<OAuthConfiguration>` is reused across refreshes and retains mode-specific fields (like `tokenPath`, `authorizePath`, `scope`, `identityTokenSentVia`) causing stale provider endpoints; change the code to build a fresh OAuth config object inside the refresh/build function (instead of the shared `config` variable) so each refresh computes fields based on the current `serverType`, and when persisting to `ServiceConfiguration` explicitly replace the stored object or unset keys that are no longer applicable (do not merge into the old object). Locate and update the code that reads/writes `config` (symbols: `config`, `OAuthConfiguration`, and the refresh/persist function that calls `$set`) to construct a new config per refresh and ensure persistence uses a full replace or explicit null/unset for removed keys.
🤖 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.
Outside diff comments:
In `@apps/meteor/app/wordpress/server/lib.ts`:
- Around line 9-18: The current module-level `config:
Partial<OAuthConfiguration>` is reused across refreshes and retains
mode-specific fields (like `tokenPath`, `authorizePath`, `scope`,
`identityTokenSentVia`) causing stale provider endpoints; change the code to
build a fresh OAuth config object inside the refresh/build function (instead of
the shared `config` variable) so each refresh computes fields based on the
current `serverType`, and when persisting to `ServiceConfiguration` explicitly
replace the stored object or unset keys that are no longer applicable (do not
merge into the old object). Locate and update the code that reads/writes
`config` (symbols: `config`, `OAuthConfiguration`, and the refresh/persist
function that calls `$set`) to construct a new config per refresh and ensure
persistence uses a full replace or explicit null/unset for removed keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da6780d4-a7f1-44bd-b84e-70612cd68403
📒 Files selected for processing (1)
apps/meteor/app/wordpress/server/lib.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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/wordpress/server/lib.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/app/wordpress/server/lib.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/wordpress/server/lib.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/app/wordpress/server/lib.ts
🔇 Additional comments (1)
apps/meteor/app/wordpress/server/lib.ts (1)
64-64: No action needed—repeated calls are safe.The helper implementation calls
passport.unuse(serviceName)beforepassport.use(), ensuring the previous Passport strategy is replaced on each invocation. The debounce (1000ms) further mitigates any performance impact from repeated settings changes. This pattern is idempotent and handles strategy updates correctly.
![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 1 file
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/phishing-resistant-mfa #40593 +/- ##
===============================================================
+ Coverage 69.71% 69.72% +0.01%
===============================================================
Files 3304 3304
Lines 121731 121731
Branches 21561 21538 -23
===============================================================
+ Hits 84866 84883 +17
+ Misses 33602 33586 -16
+ Partials 3263 3262 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
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/wordpress/server/lib.ts (1)
10-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRebuild the OAuth config per
fillSettings()run.This shared mutable object can carry
identityTokenSentVia,authorizePath,tokenPath, andscopefrom a previous server type into the next one. For example, after awordpress-comrun, switching tocustomwithout all custom paths set will keep the old wordpress.com endpoints.💡 Suggested fix
-const config: Partial<OAuthConfiguration> = { +const baseConfig: Partial<OAuthConfiguration> = { serverURL: '', identityPath: '/oauth/me', addAutopublishFields: { forLoggedInUser: ['services.wordpress'], forOtherUsers: ['services.wordpress.user_login'], }, accessTokenParam: 'access_token', }; const serviceKey = 'wordpress'; const fillSettings = _.debounce(async (): Promise<void> => { + const config: Partial<OAuthConfiguration> = { + ...baseConfig, + };🤖 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/app/wordpress/server/lib.ts` around lines 10 - 19, The shared mutable const config is reused across runs and can retain properties (identityTokenSentVia, authorizePath, tokenPath, scope, etc.) from previous providers; update fillSettings() to create a fresh OAuth configuration object for each invocation (or clone a pristine base) instead of mutating/importing the module-level config, and explicitly reset or omit identityTokenSentVia, authorizePath, tokenPath, scope (and any other provider-specific fields) before applying new provider settings so previous endpoints/values (e.g., wordpress-com) cannot leak into subsequent runs.
🤖 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/app/wordpress/server/lib.ts`:
- Around line 24-25: The passport.unuse(serviceKey) call is executed before
validating API_Wordpress_URL, which can remove the Passport strategy while
leaving the ServiceConfiguration intact if the function returns early; move the
passport.unuse(serviceKey) call so it runs after the URL validation check (i.e.,
after you confirm API_Wordpress_URL is truthy) and keep the existing
ServiceConfiguration.remove(...) cleanup logic (the same
serviceKey/ServiceConfiguration usage) in place so the strategy is only
unregistered when the flow proceeds past the URL validation.
---
Outside diff comments:
In `@apps/meteor/app/wordpress/server/lib.ts`:
- Around line 10-19: The shared mutable const config is reused across runs and
can retain properties (identityTokenSentVia, authorizePath, tokenPath, scope,
etc.) from previous providers; update fillSettings() to create a fresh OAuth
configuration object for each invocation (or clone a pristine base) instead of
mutating/importing the module-level config, and explicitly reset or omit
identityTokenSentVia, authorizePath, tokenPath, scope (and any other
provider-specific fields) before applying new provider settings so previous
endpoints/values (e.g., wordpress-com) cannot leak into subsequent runs.
🪄 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: 1448e2e9-f125-4e42-bcda-c78384bf98a1
📒 Files selected for processing (1)
apps/meteor/app/wordpress/server/lib.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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 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/wordpress/server/lib.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/app/wordpress/server/lib.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/wordpress/server/lib.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/app/wordpress/server/lib.ts
🔇 Additional comments (1)
apps/meteor/app/wordpress/server/lib.ts (1)
1-7: LGTM!Also applies to: 21-21, 59-59, 69-83
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
yash-rajpal
merged commit f709064
into
feat/phishing-resistant-mfa
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
PRM-37
Summary by CodeRabbit