-
Notifications
You must be signed in to change notification settings - Fork 915
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]Add permission control logic for workspace #6052
[Workspace]Add permission control logic for workspace #6052
Conversation
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6052 +/- ##
==========================================
- Coverage 67.50% 67.49% -0.01%
==========================================
Files 3370 3376 +6
Lines 65467 65783 +316
Branches 10564 10637 +73
==========================================
+ Hits 44192 44401 +209
- Misses 18700 18798 +98
- Partials 2575 2584 +9
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 <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
Signed-off-by: Lin Wang <[email protected]>
@wanglam How can i validate this change? Can ou add testing instructions to the PR description? It makes reviewing PR's a lot easier |
Hi Ashwin, I think we can follow the integration testing file (src/plugins/workspace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts) to do tests and validate this change. I will update the PR description later about how to call workspace CRUD and saved objects API to manual validate all changes. |
* feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]>
Thanks @wanglam, it seems like that we need to comment out all the feature flags for
I got the same issue when I comment out all the multienancy. my configuration as following and enabled security plugin, and I pull the latest commit from your PR as well
|
…ontrol-for-workspace Signed-off-by: Lin Wang <[email protected]>
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.
LGTM
The |
…ject#6052) * Add permission control for workspace Signed-off-by: Lin Wang <[email protected]> * Add changelog for permission control in workspace Signed-off-by: Lin Wang <[email protected]> * Fix integration tests and remove no need type Signed-off-by: Lin Wang <[email protected]> * Update permission enabled for workspace CRUD integration tests Signed-off-by: Lin Wang <[email protected]> * Change back to config schema Signed-off-by: Lin Wang <[email protected]> * feat: do not append workspaces field when no workspaces present (#6) * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * fix: authInfo destructure (#7) * fix: authInfo destructure Signed-off-by: SuZhou-Joe <[email protected]> * fix: unit test error Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * Fix permissions assign in attributes Signed-off-by: Lin Wang <[email protected]> * Remove deleteByWorkspace since not exists Signed-off-by: Lin Wang <[email protected]> * refactor: remove formatWorkspacePermissionModeToStringArray Signed-off-by: Lin Wang <[email protected]> * Remove current not used code Signed-off-by: Lin Wang <[email protected]> * Add missing unit tests for permission control Signed-off-by: Lin Wang <[email protected]> * Update workspaces API test describe Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests failed Signed-off-by: Lin Wang <[email protected]> * Address PR comments Signed-off-by: Lin Wang <[email protected]> * Store permissions when savedObjects.permissions.enabled Signed-off-by: Lin Wang <[email protected]> * Add permission control for deleteByWorkspace Signed-off-by: Lin Wang <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Refactor permissions field in workspace create and update API Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: SuZhou-Joe <[email protected]> Signed-off-by: Lin Wang <[email protected]>
* [Workspace]Add permission control logic for workspace (opensearch-project#6052) * Add permission control for workspace Signed-off-by: Lin Wang <[email protected]> * Add changelog for permission control in workspace Signed-off-by: Lin Wang <[email protected]> * Fix integration tests and remove no need type Signed-off-by: Lin Wang <[email protected]> * Update permission enabled for workspace CRUD integration tests Signed-off-by: Lin Wang <[email protected]> * Change back to config schema Signed-off-by: Lin Wang <[email protected]> * feat: do not append workspaces field when no workspaces present (#6) * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * fix: authInfo destructure (#7) * fix: authInfo destructure Signed-off-by: SuZhou-Joe <[email protected]> * fix: unit test error Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * Fix permissions assign in attributes Signed-off-by: Lin Wang <[email protected]> * Remove deleteByWorkspace since not exists Signed-off-by: Lin Wang <[email protected]> * refactor: remove formatWorkspacePermissionModeToStringArray Signed-off-by: Lin Wang <[email protected]> * Remove current not used code Signed-off-by: Lin Wang <[email protected]> * Add missing unit tests for permission control Signed-off-by: Lin Wang <[email protected]> * Update workspaces API test describe Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests failed Signed-off-by: Lin Wang <[email protected]> * Address PR comments Signed-off-by: Lin Wang <[email protected]> * Store permissions when savedObjects.permissions.enabled Signed-off-by: Lin Wang <[email protected]> * Add permission control for deleteByWorkspace Signed-off-by: Lin Wang <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Refactor permissions field in workspace create and update API Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: SuZhou-Joe <[email protected]> Signed-off-by: Lin Wang <[email protected]> * Convert permission settings in client side Signed-off-by: Lin Wang <[email protected]> * Fix workspace list always render Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: SuZhou-Joe <[email protected]>
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch-Dashboards/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch-Dashboards/backport-2.x
# Create a new branch
git switch --create backport/backport-6052-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 fb31b2def6a6200425492d772a8d0b1bdfcbe132
# Push it to GitHub
git push --set-upstream origin backport/backport-6052-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch-Dashboards/backport-2.x Then, create a pull request where the |
…ject#6052) * Add permission control for workspace Signed-off-by: Lin Wang <[email protected]> * Add changelog for permission control in workspace Signed-off-by: Lin Wang <[email protected]> * Fix integration tests and remove no need type Signed-off-by: Lin Wang <[email protected]> * Update permission enabled for workspace CRUD integration tests Signed-off-by: Lin Wang <[email protected]> * Change back to config schema Signed-off-by: Lin Wang <[email protected]> * feat: do not append workspaces field when no workspaces present (#6) * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * fix: authInfo destructure (#7) * fix: authInfo destructure Signed-off-by: SuZhou-Joe <[email protected]> * fix: unit test error Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * Fix permissions assign in attributes Signed-off-by: Lin Wang <[email protected]> * Remove deleteByWorkspace since not exists Signed-off-by: Lin Wang <[email protected]> * refactor: remove formatWorkspacePermissionModeToStringArray Signed-off-by: Lin Wang <[email protected]> * Remove current not used code Signed-off-by: Lin Wang <[email protected]> * Add missing unit tests for permission control Signed-off-by: Lin Wang <[email protected]> * Update workspaces API test describe Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests failed Signed-off-by: Lin Wang <[email protected]> * Address PR comments Signed-off-by: Lin Wang <[email protected]> * Store permissions when savedObjects.permissions.enabled Signed-off-by: Lin Wang <[email protected]> * Add permission control for deleteByWorkspace Signed-off-by: Lin Wang <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Refactor permissions field in workspace create and update API Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: SuZhou-Joe <[email protected]>
…ject#6052) * Add permission control for workspace Signed-off-by: Lin Wang <[email protected]> * Add changelog for permission control in workspace Signed-off-by: Lin Wang <[email protected]> * Fix integration tests and remove no need type Signed-off-by: Lin Wang <[email protected]> * Update permission enabled for workspace CRUD integration tests Signed-off-by: Lin Wang <[email protected]> * Change back to config schema Signed-off-by: Lin Wang <[email protected]> * feat: do not append workspaces field when no workspaces present (#6) * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> * feat: do not append workspaces field when no workspaces present Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * fix: authInfo destructure (#7) * fix: authInfo destructure Signed-off-by: SuZhou-Joe <[email protected]> * fix: unit test error Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * Fix permissions assign in attributes Signed-off-by: Lin Wang <[email protected]> * Remove deleteByWorkspace since not exists Signed-off-by: Lin Wang <[email protected]> * refactor: remove formatWorkspacePermissionModeToStringArray Signed-off-by: Lin Wang <[email protected]> * Remove current not used code Signed-off-by: Lin Wang <[email protected]> * Add missing unit tests for permission control Signed-off-by: Lin Wang <[email protected]> * Update workspaces API test describe Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests failed Signed-off-by: Lin Wang <[email protected]> * Address PR comments Signed-off-by: Lin Wang <[email protected]> * Store permissions when savedObjects.permissions.enabled Signed-off-by: Lin Wang <[email protected]> * Add permission control for deleteByWorkspace Signed-off-by: Lin Wang <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Update src/plugins/workspace/server/permission_control/client.ts Signed-off-by: SuZhou-Joe <[email protected]> * Refactor permissions field in workspace create and update API Signed-off-by: Lin Wang <[email protected]> * Fix workspace CRUD API integration tests Signed-off-by: Lin Wang <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: SuZhou-Joe <[email protected]>
* Add permission control for workspace * Add changelog for permission control in workspace * Fix integration tests and remove no need type * Update permission enabled for workspace CRUD integration tests * Change back to config schema * feat: do not append workspaces field when no workspaces present (#6) * feat: do not append workspaces field when no workspaces present * feat: do not append workspaces field when no workspaces present --------- * fix: authInfo destructure (#7) * fix: authInfo destructure * fix: unit test error --------- * Fix permissions assign in attributes * Remove deleteByWorkspace since not exists * refactor: remove formatWorkspacePermissionModeToStringArray * Remove current not used code * Add missing unit tests for permission control * Update workspaces API test describe * Fix workspace CRUD API integration tests failed * Address PR comments * Store permissions when savedObjects.permissions.enabled * Add permission control for deleteByWorkspace * Update src/plugins/workspace/server/permission_control/client.ts * Update src/plugins/workspace/server/permission_control/client.ts * Refactor permissions field in workspace create and update API * Fix workspace CRUD API integration tests --------- Signed-off-by: Lin Wang <[email protected]> Signed-off-by: SuZhou-Joe <[email protected]> Co-authored-by: SuZhou-Joe <[email protected]> Co-authored-by: ZilongX <[email protected]>
Description
This PR is for adding permission control logic for workspace. It's includes below changes:
savedObjects.permission
to global config objectACLSearchParams
andworkspaceSearchOperator
to repository find methodIssues Resolved
closes #6051
Screenshot
Testing the changes
Write unit tests and integration tests for workspace saved object client wrapper.
Test instructions
Since all these changes are in the server side. We need to call these APIs manual to verify if permission control work fine.
There are two types saved objects in permission control. The first one is saved object with
workspaces
property, another one is saved object withpermissions
property. In this test instruction, we will useworkspace
type saved object to verify permission control when haspermissions
property. Usedashboard
type saved object to verify permission control when hasworkspaces
property. If one saved object hasworkspaces
property, the permission control logic will check if has related permissions to the workspaces. Then if it haspermissions
property, it will do the permission validation on thepermissions
property.To run all below tests, need to add above flags in
opensearch_dashboards.yml
, and install security-dashboards-plugin. There are two internal users will be used in following tests. Here are the user details:username: admin, password: myStrongPassword123! backend-roles: admin
username: another-user, password: myStrongPassword123! backend-roles: kibanauser
The
admin
user is not a specific user here, all the permission control process is the same as a normal user.We will add
authorization: Basic YWRtaW46bXlTdHJvbmdQYXNzd29yZDEyMyE=
tocurl
command to simulateadmin
user and addauthorization: Basic YW5vdGhlci11c2VyOm15U3Ryb25nUGFzc3dvcmQxMjMh
to simulateanother-user
.After all the environments ready and user created, we can start to test permission control feature.
Create workspace
This steps is for creating test workspace for future test cases. Run below workspace create API.
Here is an example response, the workspace will be created. It will return a workspace id, can be used in following steps.
GVnXDv
is the workspace id, since the workspace was created byadmin
user. The user will be assignedlibrary_write
andwrite
permission to workspace. Theanother-user
doesn't have this permission, we can try to get this workspace by below code.Try to get workspace with
another-user
, it will response{"success":false,"error":"Invalid saved objects permission"}
.create in permitted workspace
In above commands, it will create an dashboard in the
admin-only-workspace
. Theadmin
user haslibrary_write
permission in the workspace. The create option will be succeed. The response dashboard id is87af5db0-dc52-11ee-acaf-4d315f971049
, we can try to get / update / delete this ID in next tests.create in not permitted workspace
In above commands, it try to create an dashboard in the
admin-only-workspace
. Theanother-user
user doesn't havelibrary_write
permission in the workspace. The create option will be failed. It's show permission control for saved object work as expected.create with overwrite
This is another cases, the
another-user
user wants to overwrite an existing saved object. Seems the user doesn't have permission to the dashboard's workspaces and doesn't have permission to the dashboard saved object self. The operation was denied.bulkCreate in permitted workspace
These above commands will be succeed, it will create a dashboard saved object in
admin-only-workspace
. We can write down the dashboard idfffe5ab0-dc83-11ee-9093-372beb25d7b4
. Then we can used in thebulkGet
method.bulkCreate in not permitted workspace
These above commands will be failed.
another-user
doesn't havelibrary_write
permission inadmin-only-workspace
. It can't create saved objects in the workspace.bulkCerate with override
These above commands will be failed.
another-user
doesn't have permission to the existing saved objects. The overwrite operation will be denied.get permitted dashboard
The
admin
user haslibrary_write
permission to dashboard saved object's workspace. The API return the dashboard object.get not permitted dashboard
The
another-user
user doesn't any permission to the dashboard saved object's workspace and itself. The API will response error.get permitted workspace
Since workspace is a hidden type in saved objects. It doesn't support call saved object get API directly. We need to call workspace get API instead. This will be succeed, since
admin
user has related permission.get not permitted workspace
This command will be failed. The
workspace
type saved object haspermissions
property. It will store permitted user in this property. Theanother-user
doesn't in it. So the get API call will be failed.bulk get permitted dashboard
This command will be succeed, it will return dashboards created in bulk create method.
bulk get not permitted dashboard
Response:
This command will be failed, the
another-user
doesn't have permission to related saved objects.find all permitted workspaces
Since workspace is a hidden saved object, we can't call saved objects API directly. Call workspace list API instead.
This above command will list all permitted workspaces for
admin
user. They will includesadmin-only-workspaces
.This above command will list all permitted workspaces for
another-user
user. They won't includesadmin-only-workspaces
.find all permitted saved objects in specific workspaces
The
admin
user has permission toadmin-only-workspaces
. So this find API will return all dashboard saved objects inneradmin-only-workspace
.find saved objects in not permitted workspaces
The
another
user doesn't has permission toadmin-only-workspaces
. So this find API call will be failed.update permitted dashboard
This command should be succeed, since call update the dashboard API with a permitted user.
update not permitted dashboards
This command should be failed,
another-user
doesn't havelibrary_write
permission to the workspace. The update operation should be denied.update permitted workspace
This command should be succeed, the workspace type saved object has
permissions
property. Theadmin
is in thewrite
principals list.update not permitted workspace
This command should be failed, The
another-user
doesn't have correspondingwrite
permission in the workspace type saved objects.bulk update in permitted dashboards
This command will be succeed. The
admin
user haslibrary_write
permission to every saved objects's workspace. It will return updated dashboards.bulk update in not permitted dashboards
This command will be failed. The
another-user
user doesn't havelibrary_write
permission to every saved objects's workspace.delete not permitted dashboard
Response
delete permitted dashboard
Response
delete not permitted workspace
Response
delete permitted workspace
Response
Check List
yarn test:jest
yarn test:jest_integration