-
Notifications
You must be signed in to change notification settings - Fork 0
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
add workspace attribute check and refactor saved object acl check #93
Conversation
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## workspace #93 +/- ##
==========================================
Coverage 65.79% 65.79%
==========================================
Files 3334 3335 +1
Lines 64418 64445 +27
Branches 10250 10260 +10
==========================================
+ Hits 42383 42402 +19
- Misses 19466 19475 +9
+ Partials 2569 2568 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
request: OpenSearchDashboardsRequest, | ||
permissionMode: WorkspacePermissionMode | WorkspacePermissionMode[] | ||
) { | ||
if (!workspaceId) { | ||
// PermissionMode here is an array which is merged by workspace type required permission and other saved object required permission. |
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 couldn't get this, could you elaborate a bit?
PermissionMode here is an array which is merged by workspace type required permission and other saved object required permission.
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.
Previously, our validation logic for savedobject was:
if(isWorkspaceTypeSavedObject){
validate(workspaceType, workspaceTypeNeedPermissions)
}else{
validate(type, savedObjectNeedPermissions)
}
Now, we can merge workspaceTypeNeedPermissions and savedObjectNeedPermissions and unify savedObject permission check code.
validate(type, savedObjectNeedPermissions || workspaceTypeNeedPermissions)
In this way, the verification can be performed, unless there is dirty data which cannot be controlled.
if (this.isRelatedToWorkspace(cur.type)) { | ||
acc.push(cur.id); | ||
for (const object of objects) { | ||
const workspacePermitted = await this.validateWorkspaceAttributesPermitted( |
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'm a bit concern about the bulk action here, it looks the validation of the objects
list is sequential, which means it validates the object one by one, if there was a long object list, I feel this is going to take unexpected long time. cc @wanglam
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.
Maybe we can find some way to do these parallelly, validate 5-10 objects one time. What do you think about that? I think this is out of this PR scope, we can separate one task to optimize it.
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.
The logic here is a little tricky. If we want to bulk update the four objects [a, b, c, d], if a and b only have the operation permission on the workspace attribute, and c and d only have the operation permission on the object, then the update operation can still succeed. This means we have to do validation based on the single object.
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.
From the perspective of experience, we can do several verifications based on single object at the same time in a new task.
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
b9fdd4e
to
3c86bcb
Compare
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
…ects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com>
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
Show resolved
Hide resolved
…ects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com>
Signed-off-by: tygao <tygao@amazon.com>
…to validateAtLeastOnePermittedWorkspaces Signed-off-by: tygao <tygao@amazon.com>
* feat: add permission check for saved objects Signed-off-by: tygao <tygao@amazon.com> * feat: add attribute check Signed-off-by: tygao <tygao@amazon.com> * merge workspace error fix Signed-off-by: tygao <tygao@amazon.com> * update addToWorkspacesWithPermissionControl Signed-off-by: tygao <tygao@amazon.com> * feat: add workspace num check Signed-off-by: tygao <tygao@amazon.com> * chore: pass right access Signed-off-by: tygao <tygao@amazon.com> * chore: merge validateSingleObjectPermissions Signed-off-by: tygao <tygao@amazon.com> * chore: merge validateSingleObjectPermissions Signed-off-by: tygao <tygao@amazon.com> * chore: update bulkUpdate and update logic Signed-off-by: tygao <tygao@amazon.com> * feat: remove attribute check Signed-off-by: tygao <tygao@amazon.com> * feat: pass validate for config object Signed-off-by: tygao <tygao@amazon.com> * Update src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> * Update src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> * update Signed-off-by: tygao <tygao@amazon.com> * chore: update update and bulkUpdate to reduce calling get function Signed-off-by: tygao <tygao@amazon.com> * fix: change validateUpdateWithWorkspacePermission from validateMulti to validateAtLeastOnePermittedWorkspaces Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Co-authored-by: Yulong Ruan <ruanyu1@gmail.com>
* feat: add permission check for saved objects Signed-off-by: tygao <tygao@amazon.com> * feat: add attribute check Signed-off-by: tygao <tygao@amazon.com> * merge workspace error fix Signed-off-by: tygao <tygao@amazon.com> * update addToWorkspacesWithPermissionControl Signed-off-by: tygao <tygao@amazon.com> * feat: add workspace num check Signed-off-by: tygao <tygao@amazon.com> * chore: pass right access Signed-off-by: tygao <tygao@amazon.com> * chore: merge validateSingleObjectPermissions Signed-off-by: tygao <tygao@amazon.com> * chore: merge validateSingleObjectPermissions Signed-off-by: tygao <tygao@amazon.com> * chore: update bulkUpdate and update logic Signed-off-by: tygao <tygao@amazon.com> * feat: remove attribute check Signed-off-by: tygao <tygao@amazon.com> * feat: pass validate for config object Signed-off-by: tygao <tygao@amazon.com> * Update src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> * Update src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> * update Signed-off-by: tygao <tygao@amazon.com> * chore: update update and bulkUpdate to reduce calling get function Signed-off-by: tygao <tygao@amazon.com> * fix: change validateUpdateWithWorkspacePermission from validateMulti to validateAtLeastOnePermittedWorkspaces Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Co-authored-by: Yulong Ruan <ruanyu1@gmail.com>
* feat: add permission check for saved objects Signed-off-by: tygao <tygao@amazon.com> * feat: add attribute check Signed-off-by: tygao <tygao@amazon.com> * merge workspace error fix Signed-off-by: tygao <tygao@amazon.com> * update addToWorkspacesWithPermissionControl Signed-off-by: tygao <tygao@amazon.com> * feat: add workspace num check Signed-off-by: tygao <tygao@amazon.com> * chore: pass right access Signed-off-by: tygao <tygao@amazon.com> * chore: merge validateSingleObjectPermissions Signed-off-by: tygao <tygao@amazon.com> * chore: merge validateSingleObjectPermissions Signed-off-by: tygao <tygao@amazon.com> * chore: update bulkUpdate and update logic Signed-off-by: tygao <tygao@amazon.com> * feat: remove attribute check Signed-off-by: tygao <tygao@amazon.com> * feat: pass validate for config object Signed-off-by: tygao <tygao@amazon.com> * Update src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> * Update src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts Co-authored-by: Yulong Ruan <ruanyu1@gmail.com> * update Signed-off-by: tygao <tygao@amazon.com> * chore: update update and bulkUpdate to reduce calling get function Signed-off-by: tygao <tygao@amazon.com> * fix: change validateUpdateWithWorkspacePermission from validateMulti to validateAtLeastOnePermittedWorkspaces Signed-off-by: tygao <tygao@amazon.com> --------- Signed-off-by: tygao <tygao@amazon.com> Co-authored-by: Yulong Ruan <ruanyu1@gmail.com>
Description
add workspace attribute check and refactor saved object acl check
Issues Resolved
Screenshot
Testing the changes
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr