fix: Reset Password page stuck in loop for OAuth-linked accounts

[ritikpal1122]

Summary

Fixes #36698

• Removed user?.services?.password?.bcrypt from the password-change gate condition so OAuth users without an existing local password can set one and clear requirePasswordChange
• Guarded same-password check, password-history check, and addPasswordToHistory behind user?.services?.password?.bcrypt since they only apply when the user already has a local password
• Added user?.requirePasswordChange to the outer guard so users flagged by the system can always set a password, even if Accounts_AllowPasswordChangeForOAuthUsers is disabled

Root Cause

When an OAuth user (e.g., Google login) with a pre-existing unverified local account logs in, requirePasswordChange is set to true and they're redirected to the Reset Password page. The "Reset" button calls saveUserProfile(), but the
password-change block required user.services.password.bcrypt to exist — which it doesn't for OAuth-only users. This meant:

1. The new password was never set
2. requirePasswordChange was never cleared
3. The user was stuck in an infinite loop on the reset page

Test plan

• Create a local user with email (don't verify)
• Configure OAuth (e.g., Google) with the same email
• Login via OAuth → should redirect to reset password page
• Enter new password, click Reset → password should be set, flag cleared, redirected to home
• Verify normal password change for existing users still works (same-password and history checks)
• Verify OAuth user with Accounts_AllowPasswordChangeForOAuthUsers disabled but requirePasswordChange true can still set password

Summary by CodeRabbit

• Bug Fixes

  • Improved password-change logic to correctly handle users flagged to require a password reset, ensuring validations run in the right scenarios.
  • Prevented accidental acceptance of the same password and tightened password-history checks.

• Security

  • Narrowed enforcement of history and existing-password checks to applicable accounts, preserved policy enforcement and session/token invalidation flows.

[dionisio-bot]

Looks like this PR is not ready to merge, because of the following issues:

• This PR is missing the 'stat: QA assured' label
• This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

[changeset-bot]

⚠️ No Changeset found

Latest commit: 044ed2d

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[coderabbitai]

Walkthrough

Expanded password-change gating in saveUserProfile.ts to allow processing when a user has requirePasswordChange, tightened inner validations to run only if a bcrypt password exists, and moved password-history and "same password" checks under that bcrypt guard while preserving policy enforcement and token invalidation flows.

Changes

Cohort / File(s)	Summary
Password Change Logic apps/meteor/server/methods/saveUserProfile.ts	Allow password-change processing when user.requirePasswordChange or Accounts_AllowPasswordChange and newPassword present; restrict inner validations (same-password check, password-history, history append) to cases where user.services.password.bcrypt exists; keep policy enforcement and token invalidation within updated flow.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibbled code by moonlight's glow,
Guarded bcrypt where passwords grow,
History tucked beneath the vine,
Require-change now gets its sign,
Hopping home — the flow's inline. 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main fix: addressing the Reset Password page being stuck for OAuth-linked accounts, which is the core issue resolved in this PR.
Linked Issues check	✅ Passed	The PR meets all coding requirements from issue #36698: it allows OAuth users without a bcrypt password to set one, guards same-password and history checks for users with existing passwords, adds requirePasswordChange to the outer gate, and preserves password change enforcement.
Out of Scope Changes check	✅ Passed	All changes are within scope; modifications to password-change gating logic, validations, and history tracking directly address the OAuth user password-setting issue outlined in the objectives.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

No actionable comments were generated in the recent review. 🎉

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e379c61 and dfeb4bd.

📒 Files selected for processing (1)

• apps/meteor/server/methods/saveUserProfile.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/server/methods/saveUserProfile.ts

🧠 Learnings (1)

📚 Learning: 2025-10-07T15:08:37.419Z

Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/pbkdf2.ts:13-45
Timestamp: 2025-10-07T15:08:37.419Z
Learning: In apps/meteor/client/lib/e2ee/pbkdf2.ts, PBKDF2 iteration count validation is not enforced because the iterations parameter is outside the user's control and is system-managed.

Applied to files:

• apps/meteor/server/methods/saveUserProfile.ts

🧬 Code graph analysis (1)

apps/meteor/server/methods/saveUserProfile.ts (2)

apps/meteor/app/custom-oauth/server/custom_oauth_server.js (1)

• user (488-488)

packages/models/src/index.ts (1)

• Users (201-201)

🔇 Additional comments (3)

apps/meteor/server/methods/saveUserProfile.ts (3)

124-127: Logic correctly addresses the OAuth reset-password loop — LGTM.

The outer guard (line 125) now admits users flagged with requirePasswordChange, and the inner guard (line 127) lets them bypass the Accounts_AllowPasswordChange setting. This matches the PR objective and resolves the previously flagged edge case.

---

128-161: Bcrypt-guarded validation blocks are correctly structured.

Same-password check, password-history check, and addPasswordToHistory all correctly gate on the presence of an existing bcrypt hash. For OAuth users setting their first local password, these blocks are correctly skipped (no prior hash to compare or store). For users with an existing password, the stale user reference from line 62 correctly captures the pre-change hash for history storage.

---

240-247: 2FA bypass for requirePasswordChange users is correct.

Line 242's !user.requirePasswordChange condition ensures users forced to reset their password aren't blocked by 2FA — appropriate since they may not have 2FA set up yet.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@apps/meteor/server/methods/saveUserProfile.ts`:
- Around line 125-127: The inner condition that checks settings.newPassword and
rcSettings.get<boolean>('Accounts_AllowPasswordChange') currently blocks users
with user?.requirePasswordChange from updating their password; update the
conditional in saveUserProfile.ts so that if user?.requirePasswordChange is true
it bypasses the Accounts_AllowPasswordChange check (i.e., allow the branch when
settings.newPassword is provided OR user.requirePasswordChange is true),
ensuring the code path that sets the new password and clears
user.requirePasswordChange still runs; adjust any related logic that clears the
flag so it executes after a successful password update in the same block that
handles new passwords (referencing canChangePasswordForOAuth,
user?.services?.password, user?.requirePasswordChange, settings.newPassword, and
rcSettings.get<boolean>('Accounts_AllowPasswordChange')).

🧹 Nitpick comments (1)

apps/meteor/server/methods/saveUserProfile.ts (1)

129-129: Nit: Code comment in implementation.

As per coding guidelines, "Avoid code comments in the implementation".

Proposed fix

-				// don't let user change to same password
-				if (await compareUserPassword(user, { plain: settings.newPassword })) {
+				if (await compareUserPassword(user, { plain: settings.newPassword })) {

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fbc4935 and e379c61.

📒 Files selected for processing (1)

• apps/meteor/server/methods/saveUserProfile.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/server/methods/saveUserProfile.ts

🔇 Additional comments (2)

apps/meteor/server/methods/saveUserProfile.ts (2)

128-141: Guarding same-password and history checks behind bcrypt existence looks correct.

This ensures OAuth-only users without an existing local password can set one without hitting the same-password or password-history errors. The passwordPolicy.validate (line 143) and Accounts.setPasswordAsync (line 145) remain outside this guard, which is the right behavior — all users get policy enforcement.

---

155-161: Guarding addPasswordToHistory behind bcrypt existence is correct.

No bcrypt hash means nothing meaningful to store in history.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[cubic-dev-ai]

No issues found across 1 file