-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(medusa): allow users deletion #13960
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 8 Skipped Deployments
|
🦋 Changeset detectedLatest commit: f1f8252 The changes in this PR will be included in the next version bump. This PR includes changesets to release 74 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 |
| throw new MedusaError( | ||
| MedusaError.Types.NOT_ALLOWED, | ||
| "You are not allowed to delete other users" | ||
| "You can't delete yourself" |
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.
Bug: Unauthenticated Deletion Privilege via Misconfigured Endpoint
The DELETE endpoint has AUTHENTICATE = false which allows unauthenticated requests. When actor_id is undefined (no authentication), the check if (actor_id === id) will always be false, allowing the deletion to proceed. This means an unauthenticated attacker could delete any user by making a DELETE request without credentials. The endpoint should either require authentication or explicitly check that actor_id is defined before performing the comparison.
willbouch
left a comment
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
|
@olivermrbl was approved on Slack but I'll let you merge that one still |
.changeset/dull-plants-create.md
Outdated
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| "integration-tests-http": patch | |||
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.
Todo: you can rm integration tests from here
.changeset/dull-plants-create.md
Outdated
| "@medusajs/medusa": patch | ||
| --- | ||
|
|
||
| Allow users to delete other users and prevent them from deleting themselves |
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.
suggestion: Pr title, this will also show up in the changelog
| throw new MedusaError( | ||
| MedusaError.Types.NOT_ALLOWED, | ||
| "You are not allowed to delete other users" | ||
| "You can't delete yourself" |
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.
| "You can't delete yourself" | |
| "A user cannot delete itself" |
| throw new MedusaError( | ||
| MedusaError.Types.NOT_ALLOWED, | ||
| "You are not allowed to delete other users" | ||
| "A user cannot delete itself" |
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.
nit: "cannot delete themself". Itself is for objects and themself here is a gender-neutral pronoun that replaces "herself" or "himself"
Summary
What — What changes are introduced in this PR?
Allow users to delete other users and prevent them from deleting themselves.
Why — Why are these changes relevant or necessary?
Inability to delete other users causes old users that maybe don't work anymore with the business to have access still.
How — How have these changes been implemented?
Inverted the check in the admin delete user endpoint, to allow users deleting other users but themselves.
Testing — How have these changes been tested, or how can the reviewer test the feature?
Integration tests
Examples
Provide examples or code snippets that demonstrate how this feature works, or how it can be used in practice.
This helps with documentation and ensures maintainers can quickly understand and verify the change.
// Example usageChecklist
Please ensure the following before requesting a review:
yarn changesetand follow the promptsAdditional Context
Add any additional context, related issues, or references that might help the reviewer understand this PR.
closes OPS-97
Note
Enables deleting other users via admin DELETE endpoint while blocking self-deletion, with corresponding integration tests and changeset.
DELETE /admin/users/:idinpackages/medusa/src/api/admin/users/[id]/route.ts:actor_id === idwithNOT_ALLOWEDerror.removeUserAccountWorkflowand return standard delete response.integration-tests/http/__tests__/user/admin/user.spec.ts:app_metadatano longer includesuser_id./admin/users/me)."A user cannot delete itself"..changeset/dull-plants-create.md(patch for@medusajs/medusa).Written by Cursor Bugbot for commit f1f8252. This will update automatically on new commits. Configure here.