-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Unboomify SOR space-auth errors #241391
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: main
Are you sure you want to change the base?
Unboomify SOR space-auth errors #241391
Conversation
| return Boolean(doc && 'found' in doc); | ||
| } | ||
|
|
||
| export function isForbiddenSpacesError(error: unknown): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeramysoucy FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Errors are usually defined in src/core/packages/saved-objects/server/src/saved_objects_error_helpers.ts and then given a code like CODE_BAD_REQUEST so that we can accurately test against them.
If we only test against the status code I'm afraid that not all errors that pass isForbiddenSpacesError are actually because of a spaces forbidden error.
This might be harder because it's coming from the spacesExtension but I think we should try to move towards errors that communicate the problem in domain language instead of generic 403 status codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we only test against the status code I'm afraid that not all errors that pass isForbiddenSpacesError are actually because of a spaces forbidden error
Perhaps moving the error handling into the spacesExtension.getSearchableNamespaces(options.namespaces) method would be a better approach.
Actually, we can keep it here, we just need a new space-specific error in the saved objects error helpers.
|
@elasticmachine merge upstream |
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]
|
At some point, depending directly on
Boomfor error handling snuck into the SOR APIs, which leaves the SOR and API consumers vulnerable to breaking changes from@hapiupgrades.This PR introduces an error helper in much the same way as other errors are handled in the SOR to minimise direct imports from framework packages.