[workspace] refactor: refactor the bulk_get handler in permission wrapper when item has permission error #8906

Qxisylolo · 2024-11-21T06:22:37Z

Description

This pr fixes index pattern fetch error in discover dataset modal by return response with error instead of throwing an error

Screenshot

before:

after:

Testing the changes

Changelog

feat: refactor the bulk_get handler in permission wrapper when item has permission error

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

codecov · 2024-11-21T06:50:21Z

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.95%. Comparing base (5f91e37) to head (12f1594).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
..._objects/workspace_saved_objects_client_wrapper.ts	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8906      +/-   ##
==========================================
+ Coverage   60.91%   60.95%   +0.03%     
==========================================
  Files        3808     3809       +1     
  Lines       91196    91275      +79     
  Branches    14400    14414      +14     
==========================================
+ Hits        55556    55635      +79     
- Misses      32085    32086       +1     
+ Partials     3555     3554       -1

Flag	Coverage Δ
Linux_1	`28.97% <81.81%> (-0.03%)`	⬇️
Linux_2	`56.45% <ø> (+0.06%)`	⬆️
Linux_3	`37.97% <ø> (?)`
Linux_4	`29.03% <ø> (+0.01%)`	⬆️
Windows_1	`29.00% <81.81%> (-0.03%)`	⬇️
Windows_2	`56.40% <ø> (+0.06%)`	⬆️
Windows_3	`37.97% <ø> (-0.05%)`	⬇️
Windows_4	`29.03% <ø> (+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.

raintygao · 2024-11-21T09:20:02Z

changelogs/fragments/8906.yml

@@ -0,0 +1,2 @@
+fix:
+- Index pattern fetch error in discover dataset modal ([#8906](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8906))


Although this PR may could fix #8891, but this is not the direct root cause, so I would suggest to change to refactor: refactor the bulk_get handler in permission wrapper when item has permission error

raintygao · 2024-11-21T09:28:24Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

+            if (!hasPermission) {
+              ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);
+              throw generateDataSourcePermissionError();
+            }


We may also don't need to throw Error for this verification.

raintygao · 2024-11-21T09:32:38Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

+              error: {
+                error: 'Invalid saved objects permission',
+                statusCode: 403,
+                message: 'Permission denied',
+              },


Could we extract this as a function along with other generatexxxError functions on the top of this file.

raintygao · 2024-11-21T09:34:23Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

      ACLAuditor?.increment(
        ACLAuditorStateKey.VALIDATE_SUCCESS,
        objectToBulkGet.saved_objects.length
      );


This represents all verification are successful, now in the new implementation we may need a flag in the map to indicate whether all objects are passed.

Thanks for all the comments, updated.

ruanyl · 2024-12-18T08:02:33Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

@@ -196,6 +204,32 @@ export class WorkspaceSavedObjectsClientWrapper {
    return hasPermission;
  }

+  // Data source is a workspace level object, validate if the request has access to the data source within the requested workspace.
+  private validateDataSourcePermissions = (


@yubonluo Could you please check if this change is compatible with the bulkGet changes you made recently regarding permission check?

Sure, the data source validation has been moved to id_consumer_wrapper, you can delete the method from the permission_wrapper.
related PR: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8888/files#diff-6f4daf9988f5262d2403821efd458faf7157c7e7473ec65a16ae2724a92d6556L209

SuZhou-Joe · 2024-12-27T08:10:49Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

+      let flag = true;
+      const processedObjects = await Promise.all(
+        objectToBulkGet.saved_objects.map(async (object) => {
+          try {


Do we have to wrap the code with a try catch? I raised the concern because it seems it won't throw error and it makes it hard to understand the code here.

Thanks the comment, validateIsWorkspaceDataSourceAndConnectionObjectType is unlikely to throw an exception. I will update it.

SuZhou-Joe · 2024-12-27T08:16:07Z

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

+        objectToBulkGet.saved_objects.map(async (object) => {
+          try {
+            if (validateIsWorkspaceDataSourceAndConnectionObjectType(object.type)) {
+              const hasPermission = this.validateDataSourcePermissions(


It seems we do not need to validate data source permissions in bulkGet anymore, it has been done in another wrapper.

Could you elaborate on which wrapper performed the validation?

the data source validation has been moved to id_consumer_wrapper, you can delete the method from the permission_wrapper.
related PR: https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8888/files#diff-6f4daf9988f5262d2403821efd458faf7157c7e7473ec65a16ae2724a92d6556L209

Thanks for the clarification！！

Signed-off-by: Qxisylolo <[email protected]>

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

Signed-off-by: Qxisylolo <[email protected]>

…pper when item has permission error (#8906) * return response with error Signed-off-by: Qxisylolo <[email protected]> * set Id Signed-off-by: Qxisylolo <[email protected]> * resolve tests Signed-off-by: Qxisylolo <[email protected]> * Changeset file for PR #8906 created/updated * typo Signed-off-by: Qxisylolo <[email protected]> * fix integration tests Signed-off-by: Qxisylolo <[email protected]> * add try catch Signed-off-by: Qxisylolo <[email protected]> * resolve conflicts Signed-off-by: Qxisylolo <[email protected]> * delete data source permission Signed-off-by: Qxisylolo <[email protected]> * add try catch Signed-off-by: Qxisylolo <[email protected]> --------- Signed-off-by: Qxisylolo <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit d947bd6) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…pper when item has permission error (#8906) (#9156) * return response with error * set Id * resolve tests * Changeset file for PR #8906 created/updated * typo * fix integration tests * add try catch * resolve conflicts * delete data source permission * add try catch --------- (cherry picked from commit d947bd6) Signed-off-by: Qxisylolo <[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>

…pper when item has permission error (opensearch-project#8906) * return response with error Signed-off-by: Qxisylolo <[email protected]> * set Id Signed-off-by: Qxisylolo <[email protected]> * resolve tests Signed-off-by: Qxisylolo <[email protected]> * Changeset file for PR opensearch-project#8906 created/updated * typo Signed-off-by: Qxisylolo <[email protected]> * fix integration tests Signed-off-by: Qxisylolo <[email protected]> * add try catch Signed-off-by: Qxisylolo <[email protected]> * resolve conflicts Signed-off-by: Qxisylolo <[email protected]> * delete data source permission Signed-off-by: Qxisylolo <[email protected]> * add try catch Signed-off-by: Qxisylolo <[email protected]> --------- Signed-off-by: Qxisylolo <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

github-actions bot added the seasoned-contributor label Nov 21, 2024

opensearch-changeset-bot bot added a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Nov 21, 2024

Changeset file for PR opensearch-project#8906 created/updated

6c74c43

raintygao reviewed Nov 21, 2024

View reviewed changes

Qxisylolo force-pushed the fix/index_pattern_fetch_error branch from 671af1f to 2f4dedd Compare November 22, 2024 09:22

Qxisylolo changed the title ~~[workspace] fix: index pattern fetch error in discover dataset modal~~ [workspace] refactor: index pattern fetch error in discover dataset modal Nov 22, 2024

opensearch-changeset-bot bot added a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Nov 22, 2024

Changeset file for PR opensearch-project#8906 created/updated

7ac63e8

Qxisylolo marked this pull request as ready for review November 22, 2024 09:26

ruanyl reviewed Dec 18, 2024

View reviewed changes

ruanyl previously approved these changes Dec 24, 2024

View reviewed changes

SuZhou-Joe added the workspace label Dec 26, 2024

ruanyl added backport 2.x v2.19.0 labels Dec 26, 2024

SuZhou-Joe reviewed Dec 27, 2024

View reviewed changes

SuZhou-Joe pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Dec 27, 2024

Changeset file for PR opensearch-project#8906 created/updated

6a23aa2

SuZhou-Joe force-pushed the fix/index_pattern_fetch_error branch from cfb059d to 282c4fe Compare December 27, 2024 08:12

SuZhou-Joe reviewed Dec 27, 2024

View reviewed changes

Qxisylolo and others added 9 commits December 31, 2024 11:52

return response with error

4161f78

Signed-off-by: Qxisylolo <[email protected]>

set Id

3d12e0c

Signed-off-by: Qxisylolo <[email protected]>

resolve tests

5f81b69

Signed-off-by: Qxisylolo <[email protected]>

Changeset file for PR opensearch-project#8906 created/updated

ba79aea

typo

dd54426

Signed-off-by: Qxisylolo <[email protected]>

fix integration tests

877aa90

Signed-off-by: Qxisylolo <[email protected]>

add try catch

fe2e83d

Signed-off-by: Qxisylolo <[email protected]>

resolve conflicts

0561497

Signed-off-by: Qxisylolo <[email protected]>

delete data source permission

225c6c2

Signed-off-by: Qxisylolo <[email protected]>

Qxisylolo dismissed ruanyl’s stale review via 225c6c2 December 31, 2024 04:04

Qxisylolo force-pushed the fix/index_pattern_fetch_error branch from 282c4fe to 225c6c2 Compare December 31, 2024 04:04

SuZhou-Joe reviewed Jan 3, 2025

View reviewed changes

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts Outdated Show resolved Hide resolved

add try catch

12f1594

Signed-off-by: Qxisylolo <[email protected]>

SuZhou-Joe approved these changes Jan 6, 2025

View reviewed changes

ruanyl approved these changes Jan 8, 2025

View reviewed changes

ruanyl merged commit d947bd6 into opensearch-project:main Jan 8, 2025
69 checks passed

opensearch-trigger-bot bot mentioned this pull request Jan 8, 2025

[Backport 2.x] [workspace] refactor: refactor the bulk_get handler in permission wrapper when item has permission error #9156

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[workspace] refactor: refactor the bulk_get handler in permission wrapper when item has permission error #8906

[workspace] refactor: refactor the bulk_get handler in permission wrapper when item has permission error #8906

Qxisylolo commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

raintygao Nov 21, 2024 •

edited

Loading

raintygao Nov 21, 2024

raintygao Nov 21, 2024

raintygao Nov 21, 2024

Qxisylolo Nov 22, 2024

ruanyl Dec 18, 2024

yubonluo Dec 18, 2024

SuZhou-Joe Dec 27, 2024 •

edited

Loading

Qxisylolo Dec 27, 2024

SuZhou-Joe Dec 27, 2024

Qxisylolo Dec 27, 2024

yubonluo Dec 27, 2024

Qxisylolo Dec 27, 2024

		@@ -0,0 +1,2 @@
		fix:
		- Index pattern fetch error in discover dataset modal ([#8906](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8906))

[workspace] refactor: refactor the bulk_get handler in permission wrapper when item has permission error #8906

[workspace] refactor: refactor the bulk_get handler in permission wrapper when item has permission error #8906

Conversation

Qxisylolo commented Nov 21, 2024 • edited Loading

Description

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

raintygao Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Qxisylolo commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

raintygao Nov 21, 2024 •

edited

Loading

SuZhou-Joe Dec 27, 2024 •

edited

Loading