-
Notifications
You must be signed in to change notification settings - Fork 611
'Delete user' - BACK #152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
'Delete user' - BACK #152
Conversation
hyunnbunt
commented
Jul 24, 2025
- Connect '/auth/self/deactivate-user' API request to a new module 'deactivateSelfUser'
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Thank you for your submission, we really appreciate it! ❤️ Like many open-source projects, we ask that you sign our Individual Contributor License Agreement before we can accept your contribution. If you are contributing on behalf of a company, please contact us at [email protected] to sign a Corporate Contributor License Agreement. You can sign the individual CLA by posting a comment with the below text. I have read, agree to, and hereby sign Plasmic's Individual Contributor License Agreement You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add a test for delete in routes.spec.ts
sensitiveRateLimiter, | ||
withNext(authRoutes.updateSelfPassword) | ||
); | ||
app.post( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use app.delete("/api/v1/auth/self")
as the route
} | ||
|
||
export async function deactivateSelfUser(req: Request, res: Response) { | ||
const mgr = superDbMgr(req); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would allow anyone to delete anyone in the database. Only the currently logged in user should be able to delete themselves. You need to use checkUserIdIsSelf
to get the user ID.
|
||
export async function deactivateSelfUser(req: Request, res: Response) { | ||
const mgr = superDbMgr(req); | ||
const email = req.body.email; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should not be a user identifier passed to the server, since the server already has access to the cookie that identifies the user.
res.clearCookie("plasmic-observer"); | ||
// Must reset the session to prevent session fixation attacks, reset the CSRF | ||
// token, etc. | ||
if (req.session) { | ||
await util.promisify(req.session.destroy.bind(req.session))(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just call doLogout
instead. Can you also move the highlighted lines into the doLogout
function?
); | ||
} | ||
|
||
export async function deactivateSelfUser(req: Request, res: Response) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to deleteSelf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I will merge this, no need to make the change in the comment
export async function deleteSelf(req: Request, res: Response) { | ||
const mgr = userDbMgr(req); | ||
|
||
const id = getUser(req, { allowUnverifiedEmail: true }).id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you can call checkUserIdIsSelf()
without an argument, it will return the current user ID of the DbMgr
. So getUser
should be unnecessary.
* Squash #152 Clear cookie after deleting user Change route POST -> DELETE Call doLogout inside deleteUser Switch the order of session ending and logout request Change doLogout to take Response and clear cookie inside Add selfUser verification Change code lines * Squash #154 Add menu, confirm (cherry picked from commit 06e291b) Add deactivateSelfUser (cherry picked from commit d5e9bb4) Delete front code (cherry picked from commit 16e906f) Redirect to login page (cherry picked from commit 785a527) Clear cookie after deleting user (cherry picked from commit dc6fb6e) Change route POST -> DELETE (cherry picked from commit deb40b1) Call doLogout inside deleteUser (cherry picked from commit 1c49a38) Change doLogout to take Response and clear cookie inside (cherry picked from commit 07575d2) Add selfUser verification (cherry picked from commit f1c55c9) Move deactivateUser after sendEmailVerification (cherry picked from commit 70edba0) Update Modal messages (cherry picked from commit eacedcd) Change code lines (cherry picked from commit 80bbfdb) Check if user has ownership (cherry picked from commit 6f83dee) Throw ForbiddenError (cherry picked from commit 180c3c4) Delete account request catch ForbiddenError (cherry picked from commit 60a1f89) New Error type: UserHasTeamOwnershipError (cherry picked from commit 323c85f) Removed wrong import (cherry picked from commit c72a251) Check isUserHasTeamOwnershipError (cherry picked from commit ce9c110) Simplified some code lines (cherry picked from commit 0b33561) * sync [email protected] * delete self fixes * add tests --------- Co-authored-by: hyunnbunt <[email protected]> GitOrigin-RevId: 9d897acc0bed4a7a008982429b9411a1c83f546f
Merged in 316a33c |