Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SIMSBIOHUB-154: System user table updates #1060

Merged
merged 19 commits into from
Aug 10, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions api/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions api/src/__mocks__/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Request, Response } from 'express';
import { QueryResult } from 'pg';
import sinon from 'sinon';
import { IDBConnection } from '../database/db';
import { KeycloakUserInformation } from '../utils/keycloak-utils';

/**
* Returns a mock `IDBConnection` with empty methods.
Expand All @@ -14,6 +15,9 @@ export const getMockDBConnection = (config?: Partial<IDBConnection>): IDBConnect
systemUserId: () => {
return (null as unknown) as number;
},
keycloakUserInformation: () => {
return (null as unknown) as KeycloakUserInformation;
},
open: async () => {
// do nothing
},
Expand Down
2 changes: 1 addition & 1 deletion api/src/constants/database.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export enum SYSTEM_IDENTITY_SOURCE {
IDIR = 'IDIR',
BCEID_BASIC = 'BCEIDBASIC',
BCEID_BUSINESS = 'BCEIDBUSINESS',
SYSTEM = 'SYSTEM'
UNVERIFIED = 'UNVERIFIED'
}

export enum SCHEMAS {
Expand Down
17 changes: 12 additions & 5 deletions api/src/database/db.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,16 @@ describe('db', () => {
const sinonSandbox = Sinon.createSandbox();

const mockKeycloakToken = {
preferred_username: 'testguid@idir',
idir_user_guid: 'testguid',
identity_provider: 'idir',
idir_username: 'testuser',
identity_provider: SYSTEM_IDENTITY_SOURCE.IDIR
email_verified: false,
name: 'test user',
preferred_username: 'testguid@idir',
display_name: 'test user',
given_name: 'test',
family_name: 'user',
email: '[email protected]'
};

const queryStub = sinonSandbox.stub().resolves();
Expand Down Expand Up @@ -371,9 +378,9 @@ describe('db', () => {
const DB_USERNAME = process.env.DB_USER_API;

expect(getDBConnectionStub).to.have.been.calledWith({
preferred_username: `${DB_USERNAME}@database`,
sims_system_username: DB_USERNAME,
identity_provider: 'database'
database_user_guid: DB_USERNAME,
identity_provider: SYSTEM_IDENTITY_SOURCE.DATABASE.toLowerCase(),
username: DB_USERNAME
});
});
});
Expand Down
100 changes: 52 additions & 48 deletions api/src/database/db.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@
import * as pg from 'pg';
import SQL, { SQLStatement } from 'sql-template-strings';
import { z } from 'zod';
import { SYSTEM_IDENTITY_SOURCE } from '../constants/database';
import { ApiExecuteSQLError, ApiGeneralError } from '../errors/api-error';
import { getUserGuid, getUserIdentifier, getUserIdentitySource } from '../utils/keycloak-utils';
import {
getKeycloakUserInformationFromKeycloakToken,
getUserGuid,
getUserIdentitySource,
KeycloakUserInformation
} from '../utils/keycloak-utils';
import { getLogger } from '../utils/logger';
import { asyncErrorWrapper, getZodQueryResult, syncErrorWrapper } from './db-utils';

Expand Down Expand Up @@ -158,12 +164,19 @@
/**
* Get the ID of the system user in context.
*
* Note: will always return `null` if the connection is not open.
*
* @throws If the connection is not open.
* @memberof IDBConnection
*/
systemUserId: () => number;
/**
* Get the currently logged in user's keycloak information.
*
* Note: This is only a subset of the full token, specifically the fields that hold user details (name, email, etc).
*
* @throws If the connection is not open.
* @memberof IDBConnection
*/
keycloakUserInformation: () => KeycloakUserInformation;
}

/**
Expand Down Expand Up @@ -201,6 +214,8 @@

let _systemUserId: number | null = null;

let _keycloakUserInformation: KeycloakUserInformation | null;

const _token = keycloakToken;

/**
Expand Down Expand Up @@ -305,6 +320,14 @@
return _systemUserId as number;
};

const _getKeycloakUserInformation = (): KeycloakUserInformation => {
if (!_client || !_isOpen) {
throw Error('DBConnection is not open');

Check warning on line 325 in api/src/database/db.ts

View check run for this annotation

Codecov / codecov/patch

api/src/database/db.ts#L325

Added line #L325 was not covered by tests
}

return _keycloakUserInformation as KeycloakUserInformation;

Check warning on line 328 in api/src/database/db.ts

View check run for this annotation

Codecov / codecov/patch

api/src/database/db.ts#L328

Added line #L328 was not covered by tests
};

/**
* Performs a query against this connection, returning the results.
*
Expand Down Expand Up @@ -359,58 +382,38 @@
* Sets the _systemUserId if successful.
*/
const _setUserContext = async () => {
const userGuid = getUserGuid(_token);
const userIdentifier = getUserIdentifier(_token);
const userIdentitySource = getUserIdentitySource(_token);
defaultLog.debug({ label: '_setUserContext', userGuid, userIdentifier, userIdentitySource });
_keycloakUserInformation = getKeycloakUserInformationFromKeycloakToken(_token);

if (!userGuid || !userIdentifier || !userIdentitySource) {
if (!_keycloakUserInformation) {
throw new ApiGeneralError('Failed to identify authenticated user');
}

// Patch user GUID
const patchUserGuidSqlStatement = SQL`
UPDATE
system_user
SET
user_guid = ${userGuid.toLowerCase()}
WHERE
system_user_id
IN (
SELECT
su.system_user_id
FROM
system_user su
LEFT JOIN
user_identity_source uis
ON
uis.user_identity_source_id = su.user_identity_source_id
WHERE
su.user_identifier ILIKE ${userIdentifier}
AND
uis.name ILIKE ${userIdentitySource}
AND
user_guid IS NULL
);
`;
defaultLog.debug({ label: '_setUserContext', _keycloakUserInformation });

const userGuid = getUserGuid(_keycloakUserInformation);
const userIdentitySource = getUserIdentitySource(_keycloakUserInformation);

try {
// Set the user context in the database, so database queries are aware of the calling user when writing to audit
// tables, etc.
_systemUserId = await _setSystemUserContext(userGuid, userIdentitySource);
} catch (error) {
throw new ApiExecuteSQLError('Failed to set user context', [error as object]);

Check warning on line 401 in api/src/database/db.ts

View check run for this annotation

Codecov / codecov/patch

api/src/database/db.ts#L401

Added line #L401 was not covered by tests
}
};

const _setSystemUserContext = async (userGuid: string, userIdentitySource: SYSTEM_IDENTITY_SOURCE) => {
// Set the user context for all queries made using this connection
const setSystemUserContextSQLStatement = SQL`
SELECT api_set_context(${userGuid}, ${userIdentitySource});
`;

try {
await _client.query(patchUserGuidSqlStatement.text, patchUserGuidSqlStatement.values);

const response = await _client.query(
setSystemUserContextSQLStatement.text,
setSystemUserContextSQLStatement.values
);
const response = await _client.query(
setSystemUserContextSQLStatement.text,
setSystemUserContextSQLStatement.values
);

_systemUserId = response?.rows?.[0].api_set_context;
} catch (error) {
throw new ApiExecuteSQLError('Failed to set user context', [error as object]);
}
return response?.rows?.[0].api_set_context;
};

return {
Expand All @@ -421,7 +424,8 @@
release: syncErrorWrapper(_release),
commit: asyncErrorWrapper(_commit),
rollback: asyncErrorWrapper(_rollback),
systemUserId: syncErrorWrapper(_getSystemUserID)
systemUserId: syncErrorWrapper(_getSystemUserID),
keycloakUserInformation: syncErrorWrapper(_getKeycloakUserInformation)
};
};

Expand All @@ -435,9 +439,9 @@
*/
export const getAPIUserDBConnection = (): IDBConnection => {
return getDBConnection({
preferred_username: `${getDbUsername()}@database`,
sims_system_username: getDbUsername(),
identity_provider: 'database'
database_user_guid: getDbUsername(),
identity_provider: SYSTEM_IDENTITY_SOURCE.DATABASE.toLowerCase(),
username: getDbUsername()
});
};

Expand Down
22 changes: 20 additions & 2 deletions api/src/paths/administrative-activity.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,16 @@ describe('getAdministrativeActivityStanding', () => {
const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();

mockReq['keycloak_token'] = {
idir_username: 'username'
idir_user_guid: 'testguid',
identity_provider: 'idir',
idir_username: 'testuser',
email_verified: false,
name: 'test user',
preferred_username: 'testguid@idir',
display_name: 'test user',
given_name: 'test',
family_name: 'user',
email: '[email protected]'
};
mockReq.body = {
myData: 'the data'
Expand Down Expand Up @@ -150,7 +159,16 @@ describe('getAdministrativeActivityStanding', () => {
const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();

mockReq['keycloak_token'] = {
idir_username: 'username'
idir_user_guid: 'testguid',
identity_provider: 'idir',
idir_username: 'testuser',
email_verified: false,
name: 'test user',
preferred_username: 'testguid@idir',
display_name: 'test user',
given_name: 'test',
family_name: 'user',
email: '[email protected]'
};
mockReq.body = {
myData: 'the data'
Expand Down
11 changes: 7 additions & 4 deletions api/src/paths/administrative-activity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import { getAPIUserDBConnection } from '../database/db';
import { HTTP400, HTTP500 } from '../errors/http-error';
import { AdministrativeActivityService } from '../services/administrative-activity-service';
import { getUserIdentifier } from '../utils/keycloak-utils';
import { getKeycloakUserInformationFromKeycloakToken, getUserIdentifier } from '../utils/keycloak-utils';
import { getLogger } from '../utils/logger';

const defaultLog = getLogger('paths/administrative-activity-request');
Expand Down Expand Up @@ -166,12 +166,15 @@
const connection = getAPIUserDBConnection();

try {
const userIdentifier = getUserIdentifier(req['keycloak_token']);
const keycloakUserInformation = getKeycloakUserInformationFromKeycloakToken(req['keycloak_token']);

if (!userIdentifier) {
throw new HTTP400('Missing required userIdentifier');
if (!keycloakUserInformation) {
throw new HTTP400('Failed to identify user');

Check warning on line 172 in api/src/paths/administrative-activity.ts

View check run for this annotation

Codecov / codecov/patch

api/src/paths/administrative-activity.ts#L172

Added line #L172 was not covered by tests
}

// TODO Update to use user guid instead of identifier
const userIdentifier = getUserIdentifier(keycloakUserInformation);

await connection.open();

const administrativeActivityService = new AdministrativeActivityService(connection);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ PUT.apiDoc = {
'application/json': {
schema: {
type: 'object',
required: ['userGuid', 'userIdentifier', 'identitySource'],
required: ['userGuid', 'userIdentifier', 'identitySource', 'displayName', 'email'],
properties: {
userGuid: {
type: 'string',
Expand All @@ -75,6 +75,14 @@ PUT.apiDoc = {
type: 'string',
enum: AllUserIdentitySources
},
displayName: {
type: 'string',
description: 'The display name for the user.'
},
email: {
type: 'string',
description: 'The email for the user.'
},
roleIds: {
type: 'array',
items: {
Expand Down Expand Up @@ -116,6 +124,8 @@ export function approveAccessRequest(): RequestHandler {

const userGuid = req.body.userGuid;
const userIdentifier = req.body.userIdentifier;
const displayName = req.body.displayName;
const email = req.body.email;

// Convert identity sources that have multiple variations (ie: BCEID) into a single value supported by this app
const identitySource = req.body.identitySource && coerceUserIdentitySource(req.body.identitySource);
Expand All @@ -137,7 +147,13 @@ export function approveAccessRequest(): RequestHandler {
const administrativeActivityService = new AdministrativeActivityService(connection);

// Get the system user (adding or activating them if they already existed).
const systemUserObject = await userService.ensureSystemUser(userGuid, userIdentifier, identitySource);
const systemUserObject = await userService.ensureSystemUser(
userGuid,
userIdentifier,
identitySource,
displayName,
email
);

// Filter out any system roles that have already been added to the user
const rolesIdsToAdd = roleIds.filter((roleId) => !systemUserObject.role_ids.includes(roleId));
Expand Down
Loading
Loading