From b4b22d6ff6357c2d345d40c99a2eb98452d87d17 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 6 Dec 2024 15:11:30 -0500 Subject: [PATCH] Fixes revoke script to handle duplicates Signed-off-by: Darshit Chanpura --- .../ResourceSharingIndexHandler.java | 121 ++++++++++-------- 1 file changed, 67 insertions(+), 54 deletions(-) diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java index b460fdc964..cce026b8be 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java @@ -10,8 +10,11 @@ package org.opensearch.security.resources; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.Callable; @@ -702,50 +705,45 @@ public ResourceSharing updateResourceSharingInfo(String resourceId, String sourc // Atomic operation // TODO check if this script can be updated to replace magic identifiers (i.e. users, roles and backend_roles) with the ones // supplied in shareWith - Script updateScript = new Script(ScriptType.INLINE, """ + Script updateScript = new Script(ScriptType.INLINE, "painless", """ if (ctx._source.share_with == null) { ctx._source.share_with = [:]; } + for (def entry : params.shareWith.entrySet()) { def scopeName = entry.getKey(); def newScope = entry.getValue(); - if (ctx._source.share_with.containsKey(scopeName)) { - def existingScope = ctx._source.share_with.get(scopeName); - if (newScope.users != null) { - if (existingScope.users == null) { - existingScope.users = new HashSet(); - } - existingScope.users = new HashSet<>(existingScope.users); - existingScope.users.addAll(newScope.users); - } - if (existingScope.roles == null) { - existingScope.roles = new HashSet(); - } - existingScope.roles = new HashSet<>(existingScope.roles); - existingScope.roles.addAll(newScope.roles); - } - if (newScope.backend_roles != null) { - if (existingScope.backend_roles == null) { - existingScope.backend_roles = new HashSet(); + + if (!ctx._source.share_with.containsKey(scopeName)) { + def newScopeEntry = [:]; + for (def field : newScope.entrySet()) { + if (field.getValue() != null && !field.getValue().isEmpty()) { + newScopeEntry[field.getKey()] = new HashSet(field.getValue()); } - existingScope.backend_roles = new HashSet<>(existingScope.backend_roles); - existingScope.backend_roles.addAll(newScope.backend_roles); } + ctx._source.share_with[scopeName] = newScopeEntry; } else { - def newScopeEntry = [:]; - if (newScope.users != null) { - newScopeEntry.users = new HashSet(newScope.users); - } - if (newScope.roles != null) { - newScopeEntry.roles = new HashSet(newScope.roles); - } - if (newScope.backend_roles != null) { - newScopeEntry.backend_roles = new HashSet(newScope.backend_roles); + def existingScope = ctx._source.share_with[scopeName]; + + for (def field : newScope.entrySet()) { + def fieldName = field.getKey(); + def newValues = field.getValue(); + + if (newValues != null && !newValues.isEmpty()) { + if (!existingScope.containsKey(fieldName)) { + existingScope[fieldName] = new HashSet(); + } + + for (def value : newValues) { + if (!existingScope[fieldName].contains(value)) { + existingScope[fieldName].add(value); + } + } + } } - ctx._source.share_with.put(scopeName, newScopeEntry); } } - """, "painless", Collections.singletonMap("shareWith", shareWithMap)); + """, Collections.singletonMap("shareWith", shareWithMap)); boolean success = updateByQueryResourceSharing(sourceIdx, resourceId, updateScript); return success ? new ResourceSharing(resourceId, sourceIdx, createdBy, shareWith) : null; @@ -900,34 +898,49 @@ public ResourceSharing revokeAccess( LOGGER.debug("Revoking access for resource {} in {} for entities: {} and scopes: {}", resourceId, sourceIdx, revokeAccess, scopes); try { - // Revoke resource access - Script revokeScript = new Script( - ScriptType.INLINE, - "painless", - """ - if (ctx._source.share_with != null) { - Set scopesToProcess = new HashSet(params.scopes == null || params.scopes.isEmpty() ? ctx._source.share_with.keySet() : params.scopes); - - for (def scopeName : scopesToProcess) { - if (ctx._source.share_with.containsKey(scopeName)) { - def existingScope = ctx._source.share_with.get(scopeName); - - for (def entry : params.revokeAccess.entrySet()) { - def entityType = entry.getKey(); - def entitiesToRemove = entry.getValue(); - - if (existingScope.containsKey(entityType) && existingScope[entityType] != null) { - existingScope[entityType].removeAll(entitiesToRemove); - } + Map revoke = new HashMap<>(); + for (Map.Entry> entry : revokeAccess.entrySet()) { + revoke.put(entry.getKey().name().toLowerCase(), new ArrayList<>(entry.getValue())); + } + + List scopesToUse = scopes != null ? new ArrayList<>(scopes) : new ArrayList<>(); + + Script revokeScript = new Script(ScriptType.INLINE, "painless", """ + if (ctx._source.share_with != null) { + Set scopesToProcess = new HashSet(params.scopes.isEmpty() ? ctx._source.share_with.keySet() : params.scopes); + + for (def scopeName : scopesToProcess) { + if (ctx._source.share_with.containsKey(scopeName)) { + def existingScope = ctx._source.share_with.get(scopeName); + + for (def entry : params.revokeAccess.entrySet()) { + def entityType = entry.getKey(); + def entitiesToRemove = entry.getValue(); + + if (existingScope.containsKey(entityType) && existingScope[entityType] != null) { + if (!(existingScope[entityType] instanceof HashSet)) { + existingScope[entityType] = new HashSet(existingScope[entityType]); + } + + existingScope[entityType].removeAll(entitiesToRemove); + + if (existingScope[entityType].isEmpty()) { + existingScope.remove(entityType); } } } + + if (existingScope.isEmpty()) { + ctx._source.share_with.remove(scopeName); + } } - """, - Map.of("revokeAccess", revokeAccess, "scopes", scopes) - ); + } + } + """, Map.of("revokeAccess", revoke, "scopes", scopesToUse)); + // Execute updateByQuery boolean success = updateByQueryResourceSharing(sourceIdx, resourceId, revokeScript); + return success ? fetchDocumentById(sourceIdx, resourceId) : null; } catch (Exception e) {