dashboard admin(groups/users) implementation and integrating with dynamic application config #303

yubonluo · 2024-03-20T02:30:42Z

This reverts commit 47e10e4.

Description

dashboard admin(groups/users) implementation and integrating with dynamic application config

Issues Resolved

Screenshot

Testing the changes

This function can only be used when the workspace and security plugin are open.
If you want to turn on dynamic application configuration, you must change opensearch_dashboards.yml:

application_config.enabled: true

Below is the CURL command to view all configurations.

curl --insecure {osd endpoint}/api/appconfig -u 'admin:admin' -X GET

Below is the CURL command to view the configuration of an entity.

curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.groups -u 'admin:admin' -X GET
curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.users -u 'admin:admin' -X GET

Below is the CURL command to update the configuration of an entity.

curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.users -X POST -H 'Accept: application/json' -H 'Content-Type: application/json' -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' --data-raw '{"newValue":"[\"dashboard_admin\"]"}' -u 'admin:admin'

Below is the CURL command to delete the configuration of an entity.

curl --insecure {osd endpoint}/api/appconfig/opensearchDashboards.dashboardAdmin.users -X DELETE -H 'osd-xsrf: osd-fetch' -H 'Sec-Fetch-Dest: empty' -u 'admin:admin'

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

… test This reverts commit 47e10e4.

codecov-commenter · 2024-03-20T02:55:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.93%. Comparing base (0f34d69) to head (acfb166).
Report is 13 commits behind head on workspace-pr-integr.

Additional details and impacted files

@@                   Coverage Diff                    @@
##           workspace-pr-integr     #303       +/-   ##
========================================================
+ Coverage                35.17%   55.93%   +20.76%     
========================================================
  Files                     1885     2254      +369     
  Lines                    36421    42172     +5751     
  Branches                  6672     7487      +815     
========================================================
+ Hits                     12810    23590    +10780     
+ Misses                   22761    17225     -5536     
- Partials                   850     1357      +507

Flag	Coverage Δ
_2	`55.35% <100.00%> (?)`
_4	`35.16% <0.00%> (-0.01%)`	⬇️

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.

config/opensearch_dashboards.yml

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

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

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

wanglam · 2024-03-20T06:21:46Z

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

+      const adminGroups = config?.dashboardAdmin?.groups || [];
+      const adminUsers = config?.dashboardAdmin?.users || [];
+      const groupMatchAny = groups?.some((group) => adminGroups.includes(group)) || false;
+      const userMatchAny = users?.some((user) => adminUsers.includes(user)) || false;


Shall we remove ? after users and groups?

I will delete invalid ?

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

Hailong-am · 2024-03-20T08:17:05Z

config/opensearch_dashboards.yml

+# Set the backend roles in groups or users, whoever has the backend roles or exactly match the user ids defined in this config will be regard as dashboard admin.
+# Dashboard admin will have the access to all the workspaces and objects inside OpenSearch Dashboards.
+# workspace.dashboardAdmin.groups: ["dashboard_admin"]
+# workspace.dashboardAdmin.users: ["dashboard_admin"]


The naming workspace.dashboardAdmin may confuse me, it sounds like the admin for dashboard type only. I come few names like workspace.dashboardsAdmin,
workspace.admin or workpsace.superAdmin

workspace.admin may be confusing as admin of workspace stands for the the users who have write permission on specific workspaces, vote for superAdmin.

If there is no configuration named workspace.admin, would workspace.superAdmin also be confused?

wanglam · 2024-03-20T08:47:28Z

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

@@ -140,6 +144,24 @@ export class WorkspaceSavedObjectsClientWrapper {
    return false;
  };

+  private isDashboardAdmin(request: OpenSearchDashboardsRequest): boolean {


Prefer renaming to isRequestByDashboardAdmin.

…shboards into pr-integr-dashboard-admin

Signed-off-by: yubonluo <yubonluo@amazon.com>

src/plugins/workspace/server/plugin.ts

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

Signed-off-by: yubonluo <yubonluo@amazon.com>

wanglam · 2024-04-02T08:00:07Z

src/plugins/workspace/server/plugin.ts

@@ -83,8 +88,35 @@ export class WorkspacePlugin implements Plugin<{}, {}> {
    if (isPermissionControlEnabled) {


Shall we move the permission control logic to a separate method like setupPermission? It seems over 30 lines.

Sure, I have moved the related logic to a separate setupPermission method.

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

wanglam · 2024-04-02T08:41:36Z

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

@@ -66,8 +70,32 @@ const getDefaultValuesForEmpty = <T>(values: T[] | undefined, defaultValues: T[]
  return !values || values.length === 0 ? defaultValues : values;
 };

+export const isRequestByDashboardAdmin = (


Prefer move to some utils file

Have moved to src/plugins/workspace/server/utils.ts and add unit test.

wanglam · 2024-04-02T08:42:31Z

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

@@ -51,6 +51,8 @@ const repositoryKit = (() => {

 const permittedRequest = httpServerMock.createOpenSearchDashboardsRequest();
 const notPermittedRequest = httpServerMock.createOpenSearchDashboardsRequest();
+const groupIsDashboardAdminRequest = httpServerMock.createOpenSearchDashboardsRequest();
+const UserIdIsDashboardAdminRequest = httpServerMock.createOpenSearchDashboardsRequest();


Prefer renaming to userIdIsDashboardAdminRequest

There is no relevant logic of users/groups in the wrapper, so I uniformly use the dashboardAdminRequest name in the test file.

wanglam · 2024-04-02T08:42:56Z

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

@@ -59,6 +61,8 @@ describe('WorkspaceSavedObjectsClientWrapper', () => {
  let osd: TestOpenSearchDashboardsUtils;
  let permittedSavedObjectedClient: SavedObjectsClientContract;
  let notPermittedSavedObjectedClient: SavedObjectsClientContract;
+  let groupIsDashboardAdminSavedObjectedClient: SavedObjectsClientContract;
+  let UserIdIsdashboardAdminSavedObjectedClient: SavedObjectsClientContract;


Prefer userIdIsdashboardAdminSavedObjectedClient

There is no relevant logic of users/groups in the wrapper, so I uniformly use the dashboardAdminSavedObjectedClient name in the test file.

raintygao · 2024-04-02T08:57:15Z

src/plugins/workspace/server/plugin.ts

+            request,
+            adminGroups ? [adminGroups] : [],
+            adminUsers ? [adminUsers] : [],
+            this.permissionControl!


Should we use assert here, permissionControl is inited above and its type is fixed.

You are right, the relevant logic has been deleted.

Signed-off-by: yubonluo <yubonluo@amazon.com>

SuZhou-Joe · 2024-04-03T02:51:31Z

src/plugins/workspace/server/plugin.ts

+    groups: string[],
+    users: string[],


Nit: I'd recommend combine these two params into a param named principals: Principals.

I think using deconstructed code is more conducive to subsequent use.

SuZhou-Joe · 2024-04-03T02:52:43Z

src/plugins/workspace/server/plugin.ts

@@ -55,12 +60,98 @@ export class WorkspacePlugin implements Plugin<{}, {}> {
    });
  }

+  private isRequestByDashboardAdmin(


Can be a util function inside another file.

Have moved to src/plugins/workspace/server/utils.ts and add unit test.

SuZhou-Joe · 2024-04-03T02:53:57Z

src/plugins/workspace/server/plugin.ts

+
+  private async setupPermission(
+    core: CoreSetup,
+    config: WorkspacePluginConfigType,


Can we leverage the existing property config$?

SuZhou-Joe · 2024-04-03T02:57:41Z

src/plugins/workspace/server/plugin.ts

+      } catch (e) {
+        return toolkit.next();
+      }
+      if (groups.length === 0 && users.length === 0) {


If the cluster does not require authentication, anyone can be dashboardAdmin I think.
@ruanyl @wanglam @raintygao what do you think?

Makes sense, a bit like OSD without security plugin installed that will have almost admin permissions. Also want to hear other's thoughts.

You mean to update isDashboardAdmin state to true if groups.length === 0 && users.length === 0 when both the security plugin and savedObject.permission.enabled are open.

SuZhou-Joe · 2024-04-03T03:01:28Z

src/plugins/workspace/server/plugin.ts

+    updateWorkspaceState(request, {
+      isDashboardAdmin: groupMatchAny || userMatchAny,
+    });


From the function name, I do not think it appropriate to update anything inside this function.

Sure, I will change the function name to updateDashboardAdminStateForRequest

Signed-off-by: yubonluo <yubonluo@amazon.com>

yubonluo · 2024-04-09T03:16:09Z

@ruanyl If you have any concerns, Please help me view this PR.

SuZhou-Joe · 2024-04-10T03:15:14Z

src/plugins/workspace/server/plugin.ts

-        this.workspaceSavedObjectsClientWrapper.wrapperFactory
-      );
-    }
+    if (isPermissionControlEnabled) await this.setupPermission(core, { applicationConfig });


Seems the await is not needed here.

SuZhou-Joe · 2024-04-10T03:15:36Z

src/plugins/workspace/server/plugin.ts

@@ -64,12 +70,80 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
    });
  }

+  private async setupPermission(


No need to declare as a async function here I think

SuZhou-Joe · 2024-04-10T03:17:36Z

src/plugins/workspace/server/plugin.ts

+        updateWorkspaceState(request, {
+          isDashboardAdmin: false,
+        });


Why don't we use updateDashboardAdminStateForRequest method here?

If users/groups is null, we can prevent the program from running down in advance. Meanwhile, configGroups/configUsers has not been obtained.

SuZhou-Joe · 2024-04-10T03:19:11Z

src/plugins/workspace/server/plugin.ts

+          configGroups ? [configGroups] : [],
+          configUsers ? [configUsers] : []


Suggested change

configGroups ? [configGroups] : [],

configUsers ? [configUsers] : []

configGroups ? configGroups : [],

configUsers ? configUsers : []

configGroups and configUsers should already be array I think?

Signed-off-by: yubonluo <yubonluo@amazon.com>

…shboards into pr-integr-dashboard-admin

Signed-off-by: yubonluo <yubonluo@amazon.com>

SuZhou-Joe · 2024-04-12T03:41:23Z

src/plugins/workspace/server/utils.test.ts

+import { getWorkspaceState } from '../../../core/server/utils';
+import { AppPluginSetupDependencies } from './types';
+import { Observable, of } from 'rxjs';
+import { error } from 'console';


What's this used for?

I will use throw new Error('') to replace it.

Signed-off-by: yubonluo <yubonluo@amazon.com>

dashboard admin(groups/users) implementation and add unit/integration…

72fc938

… test This reverts commit 47e10e4.

yubonluo requested a review from SuZhou-Joe as a code owner March 20, 2024 02:30

github-actions bot added the first-time-contributor label Mar 20, 2024

SuZhou-Joe reviewed Mar 20, 2024

View reviewed changes

wanglam reviewed Mar 20, 2024

View reviewed changes

yubonluo added 2 commits March 20, 2024 15:04

Add test cases of user Id matching dashboard admin

29236e3

delete useless code

22323c1

Hailong-am reviewed Mar 20, 2024

View reviewed changes

wanglam reviewed Mar 20, 2024

View reviewed changes

yubonluo added 3 commits March 21, 2024 13:57

change isDashboard function name to isRequestByDashboardAdmin

f9666a0

Merge branch 'workspace-pr-integr' of github.com:ruanyl/OpenSearch-Da…

123c87e

…shboards into pr-integr-dashboard-admin

dashboard admin config integrating with dynamic application config

166451a

Signed-off-by: yubonluo <yubonluo@amazon.com>

yubonluo changed the title ~~dashboard admin(groups/users) implementation and add unit/integration…~~ dashboard admin(groups/users) implementation and integrating with dynamic application config Apr 2, 2024

Optimize the code

363aeaa

Signed-off-by: yubonluo <yubonluo@amazon.com>

SuZhou-Joe reviewed Apr 2, 2024

View reviewed changes

fix test error

c8bb0b5

Signed-off-by: yubonluo <yubonluo@amazon.com>

wanglam reviewed Apr 2, 2024

View reviewed changes

raintygao reviewed Apr 2, 2024

View reviewed changes

yubonluo added 3 commits April 3, 2024 09:52

delete useless code

74d9a26

Signed-off-by: yubonluo <yubonluo@amazon.com>

optimize code

a07dde0

Signed-off-by: yubonluo <yubonluo@amazon.com>

optimize code and add unit test

518cd75

Signed-off-by: yubonluo <yubonluo@amazon.com>

SuZhou-Joe reviewed Apr 3, 2024

View reviewed changes

yubonluo and others added 7 commits April 3, 2024 13:14

optimize code according to comments

dc77999

Signed-off-by: yubonluo <yubonluo@amazon.com>

change dashboardAdmin yml config to openseachDashboard

71c20d9

Signed-off-by: yubonluo <yubonluo@amazon.com>

add missed code

c61dac9

Signed-off-by: yubonluo <yubonluo@amazon.com>

solve conflict

0bf41b3

Signed-off-by: yubonluo <yubonluo@amazon.com>

optimize code

f0c1b5d

Signed-off-by: yubonluo <yubonluo@amazon.com>

Merge branch 'workspace-pr-integr' into pr-integr-dashboard-admin

2018bdf

solve conflict

979851a

Signed-off-by: yubonluo <yubonluo@amazon.com>

yubonluo added 3 commits April 9, 2024 10:26

solve conflict

3a70922

Signed-off-by: yubonluo <yubonluo@amazon.com>

delete useless code

a69cc58

Signed-off-by: yubonluo <yubonluo@amazon.com>

delete useless code

7d83f48

Signed-off-by: yubonluo <yubonluo@amazon.com>

SuZhou-Joe reviewed Apr 10, 2024

View reviewed changes

yubonluo added 5 commits April 10, 2024 17:35

optimize code

f50093f

Signed-off-by: yubonluo <yubonluo@amazon.com>

delete useless code

e9fd21a

Signed-off-by: yubonluo <yubonluo@amazon.com>

add utils unit tests

909af1c

Signed-off-by: yubonluo <yubonluo@amazon.com>

Merge branch 'workspace-pr-integr' of github.com:ruanyl/OpenSearch-Da…

b342950

…shboards into pr-integr-dashboard-admin

optimize code

27d95fe

Signed-off-by: yubonluo <yubonluo@amazon.com>

SuZhou-Joe reviewed Apr 12, 2024

View reviewed changes

Fix the wrong reference

acfb166

Signed-off-by: yubonluo <yubonluo@amazon.com>

SuZhou-Joe merged commit dffd70b into ruanyl:workspace-pr-integr Apr 12, 2024
45 checks passed

		@@ -83,8 +88,35 @@ export class WorkspacePlugin implements Plugin<{}, {}> {
		if (isPermissionControlEnabled) {

		configGroups ? [configGroups] : [],
		configUsers ? [configUsers] : []

dashboard admin(groups/users) implementation and integrating with dynamic application config #303

dashboard admin(groups/users) implementation and integrating with dynamic application config #303

Conversation

yubonluo commented Mar 20, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Check List

codecov-commenter commented Mar 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yubonluo Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuZhou-Joe Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raintygao Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

yubonluo Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yubonluo commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yubonluo commented Mar 20, 2024 •

edited

Loading

codecov-commenter commented Mar 20, 2024 •

edited

Loading

yubonluo Apr 3, 2024 •

edited

Loading

SuZhou-Joe Apr 3, 2024 •

edited

Loading

raintygao Apr 3, 2024 •

edited

Loading

yubonluo Apr 3, 2024 •

edited

Loading