fix(auth): Treat authData[provider]=null as unlink; skip provider validation for unlink #9856
Conversation
Refactor authData processing for improved clarity, efficiency, and correctness. Introduce `diffAuthData` and `subsetEqual` to handle partial updates, unlinking, and change detection intelligently. Update `RestWrite` logic to utilize refined authData delta behavior, preventing redundant operations. Add comprehensive test coverage for edge cases and optimizations. Refactor existing password policy validations for readability.
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThe change updates RestWrite.handleAuthData to ignore null/undefined providers by filtering them out before calling Auth.findUsersWithAuthData. A new unit test verifies that setting authData.gpgames to null unlinks the provider without triggering validation or external calls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RW as RestWrite.handleAuthData
participant Auth as Auth API
participant Prov as Provider Adapter
Client->>RW: Save User with authData (may include provider=null)
alt Has authData
RW->>RW: Filter out null/undefined providers (unlink entries)
RW->>Auth: findUsersWithAuthData(filteredAuthData, useMaster=true)
Auth-->>RW: Matching users (ACL-filtered)
alt Providers remain after filtering
RW->>Prov: Validate remaining provider authData
Prov-->>RW: Validation result
else No providers remain (only unlinks)
note over RW: Skip provider validation
end
RW-->>Client: Proceed with save (unlinked providers removed)
else No authData
RW-->>Client: Proceed with save
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (4 passed, 1 warning)❌ Failed Checks (1 warning)
✅ Passed Checks (4 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Flag potential breaking changes that are not documented:
1. Identify changes to public APIs/exports, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints (including removed/renamed items and changes to types, required params, return values, defaults, or behavior).
2. Ignore purely internal/private changes (e.g., code not exported from package entry points or marked internal).
3. Verify documentation exists: a "Breaking Change" section in the PR description and updates to CHANGELOG.md.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 |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/RestWrite.js (1)
1045-1054: Clean refactor of email verification conditions.The extraction of verification logic into arrow functions improves readability without changing behavior.
spec/Users.authdata.spec.js (5)
84-92: Prefer creating the initial user via logInWith for clarity and stability.Creating a user by
new Parse.User()+set('authData')+save()can be environment-sensitive. UsinglogInWithensures session issuance and consistent auth semantics.- const user = new Parse.User(); - user.set('authData', authData); - await user.save(); + const user = await Parse.User.logInWith('gpgames', { authData: authData.gpgames }); + // Link someAdapter2 immediately to mirror the original initial state + user.set('authData', { someAdapter2: authData.someAdapter2 }); + await user.save(null, { sessionToken: user.getSessionToken() });
125-135: Assert no provider validation/API calls during unlink.Strengthen the unlink test by asserting that no extra fetch calls occur when
gpgames: nullis sent.- user.set('authData', { + const beforeCalls = global.fetch?.calls?.count ? global.fetch.calls.count() : 0; + user.set('authData', { ...currentAuthData, gpgames: null, }); await user.save(null, { sessionToken }); @@ - expect(finalAuthData.gpgames).toBeUndefined(); + expect(finalAuthData.gpgames).toBeUndefined(); + if (global.fetch?.calls?.count) { + expect(global.fetch.calls.count()).toBe(beforeCalls); + }
149-151: Remove the artificial delay to keep tests fast and deterministic.No timestamp-sensitive fields are compared here; the sleep only slows the suite.
- // Small delay to ensure timestamp differences don't affect comparison - await new Promise(resolve => setTimeout(resolve, 100));
210-214: Drop nested reconfiguration; rely on the outer beforeEach.Reconfiguring the server again per test here adds overhead without benefit.
- beforeEach(async () => { - await setupAuthConfig(); - });
224-229: Use master key when introspecting authData to avoid protected-field filtering.Some environments don’t expose
authDatato clients even with a valid session. UsinguseMasterKey: trueprevents flakiness.- const freshUser = await new Parse.Query(Parse.User).get(user.id, { sessionToken }); + const freshUser = await new Parse.Query(Parse.User).get(user.id, { useMasterKey: true }); @@ - const finalUser = await new Parse.Query(Parse.User).get(user.id, { sessionToken }); + const finalUser = await new Parse.Query(Parse.User).get(user.id, { useMasterKey: true });Also applies to: 253-259
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
spec/AuthenticationAdaptersV2.spec.js(1 hunks)spec/Users.authdata.spec.js(1 hunks)src/Auth.js(3 hunks)src/RestWrite.js(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/RestWrite.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/RestWrite.js
🧬 Code graph analysis (4)
spec/AuthenticationAdaptersV2.spec.js (1)
spec/AuthenticationAdapters.spec.js (2)
payload(173-173)payload(198-198)
src/Auth.js (1)
src/RestWrite.js (1)
providerAuthData(492-492)
src/RestWrite.js (1)
src/Auth.js (5)
changed(644-644)hasMutatedAuthData(456-470)hasMutatedAuthData(468-468)mutatedAuthData(458-458)user(527-527)
spec/Users.authdata.spec.js (2)
spec/helper.js (1)
reconfigureServer(171-205)src/RestWrite.js (1)
authData(462-462)
🔇 Additional comments (16)
src/Auth.js (4)
426-428: Early return optimization looks good!Good optimization to avoid unnecessary processing when
providerAuthDatais falsy. This aligns well with the delta-based approach.
430-435: Conditional adapter resolution improves performance.The refactor to only retrieve the adapter when
beforeFindis truthy and the adapter has abeforeFindmethod is a good optimization that reduces unnecessary adapter lookups.
609-636: Well-implemented subset equality with clear semantics.The
subsetEqualfunction is well-structured with proper handling of:
- Primitive types with strict equality
- Arrays requiring exact length and element-wise equality
- Objects allowing partial updates (undefined values treated as "not provided")
The implementation correctly supports the delta-based update pattern.
643-671: Excellent delta computation logic!The
diffAuthDatafunction cleanly categorizes providers into changed/unlink/unchanged states. The explicitnullcheck for unlinking and the subset equality check for detecting actual changes are particularly well done.src/RestWrite.js (6)
158-167: Session token handling for login via authData is appropriate.The logic to return the user with session during authData login is well-placed and properly checks for the storage flag before modifying the response.
538-540: Good defensive check for authData.id presence.The early return when no provider has an
idfield prevents unnecessary database queries.
554-615: Complex but necessary refactor of handleAuthData.The multi-path login/linking flow properly handles:
- User lookup by ID vs authData
- Delta computation using the new utilities
- Separation of changed/unchanged/unlinked providers
The logic is complex but appears correct for the various scenarios.
696-700: Explicit unlink handling looks correct.Setting
authData[key] = nullfor unlinked providers ensures they are properly removed from the database without validation.
616-628: Ensure existing multiple-provider tests cover the authDataBeforeFindTriggered optimization
Review spec/Users.authdata.spec.js:81 and spec/ParseUser.spec.js:1740–1780 to confirm that whenauthDataBeforeFindTriggeredis true, edge-case validations (linking a provider already on another user or updating multiple providers simultaneously) still run; if not, add tests.
683-690: No change needed for authData validation order
The early-return (if (!hasMutatedAuthData && !isLogin)) skips validation on updates with unchanged authData, so expired tokens are only validated on login or when authData is mutated, as intended and covered by existing tests.Likely an incorrect or invalid review comment.
spec/AuthenticationAdaptersV2.spec.js (1)
832-832: Test mutation is appropriate for validation.The mutation of
payload.someData = falsebetween validation calls is a good test to ensure the adapter receives the updated payload on subsequent validations.spec/Users.authdata.spec.js (5)
31-51: Config helper looks good and self-contained.
57-80: Provider linking flow LGTM.Clear demonstration of linking a second provider while keeping existing
gpgamesunchanged.
10-29: Ensure the fetch mock supports multiple calls.
mockGooglePlayGamesAPImay be invoked in tests that trigger two token exchanges (initial + partial update). Confirm the mock doesn’t exhaust its response list.If it’s single-use, adjust the helper to return responses per URL indefinitely.
1-1: Globals already provided byspec/helper.js
BothreconfigureServerandmockFetchare assigned toglobalinspec/helper.js(and listed as globals in your ESLint config), so they’re available at runtime without an explicitrequire.Likely an incorrect or invalid review comment.
10-29: Mock the/games/v1/players/meendpoint
The GPGames provider callsGET https://www.googleapis.com/games/v1/players/me, so update the test’s mock URL to avoid bypass:- { - url: `https://www.googleapis.com/games/v1/players/${MOCK_USER_ID}`, + { + url: 'https://www.googleapis.com/games/v1/players/me', method: 'GET', response: { ok: true, json: () => Promise.resolve({ playerId: MOCK_USER_ID }), }, },
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9856 +/- ##
==========================================
- Coverage 92.99% 84.35% -8.65%
==========================================
Files 187 187
Lines 15096 15183 +87
Branches 174 174
==========================================
- Hits 14039 12808 -1231
- Misses 1045 2363 +1318
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/Auth.js
Outdated
| @@ -601,6 +606,70 @@ const handleAuthDataValidation = async (authData, req, foundUser) => { | |||
| return acc; | |||
| }; | |||
|
|
|||
| const subsetEqual = (prev, next) => { | |||
There was a problem hiding this comment.
issue: diff and equality system should not be coded from scratch and if not done correctly could be an attack vector, popular libs are native implementation should be used
There was a problem hiding this comment.
Thanks for the feedback on diff/equality — agreed. I overreached trying to fix too much at once. I’ve reduced this PR to minimal changes only:
Before findUsersWithAuthData, I filter out providers where authData[provider] is null (or undefined), so lookup/validation runs only on non-null providers.
Unlink via authData[provider] = null is applied without invoking the adapter.
If we revisit partial update semantics later, I’ll rely on native or well-vetted utilities and propose that in a separate PR.
There was a problem hiding this comment.
Ohh thanks @SNtGog diffs looks much better 🚀
…dates" This reverts commit 7669bd9.
SNtGog
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
spec/RestWrite.handleAuthData.spec.js (5)
46-66: Harden the test to prove “no provider validation on unlink”After login, explicitly forbid any further external fetches so unlink would fail if it tried to validate. This removes any ambiguity about leftover mocks.
Apply this diff:
it('should unlink provider via null', async () => { - mockGooglePlayGamesAPI(); + mockGooglePlayGamesAPI(); const authData = createMockUser(); const user = await Parse.User.logInWith('gpgames', { authData }); const sessionToken = user.getSessionToken(); await user.fetch({ sessionToken }); const currentAuthData = user.get('authData') || {}; + + // Disallow any external calls during unlink; validation must be skipped. + mockFetch([]);
56-59: Set only the provider you want to unlinkMerging in currentAuthData is unnecessary and can hide intent. Setting just
{ gpgames: null }is sufficient and tighter.Apply this diff:
- user.set('authData', { - ...currentAuthData, - gpgames: null, - }); + user.set('authData', { gpgames: null });
5-9: Use a per-test unique provider user id to avoid cross-spec interferenceIf other specs reuse the same provider/user id, collisions can occur. Generate a unique id and thread it through the mocks.
Apply this diff and thread
userIdinto the mock + authData:-const createMockUser = () => ({ - id: MOCK_USER_ID, +const uniqueId = () => `gpg_${Date.now()}_${Math.random().toString(36).slice(2)}`; +const createMockUser = (id = uniqueId()) => ({ + id, code: 'C1', });And update the usages:
-const mockGooglePlayGamesAPI = () => { +const mockGooglePlayGamesAPI = userId => { mockFetch([ { url: 'https://oauth2.googleapis.com/token', @@ { - url: `https://www.googleapis.com/games/v1/players/${MOCK_USER_ID}`, + url: `https://www.googleapis.com/games/v1/players/${userId}`,- mockGooglePlayGamesAPI(); - const authData = createMockUser(); + const userId = uniqueId(); + mockGooglePlayGamesAPI(userId); + const authData = createMockUser(userId);
42-44: Consider beforeAll for speed (if config remains constant across tests)Reconfiguring the server per test is heavier. If you add more tests in this suite with the same auth config, switch to beforeAll and, if needed, reset state per test with a user cleanup.
10-29: Optional: strengthen mocks with request assertionsIf
mockFetchsupports validating request bodies/headers, assert expected token-exchange payload (grant_type=authorization_code, code, client_id, client_secret) and HTTP methods. This guards against regressions in adapter wiring without adding noise.Happy to update the mocks with strict assertions if you share the
mockFetchhelper’s API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/RestWrite.handleAuthData.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spec/RestWrite.handleAuthData.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
🔇 Additional comments (2)
spec/RestWrite.handleAuthData.spec.js (2)
31-40: Auth config setup looks goodScoped provider config is clear and minimal.
62-66: Good verification with master keyAsserting
authData.gpgamesis absent via a master-key read correctly validates persistence-layer unlink.
| await user.save(null, { sessionToken }); | ||
|
|
||
| const updatedUser = await new Parse.Query(Parse.User).get(user.id, { useMasterKey: true }); | ||
| const finalAuthData = updatedUser.get('authData') || {}; |
There was a problem hiding this comment.
issue: You should not fall back to "{}". In this test, you expect to receive authData. Otherwise, if authData becomes protected in the future, the test could produce false positives.
Additionally, you need to set another authData in this test to ensure that:
- gpgames is correctly removed without deleting all authData.
- You can then expect that the other authData entries remain unchanged and that the null operation was correctly performed.
| for (const provider of Object.keys(authData)) { | ||
| if (authData[provider] === null || authData[provider] === undefined) { | ||
| continue; | ||
| } | ||
| withoutUnlinked[provider] = authData[provider]; | ||
| } |
There was a problem hiding this comment.
nitpick: using dedicated tools and functional prog to perform ops on objects, may be you will learn something
const authDataWithoutNullish = Object.fromEntries(Object.entries(authData).filter([_, data] => data ?? false))
When you want to perform filtering on objects, combining Object.fromEntries + Object.entries + .filter() works well (in case of functional programming)
There was a problem hiding this comment.
Object.fromEntries combined with Object.entries is underrated: https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Global_Objects/Object/fromEntries
Pull Request
Issue
Closes: #9855
Approach
Summary
A single, targeted fix in the auth path:
Auth.findUsersWithAuthData, build a filtered object that excludes providers withnullorundefinedvalues.authData[provider] = null) as part of the update without invoking the provider adapter.Behavior & Compatibility
authData[provider] = null.Security & Performance
Tests
(Added/updated) scenarios:
{ provider: null }unlinks without adapter validation.Release Notes (proposed)
fix(auth): Ignore
null/undefinedproviders fromauthDataduring user lookup; unlink (provider = null) occurs without adapter validation. No other behavior changes.Tasks
Summary by CodeRabbit
Bug Fixes
Tests