Skip to content
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

Conversation

wanglam
Copy link
Contributor

@wanglam wanglam commented Oct 18, 2024

Description

This PR is for fixing 500 error when import sample data to a not permitted workspace. It should return a forbidden error.

Screenshot

image
image

Testing the changes

  • Clone branch code and run yarn osd bootstrap --single-version loose
  • Add below configs in config/opensearch_dashboards.yml
savedObjects.permission.enabled: true
workspace.enabled: true
uiSettings:
  overrides:
    'home:useNewHomePage': true
opensearchDashboards.dashboardAdmin.users: ['admin']
  • Run yarn start --no-base-path
  • Login with admin user
  • Visit internal users page and create a test-user
  • Visit a existing workspace settings page and add test-user as a read-only collaborator
  • Open an incognito window and login with test-user
  • Goto the sample data page of the same workspace
  • Click "Add data" button, it should show error like screenshot below
  • Should see "403" response in devtools

Changelog

  • feat: [Workspace] Response forbidden error for not permitted workspace

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Copy link

codecov bot commented Oct 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.87%. Comparing base (e09c137) to head (1ee8730).
Report is 4 commits behind head on main.

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     
Flag Coverage Δ
Linux_1 29.08% <ø> (ø)
Linux_2 56.39% <ø> (ø)
Linux_3 37.72% <ø> (+<0.01%) ⬆️
Linux_4 29.81% <100.00%> (+<0.01%) ⬆️
Windows_1 29.09% <ø> (ø)
Windows_2 56.34% <ø> (ø)
Windows_3 37.72% <ø> (ø)
Windows_4 29.81% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -217,6 +218,9 @@ export function createInstallRoute(
} catch (err) {
const errMsg = `bulkCreate failed, error: ${err.message}`;
logger.warn(errMsg);
if (workspaceId && SavedObjectsErrorHelpers.isForbiddenError(err)) {
Copy link
Collaborator

@Hailong-am Hailong-am Oct 18, 2024

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 });

Copy link
Contributor Author

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:

  1. OpenSearch response forbidden error:
    return SavedObjectsErrorHelpers.decorateForbiddenError(error, reason);
  2. Workspace saved object permission wrapper forbidden error:
  3. Data source permission wrapper forbidden error:
    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.

Copy link
Member

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.

@SuZhou-Joe SuZhou-Joe merged commit 8d30da4 into opensearch-project:main Oct 18, 2024
73 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 18, 2024
* Response forbiden error for not permitted workspace

Signed-off-by: Lin Wang <[email protected]>

* Changeset file for PR #8641 created/updated

* Increase test coverage

Signed-off-by: Lin Wang <[email protected]>

---------

Signed-off-by: Lin Wang <[email protected]>
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>
Hailong-am pushed a commit that referenced this pull request Oct 18, 2024
… (#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 <[email protected]>
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>
amsiglan pushed a commit to amsiglan/OpenSearch-Dashboards that referenced this pull request Oct 19, 2024
…search-project#8641)

* Response forbiden error for not permitted workspace

Signed-off-by: Lin Wang <[email protected]>

* Changeset file for PR opensearch-project#8641 created/updated

* Increase test coverage

Signed-off-by: Lin Wang <[email protected]>

---------

Signed-off-by: Lin Wang <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants