fix: unit test and address comment

Signed-off-by: SuZhou-Joe <[email protected]>
wanglam · Mar 5, 2024 · 9e88cc8 · 9e88cc8
1 parent 6386883
commit 9e88cc8
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 36 deletions.
diff --git a/src/plugins/workspace/server/permission_control/client.ts b/src/plugins/workspace/server/permission_control/client.ts
@@ -6,7 +6,6 @@
 import { i18n } from '@osd/i18n';
 import {
   ACL,
-  TransformedPermission,
   SavedObjectsBulkGetObject,
   SavedObjectsServiceStart,
   Logger,
@@ -80,7 +79,7 @@ export class SavedObjectsPermissionControl {
   }
 
   public getPrincipalsFromRequest(request: OpenSearchDashboardsRequest) {
-    return getPrincipalsFromRequest(request, this.auth);
+    return getPrincipalsFromRequest(request, this.auth as HttpAuth);
   }
 
   public validateSavedObjectsACL(

diff --git a/src/plugins/workspace/server/utils.test.ts b/src/plugins/workspace/server/utils.test.ts
@@ -3,10 +3,12 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { httpServerMock } from '../../../core/server/mocks';
+import { AuthStatus } from '../../../core/server';
+import { httpServerMock, httpServiceMock } from '../../../core/server/mocks';
 import { generateRandomId, getPrincipalsFromRequest } from './utils';
 
 describe('workspace utils', () => {
+  const authMock = httpServiceMock.createAuth();
   it('should generate id with the specified size', () => {
     expect(generateRandomId(6)).toHaveLength(6);
   });
@@ -20,38 +22,54 @@ describe('workspace utils', () => {
     expect(ids.size).toBe(NUM_OF_ID);
   });
 
-  it('should return empty map when request do not have authentication', () => {
+  it('should return empty map when authentication not enabled', () => {
     const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
-    const result = getPrincipalsFromRequest(mockRequest);
+    authMock.get.mockReturnValueOnce({
+      state: {},
+      status: AuthStatus.unknown,
+    });
+    const result = getPrincipalsFromRequest(mockRequest, authMock);
     expect(result).toEqual({});
   });
 
   it('should return normally when request has authentication', () => {
-    const mockRequest = httpServerMock.createOpenSearchDashboardsRequest({
-      auth: {
-        credentials: {
-          authInfo: {
-            backend_roles: ['foo'],
-            user_name: 'bar',
-          },
-        },
-      } as any,
+    const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
+    authMock.get.mockReturnValueOnce({
+      state: {
+        backend_roles: ['foo'],
+        user_name: 'bar',
+      },
+      status: AuthStatus.authenticated,
     });
-    const result = getPrincipalsFromRequest(mockRequest);
+    const result = getPrincipalsFromRequest(mockRequest, authMock);
     expect(result.users).toEqual(['bar']);
     expect(result.groups).toEqual(['foo']);
   });
 
-  it('should return a fake user when there is auth field but no backend_roles or user name', () => {
-    const mockRequest = httpServerMock.createOpenSearchDashboardsRequest({
-      auth: {
-        credentials: {
-          authInfo: {},
-        },
-      } as any,
+  it('should throw error when reuqest is not authenticated', () => {
+    const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
+    authMock.get.mockReturnValueOnce({
+      state: {
+        backend_roles: ['foo'],
+        user_name: 'bar',
+      },
+      status: AuthStatus.unauthenticated,
+    });
+    expect(() => getPrincipalsFromRequest(mockRequest, authMock)).toThrow('NOT_AUTHORIZED');
+  });
+
+  it('should throw error when get a unknown auth status', () => {
+    const mockRequest = httpServerMock.createOpenSearchDashboardsRequest();
+    authMock.get.mockReturnValueOnce({
+      state: {
+        backend_roles: ['foo'],
+        user_name: 'bar',
+      },
+      // @ts-ignore
+      status: 'foo',
     });
-    const result = getPrincipalsFromRequest(mockRequest);
-    expect(result.users?.[0].startsWith('_user_fake_')).toEqual(true);
-    expect(result.groups).toEqual(undefined);
+    expect(() => getPrincipalsFromRequest(mockRequest, authMock)).toThrow(
+      'UNEXPECTED_AUTHORIZATION_STATUS'
+    );
   });
 });
diff --git a/src/plugins/workspace/server/utils.ts b/src/plugins/workspace/server/utils.ts
@@ -22,7 +22,7 @@ export const generateRandomId = (size: number) => {
 
 export const getPrincipalsFromRequest = (
   request: OpenSearchDashboardsRequest,
-  auth?: HttpAuth
+  auth: HttpAuth
 ): Principals => {
   const payload: Principals = {};
   const authInfoResp = auth?.get(request);
@@ -33,19 +33,23 @@ export const getPrincipalsFromRequest = (
     return payload;
   }
 
+  if (authInfoResp?.status === AuthStatus.authenticated) {
+    const authInfo = authInfoResp?.state as AuthInfo | null;
+    if (authInfo?.backend_roles) {
+      payload[PrincipalType.Groups] = authInfo.backend_roles;
+    }
+    if (authInfo?.user_name) {
+      payload[PrincipalType.Users] = [authInfo.user_name];
+    }
+    return payload;
+  }
+
   if (authInfoResp?.status === AuthStatus.unauthenticated) {
     /**
      * use a fake user that won't be granted permission explicitly when authenticated error.
      */
-    payload[PrincipalType.Users] = [`_user_fake_${Date.now()}_`];
-    return payload;
+    throw new Error('NOT_AUTHORIZED');
   }
-  const authInfo = authInfoResp?.state as AuthInfo | null;
-  if (authInfo?.backend_roles) {
-    payload[PrincipalType.Groups] = authInfo.backend_roles;
-  }
-  if (authInfo?.user_name) {
-    payload[PrincipalType.Users] = [authInfo.user_name];
-  }
-  return payload;
+
+  throw new Error('UNEXPECTED_AUTHORIZATION_STATUS');
 };