wip(search): make sure throwing doesn't break access policy eval

packages/core/src/access.test.ts

@@ -143,7 +143,27 @@ describe('Access', () => {
  },
  ],
  };
- expect(matchesAccessPolicy(ap, { resourceType: 'Patient', meta: { compartment: [{ reference: '1' }] } }, true));
- expect(matchesAccessPolicy(ap, { resourceType: 'Patient', meta: { compartment: [{ reference: '2' }] } }, false));
+ expect(
+ matchesAccessPolicy(ap, { resourceType: 'Patient', meta: { compartment: [{ reference: '1' }] } }, true)
+ ).toEqual(true);
+ expect(
+ matchesAccessPolicy(ap, { resourceType: 'Patient', meta: { compartment: [{ reference: '2' }] } }, false)
+ ).toEqual(false);
+ });
+
+ test('Invalid SearchParameter in criteria in AccessPolicy', () => {
+ const ap: AccessPolicy = {
+ resourceType: 'AccessPolicy',
+ resource: [
+ {
+ resourceType: 'Patient',
+ criteria: 'Patient?invalid=invalid',
+ },
+ ],
+ };
+
+ expect(() =>
+ matchesAccessPolicy(ap, { resourceType: 'Patient', name: [{ given: ['John'], family: 'Doe' }] }, false)
+ ).toThrow(/Unknown search parameter: invalid/);
  });
 });

packages/core/src/search/match.test.ts

@@ -65,7 +65,7 @@ describe('Search matching', () => {
  { resourceType: 'Patient' },
  { resourceType: 'Patient', filters: [{ code: 'unknown', operator: Operator.EQUALS, value: 'xyz' }] }
  )
- ).toThrow('Unknown search parameter: unknown');
+ ).toThrow('Unknown search parameter: unknown for resource type Patient');
  });
 
  test('Boolean filter', () => {

packages/core/src/search/match.ts

@@ -34,7 +34,7 @@ export function matchesSearchRequest(resource: Resource, searchRequest: SearchRe
 function matchesSearchFilter(resource: Resource, searchRequest: SearchRequest, filter: Filter): boolean {
  const searchParam = globalSchema.types[searchRequest.resourceType]?.searchParams?.[filter.code];
  if (!searchParam) {
- throw new Error(`Unknown search parameter: ${filter.code}`);
+ throw new Error(`Unknown search parameter: ${filter.code} for resource type ${searchRequest.resourceType}`);
  }
  if (filter.operator === Operator.MISSING && searchParam) {
  const values = evalFhirPath(searchParam.expression as string, resource);

packages/server/src/fhir/accesspolicy.test.ts

@@ -2247,4 +2247,33 @@ describe('AccessPolicy', () => {
  })
  ).rejects.toThrow();
  }));
+
+ test.only('readResource with invalid access policy', async () =>
+ withTestContext(async () => {
+ const { repo, membership } = await createTestProject({
+ withRepo: true,
+ withClient: true,
+ });
+
+ const accessPolicy = await systemRepo.createResource<AccessPolicy>({
+ resourceType: 'AccessPolicy',
+ resource: [
+ {
+ resourceType: 'Patient',
+ criteria: `Patient?i=completed`,
+ readonly: true,
+ },
+ ],
+ });
+
+ const patient = await repo.createResource<Patient>({ resourceType: 'Patient' });
+ await systemRepo.updateResource<ProjectMembership>({
+ ...membership,
+ accessPolicy: createReference(accessPolicy),
+ });
+
+ await expect(
+ repo.readResource('Patient', patient.id as string, { allowReadFrom: ['database'] })
+ ).rejects.toThrow();
+ }));
 });

packages/server/src/fhir/repo.test.ts

@@ -93,8 +93,8 @@ describe('FHIR Repo', () => {
  }
  });
 
- test('Read invalid resource with `checkCacheOnly` set', async () => {
- await expect(systemRepo.readResource('Subscription', randomUUID(), { checkCacheOnly: true })).rejects.toThrow(
+ test('Read invalid resource with `allowReadFrom` only including `cache`', async () => {
+ await expect(systemRepo.readResource('Subscription', randomUUID(), { allowReadFrom: ['cache'] })).rejects.toThrow(
  new OperationOutcomeError(notFound)
  );
  });

packages/server/src/fhir/repo.ts

@@ -172,7 +172,7 @@ export interface CacheEntry<T extends Resource = Resource> {
 }
 
 export type ReadResourceOptions = {
- checkCacheOnly?: boolean;
+ allowReadFrom?: ('cache' | 'database')[];
 };
 
 /**
@@ -254,21 +254,25 @@ export class Repository extends BaseRepository implements FhirRepository<PoolCli
  throw new OperationOutcomeError(forbidden);
  }
 
- const cacheRecord = await getCacheEntry<T>(resourceType, id);
- if (cacheRecord) {
- // This is an optimization to avoid a database query.
- // However, it depends on all values in the cache having "meta.compartment"
- // Old versions of Medplum did not populate "meta.compartment"
- // So this optimization is blocked until we add a migration.
- // if (!this.canReadCacheEntry(cacheRecord)) {
- // throw new OperationOutcomeError(notFound);
- // }
- if (this.canReadCacheEntry(cacheRecord)) {
- return cacheRecord.resource;
+ const allowReadFrom = options?.allowReadFrom ?? (['cache', 'database'] as const);
+
+ if (allowReadFrom.includes('cache')) {
+ const cacheRecord = await getCacheEntry<T>(resourceType, id);
+ if (cacheRecord) {
+ // This is an optimization to avoid a database query.
+ // However, it depends on all values in the cache having "meta.compartment"
+ // Old versions of Medplum did not populate "meta.compartment"
+ // So this optimization is blocked until we add a migration.
+ // if (!this.canReadCacheEntry(cacheRecord)) {
+ // throw new OperationOutcomeError(notFound);
+ // }
+ if (this.canReadCacheEntry(cacheRecord)) {
+ return cacheRecord.resource;
+ }
  }
  }
 
- if (options?.checkCacheOnly) {
+ if (!allowReadFrom.includes('database')) {
  throw new OperationOutcomeError(notFound);
  }
 
@@ -1156,6 +1160,7 @@ export class Repository extends BaseRepository implements FhirRepository<PoolCli
 
  for (const policy of this.context.accessPolicy.resource) {
  if (policy.resourceType === resourceType) {
+ console.log({ policy });
  const policyCompartmentId = resolveId(policy.compartment);
  if (policyCompartmentId) {
  // Deprecated - to be removed
@@ -1164,6 +1169,7 @@ export class Repository extends BaseRepository implements FhirRepository<PoolCli
  } else if (policy.criteria) {
  // Add subquery for access policy criteria.
  const searchRequest = parseSearchRequest(policy.criteria);
+ console.log({ searchRequest });
  const accessPolicyExpression = buildSearchExpression(builder, searchRequest.resourceType, searchRequest);
  if (accessPolicyExpression) {
  expressions.push(accessPolicyExpression);

packages/server/src/workers/subscription.ts

@@ -400,7 +400,7 @@ async function tryGetSubscription(
 ): Promise<Subscription | undefined> {
  try {
  return await systemRepo.readResource<Subscription>('Subscription', subscriptionId, {
- checkCacheOnly: channelType === 'websocket',
+ allowReadFrom: channelType === 'websocket' ? ['cache'] : ['cache', 'database'],
  });
  } catch (err) {
  const outcome = normalizeOperationOutcome(err);