Repository navigation
Consistently use correct 4xx codes - #2454
Conversation
|
🔎 A preview deployment of this pull request is available at https://utilities.philanthropydatacommons.org/pdc-service-preview-2454-j3mz4/ (and upstream https://pdc-service-preview-2454-j3mz4.ondigitalocean.app) assuming this PR is still open. 🔍 |
9ea112c to
fa49902
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2454 +/- ##
==========================================
+ Coverage 93.59% 93.66% +0.07%
==========================================
Files 303 303
Lines 3839 3803 -36
Branches 517 488 -29
==========================================
- Hits 3593 3562 -31
+ Misses 245 240 -5
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0cd0852 to
35502c4
Compare
There was a problem hiding this comment.
Pull request overview
This PR standardizes 4xx responses across the service by introducing an explicit 403 Forbidden error type and updating handlers/middleware, OpenAPI docs, and tests to better distinguish authentication (401), authorization (403), missing/unviewable entities (404), conflicts (409), and semantic validation (422).
Changes:
- Added
ForbiddenErrorand updated the central error handler to map it to HTTP 403; updated multiple handlers/middleware to throwForbiddenErrorinstead of401/422in authorization-failure cases. - Updated OpenAPI path docs to include
403/404where relevant and added a reusableForbiddenresponse; clarified the sharedNotFoundresponse description. - Updated integration/unit tests and the changelog to reflect the new status-code behavior.
Reviewed changes
Copilot reviewed 56 out of 56 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/openapi/paths/sources.json | Documents new 403/404 responses for sources endpoint. |
| src/openapi/paths/proposalVersions.json | Documents 403/404 and refines 409/422 descriptions for proposalVersions. |
| src/openapi/paths/proposals.json | Documents 403/404 for proposal creation. |
| src/openapi/paths/permissionGrants.json | Documents 403 for permissionGrants collection endpoint. |
| src/openapi/paths/permissionGrant.json | Documents 403 for permissionGrant item endpoint. |
| src/openapi/paths/opportunities.json | Documents 403/404 for opportunities endpoint. |
| src/openapi/paths/funderCollaborativeMembers.json | Documents 403 for collaborative members list. |
| src/openapi/paths/funderCollaborativeMember.json | Documents 403 for collaborative member item. |
| src/openapi/paths/funderCollaborativeInvitationsSent.json | Documents 403 for invitations-sent list. |
| src/openapi/paths/funderCollaborativeInvitationsReceived.json | Documents 403 for invitations-received list. |
| src/openapi/paths/funderCollaborativeInvitationSent.json | Documents 403/404 for invitation-sent item. |
| src/openapi/paths/funderCollaborativeInvitationReceived.json | Documents 403/404 for invitation-received item. |
| src/openapi/paths/changemakerProposals.json | Documents 403/404/409 for changemakerProposals. |
| src/openapi/paths/changemakerFieldValues.json | Documents 403/404 and refines 409/422 wording. |
| src/openapi/paths/changemakerFieldValueBatches.json | Documents 403/404 for batches endpoint. |
| src/openapi/paths/bulkUploadTasks.json | Updates documented 4xx responses for bulk upload tasks. |
| src/openapi/paths/applicationForms.json | Documents 403 and refines 422 description for applicationForms. |
| src/openapi/paths/applicationFormField.json | Documents 403 for applicationFormField patch. |
| src/openapi/components/responses/NotFound.json | Expands description to include “not viewable” semantics. |
| src/openapi/components/responses/Forbidden.json | Adds reusable Forbidden response component. |
| src/openapi/api.json | Registers the new Forbidden response component. |
| src/middleware/requireFunderPermission.ts | Returns 403 vs 404 by loading funder on permission failure. |
| src/middleware/requireDataProviderPermission.ts | Removes unused middleware. |
| src/middleware/requireChangemakerPermission.ts | Removes unused middleware. |
| src/middleware/index.ts | Stops exporting removed middleware. |
| src/middleware/errorHandler.ts | Maps ForbiddenError to HTTP 403. |
| src/middleware/tests/requireFunderPermission.int.test.ts | Updates expectations to 403/404 behavior. |
| src/middleware/tests/requireDataProviderPermission.int.test.ts | Removes tests for deleted middleware. |
| src/middleware/tests/requireChangemakerPermission.int.test.ts | Removes tests for deleted middleware. |
| src/middleware/tests/errorHandler.unit.test.ts | Adds a unit test ensuring 403 for ForbiddenError. |
| src/handlers/sourcesHandlers.ts | Converts “no permission” cases to 403 (and 404 when org missing). |
| src/handlers/proposalVersionsHandlers.ts | Converts “no permission” cases to 403; loads source on reference denial. |
| src/handlers/proposalsHandlers.ts | Converts “no permission” cases to 403 and lets missing opportunity become 404. |
| src/handlers/permissionGrantsHandlers.ts | Converts “cannot manage grants” cases to 403. |
| src/handlers/opportunitiesHandlers.ts | Converts “no permission” case to 403/404 via loadFunder. |
| src/handlers/changemakerProposalsHandlers.ts | Converts permission failures to 403 and missing proposal to 404. |
| src/handlers/changemakerFieldValuesHandlers.ts | Converts permission failures to 403; missing changemaker/batch to 404. |
| src/handlers/changemakerFieldValueBatchesHandlers.ts | Converts “no reference permission” to 403/404 via loadSource. |
| src/handlers/bulkUploadTasksHandlers.ts | Converts “no permission” cases to 403/404 and keeps ownership as 422. |
| src/handlers/applicationFormsHandlers.ts | Converts “no permission” case to 403 and missing opportunity to 404. |
| src/handlers/applicationFormFieldsHandlers.ts | Converts “no permission” case to 403. |
| src/errors/index.ts | Exports ForbiddenError. |
| src/errors/ForbiddenError.ts | Adds new error class used for 403 responses. |
| src/tests/sources.int.test.ts | Updates expected status codes/messages for sources. |
| src/tests/proposalVersions.int.test.ts | Updates expected status codes/messages for proposalVersions. |
| src/tests/proposals.int.test.ts | Updates/extends tests for 403 vs 404 opportunity cases. |
| src/tests/permissionGrants.int.test.ts | Updates expected 401→403 behaviors for permissionGrants. |
| src/tests/opportunities.int.test.ts | Updates expected 401→403 and adds 404 case. |
| src/tests/funders.int.test.ts | Updates expected 401→403 and adds 404 case. |
| src/tests/changemakerProposals.int.test.ts | Updates expected 422→403 and missing proposal to 404. |
| src/tests/changemakerFieldValues.int.test.ts | Updates expected 422/409 to 403/404 where appropriate. |
| src/tests/changemakerFieldValueBatches.int.test.ts | Updates expected 422 to 403/404 and adds view-vs-reference test setup. |
| src/tests/bulkUploadTasks.int.test.ts | Updates expected 422→403 and adds 404 coverage for missing application form. |
| src/tests/applicationForms.int.test.ts | Updates expected 401→403 and missing opportunity to 404. |
| src/tests/applicationFormFields.int.test.ts | Updates expected 401→403 for insufficient permission. |
| CHANGELOG.md | Documents the updated status-code behavior under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'm disappointed that conditional return of HTTP status code 418 is not included in this change, but I'm not going to block approval on it. |
hminsky2002
left a comment
There was a problem hiding this comment.
This all looks consistent to me, thanks for splitting things up into individual commits!
I'm taking all tests passing as assurance that these behave as we expect them to (given that we're changing the status code returns in our handlers, and we have tests that verify exactly that).
| throw error; | ||
| const opportunity = await loadOpportunity(db, req, opportunityId); | ||
| if ( | ||
| !(await hasOpportunityPermission(db, req, { |
There was a problem hiding this comment.
Nice to see the immediate benefits of this PR in terms of code reduction!
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
The `requireChangemakerPermission` and `requireDataProviderPermission` middleware were never wired into any router; changemaker and data provider routes are protected by `requireAdministratorRole` instead. Remove the dead middleware and their tests rather than migrating them to `ForbiddenError`. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see, and NotFoundError (HTTP 404) when they cannot see it at all. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see, and NotFoundError (HTTP 404) when they cannot see it at all. The changemaker and batch are now loaded directly so a missing (or unviewable) one yields 404 rather than 409. The base field remains a 409 conflict because base fields are public reference data. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see, and NotFoundError (HTTP 404) when they cannot see it at all. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see, and NotFoundError (HTTP 404) when they cannot see it at all. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see, and NotFoundError (HTTP 404) when they cannot see it at all. The proposal is now loaded directly so a missing (or unviewable) one yields 404 rather than 409, and the source reference check yields 403/404. Application form and field relationship checks remain 409. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see, and NotFoundError (HTTP 404) when they cannot see it at all. The application form, opportunity, and source are now loaded directly so a missing (or unviewable) one yields 404 rather than a masked 422. The file-ownership checks remain 422. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see. Managing a permission grant on a context entity the user cannot manage now returns 403 instead of 401. The by-id lookup already returns 404 when the grant cannot be managed. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
We want to use ForbiddenError (HTTP 403) when users have auth but are not allowed to perform an action on an item they are allowed to see, and NotFoundError (HTTP 404) when they cannot see it at all. Creating an application form for an opportunity now returns 403 when the user lacks permission and 404 when the opportunity cannot be viewed or does not exist. Editing an application form field returns 403 instead of 401. Issue #2421 Endpoints should return 403 (or 404) instead 401 when credentials without proper authorization are provided
|
Added some more tests since codecov flagged missing lines. |
This PR updates all of our endpoints to use the correct 4xx code for the circumstance. Specifically:
401:: authentication is required but a properly formatted auth token was not provided.403:: you do not have the rights to perform the action you are attempting to perform.404:: you are attempting to perform an action involving an entity that doesn't exist (or that you are unable to view).409:: you are attempting to perform an action that would result in an inconsistent data state.422:: you are attempting to perform an action but provided input that does not pass semantic validation.Resolves #2421
Resolves #1598