-
Notifications
You must be signed in to change notification settings - Fork 884
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
[Workspace]Response forbidden error for not permitted workspace #8641
[Workspace]Response forbidden error for not permitted workspace #8641
Conversation
Signed-off-by: Lin Wang <wonglam@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8641 +/- ##
=======================================
Coverage 60.86% 60.87%
=======================================
Files 3790 3790
Lines 90222 90224 +2
Branches 14144 14145 +1
=======================================
+ Hits 54918 54925 +7
+ Misses 31838 31834 -4
+ Partials 3466 3465 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Lin Wang <wonglam@amazon.com>
@@ -217,6 +218,9 @@ export function createInstallRoute( | |||
} catch (err) { | |||
const errMsg = `bulkCreate failed, error: ${err.message}`; | |||
logger.warn(errMsg); | |||
if (workspaceId && SavedObjectsErrorHelpers.isForbiddenError(err)) { |
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.
do we have any other error except forbidden error? should we use whatever code the error has
const status = error && error[code] || 400;
return res.customError({ body: errMsg, statusCode: status });
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.
I've searched the whole org, there are three places will generate ForbiddenError:
- OpenSearch response forbidden error:
OpenSearch-Dashboards/src/core/server/saved_objects/service/lib/decorate_opensearch_error.ts
Line 80 in 32b11de
return SavedObjectsErrorHelpers.decorateForbiddenError(error, reason); - Workspace saved object permission wrapper forbidden error:
Line 47 in 32b11de
SavedObjectsErrorHelpers.decorateForbiddenError( - Data source permission wrapper forbidden error:
Line 163 in 32b11de
SavedObjectsErrorHelpers.decorateForbiddenError(
I think it's OK to response 403 for case 2 and 3, seems they are user permission related issues.
For case 1, the server response 500 in the old implementation. The current implementation will return 403.
We may need to check the error message if we want to distinguish these cases. Do you have other suggestions? I think response 403 would be OK for case 1.
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.
Nit: Why do we check if there is workspaceId present? I think returning 403 error if the error is a forbidden error seems straightforward to me.
* Response forbiden error for not permitted workspace Signed-off-by: Lin Wang <wonglam@amazon.com> * Changeset file for PR #8641 created/updated * Increase test coverage Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 8d30da4) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
… (#8647) * Response forbiden error for not permitted workspace * Changeset file for PR #8641 created/updated * Increase test coverage --------- (cherry picked from commit 8d30da4) Signed-off-by: Lin Wang <wonglam@amazon.com> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…search-project#8641) * Response forbiden error for not permitted workspace Signed-off-by: Lin Wang <wonglam@amazon.com> * Changeset file for PR opensearch-project#8641 created/updated * Increase test coverage Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
…search-project#8641) * Response forbiden error for not permitted workspace Signed-off-by: Lin Wang <wonglam@amazon.com> * Changeset file for PR opensearch-project#8641 created/updated * Increase test coverage Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Description
This PR is for fixing 500 error when import sample data to a not permitted workspace. It should return a forbidden error.
Screenshot
Testing the changes
yarn osd bootstrap --single-version loose
config/opensearch_dashboards.yml
yarn start --no-base-path
Changelog
Check List
yarn test:jest
yarn test:jest_integration