add permission control service for saved objects and workspace saved objects client wrapper #230

raintygao · 2023-10-16T06:19:11Z

Description

add permission control service for saved objects
define workspaceSavedObjectsClientWrapper class and add the client wrapper to savedObjects
add workspace permission control enabled flag

Testing the changes

Check List

codecov-commenter · 2023-10-17T02:33:48Z

Codecov Report

Attention: Patch coverage is 74.05858% with 62 lines in your changes are missing coverage. Please review.

Project coverage is 66.99%. Comparing base (d165b88) to head (b36c380).
Report is 1 commits behind head on workspace-pr-integr.

Files	Patch %	Lines
..._objects/workspace_saved_objects_client_wrapper.ts	82.97%	13 Missing and 11 partials ⚠️
...gins/workspace/server/permission_control/client.ts	53.65%	16 Missing and 3 partials ⚠️
src/plugins/workspace/server/routes/index.ts	13.33%	13 Missing ⚠️
src/plugins/workspace/server/plugin.ts	63.63%	2 Missing and 2 partials ⚠️
src/plugins/workspace/server/utils.ts	86.66%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           workspace-pr-integr     #230      +/-   ##
=======================================================
+ Coverage                66.96%   66.99%   +0.02%     
=======================================================
  Files                     3323     3325       +2     
  Lines                    64208    64439     +231     
  Branches                 10309    10382      +73     
=======================================================
+ Hits                     42998    43168     +170     
- Misses                   18687    18730      +43     
- Partials                  2523     2541      +18

Flag	Coverage Δ
Linux_	`66.32% <74.05%> (+0.03%)`	⬆️
_1	`32.14% <67.36%> (+0.16%)`	⬆️
_2	`55.35% <100.00%> (+0.02%)`	⬆️
_3	`43.30% <0.00%> (-0.12%)`	⬇️
_4	`35.11% <0.00%> (-0.08%)`	⬇️

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.

src/plugins/workspace/server/plugin.ts

...kspace/server/saved_objects/integration_tests/workspace_saved_objects_client_wrapper.test.ts

src/core/utils/constants.ts

src/core/public/saved_objects/saved_objects_client.ts

src/core/server/http/index.ts

src/plugins/workspace/server/permission_control/client.ts

ruanyl · 2024-02-29T15:19:06Z

src/plugins/workspace/server/permission_control/client.ts

+    savedObjects: SavedObjectsBulkGetObject[]
+  ): Promise<Record<string, TransformedPermission>> {
+    const detailedSavedObjects = await this.bulkGetSavedObjects(request, savedObjects);
+    return detailedSavedObjects.reduce((total, current) => {


We still need to check user's permissions to savedObjects before returning the results, isn't it?

This function is for get the principals of specific object. It's a part of permission validation. Since we do the permission validation in the server side. We need to get the principals of specific object without check user's permission. This function should not expose to the frontend. This function was only be used in ${WORKSPACES_API_BASE_URL}/principals, I think we can remove it.

I see, if there is no clear use case, I suggest to remove it.

src/plugins/workspace/server/plugin.ts

src/plugins/workspace/server/routes/index.ts

ruanyl · 2024-02-29T15:49:12Z

src/plugins/workspace/server/utils.ts

+     * It means OSD can not recognize who the user is even if authentication is enabled,
+     * use a fake user that won't be granted permission explicitly.
+     */
+    payload[PrincipalType.Users] = [`_user_fake_${Date.now()}_`];


Is there an example of when authentication is enabled but OSD can not recognize who the user is?

@SuZhou-Joe Could you help me elaborate more that part?

There are three auth status, unknown(no authentication), noauthenticated(authN fail) and authenticated(authN success). I do not know how we can reproduce the case for noauthenticated(as before we handle the unauthenticated request user will be redirect to login page) but I have to write specific if/else to handle according to the interface. Change the comment and please find the latest change in wanglam#4

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

src/plugins/workspace/server/permission_control/client.ts

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

ruanyl · 2024-03-01T04:26:36Z

src/core/server/saved_objects/types.ts

+   * The params here will be combined with bool clause and is used for filtering with ACL structure.
+   */
+  ACLSearchParams?: {
+    workspaces?: string[];


Cannot really remember the difference of ACLSearchParams.workspaces vs SavedObjectsFindOptions.workspaces, could you help me to recall? :D

From the code, the SavedObjectsFindOptions.workspaces support pass * to match workspaces. The ACLSearchParams.workspaces doesn't support that. @SuZhou-Joe Could we remove the ACLSearchParams.workspaces? It's a little bit confused.

The SavedObjectsFindOptions.workspaces support * to match all workspaces. It's using 'and' operator when generate search dsl. Here is the code.
The ACLSearchParams.workspaces doesn't support * to match all workspaces. It's using 'or' operator with permission modes and principals, which means used to find all permitted saved objects.
They are the differences in the repository part.
In the WorkspaceSavedObjectClientWrapper, the ACLSearchParams.workspaces will be set to all permitted workspaces, if no workspaces passed. It will always return saved objects inner permitted saved objects.

ruanyl · 2024-03-01T04:37:50Z

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

+           */
+          options.workspaces = undefined;
+          options.ACLSearchParams.workspaces = permittedWorkspaceIds;
+          options.ACLSearchParams.permissionModes = options.ACLSearchParams.permissionModes ?? [


Could options.ACLSearchParams.permissionModes be an empty array []? What's the consequence when permissionModes = [] is passed in options?

For now, it can be an an empty array. If empty array passed, I think it will return all the results. @SuZhou-Joe Shall we add an empty array check when generate search dsl function?

Discussed with @wanglam offline, just for transparency, when listing workspaces, passing empty array makes it to return all workspaces including those workspaces that the current user doesn't have access to.

Sure, add fail fast check in generate search dsl function. https://github.com/wanglam/OpenSearch-Dashboards/pull/4/files#diff-428b7d9ca0d879eea511a66eec222df247a1fe9ff32e13feb7ab53677b6ff044

src/plugins/workspace/server/permission_control/client.ts

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

wanglam · 2024-03-04T10:23:35Z

@ruanyl I've addressed all the comments and add an unit test for workspace saved objects client wrapper. Would you like to help me review it again?