Skip to content
Open
Show file tree
Hide file tree
Changes from all 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).then(user => 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 use the secret key linked user to set created_by", 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
1 change: 1 addition & 0 deletions packages/core/framework/src/http/middlewares/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export * from "./apply-default-filters"
export * from "./apply-params-as-filters"
export * from "./clear-filters-by-key"
export * from "./set-context"
export * from "./set-secret-api-key-context"
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import {
ContainerRegistrationKeys,
MedusaError,
MedusaErrorTypes,
} from "@medusajs/utils"
import {
AuthenticatedMedusaRequest,
MedusaNextFunction,
MedusaResponse,
} from "../types"

export async function setSecretApiKeyContext(
req: AuthenticatedMedusaRequest,
_: MedusaResponse,
next: MedusaNextFunction
) {
if (!req.auth_context) {
throw new MedusaError(
MedusaErrorTypes.INVALID_DATA,
"No `auth_context` found in request, make sure to apply this middleware after the `authenticate` middleware."
)
}

const shouldSkip = req.auth_context.actor_type !== "api-key"
if (shouldSkip) {
return next()
}
Copy link

Choose a reason for hiding this comment

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

Bug: Middleware: API Key Logic Runs Unchecked

Missing return statement after calling next() when skipping non-api-key requests. This causes the middleware to continue executing the api_key query logic for all request types, not just api-key authenticated requests. Non-api-key requests will query the api_key table unnecessarily and potentially call next() twice.

Fix in Cursor Fix in Web


const query = req.scope.resolve(ContainerRegistrationKeys.QUERY)
const {
data: [apiKey],
} = await query.graph(
{
entity: "api_key",
fields: ["created_by"],
filters: {
id: req.auth_context.actor_id,
},
withDeleted: true,
},
{
cache: {
enable: true,
},
}
)

if (!apiKey) {
throw new MedusaError(
MedusaErrorTypes.NOT_FOUND,
`API key with id ${req.auth_context.actor_id} not found`
)
}

req.secret_key_context = {
created_by: apiKey.created_by,
}
next()
}
5 changes: 5 additions & 0 deletions packages/core/framework/src/http/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,17 @@ export interface PublishableKeyContext {
sales_channel_ids: string[]
}

export interface SecretKeyContext {
created_by: string
}

export interface AuthenticatedMedusaRequest<
Body = unknown,
QueryFields = Record<string, unknown>
> extends MedusaRequest<Body, QueryFields> {
auth_context: AuthContext
publishable_key_context?: PublishableKeyContext
secret_key_context?: SecretKeyContext
}

export interface MedusaStoreRequest<
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
7 changes: 7 additions & 0 deletions packages/medusa/src/api/middlewares.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,16 @@ import { storeReturnReasonRoutesMiddlewares } from "./store/return-reasons/middl
import { storeShippingOptionRoutesMiddlewares } from "./store/shipping-options/middlewares"
import { adminShippingOptionTypeRoutesMiddlewares } from "./admin/shipping-option-types/middlewares"
import { adminIndexRoutesMiddlewares } from "./admin/index/middlewares"
import { setSecretApiKeyContext } from "@medusajs/framework"

export default defineMiddlewares([
...storeRoutesMiddlewares,
{
matcher: "/admin*",
middlewares: [
setSecretApiKeyContext,
]
},
Copy link

Choose a reason for hiding this comment

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

Bug: Middleware breaks unauthenticated admin routes

The setSecretApiKeyContext middleware is registered for all /admin* routes but throws an error when req.auth_context is missing. Some admin routes have opted out of authentication via export const AUTHENTICATE = false (e.g., /admin/feature-flags). For these routes, the global auth middleware skips setting auth_context, causing setSecretApiKeyContext to throw "No auth_context found in request" and breaking unauthenticated admin endpoints.

Fix in Cursor Fix in Web

...adminCustomerGroupRoutesMiddlewares,
...adminCustomerRoutesMiddlewares,
...adminPromotionRoutesMiddlewares,
Expand Down
Loading