Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/hungry-oranges-find.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@medusajs/medusa": patch
---

fix(medusa): resolve user_id from user linked to secret key on draft order edit with api-key auth
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { medusaIntegrationTestRunner } from "@medusajs/test-utils"
import { HttpTypes } from "@medusajs/types"
import { ModuleRegistrationName, ProductStatus } from "@medusajs/utils"
import { ApiKeyType, ModuleRegistrationName, ProductStatus } from "@medusajs/utils"
import { adminHeaders, createAdminUser, } from "../../../../helpers/create-admin-user"
import { setupTaxStructure } from "../../../../modules/__tests__/fixtures"

Expand All @@ -14,12 +14,14 @@ medusaIntegrationTestRunner({
let testDraftOrder: HttpTypes.AdminDraftOrder
let shippingOption: HttpTypes.AdminShippingOption
let shippingOptionHeavy: HttpTypes.AdminShippingOption
let apiKey: HttpTypes.AdminApiKeyResponse['api_key']
let userId: string

beforeEach(async () => {
const container = getContainer()

await setupTaxStructure(container.resolve(ModuleRegistrationName.TAX))
await createAdminUser(dbConnection, adminHeaders, container)
userId = (await createAdminUser(dbConnection, adminHeaders, container)).user.id

region = (
await api.post(
Expand Down Expand Up @@ -217,6 +219,36 @@ medusaIntegrationTestRunner({
expect(response.status).toBe(200)
expect(response.data.draft_order.email).toBe("[email protected]")
})

it("should set created_by to id of user linked to secret key", async () => {
apiKey = (await api.post('/admin/api-keys', {
title: 'secret-key',
type: ApiKeyType.SECRET
}, adminHeaders)).data.api_key

const draftOrderResponse = await api.post(
`/admin/draft-orders/${testDraftOrder.id}`,
{
email: "[email protected]",
},
{
auth: {
username: apiKey.token,
},
}
)

expect(draftOrderResponse.status).toBe(200)
expect(draftOrderResponse.data.draft_order.email).toBe("[email protected]")

const orderChange = (await api.get(`/admin/orders/${testDraftOrder.id}/changes`, adminHeaders)).data.order_changes[0]

expect(orderChange).toEqual(
expect.objectContaining({
created_by: userId
})
)
})
})

describe("DELETE /draft-orders/:id", () => {
Expand Down
16 changes: 15 additions & 1 deletion packages/medusa/src/api/admin/draft-orders/[id]/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
AuthenticatedMedusaRequest,
MedusaRequest,
MedusaResponse,
AuthType
} from "@medusajs/framework/http"
import { HttpTypes } from "@medusajs/framework/types"
import { ContainerRegistrationKeys } from "@medusajs/framework/utils"
Expand Down Expand Up @@ -39,10 +40,23 @@ export const POST = async (
) => {
const query = req.scope.resolve(ContainerRegistrationKeys.QUERY)

let userId = req.auth_context.actor_id
const authType = req.auth_context.actor_type as AuthType

const shouldResolveUser = authType === 'api-key'
if (shouldResolveUser) {
const {data: [apiKey]} = await query.graph({
entity: 'api_key',
fields: ['created_by'],
filters: { id: userId },
})
userId = apiKey.created_by
}
Comment on lines +43 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: This seems like something that should live in a middleware, as this is not really specific to draft orders ... think we should consider doing that instead

Also, this code does not follow our linting conventions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed I think we could follow something similar to existent publishable_key_context for secret_key_context and add the created_by prop in the new middleware, I started with this approach. Oh yes, forgot to run the formatter, will do :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is an issue with your linter configuration ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for middleware

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed commit with middleware, applied it to all /admin routes, could you let me know:

  • if the middleware looks good to you
  • if the way i applied to all admin routes is the correct way

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Route handler duplicates API key query unnecessarily

The route handler queries the API key to resolve created_by even though the setSecretApiKeyContext middleware already sets this value in req.secret_key_context.created_by. This duplicates the database query and could cause inconsistencies since the route query lacks withDeleted: true while the middleware includes it. The route should use req.secret_key_context?.created_by instead of performing its own query.

Fix in Cursor Fix in Web


await updateDraftOrderWorkflow(req.scope).run({
input: {
...req.validatedBody,
user_id: req.auth_context.actor_id,
user_id: userId,
id: req.params.id,
},
})
Expand Down