diff --git a/src/smartAuthorizationHelper.test.ts b/src/smartAuthorizationHelper.test.ts index 1d72cf4..f8fb1cc 100644 --- a/src/smartAuthorizationHelper.test.ts +++ b/src/smartAuthorizationHelper.test.ts @@ -542,7 +542,7 @@ describe('introspectJwtToken', () => { Math.floor(Date.now() / 1000), Math.floor(Date.now() / 1000) + 10, expectedAudValue, - `${expectedIssValue}/`, + `${expectedIssValue}`, ); const mock = new MockAdapter(axios); mock.onPost(introspectUrl).reply(200, { diff --git a/src/smartAuthorizationHelper.ts b/src/smartAuthorizationHelper.ts index 3236e72..c4560ee 100644 --- a/src/smartAuthorizationHelper.ts +++ b/src/smartAuthorizationHelper.ts @@ -8,7 +8,9 @@ import { decode, verify } from 'jsonwebtoken'; import axios from 'axios'; import resourceReferencesMatrixV4 from './schema/fhirResourceReferencesMatrix.v4.0.1.json'; import resourceReferencesMatrixV3 from './schema/fhirResourceReferencesMatrix.v3.0.1.json'; -import { FhirResource, IntrospectionOptions } from './smartConfig'; +import { AccessModifier, FhirResource, IntrospectionOptions } from './smartConfig'; +import { convertScopeToSmartScope } from './smartScopeHelper'; + import getComponentLogger from './loggerBuilder'; export const FHIR_USER_REGEX = @@ -116,14 +118,27 @@ export function isFhirUserAdmin(fhirUser: FhirResource, adminAccessTypes: string } /** - * @param usableScopes this should be usableScope set from the `verifyAccessToken` method - * @param resourceType the type of the resource we are trying to access + * @param scopes: this should be scope set from the `verifyAccessToken` method + * @param resourceType: the type of the resource the request is trying to access + * @param accessModifier: the type of access the request is asking for * @returns if there is a usable system scope for this request */ -export function hasSystemAccess(usableScopes: string[], resourceType: string): boolean { - return usableScopes.some( - (scope: string) => scope.startsWith('system/*') || scope.startsWith(`system/${resourceType}`), - ); +export function hasSystemAccess(scopes: string[], resourceType: string, accessModifier: AccessModifier): boolean { + return scopes.some((scope: string) => { + try { + const clinicalSmartScope = convertScopeToSmartScope(scope); + + return ( + clinicalSmartScope.scopeType === 'system' && + (clinicalSmartScope.resourceType === '*' || clinicalSmartScope.resourceType === resourceType) && + (clinicalSmartScope.accessType === '*' || clinicalSmartScope.accessType === accessModifier) + ); + } catch (e) { + // Error occurs from `convertScopeToSmartScope` if scope was invalid + logger.debug((e as any).message); + return false; + } + }); } export function hasAccessToResource( @@ -134,9 +149,10 @@ export function hasAccessToResource( adminAccessTypes: string[], apiUrl: string, fhirVersion: FhirVersion, + accessModifier: AccessModifier, ): boolean { return ( - hasSystemAccess(usableScopes, sourceResource.resourceType) || + hasSystemAccess(usableScopes, sourceResource.resourceType, accessModifier) || (fhirUserObject && (isFhirUserAdmin(fhirUserObject, adminAccessTypes, apiUrl) || hasReferenceToResource(fhirUserObject, sourceResource, apiUrl, fhirVersion))) || diff --git a/src/smartHandler.test.ts b/src/smartHandler.test.ts index 29be0e2..54e9d39 100644 --- a/src/smartHandler.test.ts +++ b/src/smartHandler.test.ts @@ -17,6 +17,7 @@ import { BASE_R4_RESOURCES, AuthorizationBundleRequest, GetSearchFilterBasedOnIdentityRequest, + BASE_STU3_RESOURCES, } from 'fhir-works-on-aws-interface'; import * as smartAuthorizationHelper from './smartAuthorizationHelper'; @@ -945,6 +946,12 @@ describe('authorizeAndFilterReadResponse', () => { total: 3, }; + const searchFilteredEntitiesMatch = { + ...emptySearchResult, + entry: [createEntry(validPatientObservation), createEntry(validPatientEncounter)], + total: 2, + }; + const searchSomeEntitiesMatch = { ...emptySearchResult, entry: [ @@ -1212,13 +1219,12 @@ describe('authorizeAndFilterReadResponse', () => { searchAllEntitiesMatch, ], [ - 'SEARCH: system scope; Practitioner able to search and get ALL results', + 'SEARCH: system scope; System able to search and get ALL results', { userIdentity: { ...baseAccessNoScopes, scopes: ['system/*.read'], usableScopes: ['system/*.read'], - fhirUserObject: practitionerFhirResource, }, operation: 'search-type', readResponse: searchAllEntitiesMatch, @@ -1226,6 +1232,66 @@ describe('authorizeAndFilterReadResponse', () => { true, searchAllEntitiesMatch, ], + [ + 'SEARCH: user scope; filter Patient Resource based on scopes; /Observation? search', + { + userIdentity: { + ...baseAccessNoScopes, + scopes: ['user/Observation.read', 'user/Encounter.read'], + usableScopes: ['user/Observation.read'], + fhirUserObject: practitionerFhirResource, + }, + operation: 'search-type', + readResponse: searchAllEntitiesMatch, + }, + true, + searchFilteredEntitiesMatch, + ], + [ + 'SEARCH: patient scope; filter Patient Resource based on scopes; /Observation? search', + { + userIdentity: { + ...baseAccessNoScopes, + scopes: ['patient/Observation.read', 'patient/Encounter.read'], + usableScopes: ['patient/Observation.read'], + patientLaunchContext: patientFhirResource, + }, + operation: 'search-type', + readResponse: searchAllEntitiesMatch, + }, + true, + searchFilteredEntitiesMatch, + ], + [ + 'SEARCH: system scope; filter Patient Resource based on scopes; /Observation? search', + { + userIdentity: { + ...baseAccessNoScopes, + scopes: ['system/Observation.read', 'system/Encounter.read'], + usableScopes: ['system/Observation.read'], + }, + operation: 'search-type', + readResponse: searchAllEntitiesMatch, + }, + true, + searchFilteredEntitiesMatch, + ], + [ + 'SEARCH: Invalid search result', + { + userIdentity: { + ...baseAccessNoScopes, + scopes: ['user/*.read', 'patient/*.read'], + usableScopes: ['user/*.read', 'patient/*.read'], + fhirUserObject: patientFhirResource, + patientLaunchContext: patientFhirResource, + }, + operation: 'search-system', + readResponse: {}, + }, + true, + { entry: [], total: 0 }, + ], ]; const authZHandler: SMARTHandler = new SMARTHandler(baseAuthZConfig(), apiUrl, '4.0.1'); @@ -1265,7 +1331,6 @@ describe('isWriteRequestAuthorized', () => { { userIdentity: { ...baseAccessNoScopes, - scopes: ['patient/*.write'], usableScopes: ['patient/*.write'], patientLaunchContext: patientFhirResource, }, @@ -1279,7 +1344,6 @@ describe('isWriteRequestAuthorized', () => { { userIdentity: { ...baseAccessNoScopes, - scopes: ['user/*.write'], usableScopes: ['user/*.write'], fhirUserObject: practitionerFhirResource, }, @@ -1293,7 +1357,6 @@ describe('isWriteRequestAuthorized', () => { { userIdentity: { ...baseAccessNoScopes, - scopes: ['user/*.write'], usableScopes: ['user/*.write'], fhirUserObject: externalPractitionerFhirResource, }, @@ -1303,11 +1366,10 @@ describe('isWriteRequestAuthorized', () => { true, ], [ - 'PATCH: missing user/patient context; but has system scopes', + 'PATCH: has system scopes', { userIdentity: { ...baseAccessNoScopes, - scopes: ['user/*.write', 'patient/*.write', 'system/*.write'], usableScopes: ['system/*.write'], }, operation: 'patch', @@ -1316,11 +1378,10 @@ describe('isWriteRequestAuthorized', () => { true, ], [ - 'PATCH: missing user/patient context', + 'PATCH: no usable scope', { userIdentity: { ...baseAccessNoScopes, - scopes: ['user/*.write', 'patient/*.write'], usableScopes: [], }, operation: 'patch', @@ -1333,7 +1394,6 @@ describe('isWriteRequestAuthorized', () => { { userIdentity: { ...baseAccessNoScopes, - scopes: ['user/*.write'], usableScopes: ['user/*.write'], fhirUserObject: externalPractitionerFhirResource, }, @@ -1347,7 +1407,6 @@ describe('isWriteRequestAuthorized', () => { { userIdentity: { ...baseAccessNoScopes, - scopes: ['user/*.write', 'system/*.write'], usableScopes: ['user/*.write', 'system/*.write'], fhirUserObject: externalPractitionerFhirResource, }, @@ -1356,6 +1415,44 @@ describe('isWriteRequestAuthorized', () => { }, true, ], + [ + 'UPDATE: specific scope', + { + userIdentity: { + ...baseAccessNoScopes, + usableScopes: ['system/Patient.write'], + }, + operation: 'update', + resourceBody: validPatient, + }, + true, + ], + [ + 'CREATE: specific scope', + { + userIdentity: { + ...baseAccessNoScopes, + usableScopes: ['user/Patient.write'], + fhirUserObject: practitionerFhirResource, + }, + operation: 'create', + resourceBody: validPatient, + }, + true, + ], + [ + 'PATCH: specific scope', + { + userIdentity: { + ...baseAccessNoScopes, + usableScopes: ['patient/Encounter.write'], + patientLaunchContext: patientFhirResource, + }, + operation: 'patch', + resourceBody: validPatientEncounter, + }, + true, + ], ]; const authZHandler: SMARTHandler = new SMARTHandler(baseAuthZConfig(), apiUrl, '4.0.1'); @@ -1436,7 +1533,7 @@ describe('isAccessBulkDataJobAllowed', () => { }); }); -describe('isBundleRequestAuthorized', () => { +describe('isBundleRequestAuthorized; write an Observation; read a Patient; search-type Encounter', () => { const authZConfigWithSearchTypeScope = baseAuthZConfig(); authZConfigWithSearchTypeScope.scopeRule.user.write = ['create']; const authZHandler: SMARTHandler = new SMARTHandler(authZConfigWithSearchTypeScope, apiUrl, '4.0.1'); @@ -1472,7 +1569,7 @@ describe('isBundleRequestAuthorized', () => { 'practitioner with patient&user scope: no error', { ...baseAccessNoScopes, - scopes: ['user/Observation.write', 'patient/Patient.*', 'openid'], + scopes: ['user/Observation.write', 'patient/Patient.*', 'patient/Encounter.read', 'openid'], fhirUserObject: practitionerFhirResource, patientLaunchContext: patientFhirResource, }, @@ -1482,11 +1579,20 @@ describe('isBundleRequestAuthorized', () => { 'practitioner with system&user scope: no error', { ...baseAccessNoScopes, - scopes: ['user/Observation.write', 'system/Patient.*', 'openid'], + scopes: ['user/Observation.write', 'system/Patient.*', 'user/Encounter.read', 'openid'], fhirUserObject: practitionerFhirResource, }, true, ], + [ + 'practitioner with system&user scope; but no Encounter scope: Unauthorized Error', + { + ...baseAccessNoScopes, + scopes: ['user/Observation.write', 'system/Patient.*', 'openid'], + fhirUserObject: practitionerFhirResource, + }, + false, + ], [ 'patient with user scope to create observation but no read perms: Unauthorized Error', { @@ -1544,6 +1650,13 @@ describe('isBundleRequestAuthorized', () => { resourceType: 'Patient', id: '160265f7-e8c2-45ca-a1bc-317399e23548', }, + { + operation: 'search-type', + resource: '/Encounter?_include=Patient', + fullUrl: '/Encounter?_include=Patient', + resourceType: 'Encounter', + id: '', + }, ], }; @@ -1564,8 +1677,8 @@ describe('getAllowedResourceTypesForOperation', () => { const cases: (string | string[])[][] = [ [ - 'search-type operation: read scope: returns [Patient, Observation]', - ['user/Patient.read', 'user/Observation.read', 'fhirUser', 'system/Encounter.*'], + 'search-type operation: read scope: returns [Patient, Observation, Encounter] not Fake though', + ['user/Patient.read', 'user/Observation.read', 'fhirUser', 'system/Encounter.*', 'system/Fake.*'], 'search-type', ['Patient', 'Observation', 'Encounter'], ], @@ -1608,6 +1721,20 @@ describe('getAllowedResourceTypesForOperation', () => { expectedAllowedResources, ); }); + test('ver3_CASE: search-type operation: read scope: returns [BASE_R3_RESOURCES]', async () => { + const authZHandlerVer3: SMARTHandler = new SMARTHandler(authZConfigWithSearchTypeScope, apiUrl, '3.0.1'); + + const request: AllowedResourceTypesForOperationRequest = { + userIdentity: { + scopes: ['user/*.read'], + }, + operation: 'search-type', + }; + + await expect(authZHandlerVer3.getAllowedResourceTypesForOperation(request)).resolves.toEqual( + BASE_STU3_RESOURCES, + ); + }); }); describe('getSearchFilterBasedOnIdentity', () => { diff --git a/src/smartHandler.ts b/src/smartHandler.ts index c47cf08..26e2aa6 100644 --- a/src/smartHandler.ts +++ b/src/smartHandler.ts @@ -69,10 +69,10 @@ export class SMARTHandler implements Authorization { private readonly jwksClient?: JwksClient; /** - * @param apiUrl URL of this FHIR service. Will be used to determine if a requestor is from this FHIR server or not + * @param apiUrl: URL of this FHIR service. Will be used to determine if a requestor is from this FHIR server or not * when the request does not include a fhirServiceBaseUrl - * @param adminAccessTypes a fhirUser from these resourceTypes they will be able to READ & WRITE without having to meet the reference criteria - * @param bulkDataAccessTypes a fhirUser from these resourceTypes they will be able to do bulk data operations + * @param adminAccessTypes: a fhirUser from these resourceTypes they will be able to READ & WRITE without having to meet the reference criteria + * @param bulkDataAccessTypes: a fhirUser from these resourceTypes they will be able to do bulk data operations */ constructor( config: SMARTConfig, @@ -189,7 +189,7 @@ export class SMARTHandler implements Authorization { const { fhirUserObject, patientLaunchContext, usableScopes } = request.userIdentity; const fhirServiceBaseUrl = request.fhirServiceBaseUrl ?? this.apiUrl; - if (hasSystemAccess(usableScopes, '')) { + if (hasSystemAccess(usableScopes, '', 'read')) { return []; } @@ -316,6 +316,7 @@ export class SMARTHandler implements Authorization { } } catch (e) { // Caused by trying to convert non-SmartScope to SmartScope, for example converting scope 'openid' or 'profile' + logger.debug((e as any).message); } } allowedResources = [...new Set(allowedResources)]; @@ -323,22 +324,35 @@ export class SMARTHandler implements Authorization { } async authorizeAndFilterReadResponse(request: ReadResponseAuthorizedRequest): Promise { - const { fhirUserObject, patientLaunchContext, usableScopes } = request.userIdentity; + const { fhirUserObject, patientLaunchContext, usableScopes, scopes } = request.userIdentity; const fhirServiceBaseUrl = request.fhirServiceBaseUrl ?? this.apiUrl; const { operation, readResponse } = request; - // If request is a search treat the readResponse as a bundle + // If request is a search iterate over every response object + // Must use all scopes, since a search may return more resourceTypes than just found in usableScopes if (SEARCH_OPERATIONS.includes(operation)) { - const entries: any[] = (readResponse.entry ?? []).filter((entry: { resource: any }) => - hasAccessToResource( - fhirUserObject, - patientLaunchContext, - entry.resource, - usableScopes, - this.adminAccessTypes, - fhirServiceBaseUrl, - this.fhirVersion, - ), + const entries: any[] = (readResponse.entry ?? []).filter( + (entry: { resource: any }) => + // Are the scopes the request have good enough for this entry? + scopes.some((scope: string) => + isScopeSufficient( + scope, + this.config.scopeRule, + operation, + this.isUserScopeAllowedForSystemExport, + entry.resource.resourceType, + ), + ) && // Does the user have permissions for this entry? + hasAccessToResource( + fhirUserObject, + patientLaunchContext, + entry.resource, + scopes, + this.adminAccessTypes, + fhirServiceBaseUrl, + this.fhirVersion, + 'read', + ), ); let numTotal: number = readResponse.total; if (!numTotal) { @@ -358,6 +372,7 @@ export class SMARTHandler implements Authorization { this.adminAccessTypes, fhirServiceBaseUrl, this.fhirVersion, + 'read', ) ) { return readResponse; @@ -378,6 +393,7 @@ export class SMARTHandler implements Authorization { this.adminAccessTypes, fhirServiceBaseUrl, this.fhirVersion, + 'write', ) ) { return; diff --git a/src/smartScopeHelper.test.ts b/src/smartScopeHelper.test.ts index fa6c368..ff58380 100644 --- a/src/smartScopeHelper.test.ts +++ b/src/smartScopeHelper.test.ts @@ -118,12 +118,24 @@ describe.each(isScopeSufficientCases)('ScopeType: %s: isScopeSufficient', (scope expect(isScopeSufficient(`${scopeType}/*.read`, clonedScopeRule, 'search-system', false)).toEqual(true); }); + test('scope is sufficient to do a history-system', () => { + const clonedScopeRule = emptyScopeRule(); + clonedScopeRule[scopeType].read = ['history-system']; + + expect(isScopeSufficient(`${scopeType}/*.read`, clonedScopeRule, 'history-system', false)).toEqual(true); + }); test('scope is sufficient to do a transaction', () => { const clonedScopeRule = emptyScopeRule(); clonedScopeRule[scopeType].write = ['transaction']; expect(isScopeSufficient(`${scopeType}/*.write`, clonedScopeRule, 'transaction', false)).toEqual(true); }); + test('scope is sufficient to do a batch', () => { + const clonedScopeRule = emptyScopeRule(); + clonedScopeRule[scopeType].write = ['batch']; + + expect(isScopeSufficient(`${scopeType}/*.write`, clonedScopeRule, 'batch', false)).toEqual(true); + }); test('scope is insufficient to do a transaction', () => { const clonedScopeRule = emptyScopeRule(); clonedScopeRule[scopeType].read = ['read']; @@ -131,11 +143,11 @@ describe.each(isScopeSufficientCases)('ScopeType: %s: isScopeSufficient', (scope expect(isScopeSufficient(`${scopeType}/*.*`, clonedScopeRule, 'transaction', false)).toEqual(false); }); - test('invalid scope', () => { + test('invalid; `read` scope with no resourceType', () => { const clonedScopeRule = emptyScopeRule(); clonedScopeRule[scopeType].read = ['read']; - expect(isScopeSufficient(`fake`, clonedScopeRule, 'read', false)).toEqual(false); + expect(isScopeSufficient(`${scopeType}/Patient.*`, clonedScopeRule, 'read', false)).toEqual(false); }); describe('BulkDataAuth', () => { @@ -149,6 +161,16 @@ describe.each(isScopeSufficientCases)('ScopeType: %s: isScopeSufficient', (scope isScopeSufficient(`${scopeType}/*.read`, clonedScopeRule, 'read', true, undefined, bulkDataAuth), ).toEqual(scopeType !== 'patient'); }); + test('initiate-export: exportType is undefined; should fail', () => { + const clonedScopeRule = emptyScopeRule(); + clonedScopeRule[scopeType].read = ['read']; + const bulkDataAuth: BulkDataAuth = { operation: 'initiate-export' }; + + // Only scopeType of user has bulkDataAccess + expect( + isScopeSufficient(`${scopeType}/*.read`, clonedScopeRule, 'read', true, undefined, bulkDataAuth), + ).toEqual(false); + }); test('scope is NOT sufficient for `system` initiate-export: Scope needs to have resourceType "*"', () => { const clonedScopeRule = emptyScopeRule(); @@ -253,6 +275,12 @@ describe.each(isScopeSufficientCases)('ScopeType: %s: isScopeSufficient', (scope }); }); }); +test('isScopeSufficient: invalid scope non-smart', () => { + const clonedScopeRule = emptyScopeRule(); + clonedScopeRule.system.read = ['read']; + + expect(isScopeSufficient(`fhirUser`, clonedScopeRule, 'read', false)).toEqual(false); +}); describe('getScopes', () => { test('scope type delimited by space', () => { @@ -420,6 +448,9 @@ describe('filterOutUnusableScope', () => { 'search-type', false, 'Practitioner', + undefined, + undefined, + 'fhirUser', ), ).toEqual([]); }); @@ -433,10 +464,12 @@ describe('filterOutUnusableScope', () => { 'search-type', false, 'Practitioner', + undefined, + 'patient', ), ).toEqual([]); }); - test('do not filter patient scope out in type-search use case', () => { + test('SYSTEM Scope: do filter Patient resourceType scope out in type-search use case', () => { const clonedScopeRule = emptyScopeRule(); clonedScopeRule.system.read = ['search-type']; expect( @@ -447,7 +480,38 @@ describe('filterOutUnusableScope', () => { false, 'DocumentReference', ), - ).toEqual(['system/DocumentReference.read', 'system/Patient.read']); + ).toEqual(['system/DocumentReference.read']); + }); + test('USER Scope: do filter Patient resourceType scope out in type-search use case', () => { + const clonedScopeRule = emptyScopeRule(); + clonedScopeRule.user.read = ['search-type']; + expect( + filterOutUnusableScope( + ['user/DocumentReference.read', 'user/Patient.read'], + clonedScopeRule, + 'search-type', + false, + 'DocumentReference', + undefined, + undefined, + 'fhirUser', + ), + ).toEqual(['user/DocumentReference.read']); + }); + test('PATIENT Scope: do filter Patient resourceType scope out in type-search use case', () => { + const clonedScopeRule = emptyScopeRule(); + clonedScopeRule.patient.read = ['search-type']; + expect( + filterOutUnusableScope( + ['patient/DocumentReference.read', 'patient/Patient.read'], + clonedScopeRule, + 'search-type', + false, + 'DocumentReference', + undefined, + 'patient', + ), + ).toEqual(['patient/DocumentReference.read']); }); }); diff --git a/src/smartScopeHelper.ts b/src/smartScopeHelper.ts index 2113a57..9c46fe0 100644 --- a/src/smartScopeHelper.ts +++ b/src/smartScopeHelper.ts @@ -4,6 +4,9 @@ */ import { BulkDataAuth, SystemOperation, TypeOperation } from 'fhir-works-on-aws-interface'; import { AccessModifier, ClinicalSmartScope, ScopeRule, ScopeType } from './smartConfig'; +import getComponentLogger from './loggerBuilder'; + +const logger = getComponentLogger(); export const SEARCH_OPERATIONS: (TypeOperation | SystemOperation)[] = [ 'search-type', @@ -55,7 +58,7 @@ function getValidOperationsForScope( let validOperations: (TypeOperation | SystemOperation)[] = []; const { scopeType, resourceType, accessType } = smartScope; if (reqResourceType) { - if (resourceType === '*' || resourceType === reqResourceType || reqOperation === 'search-type') { + if (resourceType === '*' || resourceType === reqResourceType) { validOperations = getValidOperationsForScopeTypeAndAccessType(scopeType, accessType, scopeRule); } } @@ -136,6 +139,7 @@ export function isScopeSufficient( return validOperations.includes(reqOperation); } catch (e) { // Caused by trying to convert non-SmartScope to SmartScope, for example converting non-SMART scope 'openid' + logger.debug((e as any).message); } return false; @@ -172,16 +176,5 @@ export function filterOutUnusableScope( ), ); - // We should only return the scopes iff there is at least 1 valid scope for the given resourceType - if ( - reqOperation === 'search-type' && - !filteredScopes.some((scope: string) => { - const smartScope = convertScopeToSmartScope(scope); - return smartScope.resourceType === '*' || smartScope.resourceType === reqResourceType; - }) - ) { - return []; - } - return filteredScopes; }