Increase role http error and test quality #222
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This updates access check requests to ensure the requested action exists in the policy for the provided resource beforce executing the request to spicedb and failing for this error. Knowing this we can return a relevant error (ErrInvalidAction) which we can now handle and return a 400 Bad Request status code instead of the previous 500 Internal Server Error status code we were returning due to the spicedb error we were receiving before. Signed-off-by: Mike Mason <[email protected]>
mikemrm
changed the title
mikemrm
force-pushed
the
handle-request-errors
branch
from
87b0968
to
bb07b79
Compare
Context Canceled errors from http requests should not produce 500 errors as those are client requests. This change adds an error middleware which can capture the canceled context error and return a 422 status instead. Signed-off-by: Mike Mason <[email protected]>
This validates create/update role action requests before submitting the request to spicedb and handles the errors gracefully by returning 400 errors instead of generic 500 errors to the client. Signed-off-by: Mike Mason <[email protected]>
mikemrm
force-pushed
the
handle-request-errors
branch
5 times, most recently
from
f6533cf
to
f258fc5
Compare
mikemrm
changed the title
mikemrm
changed the title
This handles role http endpoint errors better by better handling the errors returned and setting the response status codes. Additionally this adds testing to the http api so we may test these http status codes to ensure they are working as we expect. Signed-off-by: Mike Mason <[email protected]>
This adds http tests and fixes the http status codes for the role assignment and unassignment endpoints to resolve 500 errors occurring when provided role ids were didn't exist. Signed-off-by: Mike Mason <[email protected]>
mikemrm
force-pushed
the
handle-request-errors
branch
from
f258fc5
to
a61e1f6
Compare
jacobsee
reviewed
Mar 15, 2024
| switch errType { | ||
| case "": | ||
| case "echo": | ||
| return echo.ErrTeapot |
jacobsee
approved these changes
Mar 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This begins adding better request error handling.
Adds action validation checking against the policy before spending cycles hitting spicedb. This allows us to return better errors, 400's instead of 500's.
Also handles client disconnects gracefully. If the client disconnects, context canceled errors are captured and status is changed to 422's instead of an incorrect 500.
This also adds http tests for roles which resulted in the testauth package to simplify tests as auth is required in the http endpoints. This package will probably at some point be moved into the infratographer/x repo. But for now, while we develop the requirements it will be easier to develop it within the needed repo.