Skip to content

Commit

Permalink
issue #3576 - remove unnecessary check for null ids
Browse files Browse the repository at this point in the history 
Now that our persistence layer requires resources to have proper
metadata prior to calling create/update/delete, I thought that resources
will always have an id before we call the persistence interceptors.

However, it turns out that we call the beforeCreate from the REST layer
BEFORE setting this metadata on the resource.
After a brief discussion, we decided to leave that as-is and update the
interceptor to allow a null id.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Apr 19, 2022
1 parent 2715bb3 commit 9bf079c
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 11 deletions.
7 changes: 4 additions & 3 deletions fhir-server/src/main/java/com/ibm/fhir/server/util/FHIRRestHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public FHIRRestHelper(FHIRPersistence persistence, SearchHelper searchHelper) {

@Override
public FHIRRestOperationResponse doCreate(String type, Resource resource, String ifNoneExist,
boolean doValidation) throws Exception {
boolean doValidation) throws Exception {

// Validate that interaction is allowed for given resource type
validateInteraction(Interaction.CREATE, type);
Expand Down Expand Up @@ -244,7 +244,7 @@ public FHIRRestOperationResponse doCreate(String type, Resource resource, String

@Override
public FHIRRestOperationResponse doCreateMeta(FHIRPersistenceEvent event, List<Issue> warnings, String type, Resource resource,
String ifNoneExist) throws Exception {
String ifNoneExist) throws Exception {
log.entering(this.getClass().getName(), "doCreateMeta");

// Save the current request context.
Expand Down Expand Up @@ -343,7 +343,8 @@ public FHIRRestOperationResponse doCreateMeta(FHIRPersistenceEvent event, List<I
}

@Override
public FHIRRestOperationResponse doCreatePersist(FHIRPersistenceEvent event, List<Issue> warnings, Resource resource, PayloadPersistenceResponse offloadResponse) throws Exception {
public FHIRRestOperationResponse doCreatePersist(FHIRPersistenceEvent event, List<Issue> warnings, Resource resource,
PayloadPersistenceResponse offloadResponse) throws Exception {
log.entering(this.getClass().getName(), "doCreatePersist");

FHIRRestOperationResponse ior = new FHIRRestOperationResponse();
Expand Down
19 changes: 11 additions & 8 deletions ...-smart/src/main/java/com/ibm/fhir/smart/AuthzPolicyEnforcementPersistenceInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@ private void enforce(Resource resource, Set<String> contextIds, Permission requi
* Enforce the authorizations granted by the end user in the form of scope strings.
*
* @param resourceType the resource type
* @param resourceId the resource ID
* @param resourceId the resource ID; may be null for a FHIR create (POST)
* @param resource the resource to check, or null if resource was not retrieved
* @param contextIds an identifier for the current context (e.g. patient or user) as determined by the scope strings
* @param requiredPermission the permission required for the requested interaction
Expand All @@ -810,8 +810,8 @@ private void enforce(String resourceType, String resourceId, Resource resource,
* Determine whether authorization to a given resource is granted by the end user in the form of scope strings.
*
* @param resourceType the resource type
* @param resourceId the resource ID
* @param resource the resource to check, or null if resource not retrieved
* @param resourceId the resource ID; may be null in the case of a FHIR create (POST)
* @param resource the resource to check; null if resource not retrieved
* @param contextIds an identifier for the current context (e.g. patient or user) as determined by the scope strings
* @param requiredPermission the permission required for the requested interaction
* @param grantedScopes the SMART scopes associated with the request, indexed by ContextType
Expand All @@ -820,7 +820,6 @@ private void enforce(String resourceType, String resourceId, Resource resource,
private boolean isAllowed(String resourceType, String resourceId, Resource resource, Set<String> contextIds, Permission requiredPermission, Map<ContextType, List<Scope>> grantedScopes)
throws FHIRPersistenceInterceptorException {
Objects.requireNonNull(resourceType, "resourceType");
Objects.requireNonNull(resourceId, "resourceId");
Objects.requireNonNull(contextIds, "contextIds");

// For `system` and `user` scopes, we grant access to all resources of the requested type.
Expand All @@ -837,7 +836,11 @@ private boolean isAllowed(String resourceType, String resourceId, Resource resou
.findAny();
if (approvingScope.isPresent()) {
if (log.isLoggable(Level.FINE)) {
log.fine(requiredPermission.value() + " permission for '" + resourceType + "/" + resourceId +
String typeWithOptionalId = resourceType;
if (resourceId != null) {
typeWithOptionalId = typeWithOptionalId + "/" + resourceId;
}
log.fine(requiredPermission.value() + " permission for '" + typeWithOptionalId +
"' is granted via scope " + approvingScope.get());
}
return true;
Expand Down Expand Up @@ -907,7 +910,7 @@ private boolean isAllowed(String resourceType, String resourceId, Resource resou
* Internal helper for checking compartment membership.
*
* @param resourceType
* @param resourceId
* @param resourceId possibly null
* @param resource possibly null
* @param compartmentType
* @param contextIds
Expand Down Expand Up @@ -942,7 +945,7 @@ private boolean isInCompartment(String resourceType, String resourceId, Resource
// If the target resource type matches the compartment type, allow it if the id is one of the passed contextIds
if (compartment.equals(resourceType) && contextIds.contains(resourceId)) {
if (log.isLoggable(Level.FINE)) {
log.fine("Bypassing compartment check for the compartment identity resource " + resourceType + "/" + resource.getId());
log.fine("Bypassing compartment check for the compartment identity resource " + resourceType + "/" + resourceId);
}
return true;
}
Expand All @@ -954,7 +957,7 @@ private boolean isInCompartment(String resourceType, String resourceId, Resource
for (String searchParmCode : inclusionCriteria) {
try {
SearchParameter inclusionParm = searchHelper.getSearchParameter(resourceType, searchParmCode);
if (inclusionParm != null & inclusionParm.getExpression() != null) {
if (inclusionParm != null && inclusionParm.getExpression() != null) {
String expression = inclusionParm.getExpression().getValue();
Collection<FHIRPathNode> nodes = FHIRPathEvaluator.evaluator().evaluate(resourceContext, expression);
for (FHIRPathNode node : nodes) {
Expand Down

0 comments on commit 9bf079c

Please sign in to comment.