refactor(medusa): refactor cart ownership checks and update order list query#837
Merged
lukasz-hycom merged 3 commits intomainfrom Mar 26, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughRemoved Auth.Service injection from CartsService and OrdersService; ownership checks now validate caller identity via Medusa Store API ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as Carts/Orders Service
participant SDK as Medusa SDK
participant Store as Medusa Store API
participant Admin as Medusa Admin API
Note over Client,Service: Flow when authorization token is provided
Client->>Service: request + authorization
Service->>SDK: store.customer.retrieve (with storeHeaders)
SDK->>Store: GET /store/customers/me
Store-->>SDK: { customer: { id: callerId } }
SDK-->>Service: callerId
alt callerId == targetCustomerId
Service-->>Client: allow access / proceed
else callerId != targetCustomerId
Service->>SDK: admin.customer.retrieve (with adminHeaders)
SDK->>Admin: GET /admin/customers/:customerId
Admin-->>SDK: { customer: { id: ..., has_account: bool } }
SDK-->>Service: targetCustomer (has_account)
alt target is guest (has_account == false)
Service-->>Client: allow access
else
Service-->>Client: Unauthorized
end
end
sequenceDiagram
participant Client as Client
participant Service as Carts/Orders Service
participant SDK as Medusa SDK
participant Admin as Medusa Admin API
Note over Client,Service: Flow when no authorization token provided
Client->>Service: request (no authorization)
Service->>SDK: admin.customer.retrieve (with adminHeaders)
SDK->>Admin: GET /admin/customers/:customerId
Admin-->>SDK: { customer: { has_account: bool } }
SDK-->>Service: targetCustomer (has_account)
alt target is guest (has_account == false)
Service-->>Client: allow access
else
Service-->>Client: Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts`:
- Line 7: The test still stubs mockAuthService.getCustomerId() and expects the
unauthorized flow, but CartsService no longer uses Auth; update the spec to
remove reliance on mockAuthService and instead stub the new
sdk.admin.customer.retrieve path: add a mock for sdk.admin.customer.retrieve
(returning either a customer for registered flows or throwing/not-found for
guest), update the unauthorized/guest test to assert guest-cart behavior when
retrieve indicates no customer, and add a new test asserting registered-customer
cart behavior when retrieve returns a customer; locate references to
CartsService, mockAuthService.getCustomerId, and sdk.admin.customer.retrieve to
make these changes.
In `@packages/integrations/medusajs/src/modules/carts/carts.service.ts`:
- Around line 149-164: The cart ownership check was removed, which allows
unauthenticated users to add items to other customers' carts; restore the
verification by invoking the same preflight used previously (call getCart(...)
then verifyResourceAccess(...) with the fetched cart and the current
authorization) before calling this.sdk.store.cart.createLineItem; ensure you
reject the request if verifyResourceAccess fails and only proceed to
createLineItem when access is confirmed (apply this in the method wrapping the
createLineItem call and keep existing error handling via handleHttpError).
In `@packages/integrations/medusajs/src/utils/customer-access.ts`:
- Around line 18-23: The current early-return skips ownership validation when an
Authorization header is present; restore owner checking by removing the bypass
and validating that the authenticated customer ID matches the resource's
customer ID after retrieving the resource via sdk.admin.customer.retrieve(...,
adminHeaders). Specifically, in the function containing the
customerId/authorization logic, do not return early on authorization — instead
call sdk.admin.customer.retrieve(customerId, {}, adminHeaders), then compare the
retrieved customer's id (or customer.id on the resource you fetch) to the
authenticated customer id extracted from the Authorization token and return an
unauthorized/empty response if they don't match; alternatively ensure this
ownership check is enforced in custom auth middleware invoked before this
function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 368869cb-568d-4d34-88ee-36b8d66a45cc
📒 Files selected for processing (5)
packages/integrations/medusajs/src/modules/carts/carts.service.spec.tspackages/integrations/medusajs/src/modules/carts/carts.service.tspackages/integrations/medusajs/src/modules/orders/orders.service.spec.tspackages/integrations/medusajs/src/modules/orders/orders.service.tspackages/integrations/medusajs/src/utils/customer-access.ts
💤 Files with no reviewable changes (1)
- packages/integrations/medusajs/src/modules/orders/orders.service.spec.ts
packages/integrations/medusajs/src/modules/carts/carts.service.ts
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts (1)
79-79: 🛠️ Refactor suggestion | 🟠 MajorRemove dead
mockAuthServicecode.
mockAuthServiceis defined, initialized, and itsgetCustomerIdmethod is stubbed in several tests, but it is never injected intoCartsService(lines 124-129). These stubs have no effect sinceCartsServiceno longer depends onAuth.Service. This makes tests misleading—they appear to configure authorization behavior but actually don't.Clean up by removing
mockAuthServiceentirely and any references to it.🧹 Remove dead mockAuthService code
- let mockAuthService: { getCustomerId: ReturnType<typeof vi.fn> };- mockAuthService = { getCustomerId: vi.fn() };And remove all occurrences of:
- mockAuthService.getCustomerId.mockReturnValue(undefined); - mockAuthService.getCustomerId.mockReturnValue('cust_1');Also applies to: 113-113, 220-220, 248-248, 263-263, 290-290, 315-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts` at line 79, Remove the dead mockAuthService test fixture and all of its uses: delete the declaration "mockAuthService", any vi.fn() stubs for mockAuthService.getCustomerId, and any assignments or calls that configure or reference mockAuthService in the CartsService test setup and individual specs; since CartsService no longer depends on Auth.Service, ensure you also remove any leftover imports or local variables related to Auth or mockAuthService so there are no unused items or misleading stubs remaining in carts.service.spec.ts (look for occurrences of mockAuthService, getCustomerId, and its vi.fn() setup in beforeEach/it blocks and remove them).
🧹 Nitpick comments (1)
packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts (1)
148-181: Consider adding test coverage for additional authorization scenarios.The
getCarttests cover:
- ✅ Public resource (no customer_id)
- ✅ Unauthorized access (customer mismatch with registered user)
- ✅ 404 error handling
Missing coverage for the new
verifyResourceAccesslogic:
- Authenticated user accessing their own cart (customer IDs match)
- Authenticated user accessing a guest cart (should succeed for cart merge)
- Unauthenticated access to a guest cart (guest checkout flow)
- Unauthenticated access to a registered user's cart (should fail)
🧪 Example test for authenticated owner access
it('should allow access when authenticated customer owns the cart', async () => { mockSdk.store.cart.retrieve.mockResolvedValue({ cart: { ...minimalCart, customer_id: 'cust_1' } }); mockSdk.store.customer.retrieve.mockResolvedValue({ customer: { id: 'cust_1' } }); const result = await firstValueFrom(service.getCart({ id: 'cart_1' }, 'Bearer token')); expect(result?.id).toBe('cart_1'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts` around lines 148 - 181, Add tests exercising the verifyResourceAccess branches in the getCart suite: create one test where mockSdk.store.cart.retrieve returns a cart with customer_id 'cust_1' and mockSdk.store.customer.retrieve returns { id: 'cust_1' } to assert getCart succeeds for an authenticated owner; one test where the cart has no customer_id (guest cart) and an authenticated user calls getCart to assert success (simulate merge path); one test where cart has no customer_id and no auth token to assert guest access succeeds; and one test where cart has customer_id 'cust_other' and no auth (or mismatched auth) to assert getCart throws UnauthorizedException. Use the existing helpers/mocks (mockSdk.store.cart.retrieve, mockSdk.store.customer.retrieve, mockSdk.admin.customer.retrieve, minimalCart) and assert either a successful result.id or that the call rejects with UnauthorizedException as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts`:
- Line 79: Remove the dead mockAuthService test fixture and all of its uses:
delete the declaration "mockAuthService", any vi.fn() stubs for
mockAuthService.getCustomerId, and any assignments or calls that configure or
reference mockAuthService in the CartsService test setup and individual specs;
since CartsService no longer depends on Auth.Service, ensure you also remove any
leftover imports or local variables related to Auth or mockAuthService so there
are no unused items or misleading stubs remaining in carts.service.spec.ts (look
for occurrences of mockAuthService, getCustomerId, and its vi.fn() setup in
beforeEach/it blocks and remove them).
---
Nitpick comments:
In `@packages/integrations/medusajs/src/modules/carts/carts.service.spec.ts`:
- Around line 148-181: Add tests exercising the verifyResourceAccess branches in
the getCart suite: create one test where mockSdk.store.cart.retrieve returns a
cart with customer_id 'cust_1' and mockSdk.store.customer.retrieve returns { id:
'cust_1' } to assert getCart succeeds for an authenticated owner; one test where
the cart has no customer_id (guest cart) and an authenticated user calls getCart
to assert success (simulate merge path); one test where cart has no customer_id
and no auth token to assert guest access succeeds; and one test where cart has
customer_id 'cust_other' and no auth (or mismatched auth) to assert getCart
throws UnauthorizedException. Use the existing helpers/mocks
(mockSdk.store.cart.retrieve, mockSdk.store.customer.retrieve,
mockSdk.admin.customer.retrieve, minimalCart) and assert either a successful
result.id or that the call rejects with UnauthorizedException as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e0fa77a-7918-4d1b-9818-99115f1e86ef
📒 Files selected for processing (5)
packages/integrations/medusajs/src/modules/carts/carts.service.spec.tspackages/integrations/medusajs/src/modules/carts/carts.service.tspackages/integrations/medusajs/src/modules/orders/orders.service.spec.tspackages/integrations/medusajs/src/modules/orders/orders.service.tspackages/integrations/medusajs/src/utils/customer-access.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/integrations/medusajs/src/modules/orders/orders.service.ts
- packages/integrations/medusajs/src/modules/orders/orders.service.spec.ts
- packages/integrations/medusajs/src/modules/carts/carts.service.ts
Contributor
Coverage Report for packages/configs/vitest-config
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
marcinkrasowski
approved these changes
Mar 26, 2026
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.
Key Changes
Summary by CodeRabbit
Refactor
Tests
Bug Fixes
Chores