-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix(medusa): resolve user_id from user linked to secret key on draft order edit with api-key auth #14053
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: develop
Are you sure you want to change the base?
fix(medusa): resolve user_id from user linked to secret key on draft order edit with api-key auth #14053
Changes from 6 commits
339d873
faeaf8e
a410cf3
2098d19
bf8ee12
31562b8
1ffdffc
6a6b0b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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" | ||
|
|
||
|
|
@@ -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( | ||
|
|
@@ -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", () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| 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) { | ||
| next() | ||
NicolasGorga marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Middleware: API Key Logic Runs UncheckedMissing |
||
|
|
||
| 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, | ||
adrien2p marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, | ||
| { | ||
| cache: { | ||
| enable: true, | ||
| }, | ||
| } | ||
| ) | ||
|
|
||
| req.secret_key_context = { | ||
NicolasGorga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| created_by: apiKey.created_by, | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| next() | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
@@ -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({ | ||
NicolasGorga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| entity: 'api_key', | ||
| fields: ['created_by'], | ||
| filters: { id: userId }, | ||
| }) | ||
| userId = apiKey.created_by | ||
| } | ||
|
Comment on lines
+43
to
+54
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed I think we could follow something similar to existent
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there is an issue with your linter configuration ;)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for middleware
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pushed commit with middleware, applied it to all
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Route handler duplicates API key query unnecessarilyThe route handler queries the API key to resolve |
||
|
|
||
| await updateDraftOrderWorkflow(req.scope).run({ | ||
| input: { | ||
| ...req.validatedBody, | ||
| user_id: req.auth_context.actor_id, | ||
| user_id: userId, | ||
| id: req.params.id, | ||
| }, | ||
| }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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*", | ||
NicolasGorga marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| middlewares: [ | ||
| setSecretApiKeyContext, | ||
| ] | ||
| }, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Middleware breaks unauthenticated admin routesThe |
||
| ...adminCustomerGroupRoutesMiddlewares, | ||
| ...adminCustomerRoutesMiddlewares, | ||
| ...adminPromotionRoutesMiddlewares, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.