Implement permission checks to prevent unnecessary overwrit… #2506
Conversation
aktaskaan
changed the title
…es in recursive field processing
| CaseFieldDefinition rootFieldDefinition = caseTypeDefinition.getCaseFieldDefinitions() | ||
| .stream() | ||
| .filter(field -> field.getId().equals(key)) | ||
| .findFirst() | ||
| .orElse(null); |
There was a problem hiding this comment.
I think we can instead use an existing method?
| CaseFieldDefinition rootFieldDefinition = caseTypeDefinition.getCaseFieldDefinitions() | |
| .stream() | |
| .filter(field -> field.getId().equals(key)) | |
| .findFirst() | |
| .orElse(null); | |
| Optional<CaseFieldDefinition> rootFieldDefinition = caseTypeDefinition.getCaseField(key); |
|
|
||
| @Slf4j | ||
| @Service | ||
| public class RestrictedFieldProcessor { |
There was a problem hiding this comment.
"Restricted fields" are already an existing concept to do with the classification, which this new logic is not related to, so perhaps we should rename this to something a bit more specific around the use case it's for. I think a comment on this one would be useful too for people looking back at this in future to explain the purpose of this class, since it's for a very specific type of scenario.
| return parentFieldType.getComplexFields() | ||
| .stream() | ||
| .filter(subField -> subField.getId().equals(fieldName)) | ||
| .findFirst() |
There was a problem hiding this comment.
This is a useful helper so worth moving to FieldTypeDefinition.
| return parentFieldType.getCollectionFieldTypeDefinition().getComplexFields() | ||
| .stream() | ||
| .filter(subField -> subField.getId().equals(fieldName)) | ||
| .findFirst() | ||
| .orElse(parentFieldDefinition); |
There was a problem hiding this comment.
This could then use the same helper as the previous comment!
| private boolean isNullId(JsonNode newItem) { | ||
| return newItem.get(ID) == null | ||
| || newItem.get(ID).equals(NullNode.getInstance()) | ||
| || "null".equalsIgnoreCase(newItem.get(ID).asText()); | ||
| } |
There was a problem hiding this comment.
Wondering whether we can delete this method and sufficiently use newItem.hasNonNull(ID) in the single place this is being used instead, or are we treating the string "null" as null elsewhere in CCD too? While it's an edge case and would be strange for anyone to try, from a quick look at CollectionValidator we just verify it's some text so technically one item could be set to "null"!
| this.caseAccessService = caseAccessService; | ||
| } | ||
|
|
||
| public Map<String, JsonNode> filterRestrictedFields(final CaseTypeDefinition caseTypeDefinition, |
There was a problem hiding this comment.
We're not really filtering here, if anything it's sometimes the opposite? Maybe just call it process() or apply() and with a suitable class name should make sense!
| .anyMatch(acl -> accessProfileNames.contains(acl.getAccessProfile()) | ||
| && Boolean.TRUE.equals(acl.isRead())); |
There was a problem hiding this comment.
| .anyMatch(acl -> accessProfileNames.contains(acl.getAccessProfile()) | |
| && Boolean.TRUE.equals(acl.isRead())); | |
| .anyMatch(acl -> accessProfileNames.contains(acl.getAccessProfile()) && acl.isRead()); |
| .anyMatch(acl -> accessProfileNames.contains(acl.getAccessProfile()) | ||
| && Boolean.TRUE.equals(acl.isCreate())); |
There was a problem hiding this comment.
| .anyMatch(acl -> accessProfileNames.contains(acl.getAccessProfile()) | |
| && Boolean.TRUE.equals(acl.isCreate())); | |
| .anyMatch(acl -> accessProfileNames.contains(acl.getAccessProfile()) && acl.isCreate()); |
| return !hasReadPermission && hasCreatePermission; | ||
| } | ||
|
|
||
| private CaseFieldDefinition getFieldDefinition(String fieldName, |
There was a problem hiding this comment.
This method might sit better in CaseFieldDefinition as getSubfieldDefinition(fieldName).
| } | ||
| } | ||
|
|
||
| private JsonNode processValueField(CaseFieldDefinition subFieldDefinition, |
There was a problem hiding this comment.
Would this work if we had collections of collections?
JIRA link (if applicable)
https://tools.hmcts.net/jira/browse/CCD-6022
Change description
Does this PR introduce a breaking change? (check one with "x")