TechDebt: System User Enhancements/Fixes

api/src/paths/administrative-activity.ts

@@ -28,7 +28,16 @@ POST.apiDoc = {
         schema: {
           title: 'Administrative Activity request object',
           type: 'object',
-          properties: {}
+          additionalProperties: false,
+          properties: {
+            reason: { type: 'string', description: 'Reason for the access request.' },
+            userGuid: { type: 'string', description: 'Unique keycloak GUID for the user.' },
+            name: { type: 'string' },
+            username: { type: 'string' },
+            email: { type: 'string' },
+            identitySource: { type: 'string', description: 'Whether the account is an IDIR or BCeID.' },
+            displayName: { type: 'string' }
+          }
         }
       }
     }

api/src/paths/user/add.test.ts

@@ -8,7 +8,7 @@ import { SystemUser } from '../../repositories/user-repository';
 import { UserService } from '../../services/user-service';
 import * as keycloakUtils from '../../utils/keycloak-utils';
 import { getMockDBConnection, getRequestHandlerMocks } from '../../__mocks__/db';
-import * as user from './add';
+import { addSystemRoleUser } from './add';
 
 chai.use(sinonChai);
 
@@ -52,14 +52,17 @@ describe('user', () => {
         agency: null
       };
 
+      const getUserByIdentifierStub = sinon.stub(UserService.prototype, 'getUserByIdentifier').resolves();
+
       const ensureSystemUserStub = sinon.stub(UserService.prototype, 'ensureSystemUser').resolves(mockUserObject);
 
       const adduserSystemRolesStub = sinon.stub(UserService.prototype, 'addUserSystemRoles');
 
-      const requestHandler = user.addSystemRoleUser();
+      const requestHandler = addSystemRoleUser();
 
       await requestHandler(mockReq, mockRes, mockNext);
 
+      expect(getUserByIdentifierStub).to.have.been.calledOnce;
       expect(ensureSystemUserStub).to.have.been.calledOnce;
       expect(adduserSystemRolesStub).to.have.been.calledOnce;
     });
@@ -97,13 +100,16 @@ describe('user', () => {
         agency: null
       };
 
+      const getUserByIdentifierStub = sinon.stub(UserService.prototype, 'getUserByIdentifier').resolves();
+
       const ensureSystemUserStub = sinon.stub(UserService.prototype, 'ensureSystemUser').resolves(mockUserObject);
 
       const adduserSystemRolesStub = sinon.stub(UserService.prototype, 'addUserSystemRoles');
 
-      const requestHandler = user.addSystemRoleUser();
+      const requestHandler = addSystemRoleUser();
 
       await requestHandler(mockReq, mockRes, mockNext);
+      expect(getUserByIdentifierStub).to.have.been.calledOnce;
       expect(ensureSystemUserStub).to.have.been.calledOnce;
       expect(adduserSystemRolesStub).to.have.been.calledOnce;
     });

api/src/paths/user/add.ts

@@ -3,6 +3,7 @@ import { Operation } from 'express-openapi';
 import { SOURCE_SYSTEM, SYSTEM_IDENTITY_SOURCE } from '../../constants/database';
 import { SYSTEM_ROLE } from '../../constants/roles';
 import { getDBConnection, getServiceClientDBConnection } from '../../database/db';
+import { HTTP409 } from '../../errors/http-error';
 import { authorizeRequestHandler } from '../../request-handlers/security/authorization';
 import { UserService } from '../../services/user-service';
 import { getKeycloakSource } from '../../utils/keycloak-utils';
@@ -105,6 +106,9 @@ POST.apiDoc = {
     403: {
       $ref: '#/components/responses/403'
     },
+    409: {
+      $ref: '#/components/responses/409'
+    },
     500: {
       $ref: '#/components/responses/500'
     },
@@ -144,6 +148,14 @@ export function addSystemRoleUser(): RequestHandler {
 
       const userService = new UserService(connection);
 
+      // If user already exists, do nothing and return early
+      const user = await userService.getUserByIdentifier(userIdentifier, identitySource);
+
+      if (user) {
+        throw new HTTP409('Failed to add user. User with matching identifier already exists.');
+      }
+
+      // This function also verifies that the user exists, but we explicitly check above to return a useful error message.
       const userObject = await userService.ensureSystemUser(
         userGuid,
         userIdentifier,
@@ -166,7 +178,7 @@ export function addSystemRoleUser(): RequestHandler {
 
       return res.status(200).send();
     } catch (error) {
-      defaultLog.error({ label: 'getUser', message: 'error', error });
+      defaultLog.error({ label: 'addSystemRoleUser', message: 'error', error });
       await connection.rollback();
       throw error;
     } finally {

api/src/services/authorization-service.test.ts

@@ -137,6 +137,23 @@ describe('AuthorizationService', () => {
       expect(isAuthorizedByServiceClient).to.equal(false);
     });
 
+    it('returns false if `record_end_date` is null', async function () {
+      const mockDBConnection = getMockDBConnection();
+
+      const mockGetSystemUsersObjectResponse = {
+        record_end_date: '2021-01-01',
+        role_names: [SYSTEM_ROLE.SYSTEM_ADMIN]
+      } as unknown as SystemUser;
+
+      sinon.stub(AuthorizationService.prototype, 'getSystemUserObject').resolves(mockGetSystemUsersObjectResponse);
+
+      const authorizationService = new AuthorizationService(mockDBConnection);
+
+      const isAuthorizedByServiceClient = await authorizationService.authorizeSystemAdministrator();
+
+      expect(isAuthorizedByServiceClient).to.equal(false);
+    });
+
     it('returns true if `systemUserObject` is not null and includes admin role', async function () {
       const mockDBConnection = getMockDBConnection();
 
@@ -194,7 +211,7 @@ describe('AuthorizationService', () => {
       };
       const mockDBConnection = getMockDBConnection();
 
-      const mockGetSystemUsersObjectResponse = { record_end_date: 'datetime' } as unknown as SystemUser;
+      const mockGetSystemUsersObjectResponse = { record_end_date: '2021-01-01' } as unknown as SystemUser;
       sinon.stub(AuthorizationService.prototype, 'getSystemUserObject').resolves(mockGetSystemUsersObjectResponse);
 
       const authorizationService = new AuthorizationService(mockDBConnection);
@@ -415,7 +432,8 @@ describe('AuthorizationService', () => {
         };
         const mockDBConnection = getMockDBConnection();
 
-        const mockGetSystemUsersObjectResponse = { record_end_date: 'datetime' } as unknown as ProjectUser & SystemUser;
+        const mockGetSystemUsersObjectResponse = { record_end_date: '2021-01-01' } as unknown as ProjectUser &
+          SystemUser;
         sinon
           .stub(AuthorizationService.prototype, 'getProjectUserObjectByProjectId')
           .resolves(mockGetSystemUsersObjectResponse);
@@ -538,7 +556,8 @@ describe('AuthorizationService', () => {
         };
         const mockDBConnection = getMockDBConnection();
 
-        const mockGetSystemUsersObjectResponse = { record_end_date: 'datetime' } as unknown as ProjectUser & SystemUser;
+        const mockGetSystemUsersObjectResponse = { record_end_date: '2021-01-01' } as unknown as ProjectUser &
+          SystemUser;
         sinon
           .stub(AuthorizationService.prototype, 'getProjectUserObjectByProjectId')
           .resolves(mockGetSystemUsersObjectResponse);

api/src/services/authorization-service.ts

@@ -199,6 +199,11 @@ export class AuthorizationService extends DBService {
     // Cache the _systemUser for future use, if needed
     this._systemUser = systemUserObject;
 
+    if (systemUserObject.record_end_date) {
+      // system user has an expired record
+      return false;
+    }
+
     return systemUserObject.role_names.includes(SYSTEM_ROLE.SYSTEM_ADMIN);
   }
 

app/src/features/admin/users/AccessRequestList.test.tsx

@@ -74,7 +74,6 @@ describe('AccessRequestList', () => {
             username: 'testusername',
             userGuid: 'aaaa',
             email: 'email@email.com',
-            role: 2,
             identitySource: SYSTEM_IDENTITY_SOURCE.IDIR,
             displayName: 'test user',
             company: 'test company',
@@ -110,7 +109,6 @@ describe('AccessRequestList', () => {
             username: 'testusername',
             userGuid: 'aaaa',
             email: 'email@email.com',
-            role: 2,
             identitySource: SYSTEM_IDENTITY_SOURCE.IDIR,
             displayName: 'test user',
             company: 'test company',
@@ -147,7 +145,6 @@ describe('AccessRequestList', () => {
             username: 'testusername',
             userGuid: 'aaaa',
             email: 'email@email.com',
-            role: 2,
             identitySource: SYSTEM_IDENTITY_SOURCE.IDIR,
             displayName: 'test user',
             company: 'test company',
@@ -211,7 +208,6 @@ describe('AccessRequestList', () => {
             username: 'testusername',
             userGuid: 'aaaa',
             email: 'email@email.com',
-            role: 2,
             identitySource: SYSTEM_IDENTITY_SOURCE.IDIR,
             displayName: 'test user',
             company: 'test company',
@@ -240,7 +236,7 @@ describe('AccessRequestList', () => {
         displayName: 'test user',
         email: 'email@email.com',
         identitySource: 'IDIR',
-        roleIds: [2],
+        roleIds: [],
         userGuid: 'aaaa',
         userIdentifier: 'testusername'
       });
@@ -265,7 +261,6 @@ describe('AccessRequestList', () => {
             username: 'testusername',
             userGuid: 'aaaa',
             email: 'email@email.com',
-            role: 1,
             identitySource: SYSTEM_IDENTITY_SOURCE.IDIR,
             displayName: 'test user',
             company: 'test company',

app/src/features/admin/users/AccessRequestList.tsx

@@ -14,11 +14,7 @@ import { AdministrativeActivityStatusType } from 'constants/misc';
 import { DialogContext } from 'contexts/dialogContext';
 import { APIError } from 'hooks/api/useAxios';
 import { useBiohubApi } from 'hooks/useBioHubApi';
-import {
-  IAccessRequestDataObject,
-  IGetAccessRequestsListResponse,
-  IIDIRAccessRequestDataObject
-} from 'interfaces/useAdminApi.interface';
+import { IGetAccessRequestsListResponse } from 'interfaces/useAdminApi.interface';
 import { IGetAllCodeSetsResponse } from 'interfaces/useCodesApi.interface';
 import { useContext, useState } from 'react';
 import { getFormattedDate } from 'utils/Utils';
@@ -245,11 +241,7 @@ const AccessRequestList = (props: IAccessRequestListProps) => {
         onDeny={handleReviewDialogDeny}
         onApprove={handleReviewDialogApprove}
         component={{
-          initialValues: {
-            ...ReviewAccessRequestFormInitialValues,
-            system_role:
-              (activeReview?.data as (IAccessRequestDataObject & IIDIRAccessRequestDataObject) | undefined)?.role ?? ''
-          },
+          initialValues: ReviewAccessRequestFormInitialValues,
           validationSchema: ReviewAccessRequestFormYupSchema,
           element: activeReview ? (
             <ReviewAccessRequestForm

app/src/features/admin/users/ActiveUsersList.tsx

@@ -303,19 +303,34 @@ const ActiveUsersList = (props: IActiveUsersListProps) => {
       });
     } catch (error) {
       const apiError = error as APIError;
-      dialogContext.setErrorDialog({
-        open: true,
-        dialogTitle: AddSystemUserI18N.addUserErrorTitle,
-        dialogText: AddSystemUserI18N.addUserErrorText,
-        dialogError: apiError.message,
-        dialogErrorDetails: apiError.errors,
-        onClose: () => {
-          dialogContext.setErrorDialog({ open: false });
-        },
-        onOk: () => {
-          dialogContext.setErrorDialog({ open: false });
-        }
-      });
+
+      if (apiError.status === 409) {
+        dialogContext.setErrorDialog({
+          open: true,
+          dialogTitle: 'Failed to create users',
+          dialogText: 'One of the users you added already exists.',
+          onClose: () => {
+            dialogContext.setErrorDialog({ open: false });
+          },
+          onOk: () => {
+            dialogContext.setErrorDialog({ open: false });
+          }
+        });
+      } else {
+        dialogContext.setErrorDialog({
+          open: true,
+          dialogTitle: AddSystemUserI18N.addUserErrorTitle,
+          dialogText: AddSystemUserI18N.addUserErrorText,
+          dialogError: apiError.message,
+          dialogErrorDetails: apiError.errors,
+          onClose: () => {
+            dialogContext.setErrorDialog({ open: false });
+          },
+          onOk: () => {
+            dialogContext.setErrorDialog({ open: false });
+          }
+        });
+      }
     }
   };
 

app/src/features/admin/users/ReviewAccessRequestForm.test.tsx

@@ -27,7 +27,6 @@ describe('ReviewAccessRequestForm', () => {
           email: 'test data email',
           identitySource: SYSTEM_IDENTITY_SOURCE.IDIR,
           displayName: 'test user',
-          role: 2,
           reason: 'reason for request'
         }
       };

app/src/interfaces/useAdminApi.interface.ts

@@ -1,5 +1,4 @@
 export type IIDIRAccessRequestDataObject = {
-  role: number;
   reason: string;
 };
 

app/src/pages/access/AccessRequestPage.test.tsx

@@ -15,9 +15,6 @@ jest.mock('../../hooks/useBioHubApi');
 const mockBiohubApi = useBiohubApi as jest.Mock;
 
 const mockUseApi = {
-  codes: {
-    getAllCodeSets: jest.fn<Promise<object>, []>()
-  },
   admin: {
     createAdministrativeActivity: jest.fn()
   }
@@ -42,7 +39,6 @@ const renderContainer = () => {
 describe('AccessRequestPage', () => {
   beforeEach(() => {
     mockBiohubApi.mockImplementation(() => mockUseApi);
-    mockUseApi.codes.getAllCodeSets.mockClear();
     mockUseApi.admin.createAdministrativeActivity.mockClear();
   });
 
@@ -54,10 +50,6 @@ describe('AccessRequestPage', () => {
     const history = createMemoryHistory();
 
     it('should call the auth signoutRedirect function', async () => {
-      mockUseApi.codes.getAllCodeSets.mockResolvedValue({
-        system_roles: [{ id: 1, name: 'Creator' }]
-      });
-
       const signoutRedirectStub = jest.fn();
 
       const authState = getMockAuthState({
@@ -90,10 +82,6 @@ describe('AccessRequestPage', () => {
   });
 
   it.skip('processes a successful request submission', async () => {
-    mockUseApi.codes.getAllCodeSets.mockResolvedValue({
-      system_roles: [{ id: 1, name: 'Creator' }]
-    });
-
     mockUseApi.admin.createAdministrativeActivity.mockResolvedValue({
       id: 1
     });
@@ -118,10 +106,6 @@ describe('AccessRequestPage', () => {
   });
 
   it.skip('takes the user to the request-submitted page immediately if they already have an access request', async () => {
-    mockUseApi.codes.getAllCodeSets.mockResolvedValue({
-      system_roles: [{ id: 1, name: 'Creator' }]
-    });
-
     const authState = getMockAuthState({ base: SystemAdminAuthState });
 
     render(
@@ -140,10 +124,6 @@ describe('AccessRequestPage', () => {
   });
 
   it.skip('shows error dialog with api error message when submission fails', async () => {
-    mockUseApi.codes.getAllCodeSets.mockResolvedValue({
-      system_roles: [{ id: 1, name: 'Creator' }]
-    });
-
     mockUseApi.admin.createAdministrativeActivity = jest.fn(() => Promise.reject(new Error('API Error is Here')));
 
     const { getByText, getAllByRole, getByRole, queryByText } = renderContainer();
@@ -172,10 +152,6 @@ describe('AccessRequestPage', () => {
   });
 
   it.skip('shows error dialog with default error message when response from createAdministrativeActivity is invalid', async () => {
-    mockUseApi.codes.getAllCodeSets.mockResolvedValue({
-      system_roles: [{ id: 1, name: 'Creator' }]
-    });
-
     mockUseApi.admin.createAdministrativeActivity.mockResolvedValue({
       id: null
     });