Skip to content

chore: Passport Dolphin#40611

Merged
yash-rajpal merged 1 commit into
feat/phishing-resistant-mfafrom
passport-dolphin
May 19, 2026
Merged

chore: Passport Dolphin#40611
yash-rajpal merged 1 commit into
feat/phishing-resistant-mfafrom
passport-dolphin

Conversation

@yash-rajpal
Copy link
Copy Markdown
Member

@yash-rajpal yash-rajpal commented May 19, 2026

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

PRM-44

Summary by CodeRabbit

  • Refactor
    • Improved Dolphin OAuth configuration handling to dynamically update when settings change, eliminating the need for manual reconfiguration between setting adjustments.

Review Change Stack

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 19, 2026

⚠️ No Changeset found

Latest commit: e48610e

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
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9191fb3-bf38-4d42-b859-7d6b9768bcfe

📥 Commits

Reviewing files that changed from the base of the PR and between dc44d7c and e48610e.

📒 Files selected for processing (1)
  • apps/meteor/app/dolphin/server/lib.ts

Walkthrough

This PR refactors the Dolphin OAuth integration from a stateful CustomOAuth instance to a dynamic Passport-based configuration function. Imports are updated to add Passport support, and the config constant is now typed as Partial<OAuthConfiguration>. A new configureDolphinOAuth function unuses and re-registers the strategy on-demand, validated against multiple Dolphin-related settings keys via debounced watchers.

Changes

Dolphin OAuth Passport Migration

Layer / File(s) Summary
Passport strategy registration and dynamic configuration
apps/meteor/app/dolphin/server/lib.ts
Imports updated to support Passport and remove CustomOAuth. Config typed as Partial<OAuthConfiguration>. New configureDolphinOAuth function unuses existing strategy, validates required OAuth settings, normalizes server URL, and registers via addPassportCustomOAuth. Multi-key settings.watchMultiple debounces reconfiguration across Accounts_OAuth_Dolphin, URL, and credential keys.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • RocketChat/Rocket.Chat#40609: Refactors Gitlab OAuth using the same CustomOAuth-to-Passport migration pattern with dynamic configuration function and multi-key settings watchers.
  • RocketChat/Rocket.Chat#40591: Refactors Nextcloud OAuth following the same Passport-based dynamic configuration pattern with settings.watchMultiple triggers.
  • RocketChat/Rocket.Chat#40203: Introduces the underlying Passport-based addPassportCustomOAuth and CustomOAuthStrategy flow that this PR adopts for Dolphin.

Suggested labels

type: chore

Suggested reviewers

  • ricardogarim
  • KevLehman
  • tassoevan

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

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

@dionisio-bot
Copy link
Copy Markdown
Contributor

dionisio-bot Bot commented May 19, 2026

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

@yash-rajpal yash-rajpal marked this pull request as ready for review May 19, 2026 15:24
@yash-rajpal yash-rajpal merged commit f052ca8 into feat/phishing-resistant-mfa May 19, 2026
15 of 17 checks passed
@yash-rajpal yash-rajpal deleted the passport-dolphin branch May 19, 2026 15:25
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.69%. Comparing base (dc44d7c) to head (e48610e).
⚠️ Report is 2 commits behind head on feat/phishing-resistant-mfa.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                      @@
##           feat/phishing-resistant-mfa   #40611   +/-   ##
============================================================
  Coverage                        69.68%   69.69%           
============================================================
  Files                             3322     3320    -2     
  Lines                           122760   122705   -55     
  Branches                         21888    21854   -34     
============================================================
- Hits                             85551    85521   -30     
+ Misses                           33867    33850   -17     
+ Partials                          3342     3334    -8     
Flag Coverage Δ
unit 70.37% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants