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.

Sign up for GitHub

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] refactor: refactor the bulk_get handler in permission wrapper when item has permission error #8906

Merged
merged 10 commits into from
Jan 8, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4161f78
return response with error
Qxisylolo Nov 21, 2024
3d12e0c
set Id
Qxisylolo Nov 21, 2024
5f81b69
resolve tests
Qxisylolo Nov 22, 2024
ba79aea
Changeset file for PR #8906 created/updated
opensearch-changeset-bot[bot] Nov 22, 2024
dd54426
typo
Qxisylolo Nov 22, 2024
877aa90
fix integration tests
Qxisylolo Nov 22, 2024
fe2e83d
add try catch
Qxisylolo Nov 28, 2024
0561497
resolve conflicts
Qxisylolo Dec 5, 2024
225c6c2
delete data source permission
Qxisylolo Dec 31, 2024
12f1594
add try catch
Qxisylolo Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/8906.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Refactor the bulk_get handler in permission wrapper when item has permission error ([#8906](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8906))
37 changes: 17 additions & 20 deletions ...ace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,27 +200,24 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {

describe('bulkGet', () => {
it('should throw forbidden error when user not permitted', async () => {
let error;
try {
await notPermittedSavedObjectedClient.bulkGet([
{ type: 'dashboard', id: 'inner-workspace-dashboard-1' },
]);
} catch (e) {
error = e;
}
expect(error).not.toBeUndefined();
expect(SavedObjectsErrorHelpers.isForbiddenError(error)).toBe(true);
const result = await notPermittedSavedObjectedClient.bulkGet([
{ type: 'dashboard', id: 'acl-controlled-dashboard-2' },
]);

error = undefined;
try {
await notPermittedSavedObjectedClient.bulkGet([
{ type: 'dashboard', id: 'acl-controlled-dashboard-2' },
]);
} catch (e) {
error = e;
}
expect(error).not.toBeUndefined();
expect(SavedObjectsErrorHelpers.isForbiddenError(error)).toBe(true);
expect(result.saved_objects).toEqual([
{
...result.saved_objects[0],
id: 'acl-controlled-dashboard-2',
type: 'dashboard',
attributes: {},
error: {
error: 'Forbidden',
statusCode: 403,
message: 'Invalid saved objects permission',
},
workspaces: [],
},
]);
});

it('should return consistent dashboard when user permitted', async () => {
Expand Down
28 changes: 21 additions & 7 deletions src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,12 +660,11 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
permissionControlMock,
requestMock,
} = generateWorkspaceSavedObjectsClientWrapper();
let errorCatched;
try {
await wrapper.bulkGet([{ type: 'dashboard', id: 'not-permitted-dashboard' }]);
} catch (e) {
errorCatched = e;
}

const result = await wrapper.bulkGet([
{ type: 'dashboard', id: 'not-permitted-dashboard' },
]);

expect(permissionControlMock.validate).toHaveBeenCalledWith(
requestMock,
{
Expand All @@ -674,7 +673,22 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
},
['library_read', 'library_write']
);
expect(errorCatched?.message).toEqual('Invalid saved objects permission');

expect(result.saved_objects).toEqual([
{
id: 'not-permitted-dashboard',
type: 'dashboard',
attributes: {},
error: {
error: 'Forbidden',
message: 'Invalid saved objects permission',
statusCode: 403,
},
workspaces: [],
permissions: {},
references: [],
},
]);
});
it('should call permission validateSavedObjectsACL with object', async () => {
const { wrapper, permissionControlMock } = generateWorkspaceSavedObjectsClientWrapper();
Expand Down
60 changes: 41 additions & 19 deletions src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,26 +450,48 @@
wrapperOptions.request,
getWorkspacesFromSavedObjects(objectToBulkGet.saved_objects)
);

for (const object of objectToBulkGet.saved_objects) {
if (
!(await this.validateWorkspacesAndSavedObjectsPermissions(
object,
wrapperOptions.request,
[WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.LibraryWrite],
[WorkspacePermissionMode.Write, WorkspacePermissionMode.Read],
false
))
) {
ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);
throw generateSavedObjectsPermissionError();
}
}
ACLAuditor?.increment(
ACLAuditorStateKey.VALIDATE_SUCCESS,
objectToBulkGet.saved_objects.length
const processedObjects = await Promise.all(
objectToBulkGet.saved_objects.map(async (object) => {
try {
Copy link
Member

@SuZhou-Joe SuZhou-Joe Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const hasPermission = await this.validateWorkspacesAndSavedObjectsPermissions(
object,
wrapperOptions.request,
[WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.LibraryWrite],
[WorkspacePermissionMode.Write, WorkspacePermissionMode.Read],
false
);
if (hasPermission) {
ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_SUCCESS, 1);
return object;
} else {
ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);
return {
...object,
workspaces: [],
attributes: {} as T,
error: {
...generateSavedObjectsPermissionError().output.payload,
statusCode: 403,
},
};
}
} catch (error) {
ACLAuditor?.increment(ACLAuditorStateKey.VALIDATE_FAILURE, 1);
return {

Check warning on line 480 in src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts#L479-L480 

Added lines #L479 - L480 were not covered by tests
...object,
workspaces: [],
attributes: {} as T,
error: {
...generateWorkspacePermissionError().output.payload,
statusCode: error.statusCode,
message: error.message,
},
};
}
})
);
return objectToBulkGet;

return { saved_objects: processedObjects };
};

const findWithWorkspacePermissionControl = async <T = unknown>(
Expand Down
Loading