Support disable permission check on workspace #228

Hailong-am · 2023-10-13T02:38:22Z

Description

When OSD don't have authentication enabled, there is no user/role. In this case, permission check for workspace need to be disabled, otherwise it will block the access to saved objects belongs to workspaces.

To support this we could check whether authentication has configured, but it lose the flexibility to turn on/off based on user's need.

By adding a new flag in yml file, it is more flexible, user could turn permission check off even if authentication is enabled.

This flag will skip permission check totally. However, for creating/updating workspace, it will still persistent the ACL related information, that info could be reuse when permission check turned on.

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter · 2023-10-13T02:57:18Z

Codecov Report

Merging #228 (b77b497) into workspace (dd486e9) will decrease coverage by 11.00%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           workspace     #228       +/-   ##
==============================================
- Coverage      66.22%   55.22%   -11.00%     
==============================================
  Files           3422     3133      -289     
  Lines          65748    61758     -3990     
  Branches       10587     9974      -613     
==============================================
- Hits           43542    34107     -9435     
- Misses         19564    25576     +6012     
+ Partials        2642     2075      -567

Flag	Coverage Δ
Linux_1	`30.42% <ø> (ø)`
Linux_2	`?`
Linux_3	`42.78% <ø> (-0.02%)`	⬇️
Linux_4	`34.53% <ø> (ø)`

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

see 700 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

SuZhou-Joe · 2023-10-13T08:45:10Z

src/plugins/workspace/server/integration_tests/routes.test.ts

@@ -49,7 +54,10 @@ describe('workspace service', () => {
        .expect(200);
      await Promise.all(
        listResult.body.result.workspaces.map((item: WorkspaceAttribute) =>
-          osdTestServer.request.delete(root, `/api/workspaces/${item.id}`).expect(200)
+          // this will delete reserved workspace


I am a little bit confused for this comment, shall we add more details?

sure, use workspace delete api will not able to delete reserved workspace, that will not able clean up the test data. Use saved objects api will not have this issue.

Get it, could we add this info to this comment? It would be much easier to understand after adding the context you provide.

i have drop the testing related commit, will have separate PR to add test cases.

src/plugins/workspace/config.ts

src/plugins/workspace/server/integration_tests/routes.test.ts

src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx

ruanyl · 2023-10-13T09:49:47Z

Noticed that there are 3 unrelated commits in this PR, maybe should be dropped?

config/opensearch_dashboards.yml

src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx

src/plugins/workspace/public/components/workspace_creator/workspace_form.tsx

xluo-aws · 2023-10-13T10:01:45Z

@Hailong-am , could you explain why we add this new flag?

Signed-off-by: Hailong Cui <[email protected]>

Hailong-am · 2023-10-13T10:14:30Z

Noticed that there are 3 unrelated commits in this PR, maybe should be dropped?

good suggestion, i have drop the 3 unrelated commits. will raise separate PR for backporting test cases.

Hailong-am · 2023-10-13T10:24:49Z

@Hailong-am , could you explain why we add this new flag?

update it in PR description

xluo-aws · 2023-10-13T10:34:48Z

@Hailong-am , could you explain why we add this new flag?

update it in PR description

If sets to false, all ACL check will be skipped including user level, right?

Hailong-am · 2023-10-16T01:04:21Z

If sets to false, all ACL check will be skipped including user level, right?

Yes, that will disable ACL totally.

src/plugins/workspace/server/plugin.ts

…objects client wrapper (#230) * feat: add basic workspace saved objects client wrapper Signed-off-by: Lin Wang <[email protected]> * feat: add unit test (#2) Signed-off-by: SuZhou-Joe <[email protected]> * feat: update client wrapper Signed-off-by: tygao <[email protected]> * feat: init permission control in workspace plugin Signed-off-by: Lin Wang <[email protected]> * Support disable permission check on workspace (#228) * support disable permission check for workspace Signed-off-by: Hailong Cui <[email protected]> * fix typos Signed-off-by: Hailong Cui <[email protected]> --------- Signed-off-by: Hailong Cui <[email protected]> * feat: add ACLSearchParams consumer in repository (#3) Signed-off-by: SuZhou-Joe <[email protected]> * fix: ACLSearchParams missing in search dsl Signed-off-by: Lin Wang <[email protected]> * test: add integration test for workspace saved objects client wrapper Signed-off-by: Lin Wang <[email protected]> * style: add empty line under license Signed-off-by: Lin Wang <[email protected]> * test: enable workspace permission control for integration tests Signed-off-by: Lin Wang <[email protected]> * feat: add workspace into includeHiddenTypes (#249) * feat: add workspace into includeHiddenTypes of client wrapper and permission control client Signed-off-by: SuZhou-Joe <[email protected]> * fix: hiddenType side effect Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * fix workspace client wrapper integration tests Signed-off-by: Lin Wang <[email protected]> * add permissions fields to workspace CRUD APIs Signed-off-by: Lin Wang <[email protected]> * Move WorkspacePermissionMode inside workspace plugin Signed-off-by: Lin Wang <[email protected]> * Address pr comments Signed-off-by: Lin Wang <[email protected]> * Remove ACLSearchParams in public SavedObjectsFindOptions Signed-off-by: Lin Wang <[email protected]> * Remove lodash and Add default permissionModes Signed-off-by: Lin Wang <[email protected]> * feat: address concerns on ensureRawRequest (#4) * feat: address concerns on ensureRawRequest Signed-off-by: SuZhou-Joe <[email protected]> * feat: add check for empty array Signed-off-by: SuZhou-Joe <[email protected]> * feat: make find api backward compatible Signed-off-by: SuZhou-Joe <[email protected]> * feat: remove useless code Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: SuZhou-Joe <[email protected]> * Update annotations and error Signed-off-by: Lin Wang <[email protected]> * Add unit tests for worksapce saved objects client wrapper Signed-off-by: Lin Wang <[email protected]> * Remove getPrincipalsOfObjects in permission Signed-off-by: Lin Wang <[email protected]> * Fix permissionEnabled flag missed in workspace plugin setup test Signed-off-by: Lin Wang <[email protected]> * Change back to Not Authorized error Signed-off-by: Lin Wang <[email protected]> * Fix unit tests for query_params and plugin setup Signed-off-by: Lin Wang <[email protected]> * Fix unittests in workspace server utils Signed-off-by: Lin Wang <[email protected]> * feat: add workspacesSearchOperators to decouple ACLSearchParams Signed-off-by: SuZhou-Joe <[email protected]> * feat: update test cases Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize test cases Signed-off-by: SuZhou-Joe <[email protected]> * feat: optimize comment Signed-off-by: SuZhou-Joe <[email protected]> * feat: omit defaultSearchOperator in public savedobjetcs client Signed-off-by: SuZhou-Joe <[email protected]> * feat: omit workspacesSearchOperator field Signed-off-by: SuZhou-Joe <[email protected]> --------- Signed-off-by: Lin Wang <[email protected]> Signed-off-by: SuZhou-Joe <[email protected]> Signed-off-by: tygao <[email protected]> Signed-off-by: Hailong Cui <[email protected]> Co-authored-by: Lin Wang <[email protected]> Co-authored-by: SuZhou-Joe <[email protected]> Co-authored-by: Hailong Cui <[email protected]>

Hailong-am changed the title ~~Backport/backport 216 to workspace~~ Backport #216 [Delete workspace API] to workspace branch Oct 13, 2023

github-actions bot added the seasoned-contributor label Oct 13, 2023

Hailong-am force-pushed the backport/backport-216-to-workspace branch from 2b38868 to e4557b7 Compare October 13, 2023 07:26

Hailong-am marked this pull request as ready for review October 13, 2023 08:40

Hailong-am changed the title ~~Backport #216 [Delete workspace API] to workspace branch~~ Support disable permission check on workspace Oct 13, 2023

Hailong-am requested review from wanglam, ruanyl and SuZhou-Joe October 13, 2023 08:41

SuZhou-Joe reviewed Oct 13, 2023

View reviewed changes

src/plugins/workspace/config.ts Show resolved Hide resolved

SuZhou-Joe approved these changes Oct 13, 2023

View reviewed changes

SuZhou-Joe reviewed Oct 13, 2023

View reviewed changes

src/plugins/workspace/server/integration_tests/routes.test.ts Outdated Show resolved Hide resolved

ruanyl reviewed Oct 13, 2023

View reviewed changes

src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx Outdated Show resolved Hide resolved

xluo-aws reviewed Oct 13, 2023

View reviewed changes

config/opensearch_dashboards.yml Outdated Show resolved Hide resolved

xluo-aws reviewed Oct 13, 2023

View reviewed changes

src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx Outdated Show resolved Hide resolved

xluo-aws reviewed Oct 13, 2023

View reviewed changes

src/plugins/workspace/public/components/workspace_creator/workspace_form.tsx Outdated Show resolved Hide resolved

Hailong-am added 2 commits October 13, 2023 18:04

support disable permission check for workspace

df45b39

Signed-off-by: Hailong Cui <[email protected]>

fix typos

b77b497

Signed-off-by: Hailong Cui <[email protected]>

Hailong-am force-pushed the backport/backport-216-to-workspace branch from e4557b7 to b77b497 Compare October 13, 2023 10:04

ruanyl approved these changes Oct 16, 2023

View reviewed changes

wanglam reviewed Oct 16, 2023

View reviewed changes

src/plugins/workspace/server/plugin.ts Show resolved Hide resolved

wanglam approved these changes Oct 16, 2023

View reviewed changes

Hailong-am merged commit 2964934 into ruanyl:workspace Oct 16, 2023
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support disable permission check on workspace #228

Support disable permission check on workspace #228

Hailong-am commented Oct 13, 2023 •

edited

Loading

codecov-commenter commented Oct 13, 2023 •

edited

Loading

SuZhou-Joe Oct 13, 2023

Hailong-am Oct 13, 2023

SuZhou-Joe Oct 13, 2023

Hailong-am Oct 13, 2023

ruanyl commented Oct 13, 2023

xluo-aws commented Oct 13, 2023

Hailong-am commented Oct 13, 2023

Hailong-am commented Oct 13, 2023

xluo-aws commented Oct 13, 2023 •

edited

Loading

Hailong-am commented Oct 16, 2023

Support disable permission check on workspace #228

Support disable permission check on workspace #228

Conversation

Hailong-am commented Oct 13, 2023 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter commented Oct 13, 2023 • edited Loading

Codecov Report

SuZhou-Joe Oct 13, 2023

Choose a reason for hiding this comment

Hailong-am Oct 13, 2023

Choose a reason for hiding this comment

SuZhou-Joe Oct 13, 2023

Choose a reason for hiding this comment

Hailong-am Oct 13, 2023

Choose a reason for hiding this comment

ruanyl commented Oct 13, 2023

xluo-aws commented Oct 13, 2023

Hailong-am commented Oct 13, 2023

Hailong-am commented Oct 13, 2023

xluo-aws commented Oct 13, 2023 • edited Loading

Hailong-am commented Oct 16, 2023

Hailong-am commented Oct 13, 2023 •

edited

Loading

codecov-commenter commented Oct 13, 2023 •

edited

Loading

xluo-aws commented Oct 13, 2023 •

edited

Loading