refactor(client): port Meteor auth/oauth packages to @rocket.chat/ddp-client#40413
Draft
refactor(client): port Meteor auth/oauth packages to @rocket.chat/ddp-client#40413
Conversation
DDPDispatcher's pushItem path runs for every payload that goes through dispatch — including connect, sub, unsub, and ping/pong. When the head of the queue is a wait block (e.g. a login), pushItem deliberately returns without sending so the next method waits. Applied to a non- method payload, that means the connect frame ws.onopen emits gets dropped on the floor: the socket sits open, the DDP handshake never happens, and any caller waiting on `connected` hangs forever. Visible failure mode: token-resume on page reload dispatched the wait login while the socket was still connecting; when ws.onopen later fired the connect frame, it was queued behind that login and never sent. Status stuck at "connecting". Fix: short-circuit dispatch for any payload whose msg is not 'method' — emit it straight away. Wait/queue serialization is a property of method calls, not protocol frames.
Previously both rejected with `Error('Connection in progress')` when
called against an already-connecting/connected state. That's hostile to
the only callers we have:
- The retry timer scheduled by `ws.onclose` fires `void this.reconnect()`
with no `.catch`. If an external `connect()` won the race, the timer
rejected and the unhandled rejection surfaced as a pageError on every
reconnect cycle.
- Bootstrap paths in SDK consumers (e.g. apps/meteor/client/lib/sdk/ddpSdk.ts)
already wrap in `.catch(() => {})` purely to silence the same noise.
Make both methods resolve with the current status when the connection is
already in flight or established, and have them clear the pending retry
timer on entry. The retry timer's callback also re-checks the status
before calling `reconnect()` so a connection re-established in the
meantime doesn't trigger a redundant reset cycle.
Also short-circuit `ws.onclose` when the closed socket isn't `this.ws`
anymore — a stale close from a replaced socket otherwise flipped
`status` back to `disconnected`, bumped `retryCount`, and scheduled a
fresh timer while a newer socket was healthy.
Tests cover all four shapes:
- reconnect/connect when already connected → resolves, doesn't throw
- retry timer firing after an external connect won the race → no
unhandled rejection, status stays connected
- stale ws.onclose after socket replacement → status and retryCount
untouched, no extra timer scheduled
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Companion to the 'idempotent connect/reconnect' fix in 7742877. retryCount was incremented on every disconnect but never zeroed on successful (re)connect. With the default budget of 1, a single force-logout cycle that chains back-to-back ws.close events (server broadcasts force_logout → SDK reconnects → app calls Meteor.logout() on top → server's logout handler closes the WS again) drained the budget. The next disconnect's onclose handler bailed at `retryCount >= retryOptions.retryCount`, leaving the SDK permanently disconnected. Method frames already in the dispatcher queue stayed queued forever — observed in EE microservices CI as e2ee-passphrase-management.spec.ts:87 (and :76) where the final loginByUserState's login frame never reached the server because the SDK socket never reconnected. Stack trace pointing at the offending second logout: Accounts.logout → standardLogout (saml.ts) → logout (UserProvider) → useResetE2EPasswordMutation.onSuccess The reset E2E password mutation calls `Meteor.logout()` on success by design (to force re-login with the new keys). In monolith CE that went via Meteor's own connection (separate retry policy); in EE microservices it now hits the SDK socket via stubMeteorStream and exposes the budget exhaustion. Verified locally with BASE_URL=http://localhost:4000 IS_EE=true: - e2ee-passphrase-management.spec.ts:76 ✓ - e2ee-passphrase-management.spec.ts:87 ✓ - e2ee-key-reset.spec.ts ✓ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Stand up a singleton DDPSDK instance (apps/meteor/client/lib/sdk/ddpSdk.ts) pointing at the current origin and keep its connection in sync with the authenticated session via a userIdStore subscription: - ensureConnectedAndAuthenticated() drives DDPSDK.connect() + login-with-token and is awaited from runUserDataSync(uid) so subscribe-on-resume races resolve in the right order. Recognized auth-rejection reasons trigger Meteor.logout() so a server-side force-logout cleanly drains client state; a token-stable guard avoids that path firing on transient 401s where a parallel flow has already swapped the stored token. - adoptAccountFromMeteorLoginResult() syncs DDPSDK.account from a Meteor login result so a downstream ensureConnectedAndAuthenticated() doesn't fire a second redundant login on the same socket. - onLoggedIn now bridges off Accounts.onLogin AND userIdStore so the callback fires reliably when Meteor's autorun chain is wedged (logout → fresh login over the SDK socket). - CachedStore version bumped (18 → 19) to invalidate caches written before the DDP wire encoding switched from JSON to EJSON, since fields like Date were stringified incorrectly in the JSON window. Also wires the SDK module into client/main.ts and bumps @rocket.chat/ddp-client into apps/meteor's manifest. The streamerAdapter file gets the symbol the override layer (next commit) will consume.
The previous routing in ddpOverREST shipped methods over the DDPSDK
WebSocket whenever the SDK socket was up — including cached-store gets
that fire immediately after re-login, where the SDK session was briefly
unauthenticated and the server returned [] for everything. The earlier
mitigation (auth-gating) added complexity without resolving the deeper
mismatch with develop's transport choice. Realign on develop's logic
and only use DDPSDK for what it's specialised for:
- All non-bypassed DDP methods now route to REST (`method.call` /
`method.callAnon`), matching develop. `bypassMethods` is restored to
['setUserStatus', 'logout'] and `UserPresence:*` / `stream-*` keep
bypassing.
- `login` (resume + password) routes through the DDPSDK socket when
it's connected so the same login authenticates Meteor's session AND
the SDK socket in one hop. `adoptAccountFromMeteorLoginResult` syncs
`sdk.account` so a downstream `ensureConnectedAndAuthenticated`
short-circuits instead of firing a redundant second login.
- `sdk.call` (used by cached stores) now goes via `Meteor.callAsync`,
same as develop, so methods that previously bypassed ddpOverREST
(`permissions/get`, `subscriptions/get`, `private-settings/get`)
now hit REST too.
Two sub-fixes that fall out of this:
- ddpOverREST's REST error path was rethrowing the API middleware's
parsed JSON body (a plain string `error.message`) into
`processResult`. Meteor's stream handler couldn't parse it as a DDP
result frame, so the resume invoker never saw the rejection, the
stale token stayed in localStorage, and the user wedged on /home with
no main UI. Re-encode it as a proper DDP error result.
- `ensureConnectedAndAuthenticated` now drops the local credentials on
an authentication error (`Accounts._unstoreLoginToken` +
`Meteor.connection.setUserId(null)`) when the stored token didn't
change mid-flight. Keeps the dead-WS path off limits — the previous
fix using `Meteor.logout()` flaked in CI's parallel-shard runs by
racing fresh registration / re-auth.
Reverts `killMeteorStream` to leave Meteor's WS connected: the
permanent-disconnect path broke
`MethodInvoker.sendMessage()`'s `if (this.connection._stream._connected)
{ _send(...) }` gate, leaving every method invoker queued behind a
connection that never returned and ddpOverREST's `_send` wrapper never
firing.
Verified locally:
- Reload with valid token: `/home` renders, 18 REST method calls, 2 WS
frames (login resume on DDPSDK + the SDK login from
ensureConnectedAndAuthenticated).
- Reload with invalid token: localStorage cleared, userId null, login
form rendered.
- administration-settings.spec.ts:26 passes locally in 8.3s.
Replace Meteor.connection._stream with a stub that pretends to be
connected and forwards outbound DDP frames through the DDPSDK socket.
Goal: one WebSocket per page (the DDPSDK one), eliminating the second
auth roundtrip that was inflating boot time by ~1.5s in EE microservices.
- stubMeteorStream: disconnects Meteor's real stream, swaps in a stub
with currentStatus.connected=true, routes method/sub/unsub frames via
sdk.client.ddp.emit('send', ...) using Meteor's id namespace, and
answers heartbeat pings locally with synthetic pongs. Carries the
message/reset/disconnect listeners that Meteor registered before the
swap onto the stub. Synthesizes a `connected` frame after a microtask
if Meteor's WS hadn't finished its DDP handshake yet.
- ddpSdkCollectionBridge: also forwards `result` and `updated` frames so
bypassed methods routed through the SDK socket reach Meteor's
_methodInvokers (SDK-internal ids never collide with Meteor's numeric
ids, so the bridge is a no-op for SDK's own callers).
- overrides/index: import order now guarantees _send override and the
inbound bridge are wired before the stream swap.
When ensureConnectedAndAuthenticated runs at boot (from the userIdStore subscriber) it can race Meteor's own resume login that's routed through stubMeteorStream. Both end up as `login` method frames on the SDK socket within the same tick. ddp-streamer's Account.login has no dedup, so each fires Accounts.onLogin → Presence.newConnection → a duplicate row in usersSessions for the same session id. The duplicate stays ONLINE while the active connection flips to AWAY on idle. processConnectionStatus prefers ONLINE over AWAY in the aggregate, so the user.status update is a no-op (modifiedCount=0) and the `presence.status` broadcast never fires — the navbar badge stays online even after `UserPresence:away` succeeds server-side. Fixes: - Drop the boot-time `ensureConnectedAndAuthenticated` call. The Meteor login resume going through stubMeteorStream (with adoptAccountFromMeteorLoginResult populating sdk.account on the result) is the only auth path needed at boot. - Gate the userIdStore-subscriber path on `Accounts.loggingIn()` and `sdk.account.uid`: if Meteor's login is in flight (or has just finished and the adopt callback set sdk.account.uid), short-circuit instead of issuing a redundant loginWithToken. - Single-flight `inflightLogin` so concurrent calls share one promise. Verified: tests/e2e/omnichannel/omnichannel-rooms-forward.spec.ts "should be set to the queue" passes locally in 7.2s with IS_EE=true.
…nnect DDPSDK auto-fires loginWithToken on every `connected` event using the in-memory account.user.token (DDPSDK.create line 115-122). When the server force-logs the user out (resetUserE2EKey, account-manage-devices logout, admin device management, etc.), the flow on the server is: 1. broadcast 'user.forceLogout' → meteor.service listener closes the user's WebSocket sessions 2. Users.unsetLoginTokens(uid) wipes services.resume.loginTokens The client sees the WS close, the SDK reconnects, and on the new `connected` it auto-retries loginWithToken with the now-dead token. DDPSDK calls this with `void` so the rejection is swallowed — `account.user` stays populated, `Meteor.userId()` stays set, the router never falls back to Login, and the navbar continues to render Home with a stale session. Wrap account.loginWithToken to observe rejections from this auto-retry path: on auth error (and only when the token in localStorage is still the same one we tried — guard against a concurrent fresh login that already rotated it), drop local credentials so the router goes to Login. Mirrors what ensureConnectedAndAuthenticated already does for its own loginWithToken call. Verified: e2ee-key-reset and e2ee-passphrase-management now pass.
The microservices ddp-streamer was using ws.terminate() (TCP RST) for broadcast force-logouts. The monolith path uses session.connectionHandle.close() which is graceful and flushes the WS buffer first — letting the `notify-user/<uid>/force_logout` stream message (queued by apps/meteor/server/modules/listeners/listeners.module.ts:49) reach the client before the socket goes down. In EE that stream message races with the terminate, terminate wins, the client's useForceLogout hook never fires, and tests like e2e-encryption/e2ee-passphrase-management.spec.ts:87 are left with stale localStorage credentials and a Login button that never hides. Switch to ws.close() with a 5s setTimeout fallback to terminate() for unresponsive sockets — matches the graceful-close semantics the monolith already relies on without losing the safety net for zombies. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the `Use_RC_SDK` admin setting (General → Use Rocket.Chat SDK) and
matching client-side opt-in helper. The migration ships dormant — legacy
Meteor DDP transport remains the default. Three sources resolve the flag,
URL > localStorage > meta tag:
- `?sdk_transport=on|off` URL parameter (per-tab)
- `rc-config-sdk_transport` localStorage entry (per-user)
- `<meta name="rc-sdk-transport-enabled">` injected from the
`Use_RC_SDK` setting (workspace-wide opt-in / kill-switch)
The flag is read once at module init and used to dispatch the SDK code
paths back to their Meteor equivalents when off:
- The 5 SDK-transport overrides (`ddpOverREST`, `ddpSdkCollectionBridge`,
`subscribeViaSDK`, `stubMeteorStream`, `killMeteorStream`) wrap their
bodies in `if (isSdkTransportEnabled())` — `Meteor.connection._send`,
`_subscribe`, and `_stream` stay un-wrapped when off.
- `apps/meteor/app/utils/client/lib/SDKClient.ts` keeps both
`createNewMeteorStream` and `createNewDdpSdkStream`. With the flag off
streams use Meteor's subscribe + the original
`Meteor.connection._stream.on('message')` bridge; with the flag on the
SDK stream is used. `publish` similarly dispatches between
`Meteor.call` and `getDdpSdk().client.callAsync`.
- `apps/meteor/client/providers/ServerProvider.tsx` only subscribes to
DDPSDK connection events, combines DDPSDK status into `getStatus`,
and calls `ensureConnectedAndAuthenticated` from `reconnect()` /
`getDdpSdk().connection.close()` from `disconnect()` when the flag is
on. With the flag off, `getStatus` returns `Meteor.status()` directly.
- The boot-time side-effect block in `ddpSdk.ts` (loginWithToken wrap,
userIdStore subscriber, reset-on-reconnect listener) is gated as well
— keeping the SDK socket "anonymous" when off so it doesn't create
duplicate Presence sessions.
E2e default stays off, matching CI's dormant baseline; the SDK-transport
path is reachable via URL/localStorage for manual verification or a
dedicated future job.
Contributor
|
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 |
🦋 Changeset detectedLatest commit: eae0056 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40413 +/- ##
===========================================
- Coverage 70.00% 69.58% -0.43%
===========================================
Files 3301 3314 +13
Lines 120462 121567 +1105
Branches 21566 21753 +187
===========================================
+ Hits 84330 84588 +258
- Misses 32834 33665 +831
- Partials 3298 3314 +16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ggazzo
force-pushed
the
port-meteor-auth-to-sdk
branch
from
4ccfed2 to
b96e1d4
Compare
Replace `import { Google/Facebook/Twitter/MeteorDeveloperAccounts } from
'meteor/<provider>-oauth'` with local IOAuthProvider objects that wire the
existing requestCredential implementation into createOAuthTotpLoginMethod.
The provider globals were only used to (1) attach requestCredential and (2)
hand a typed `provider` to the TOTP factory — both fully covered locally.
Constants pulled inline:
- twitter: `TWITTER_VALID_PARAMS_AUTHENTICATE = ['force_login', 'screen_name']`
- meteor-developer: `MDG_SERVER = 'https://www.meteor.com'`
Cordova `Google.signIn` is left undefined; Rocket.Chat web doesn't bundle
the `plugins.googleplus` cordova plugin so the existing isCordova check
becomes a no-op, matching today's web behaviour.
Phase 1 of the plan to remove client-side coupling to Meteor accounts/oauth
packages — the four provider-oauth packages remain in .meteor/packages for
the server, but `apps/meteor/client/` no longer references them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…th to SDK
Move launchLogin / stateParam / loginStyle / showPopup from meteor/oauth
into @rocket.chat/ddp-client as a tree-shakeable `oauth` namespace, and
adapt 7 of the 8 client call sites. The 8th (meteor/login/oauth.ts) keeps
its meteor/oauth import because it's coupled to accounts-base
(_retrieveCredentialSecret + _storageTokenPrefix override) and is handled
in Phase 3.
Also drops `client/meteor/overrides/oauthRedirectUri.ts` — its legacy
`?close` patch is folded into the new `client/lib/oauth/redirectUri.ts`,
which builds on `client/lib/absoluteUrl.ts` instead of Meteor.absoluteUrl.
What lives where:
packages/ddp-client/src/oauth/ (browser-pure, no Meteor deps)
launchLogin popup or redirect, sessionStorage for redirect data
stateParam base64 state JSON (loginStyle + credentialToken + flags)
loginStyle popup vs redirect, cordova/sessionStorage fallbacks
showPopup centered popup + 100ms close polling
types LoginStyle / LaunchLoginOptions / etc.
apps/meteor/client/lib/oauth/redirectUri.ts
`_oauth/<service>` URL with optional ?close back-compat — has to live
next to the call site because it depends on RC's absoluteUrl.
Why not _retrieveCredentialSecret yet:
The Meteor oauth package exposes a popup-window API
(OAuth._handleCredentialSecret) that is called from the popup callback
page served by Meteor's server. Reimplementing it in the SDK without
also moving the popup callback would split the credentialSecrets map
across two universes. Phase 3 will fix the override in
client/meteor/login/oauth.ts together with accounts-base.
…count
Phase 3a — replace `Accounts.onLogin`/`Accounts.onLogout` call sites with
`getDdpSdk().account.onLogin`/`onLogout` and back the SDK's emitter with
Meteor in pass-through mode (flag OFF) and with the existing `uid` event
in SDK transport mode (flag ON). Also add `LoginCancelledError` to the
SDK so `meteor/login/oauth.ts` can drop its `Accounts.LoginCancelledError`
reliance in a follow-up.
SDK side (`packages/ddp-client/src/`):
- `types/Account.ts` adds `onLogin(fn)` / `onLogout(fn)` that subscribe to
the existing `uid` Emitter event and only fire on falsy↔truthy
transitions, so any code path that ends with `account.uid` populated
triggers the right callback (no new events to instrument across
loginWithToken / loginWithPassword / external sync).
- `types/LoginCancelledError.ts` mirrors `Accounts.LoginCancelledError`
including the magic `numericError = 0x8acdc2f` so existing `instanceof`
+ `.error === numericError` checks keep working over the wire.
- `index.ts` re-exports `LoginCancelledError`.
Adopt sync (`apps/meteor/client/lib/sdk/ddpSdk.ts`):
- `adoptAccountFromMeteorLoginResult` now emits `uid` after assignment so
subscribers of `account.onLogin` see the Meteor-side login. Skipped on
same-uid token-refreshes.
Pass-through (`apps/meteor/client/lib/sdk/meteorBackedSdk.ts`):
- `account.onLogin/onLogout` delegate to `Accounts.onLogin/onLogout`
when the SDK transport is OFF (Meteor still owns auth lifecycle).
Migrated call sites (5 files, 8 hooks):
- client/lib/loggedIn.ts — drops the flag-on/off split,
single SDK subscription.
- client/lib/cachedStores/CachedStore.ts — also replaces
`Accounts._storedLoginToken()`
with direct localStorage read.
- client/lib/e2ee/rocketchat.e2e.ts — onLogout (storageLocation use
stays — separate phase).
- client/providers/UserProvider/UserProvider.tsx
- client/startup/startup.ts — both onLogout calls.
Out of scope for this commit:
- `Accounts.callLoginMethod` (oauth.ts, password.ts, AuthenticationProvider)
- `Accounts._unstoreLoginToken`, `Accounts.LoginCancelledError`,
`Accounts.onPageLoadLogin`, `Accounts.oauth.*`, `Accounts.storageLocation`
- `client/meteor/overrides/killMeteorStream.ts` keeps `Accounts._setLoggingIn`
Phase 3b — three small leaf files that only touched accounts-base for token
storage / config-error reporting:
- client/views/root/hooks/loggedIn/useStoreCookiesOnLogin.ts
Replace Accounts._storedLoginToken() with direct localStorage read
('Meteor.loginToken' is the same key Meteor uses internally; the
override in client/meteor/overrides/ddpOverREST.ts already reads from
this exact key, so no compatibility risk).
- client/views/oauth/components/AuthorizationFormPage.tsx
Same: Accounts.storageLocation.getItem(Accounts.LOGIN_TOKEN_KEY)
→ window.localStorage.getItem('Meteor.loginToken').
- client/lib/wrapRequestCredentialFn.ts
Drop Accounts.ConfigError; introduce client/lib/oauth/errors.ts with
OAuthConfigError mirroring the same shape (`name`, `error`) that the
callback consumers check via duck typing on `error.error`.
`grep -r "from 'meteor/accounts-base'" apps/meteor/client/` now down to 16
(from 22 at branch start, 19 after Phase 3a).
Phase 3c — extract clearStoredCredentials() into
client/lib/sdk/storedCredentials.ts and use it in place of
Accounts._unstoreLoginToken() at the two SDK-aware call sites:
client/lib/sdk/ddpSdk.ts (auth error recovery
inside ensureConnectedAndAuthenticated)
client/views/root/hooks/loggedIn/useForceLogout.ts (force-logout broadcast)
The new helper does the same job as the override in
client/meteor/overrides/unstoreLoginToken.ts: drops the standard Meteor
keys (`Meteor.loginToken`, `Meteor.loginTokenExpires`, `Meteor.userId`)
and runs CachedStoresManager.clearAllCachesOnLogout() so per-user caches
don't leak across accounts.
The override itself stays in place for now — anything in the codebase
still calling `Accounts._unstoreLoginToken()` (server-injected paths,
Meteor's own internal logout) keeps the same behaviour. Removing the
override is a later cleanup once we've audited those callers.
Imports remaining: 14 (was 16).
Three changes:
- The `if (!Accounts.saml) Accounts.saml = {};` namespace stub plus the
matching `declare module 'meteor/accounts-base'` augmentation were dead
code — `Accounts.saml.{credentialToken,credentialSecret}` is never read
anywhere in the codebase. Drop both.
- Same for the `function onLogout(...)` augmentation in saml.ts: nothing
in saml.ts calls `Accounts.onLogout`, and the existing call sites that
do already migrated to `getDdpSdk().account.onLogout` in the previous
commit, so the augmentation has no remaining consumers.
- `Accounts.storageLocation.removeItem(Accounts.USER_ID_KEY)` →
`window.localStorage.removeItem('Meteor.userId')`. `storageLocation` is
always `localStorage` on the web client and `USER_ID_KEY` is the
literal `'Meteor.userId'` (Meteor source: accounts-client.js).
Imports remaining: 13 (was 14).
ggazzo
force-pushed
the
port-meteor-auth-to-sdk
branch
from
26be7e7 to
d49077a
Compare
…ount
Phase 3e — add `sdk.account.callLoginMethod()` and `LoginCancelledError` to
the SDK so client-side login dispatchers no longer need accounts-base.
SDK side (`packages/ddp-client/src/`):
- `types/Account.ts` adds `callLoginMethod({ methodName?, methodArguments?,
userCallback? })` mirroring `Accounts.callLoginMethod`. Defaults to
`methodName: 'login'`. Dispatches via `client.callAsyncWithOptions(...,
{ wait: true })` and adopts the resulting `id`/`token`/`tokenExpires`
into `account.uid`/`user`/`token` so the existing `uid` event path
(which feeds `onLogin`) fires consistently with the standalone
`loginWithPassword`/`loginWithToken` methods.
- `index.ts` re-exports `CallLoginMethodOptions` and `LoginCallback` types.
Pass-through (`apps/meteor/client/lib/sdk/meteorBackedSdk.ts`):
- `account.callLoginMethod` delegates to the real
`Accounts.callLoginMethod` so flag-OFF mode keeps Meteor's resume /
loggingIn / onLogin bookkeeping intact.
Migrated call sites:
- client/lib/2fa/overrideLoginMethod.ts — `callLoginMethod` Promise wrapper
now hits the SDK; no Accounts import.
- client/meteor/login/oauth.ts — `tryLoginAfterPopupClosed`
dispatch + `LoginCancelledError`
(now from @rocket.chat/ddp-client).
- client/meteor/login/password.ts — TOTP login dispatch +
`Accounts._hashPassword(password)`
replaced by an inline
`hashPassword()` using
`@rocket.chat/sha256`.
- client/providers/AuthenticationProvider/AuthenticationProvider.tsx
— `loginWithIframe` /
`loginWithTokenRoute` dispatchers.
Out of scope (kept on Meteor for now): `Accounts.loggingIn`,
`Accounts._unstoreLoginToken` (used by AuthenticationProvider for the
unstoreLoginToken callback registry), `Accounts.oauth.*` (popup callback
coordination — Meteor server-served page), `Accounts.onPageLoadLogin`
(2FA OAuth challenge bridge).
Imports remaining: 11 (was 13).
…cal helper Use OAuthConfigError (already introduced in 962ab9e for wrapRequestCredentialFn) so both OAuth provider entry points throw the same shape. CustomOAuth.ts still imports Accounts for the popup-callback coordination APIs (`Accounts.oauth.registerService` / `Accounts.oauth.credentialRequestCompleteHandler` / `Accounts.oauth.serviceNames`) — those bridge to Meteor's server-served OAuth callback page and need a separate plan to migrate.
`Accounts.storageLocation` is always `localStorage` on the web client (meteor/accounts-base sets it to `Meteor._localStorage`, which equals `window.localStorage` outside of test/SSR), and all 11 call sites here read/write E2EE-specific keys (`public_key`, `private_key`, `e2e.randomPassword`) — none of them are coordinated with the rest of the accounts-base storage namespace. Replace with direct `localStorage` reads/writes, no behaviour change. Imports remaining: 10 (was 11).
| if (Accounts.storageLocation.getItem('e2e.randomPassword')) { | ||
| Accounts.storageLocation.setItem('e2e.randomPassword', newPassword); | ||
| if (localStorage.getItem('e2e.randomPassword')) { | ||
| localStorage.setItem('e2e.randomPassword', newPassword); |
| async createRandomPassword(): Promise<string> { | ||
| const randomPassword = await generatePassphrase(); | ||
| Accounts.storageLocation.setItem('e2e.randomPassword', randomPassword); | ||
| localStorage.setItem('e2e.randomPassword', randomPassword); |
…icationProvider Add an `onBeforeClearCredentials` Emitter to `client/lib/sdk/storedCredentials.ts` so consumers can react to a credential clear without monkey-patching `Accounts._unstoreLoginToken` on the fly. The `client/meteor/overrides/unstoreLoginToken.ts` override now bridges Meteor's internal `_unstoreLoginToken` (called by Meteor's logout flow and token-rotation paths) into our local `clearStoredCredentials()` so the same emitter fires regardless of who triggered the drop — the override existed before to flush per-user caches, this just consolidates both responsibilities behind the same helper. `AuthenticationProvider.unstoreLoginToken` now subscribes to that emitter instead of swapping `Accounts._unstoreLoginToken` for a closure that wraps the previous reference and unwraps in the cleanup. The old swap pattern was racey on multiple subscribers (last writer wins) and leaked references across re-renders if the cleanup didn't fire. Single consumer of `useUnstoreLoginToken` is `client/views/root/hooks/useIframeLoginListener.ts`, which keeps the same sub/unsub semantics — verified by reading the call site. The `Accounts` import in AuthenticationProvider stays for `Accounts.loggingIn()` which is a Meteor Tracker reactive var; that's a separate plan.
…lizer
Pure mechanical fixes — prettier formatting (multi-line argument lists
collapsed) plus the nested-ternary in `Account.callLoginMethod`'s
`tokenExpires` handling pulled out to a `normalizeTokenExpires` helper
to satisfy `no-nested-ternary`. Also `${window.location}` →
`window.location.toString()` in `stateParam.ts` to drop the
`restrict-template-expressions` warning.
No behavioural changes.
|
|
||
| const saveDataForRedirect = (loginService: string, credentialToken: string): void => { | ||
| try { | ||
| sessionStorage.setItem(REDIRECT_DATA_KEY, JSON.stringify({ loginService, credentialToken })); |
Replace every direct `localStorage.{get,set,remove}Item('Meteor.loginToken' /
'Meteor.loginTokenExpires' / 'Meteor.userId')` in the client with a
typed `CredentialStorage` interface exposed by `sdk.account.storage`.
Why: client code was reading/writing Meteor's localStorage keys in 6+
places, each making implicit assumptions about the storage backend
(window.localStorage being available, the exact key strings, the
cleanup semantics). Pulling that into a single SDK abstraction:
- single source of truth for the key names + persistence backend, so
swapping to sessionStorage / a secure cookie / a no-op (tests) is one
change instead of grepping the codebase
- typed API: `getToken()` / `getUserId()` / `getTokenExpires()` /
`setCredentials()` / `clearUserId()` / `clear()` instead of raw
string keys and `parseInt` for the expiry timestamp
- SSR safe by construction (the LocalStorage impl guards `typeof window`
internally, so callers don't have to)
SDK side (`packages/ddp-client/src/`):
- `types/CredentialStorage.ts` defines the interface, plus
`LocalStorageCredentialStorage` (default — keeps the Meteor key names
for back-compat with `client/meteor/overrides/ddpOverREST.ts` and
existing user sessions) and `MemoryCredentialStorage` (no-op for
Node/SSR test envs).
- `Account` interface gains `storage: CredentialStorage`. `AccountImpl`
takes it as a constructor arg, defaulting to `LocalStorageCredentialStorage`.
- `index.ts` re-exports the interface and both impls.
Pass-through (`apps/meteor/client/lib/sdk/meteorBackedSdk.ts`):
- `account.storage` is a `LocalStorageCredentialStorage` so flag-OFF
reads and writes hit the same persistent slot Meteor uses.
Migrated call sites (5 files):
- client/lib/sdk/ddpSdk.ts `readStoredLoginToken()`
- client/lib/sdk/storedCredentials.ts `clearStoredCredentials()` body
- client/lib/cachedStores/CachedStore.ts `getToken()` (PrivateCachedStore)
- client/views/root/hooks/loggedIn/useStoreCookiesOnLogin.ts `rc_token` cookie
- client/views/oauth/components/AuthorizationFormPage.tsx access_token form input
- client/startup/startup.ts session-resume failure detection
- client/meteor/login/saml.ts SAML SLO `clearUserId()`
`storedCredentials.ts` was importing `getDdpSdk` from `./ddpSdk` while `ddpSdk.ts` imports `clearStoredCredentials` from `./storedCredentials`. The cycle didn't crash on initialisation (every reference was deferred to call-time inside functions), but it's a code smell — bundlers disagree on live-binding semantics for circular ESM, and a future top-level use of either binding would break in subtle ways. Pull a single shared `LocalStorageCredentialStorage` instance into `apps/meteor/client/lib/sdk/credentialStorage.ts` and have every consumer (storedCredentials, ddpSdk, meteorBackedSdk, CachedStore, useStoreCookiesOnLogin, AuthorizationFormPage, saml, startup) import that module directly. The `Account` / `AccountImpl` SDK-side abstraction stays unchanged: when the real DDPSDK path is taken, we monkey-assign `sdk.account.storage = credentialStorage` so flag-ON and flag-OFF modes share the same localStorage slot. While here, drop the `Meteor.loginToken` literal that startup.ts was still reading — now goes through `credentialStorage.getToken()` like everything else.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Substitui imports
from 'meteor/<pkg>'no client por uso do@rocket.chat/ddp-clientSDK, em ordem topológica (folhas primeiro). Os pacotes Meteor seguem em.meteor/packages(server depende — fora do escopo); o que muda é que o bundle client deixa de tocar neles.Status
google-oauth,facebook-oauth,twitter-oauth,meteor-developer-oauth— cada provider vira umIOAuthProviderlocal.oauthcore: namespaceoauthem@rocket.chat/ddp-client(launchLogin/stateParam/resolveLoginStyle/showPopup);client/lib/oauth/redirectUri.tssubstituiOAuth._redirectUri+oauthRedirectUri.ts.accounts-base(12 de 22 imports removidos):Accounts.onLogin/onLogout→sdk.account.onLogin/onLogout.LoginCancelledErrorna SDK.adoptAccountFromMeteorLoginResultagora emiteuid.Accounts._storedLoginToken/Accounts.ConfigError→ leitura direta delocalStorage+OAuthConfigErrorlocal.Accounts._unstoreLoginToken()→clearStoredCredentials()emclient/lib/sdk/storedCredentials.ts.storageLocation+USER_ID_KEY→localStorage/'Meteor.userId'.Accounts.callLoginMethod→sdk.account.callLoginMethodna SDK;meteorBackedSdkdelega.Accounts._hashPassword→@rocket.chat/sha256inline.Accounts.LoginCancelledError→LoginCancelledErrorda SDK emmeteor/login/oauth.ts.Accounts.ConfigErroremCustomOAuth→OAuthConfigError.Accounts.storageLocationeme2ee/rocketchat.e2e.ts(11 chamadas) →localStoragedireto.Accounts._unstoreLoginTokenmonkey-patch emAuthenticationProvider→onBeforeClearCredentialsemitter (subscription-based, race-free); overrideunstoreLoginToken.tsagora delega aclearStoredCredentials().Imports
from 'meteor/<pkg>'emapps/meteor/client/google-oauth/facebook-oauth/twitter-oauth/meteor-developer-oauthoauthmeteor/login/oauth.ts— popup callback)accounts-base10 imports restantes em
accounts-baseTodos com motivo concreto que exige trabalho fora deste PR:
meteor/overrides/unstoreLoginToken.tsAccounts._unstoreLoginTokenmonkey-patchmeteor/overrides/killMeteorStream.tsAccounts._setLoggingInmeteor/overrides/userAndUsers.tsAccounts.connection.userId()meteor/overrides/stubMeteorStream.tsAccounts._lastLoginTokenWhenPolledmeteor/login/oauth.tsAccounts.oauth.credentialRequestCompleteHandler,onPageLoadLoginmeteor/login/google.tsAccounts._options.restrictCreationByEmailDomainmeteor/startup/accounts.tsAccounts.onEmailVerificationLink,Accounts.verifyEmailproviders/AuthenticationProvider/AuthenticationProvider.tsxAccounts.loggingIn()lib/customOAuth/CustomOAuth.tsAccounts.oauth.{registerService,credentialRequestCompleteHandler,serviceNames}lib/sdk/meteorBackedSdk.tsAccounts.onLogin/onLogout/callLoginMethodEscopo
Apenas
apps/meteor/client/. Server (apps/meteor/server,apps/meteor/app/*/server,ee/) continua usando os pacotes Meteor — fora deste PR.Test plan
tsc --noEmitlimpo nos arquivos modificadosgrep -rn "from 'meteor/{google,facebook,twitter,meteor-developer}-oauth'" apps/meteor/client→ 0grep -rn "from 'meteor/oauth'" apps/meteor/client→ 1 (esperado)grep -rn "from 'meteor/accounts-base'" apps/meteor/client→ 10 (de 22)?closeonPageLoadLoginhandler)callLoginMethod)useForceLogout) limpa creds + router cai pra/loginonLogout/onLoginlisteners)loginWithIframe/loginWithTokenRoute(AuthenticationProvider) ainda fazem o login completouseIframeLoginListener+onBeforeClearCredentials) ainda dispara em logoutstorageLocation→localStorage)