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.

Sign up for GitHub

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

Jump to bottom

Global caches in ProfileSupport should be tenant-specific #2802

Closed
lmsurpre opened this issue Sep 23, 2021 · 1 comment
Closed

Global caches in ProfileSupport should be tenant-specific #2802

lmsurpre opened this issue Sep 23, 2021 · 1 comment
Assignees
@lmsurpre
Labels
bug Something isn't working P1 Priority 1 - Must Have security
Milestone
Sprint 2021-13

Comments

@lmsurpre
Copy link
Member

lmsurpre commented Sep 23, 2021 •
edited
Loading

Describe the bug
Global caches in ProfileSupport are not currently tenant-specific which means that tenant-specific profiles acquired through the ServerRegistryResourceProvider may be visible globally. Instead, these caches should be tenant-specific.

Environment
Which version of IBM FHIR Server?

To Reproduce
Steps to reproduce the behavior:

  1. Enable ServerRegistryResourceProvider on a server with two tenants (A and B)
  2. As a user of tenant A, create a StructureDefinition that defines an extension
  3. As a user of tenant A, validate a resource that contains this extension (loading this StructureDefinition and its generated constraints in our caches)
  4. As a user of tenant B, validate a resource that contains this extension

Note that the extension is validated with the constraints from tenant A even though tenant B may have a different version of the extension definition.

Expected behavior
Users on tenant B should not have visibility into any conformance resources posted to tenant A.

Additional context
Very similar to #2256.

I hope there's not more of these hiding, but we should consider advising users to avoid multiTenant deployments with serverRegistryResourceProvider enabled (unless they control who is allowed to create conformance resources).
@lmsurpre lmsurpre added bug Something isn't working security P1 Priority 1 - Must Have labels Sep 23, 2021
@lmsurpre lmsurpre added this to the Sprint 2021-13 milestone Sep 27, 2021
@lmsurpre lmsurpre self-assigned this Sep 27, 2021
lmsurpre added a commit that referenced this issue Sep 27, 2021
@lmsurpre
issue #2802 - use CacheManager for tenant-specific caches
f2fd315 
I followed the pattern from CodeSystemSupport.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 28, 2021
@lmsurpre
issue #2802 - add TenantIsolationTest to verify the fix
1371a12 
This test does the following:
1. create an extension definition
(StructureDefinition) and a CodeSystem in the default tenant
2. invokes code that loads these conformance resources into the internal
caches
3. confirms that an alternate tenant (tenant1) cannot hit those cached
values

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 28, 2021
@lmsurpre
issue #2802 - add TenantIsolationTest to verify the fix
5bcdab2 
This test does the following:
1. create an extension definition
(StructureDefinition) and a CodeSystem in the default tenant
2. invokes code that loads these conformance resources into the internal
caches
3. confirms that an alternate tenant (tenant1) cannot hit those cached
values

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 28, 2021
@lmsurpre
issue #2802 - add TenantIsolationTest to verify the fix
f72e4d4 
This test does the following:
1. create an extension definition
(StructureDefinition) and a CodeSystem in the default tenant
2. invokes code that loads these conformance resources into the internal
caches
3. confirms that an alternate tenant (tenant1) cannot hit those cached
values

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Sep 28, 2021
@lmsurpre
issue #2802 - use CacheManager for tenant-specific caches (#2816)
7ce2079 
* issue #2802 - use CacheManager for tenant-specific caches

I followed the pattern from CodeSystemSupport.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* issue #2802 - add TenantIsolationTest to verify the fix

This test does the following:
1. create an extension definition
(StructureDefinition) and a CodeSystem in the default tenant
2. invokes code that loads these conformance resources into the internal
caches
3. confirms that an alternate tenant (tenant1) cannot hit those cached
values

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Moved ProfileBuilder and ExtensionBuilder to src/test

I also moved the `context` field out of the ProfileBuilder constructor
and into its own setter on ExtensionBuilder because this field is only
applicable to extension definitions.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Beefed up TenantIsolationTest per PR feedback

I confirmed that this flavor fails before the changes to
ProfileSupport and succeeds afterwards.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@prb112
Copy link
Contributor

prb112 commented Oct 5, 2021

QA: Passed

I ran with two different datastores (Postgres and Db2). One for default and one for tenant1.
I created a StructureDefinition, I showed that the cache'd rule was not applicable for the default tenant and available for the tenant1 and failed.

Negative tests showed the only way to screw this up is to misconfigure the data store so they share the tenants share the same exact schema.

@prb112 prb112 closed this as completed Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lmsurpre lmsurpre

Labels
bug Something isn't working P1 Priority 1 - Must Have security
Projects
None yet
Milestone
Sprint 2021-13
Development

No branches or pull requests
2 participants
@prb112 @lmsurpre