feat: address concerns on ensureRawRequest (#4)

* feat: address concerns on ensureRawRequest Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: add check for empty array Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: make find api backward compatible Signed-off-by: SuZhou-Joe <suzhou@amazon.com> * feat: remove useless code Signed-off-by: SuZhou-Joe <suzhou@amazon.com> --------- Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
wanglam · Mar 5, 2024 · baac4c9 · baac4c9
1 parent 1a4b909
commit baac4c9
Show file tree

Hide file tree

Showing 9 changed files with 85 additions and 42 deletions.
diff --git a/src/core/server/http/index.ts b/src/core/server/http/index.ts
@@ -74,7 +74,6 @@ export {
   RouteValidationResultFactory,
   DestructiveRouteMethod,
   SafeRouteMethod,
-  ensureRawRequest,
 } from './router';
 export { BasePathProxyServer } from './base_path_proxy_server';
 export { OnPreRoutingHandler, OnPreRoutingToolkit } from './lifecycle/on_pre_routing';

diff --git a/src/core/server/index.ts b/src/core/server/index.ts
@@ -220,7 +220,6 @@ export {
   SessionStorageFactory,
   DestructiveRouteMethod,
   SafeRouteMethod,
-  ensureRawRequest,
 } from './http';
 
 export {

diff --git a/src/core/server/saved_objects/service/lib/search_dsl/query_params.ts b/src/core/server/saved_objects/service/lib/search_dsl/query_params.ts
@@ -287,15 +287,15 @@ export function getQueryParams({
 
   if (ACLSearchParams) {
     const shouldClause: any = [];
-    if (ACLSearchParams.permissionModes && ACLSearchParams.principals) {
+    if (ACLSearchParams.permissionModes?.length && ACLSearchParams.principals) {
       const permissionDSL = ACL.generateGetPermittedSavedObjectsQueryDSL(
         ACLSearchParams.permissionModes,
         ACLSearchParams.principals
       );
       shouldClause.push(permissionDSL.query);
     }
 
-    if (ACLSearchParams.workspaces) {
+    if (ACLSearchParams.workspaces?.length) {
       shouldClause.push({
         terms: {
           workspaces: ACLSearchParams.workspaces,
@@ -306,7 +306,28 @@ export function getQueryParams({
     if (shouldClause.length) {
       bool.filter.push({
         bool: {
-          should: shouldClause,
+          should: [
+            /**
+             * Return those objects without workspaces field and permissions field to keep find find API backward compatible
+             */
+            {
+              bool: {
+                must_not: [
+                  {
+                    exists: {
+                      field: 'workspaces',
+                    },
+                  },
+                  {
+                    exists: {
+                      field: 'permissions',
+                    },
+                  },
+                ],
+              },
+            },
+            ...shouldClause,
+          ],
         },
       });
     }

diff --git a/src/plugins/workspace/server/permission_control/client.test.ts b/src/plugins/workspace/server/permission_control/client.test.ts
@@ -5,9 +5,18 @@
 
 import { loggerMock } from '@osd/logging/target/mocks';
 import { SavedObjectsPermissionControl } from './client';
-import { httpServerMock, savedObjectsClientMock } from '../../../../core/server/mocks';
+import {
+  httpServerMock,
+  httpServiceMock,
+  savedObjectsClientMock,
+} from '../../../../core/server/mocks';
+import * as utilsExports from '../utils';
 
-describe('workspace utils', () => {
+describe('PermissionControl', () => {
+  jest.spyOn(utilsExports, 'getPrincipalsFromRequest').mockImplementation(() => ({
+    users: ['bar'],
+  }));
+  const mockAuth = httpServiceMock.createAuth();
   it('should return principals when calling getPrincipalsOfObjects', async () => {
     const permissionControlClient = new SavedObjectsPermissionControl(loggerMock.create());
     const getScopedClient = jest.fn();
@@ -27,7 +36,7 @@ describe('workspace utils', () => {
       });
       return clientMock;
     });
-    permissionControlClient.setup(getScopedClient);
+    permissionControlClient.setup(getScopedClient, mockAuth);
     const result = await permissionControlClient.getPrincipalsOfObjects(
       httpServerMock.createOpenSearchDashboardsRequest(),
       []
@@ -50,7 +59,7 @@ describe('workspace utils', () => {
     getScopedClient.mockImplementation((request) => {
       return clientMock;
     });
-    permissionControlClient.setup(getScopedClient);
+    permissionControlClient.setup(getScopedClient, mockAuth);
     clientMock.bulkGet.mockResolvedValue({
       saved_objects: [],
     });
@@ -69,7 +78,7 @@ describe('workspace utils', () => {
     getScopedClient.mockImplementation((request) => {
       return clientMock;
     });
-    permissionControlClient.setup(getScopedClient);
+    permissionControlClient.setup(getScopedClient, mockAuth);
 
     clientMock.bulkGet.mockResolvedValue({
       saved_objects: [
@@ -102,7 +111,7 @@ describe('workspace utils', () => {
     getScopedClient.mockImplementation((request) => {
       return clientMock;
     });
-    permissionControlClient.setup(getScopedClient);
+    permissionControlClient.setup(getScopedClient, mockAuth);
 
     clientMock.bulkGet.mockResolvedValue({
       saved_objects: [
@@ -126,19 +135,23 @@ describe('workspace utils', () => {
       ],
     });
     const batchValidateResult = await permissionControlClient.batchValidate(
-      httpServerMock.createOpenSearchDashboardsRequest({
-        auth: {
-          credentials: {
-            authInfo: {
-              user_name: 'bar',
-            },
-          },
-        } as any,
-      }),
+      httpServerMock.createOpenSearchDashboardsRequest(),
       [],
       ['read']
     );
     expect(batchValidateResult.success).toEqual(true);
     expect(batchValidateResult.result).toEqual(true);
   });
+
+  describe('getPrincipalsFromRequest', () => {
+    const permissionControlClient = new SavedObjectsPermissionControl(loggerMock.create());
+    const getScopedClient = jest.fn();
+    permissionControlClient.setup(getScopedClient, mockAuth);
+
+    it('should return normally when calling getPrincipalsFromRequest', () => {
+      const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
+      const result = permissionControlClient.getPrincipalsFromRequest(mockRequest);
+      expect(result.users).toEqual(['bar']);
+    });
+  });
 });
diff --git a/src/plugins/workspace/server/permission_control/client.ts b/src/plugins/workspace/server/permission_control/client.ts
@@ -15,6 +15,7 @@ import {
   SavedObject,
   WORKSPACE_TYPE,
   Permissions,
+  HttpAuth,
 } from '../../../../core/server';
 import { WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID } from '../../common/constants';
 import { getPrincipalsFromRequest } from '../utils';
@@ -29,6 +30,7 @@ export type SavedObjectsPermissionModes = string[];
 export class SavedObjectsPermissionControl {
   private readonly logger: Logger;
   private _getScopedClient?: SavedObjectsServiceStart['getScopedClient'];
+  private auth?: HttpAuth;
   private getScopedClient(request: OpenSearchDashboardsRequest) {
     return this._getScopedClient?.(request, {
       excludedWrappers: [WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID],
@@ -46,8 +48,9 @@ export class SavedObjectsPermissionControl {
   ) {
     return (await this.getScopedClient?.(request)?.bulkGet(savedObjects))?.saved_objects || [];
   }
-  public async setup(getScopedClient: SavedObjectsServiceStart['getScopedClient']) {
+  public async setup(getScopedClient: SavedObjectsServiceStart['getScopedClient'], auth: HttpAuth) {
     this._getScopedClient = getScopedClient;
+    this.auth = auth;
   }
   public async validate(
     request: OpenSearchDashboardsRequest,
@@ -76,6 +79,10 @@ export class SavedObjectsPermissionControl {
     );
   }
 
+  public getPrincipalsFromRequest(request: OpenSearchDashboardsRequest) {
+    return getPrincipalsFromRequest(request, this.auth);
+  }
+
   public validateSavedObjectsACL(
     savedObjects: Array<Pick<SavedObject<unknown>, 'id' | 'type' | 'workspaces' | 'permissions'>>,
     principals: Principals,
@@ -137,7 +144,7 @@ export class SavedObjectsPermissionControl {
       };
     }
 
-    const principals = getPrincipalsFromRequest(request);
+    const principals = this.getPrincipalsFromRequest(request);
     const deniedObjects: Array<
       Pick<SavedObjectsBulkGetObject, 'id' | 'type'> & {
         workspaces?: string[];

diff --git a/src/plugins/workspace/server/plugin.ts b/src/plugins/workspace/server/plugin.ts
@@ -95,6 +95,7 @@ export class WorkspacePlugin implements Plugin<{}, {}> {
       http: core.http,
       logger: this.logger,
       client: this.client as IWorkspaceClientImpl,
+      permissionControlClient: this.permissionControl,
     });
 
     core.capabilities.registerProvider(() => ({
@@ -111,7 +112,7 @@ export class WorkspacePlugin implements Plugin<{}, {}> {
 
   public start(core: CoreStart) {
     this.logger.debug('Starting Workspace service');
-    this.permissionControl?.setup(core.savedObjects.getScopedClient);
+    this.permissionControl?.setup(core.savedObjects.getScopedClient, core.http.auth);
     this.client?.setSavedObjects(core.savedObjects);
     this.workspaceConflictControl?.setSerializer(core.savedObjects.createSerializer());
     this.workspaceSavedObjectsClientWrapper?.setScopedClient(core.savedObjects.getScopedClient);

diff --git a/src/plugins/workspace/server/routes/index.ts b/src/plugins/workspace/server/routes/index.ts
@@ -4,9 +4,10 @@
  */
 
 import { schema } from '@osd/config-schema';
-import { CoreSetup, Logger, ensureRawRequest } from '../../../../core/server';
+import { CoreSetup, Logger } from '../../../../core/server';
 import { WorkspacePermissionMode } from '../../common/constants';
-import { AuthInfo, IWorkspaceClientImpl, WorkspacePermissionItem } from '../types';
+import { IWorkspaceClientImpl, WorkspacePermissionItem } from '../types';
+import { SavedObjectsPermissionControlContract } from '../permission_control/client';
 
 const WORKSPACES_API_BASE_URL = '/api/workspaces';
 
@@ -44,10 +45,12 @@ export function registerRoutes({
   client,
   logger,
   http,
+  permissionControlClient,
 }: {
   client: IWorkspaceClientImpl;
   logger: Logger;
   http: CoreSetup['http'];
+  permissionControlClient?: SavedObjectsPermissionControlContract;
 }) {
   const router = http.createRouter();
   router.post(
@@ -124,8 +127,7 @@ export function registerRoutes({
     },
     router.handleLegacyErrors(async (context, req, res) => {
       const { attributes, permissions: permissionsInRequest } = req.body;
-      const rawRequest = ensureRawRequest(req);
-      const authInfo = rawRequest?.auth?.credentials?.authInfo as AuthInfo | null;
+      const authInfo = permissionControlClient?.getPrincipalsFromRequest(req);
       let permissions: WorkspacePermissionItem[] = [];
       if (permissionsInRequest) {
         permissions = Array.isArray(permissionsInRequest)
@@ -134,10 +136,10 @@ export function registerRoutes({
       }
 
       // Assign workspace owner to current user
-      if (!!authInfo?.user_name) {
+      if (!!authInfo?.users?.length) {
         permissions.push({
           type: 'user',
-          userId: authInfo.user_name,
+          userId: authInfo.users[0],
           modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write],
         });
       }

diff --git a/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts b/src/plugins/workspace/server/saved_objects/workspace_saved_objects_client_wrapper.ts
@@ -28,7 +28,6 @@ import {
   SavedObjectsClientContract,
 } from '../../../../core/server';
 import { SavedObjectsPermissionControlContract } from '../permission_control/client';
-import { getPrincipalsFromRequest } from '../utils';
 import {
   WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID,
   WorkspacePermissionMode,
@@ -190,7 +189,7 @@ export class WorkspaceSavedObjectsClientWrapper {
     if (savedObject.permissions) {
       hasPermission = await this.permissionControl.validateSavedObjectsACL(
         [savedObject],
-        getPrincipalsFromRequest(request),
+        this.permissionControl.getPrincipalsFromRequest(request),
         objectPermissionModes
       );
     }
@@ -422,7 +421,7 @@ export class WorkspaceSavedObjectsClientWrapper {
     const findWithWorkspacePermissionControl = async <T = unknown>(
       options: SavedObjectsFindOptions
     ) => {
-      const principals = getPrincipalsFromRequest(wrapperOptions.request);
+      const principals = this.permissionControl.getPrincipalsFromRequest(wrapperOptions.request);
       if (!options.ACLSearchParams) {
         options.ACLSearchParams = {};
       }
@@ -497,7 +496,6 @@ export class WorkspaceSavedObjectsClientWrapper {
            * Select all the docs that
            * 1. ACL matches read / write / user passed permission OR
            * 2. workspaces matches library_read or library_write OR
-           * 3. Advanced settings
            */
           options.workspaces = undefined;
           options.ACLSearchParams.workspaces = permittedWorkspaceIds;

diff --git a/src/plugins/workspace/server/utils.ts b/src/plugins/workspace/server/utils.ts
@@ -5,7 +5,8 @@
 
 import crypto from 'crypto';
 import {
-  ensureRawRequest,
+  AuthStatus,
+  HttpAuth,
   OpenSearchDashboardsRequest,
   Principals,
   PrincipalType,
@@ -19,25 +20,27 @@ export const generateRandomId = (size: number) => {
   return crypto.randomBytes(size).toString('base64url').slice(0, size);
 };
 
-export const getPrincipalsFromRequest = (request: OpenSearchDashboardsRequest): Principals => {
-  const rawRequest = ensureRawRequest(request);
-  const authInfo = rawRequest?.auth?.credentials?.authInfo as AuthInfo | null;
+export const getPrincipalsFromRequest = (
+  request: OpenSearchDashboardsRequest,
+  auth?: HttpAuth
+): Principals => {
   const payload: Principals = {};
-  if (!authInfo) {
+  const authInfoResp = auth?.get(request);
+  if (authInfoResp?.status === AuthStatus.unknown) {
     /**
      * Login user have access to all the workspaces when no authentication is presented.
-     * The logic will be used when users create workspaces with authentication enabled but turn off authentication for any reason.
      */
     return payload;
   }
-  if (!authInfo?.backend_roles?.length && !authInfo.user_name) {
+
+  if (authInfoResp?.status === AuthStatus.unauthenticated) {
     /**
-     * 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.
+     * use a fake user that won't be granted permission explicitly when authenticated error.
      */
     payload[PrincipalType.Users] = [`_user_fake_${Date.now()}_`];
     return payload;
   }
+  const authInfo = authInfoResp?.state as AuthInfo | null;
   if (authInfo?.backend_roles) {
     payload[PrincipalType.Groups] = authInfo.backend_roles;
   }