feat: Allow logout with invalid session token #1803
Conversation
Hi all, Not sure if my formatting is perfect but closes parse-community#1176 (removes pre. SDK 2.0 success functions), and adds documentation for LiveQuery triggers. Also, for me, when I google "Parse Javascript SDK", it takes me to v1.11.0. Is there any way we can add "This SDK is outdated" or some other warning to point users to the latest SDK? Thank you!
Update Cloud Code
|
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! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #1803 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 63 63
Lines 6174 6180 +6
Branches 1449 1466 +17
=========================================
+ Hits 6174 6180 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
What would be the difference between the first and second logout request? What makes the first fail and the second succeed? |
|
@dblythy Friendly ping regarding the previous question. If the 1st logout attempt fails, why does the 2nd attempt (after catching the error) succeed? |
|
@dblythy We should add this to the server side like parse-community/parse-server#8722 so that it can work for all SDK's @mtrezza I have no idea why there is a need to call |
|
Could you please open a separate issue for this? |
|
are we still tracking this? |
|
@dblythy Nice job! Can you fix the merge conflicts? |
| @@ -812,11 +812,13 @@ class ParseUser<T extends Attributes = Attributes> extends ParseObject<T> { | |||
| * <code>current</code> will return <code>null</code>. | |||
| * | |||
| * @param {object} options | |||
| * @param {boolean} [options.clearSession] If true, the session token will be | |||
There was a problem hiding this comment.
I think this needs a better explanation. It's now a rather technical description but not so clear what the purpose of the option is and why / when one should use it.
There was a problem hiding this comment.
What do you recommend we change it to?
There was a problem hiding this comment.
I don't have a suggestion because I don't understand what the option exactly is for, from a practical perspective.
See #1803 (comment).
📝 WalkthroughWalkthroughThe changes update the logout functionality to allow an optional Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant ParseUser as ParseUser
participant Controller as DefaultController
participant Server as Parse Server
App->>ParseUser: logOut({ clearSession })
ParseUser->>Controller: logOut({ clearSession })
Controller->>Server: Request logout (with sessionToken)
alt Server returns INVALID_SESSION_TOKEN
alt clearSession is true
Controller-->>ParseUser: Suppress error, clear session
else clearSession is false or undefined
Controller-->>ParseUser: Propagate INVALID_SESSION_TOKEN error
end
else Logout successful
Controller-->>ParseUser: Success
end
ParseUser-->>App: Promise resolved/rejected
Assessment against linked issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
♻️ Duplicate comments (1)
src/ParseUser.ts (1)
815-816: Improve documentation to clarify purpose and use casesThe current documentation is rather technical and doesn't clearly explain when and why a developer would use this option. Consider expanding it to describe practical use cases, such as handling logout during invalid session scenarios without requiring error catching.
🧹 Nitpick comments (2)
types/ParseUser.d.ts (1)
401-403: Method signature matches implementation, but has indentation issue.The static
logOutmethod signature is properly updated to include theclearSessionoption, maintaining consistency with other changes.The indentation is inconsistent with the rest of the file according to ESLint. Adjust to use 2 spaces instead of 4 for proper alignment.
- static logOut(options?: RequestOptions & { - clearSession?: boolean; - }): Promise<void>; + static logOut(options?: RequestOptions & { + clearSession?: boolean; + }): Promise<void>;🧰 Tools
🪛 ESLint
[error] 401-401: Expected indentation of 2 spaces but found 4.
(indent)
src/ParseUser.ts (1)
1212-1218: Remove console.log statement from production codeThe console.log statement appears to be left over from debugging and should be removed before merging to production.
const promiseCatch = e => { - console.log(e, options); if (e.code === ParseError.INVALID_SESSION_TOKEN && options?.clearSession) { return; } throw e; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/CoreManager.ts(1 hunks)src/ParseUser.ts(2 hunks)src/__tests__/ParseUser-test.js(1 hunks)types/CoreManager.d.ts(1 hunks)types/ParseUser.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
types/CoreManager.d.ts (2)
src/RESTController.ts (1)
RequestOptions(8-21)types/RESTController.d.ts (1)
RequestOptions(1-14)
types/ParseUser.d.ts (2)
src/RESTController.ts (1)
RequestOptions(8-21)types/RESTController.d.ts (1)
RequestOptions(1-14)
src/__tests__/ParseUser-test.js (4)
src/__tests__/Storage-test.js (2)
Storage(277-277)CoreManager(6-6)src/CoreManager.ts (1)
RESTController(135-145)types/CoreManager.d.ts (1)
RESTController(126-130)src/__tests__/ParseError-test.js (1)
ParseError(4-4)
src/CoreManager.ts (2)
src/RESTController.ts (1)
RequestOptions(8-21)types/RESTController.d.ts (1)
RequestOptions(1-14)
🪛 ESLint
types/ParseUser.d.ts
[error] 401-401: Expected indentation of 2 spaces but found 4.
(indent)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (Node 20, 20.15.1)
- GitHub Check: build (Node 22, 22.4.1)
- GitHub Check: build (Node 18, 18.20.4)
🔇 Additional comments (9)
src/CoreManager.ts (1)
200-200: Clear and appropriate parameter extension for logout.Adding the optional
clearSessionboolean to thelogOutmethod signature in theUserControllerinterface allows for more flexible session handling during logout, particularly when dealing with invalid session tokens.types/CoreManager.d.ts (1)
181-183: Type declaration matches implementation.The declaration file correctly reflects the changes made to the
UserControllerinterface in the source file, ensuring type safety across the codebase.types/ParseUser.d.ts (1)
395-396: Clear documentation for the new option.The JSDoc comment effectively explains the purpose of the
clearSessionparameter, making it clear for developers how to use this feature.src/__tests__/ParseUser-test.js (3)
1044-1059: Good test coverage for basic clearSession functionality.This test case verifies that when
clearSession: trueis provided, the logout process completes successfully even when the server responds with an invalid session token error.
1061-1076: Comprehensive test for clearSession with explicit sessionToken.This test confirms that the
clearSessionflag works correctly even when a specific session token is provided in the options, which is important for non-current user logout scenarios.
1078-1098: Thorough validation of error handling with clearSession: false.This test verifies that when
clearSessionis explicitly set to false, the invalid session token error is properly propagated to the caller, maintaining backward compatibility with existing error handling patterns.src/ParseUser.ts (3)
821-821: LGTM! Method signature properly updatedThe method signature correctly includes the new
clearSessionoptional parameter with appropriate typing.
1220-1220: LGTM! Error handling properly implementedThe error handler is correctly applied to handle invalid session token errors when the clearSession option is set.
1228-1233: LGTM! Consistent error handling for current user logoutThe same error handling approach is consistently applied to the current user logout scenario, ensuring the feature works in both contexts.
|
I understand that with the new option this PR introduces, the session token will be removed if the server returns that it's invalid, but:
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
Pull Request
Issue
Currently, if trying to logout with an invalid session token, you will need to catch the error, and call logout again. It can be a bit tedious.
Closes: #307
Approach
Improves clearing session by allowing
Parse.User.logOut({ clearSession: true }), which resolves even if invalid session token is returned.Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation