oxidecomputer / console

Oxide Web Console
https://console-preview.oxide.computer
Mozilla Public License 2.0
139 stars 11 forks source link

Mock API has no response body on `throw 403` #1798

Open david-crespo opened 1 year ago

david-crespo commented 1 year ago

We have logic in the wrapper for the mock server that detects when a number is thrown and treats it as a status code.

https://github.com/oxidecomputer/console/blob/e5b8d029d48b9d5c9212fbf5156d6a57cb0b5d5b/libs/api/__generated__/msw-handlers.ts#L935-L937

Original oxide.ts source

We use this in 5 spots:

$ rg 'throw \d' libs/api-mocks/msw
libs/api-mocks/msw/util.ts
344: * throw 403 if no.
352: * for the user as well as for the user's groups. Do nothing if yes, throw 403
363:  if (!userHasRole(user, resourceType, resourceId, role)) throw 403

libs/api-mocks/msw/handlers.ts
108:    if (body.name === 'disk-create-500') throw 500
160:    if (disk.name === 'import-start-500') throw 500
166:    // throw 400
175:    if (disk.name === 'import-stop-500') throw 500
189:    // if (Math.random() < 0.01) throw 400
196:    if (disk.name === 'disk-finalize-500') throw 500

It's convenient, but the resulting responses have no body, and in our client-side error handling code, we are looking at error_code in the body rather than the status code to figure out what kind of error it is.

https://github.com/oxidecomputer/console/blob/e5b8d029d48b9d5c9212fbf5156d6a57cb0b5d5b/libs/api/errors.ts#L56-L65

This means that in local dev and tests we get Unknown server error when we should be getting a message more appropriate for a 403.

Screenshot 2023-10-19 at 5 11 36 PM

The simple fix is to just change these spots to use a canned response object like these:

https://github.com/oxidecomputer/console/blob/e5b8d029d48b9d5c9212fbf5156d6a57cb0b5d5b/libs/api-mocks/msw/util.ts#L92-L96

More involved would be to either have the mock API insert a body for certain error codes, or blow up when you try to throw a number so you can't use this shortcut anymore. The former sounds convenient but it would be pretty hard for someone to tell what happens when you throw a 403 if they didn't already know. I think it would be more readable to explicitly throw a canned 403 response.

zephraph commented 1 year ago

I agree w/ taking the simpler approach of using a canned response as we do w/ unavailableErr and NotImplemented. No need for magic here.