From 5ad813bcb0d4b7e3516a5fdd29f6340db6e42bf4 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Sat, 7 Dec 2024 18:27:11 -0500 Subject: [PATCH] Adds validation for resource ownership when granting and revoking access Signed-off-by: Darshit Chanpura --- .../resources/ResourceAccessHandler.java | 8 +- .../ResourceSharingIndexHandler.java | 146 ++++++++++-------- 2 files changed, 85 insertions(+), 69 deletions(-) diff --git a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java index 74d83db8c1..d060ce6f38 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java @@ -19,7 +19,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.opensearch.accesscontrol.resources.CreatedBy; import org.opensearch.accesscontrol.resources.EntityType; import org.opensearch.accesscontrol.resources.ResourceSharing; import org.opensearch.accesscontrol.resources.ShareWith; @@ -109,10 +108,9 @@ public boolean hasPermission(String resourceId, String systemIndexName, String s public ResourceSharing shareWith(String resourceId, String systemIndexName, ShareWith shareWith) { final User user = threadContext.getPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER); - LOGGER.info("Sharing resource {} created by {} with {}", resourceId, user, shareWith.toString()); + LOGGER.info("Sharing resource {} created by {} with {}", resourceId, user.getName(), shareWith.toString()); - CreatedBy createdBy = new CreatedBy(user.getName()); - return this.resourceSharingIndexHandler.updateResourceSharingInfo(resourceId, systemIndexName, createdBy, shareWith); + return this.resourceSharingIndexHandler.updateResourceSharingInfo(resourceId, systemIndexName, user.getName(), shareWith); } public ResourceSharing revokeAccess( @@ -124,7 +122,7 @@ public ResourceSharing revokeAccess( final User user = threadContext.getPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER); LOGGER.info("User {} revoking access to resource {} for {} for scopes {} ", user.getName(), resourceId, revokeAccess, scopes); - return this.resourceSharingIndexHandler.revokeAccess(resourceId, systemIndexName, revokeAccess, scopes); + return this.resourceSharingIndexHandler.revokeAccess(resourceId, systemIndexName, revokeAccess, scopes, user.getName()); } public boolean deleteResourceSharingRecord(String resourceId, String systemIndexName) { diff --git a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java index bfc47a907e..6f07f608c9 100644 --- a/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java +++ b/src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java @@ -148,7 +148,6 @@ public void createResourceSharingIndexIfAbsent(Callable callable) { public ResourceSharing indexResourceSharing(String resourceId, String resourceIndex, CreatedBy createdBy, ShareWith shareWith) throws IOException { - try { ResourceSharing entry = new ResourceSharing(resourceIndex, resourceId, createdBy, shareWith); @@ -516,7 +515,6 @@ public Set fetchDocumentsByField(String systemIndex, String field, Strin LOGGER.info("Found {} documents in {} where {} = {}", resourceIds.size(), resourceSharingIndex, field, value); return resourceIds; - } catch (Exception e) { LOGGER.error("Failed to fetch documents from {} where {} = {}", resourceSharingIndex, field, value, e); throw new RuntimeException("Failed to fetch documents: " + e.getMessage(), e); @@ -579,7 +577,6 @@ public Set fetchDocumentsByField(String systemIndex, String field, Strin */ public ResourceSharing fetchDocumentById(String pluginIndex, String resourceId) { - // Input validation if (StringUtils.isBlank(pluginIndex) || StringUtils.isBlank(resourceId)) { throw new IllegalArgumentException("systemIndexName and resourceId must not be null or empty"); } @@ -668,12 +665,13 @@ private void executeSearchRequest(Set resourceIds, Scroll scroll, Search /** * Updates the sharing configuration for an existing resource in the resource sharing index. - * NOTE: This method only grants new access. To remove access use {@link #revokeAccess(String, String, Map, Set)} + * NOTE: This method only grants new access. To remove access use {@link #revokeAccess(String, String, Map, Set, String)} * This method modifies the sharing permissions for a specific resource identified by its * resource ID and source index. * * @param resourceId The unique identifier of the resource whose sharing configuration needs to be updated * @param sourceIdx The source index where the original resource is stored + * @param requestUserName The user requesting to share the resource * @param shareWith Updated sharing configuration object containing access control settings: * { * "scope": { @@ -685,7 +683,7 @@ private void executeSearchRequest(Set resourceIds, Scroll scroll, Search * @return ResourceSharing Returns resourceSharing object if the update was successful, null otherwise * @throws RuntimeException if there's an error during the update operation */ - public ResourceSharing updateResourceSharingInfo(String resourceId, String sourceIdx, CreatedBy createdBy, ShareWith shareWith) { + public ResourceSharing updateResourceSharingInfo(String resourceId, String sourceIdx, String requestUserName, ShareWith shareWith) { XContentBuilder builder; Map shareWithMap; try { @@ -700,9 +698,22 @@ public ResourceSharing updateResourceSharingInfo(String resourceId, String sourc return null; } + // Check if the user requesting to share is the owner of the resource + // TODO Add a way for users who are not creators to be able to share the resource + ResourceSharing currentSharingInfo = fetchDocumentById(sourceIdx, resourceId); + if (currentSharingInfo != null && !currentSharingInfo.getCreatedBy().getUser().equals(requestUserName)) { + LOGGER.error("User {} is not authorized to share resource {}", requestUserName, resourceId); + return null; + } + + CreatedBy createdBy; + if (currentSharingInfo == null) { + createdBy = new CreatedBy(requestUserName); + } else { + createdBy = currentSharingInfo.getCreatedBy(); + } + // 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, "painless", """ if (ctx._source.share_with == null) { ctx._source.share_with = [:]; @@ -792,8 +803,8 @@ public ResourceSharing updateResourceSharingInfo(String resourceId, String sourc * "query": { * "bool": { * "must": [ - * { "term": { "source_idx": sourceIdx } }, - * { "term": { "resource_id": resourceId } } + * { "term": { "source_idx.keyword": sourceIdx } }, + * { "term": { "resource_id.keyword": resourceId } } * ] * } * } @@ -810,8 +821,6 @@ private boolean updateByQueryResourceSharing(String sourceIdx, String resourceId .setScript(updateScript) .setRefresh(true); - LOGGER.debug("Update By Query Request: {}", ubq.toString()); - BulkByScrollResponse response = client.execute(UpdateByQueryAction.INSTANCE, ubq).actionGet(); if (response.getUpdated() > 0) { @@ -866,6 +875,7 @@ private boolean updateByQueryResourceSharing(String sourceIdx, String resourceId * @param revokeAccess A map containing entity types (USER, ROLE, BACKEND_ROLE) and their corresponding * values to be removed from the sharing configuration * @param scopes A list of scopes to revoke access from. If null or empty, access is revoked from all scopes + * @param requestUserName The user trying to revoke the accesses * @return The updated ResourceSharing object after revoking access, or null if the document doesn't exist * @throws IllegalArgumentException if resourceId, sourceIdx is null/empty, or if revokeAccess is null/empty * @throws RuntimeException if the update operation fails or encounters an error @@ -887,12 +897,20 @@ public ResourceSharing revokeAccess( String resourceId, String sourceIdx, Map> revokeAccess, - Set scopes + Set scopes, + String requestUserName ) { if (StringUtils.isBlank(resourceId) || StringUtils.isBlank(sourceIdx) || revokeAccess == null || revokeAccess.isEmpty()) { throw new IllegalArgumentException("resourceId, sourceIdx, and revokeAccess must not be null or empty"); } + // TODO Check if access can be revoked by non-creator + ResourceSharing currentSharingInfo = fetchDocumentById(sourceIdx, resourceId); + if (currentSharingInfo != null && !currentSharingInfo.getCreatedBy().getUser().equals(requestUserName)) { + LOGGER.error("User {} is not authorized to revoke access to resource {}", requestUserName, resourceId); + return null; + } + LOGGER.debug("Revoking access for resource {} in {} for entities: {} and scopes: {}", resourceId, sourceIdx, revokeAccess, scopes); try { @@ -1019,58 +1037,58 @@ public boolean deleteResourceSharingRecord(String resourceId, String sourceIdx) } /** - * Deletes all resource sharing records that were created by a specific user. - * This method performs a delete-by-query operation to remove all documents where - * the created_by.user field matches the specified username. - * - *

The method executes the following steps: - *

    - *
  1. Validates the input username parameter
    2. - *
  2. Creates a delete-by-query request with term query matching
    3. - *
  3. Executes the delete operation with immediate refresh
    4. - *
  4. Returns the operation status based on number of deleted documents
    5. - *
- * - *

Example query structure: - * 

-        * {
-        *   "query": {
-        *     "term": {
-        *       "created_by.user": "username"
-        *     }
-        *   }
-        * }
-        *
- * - * @param name The username to match against the created_by.user field - * @return boolean indicating whether the deletion was successful: - *
    - *
  • true - if one or more documents were deleted
    • - *
  • false - if no documents were found
    • - *
  • false - if the operation failed due to an error
    • - *
- * - * @throws IllegalArgumentException if name is null or empty - * - * - * @implNote Implementation details: - *
    - *
  • Uses DeleteByQueryRequest for efficient bulk deletion
    • - *
  • Sets refresh=true for immediate consistency
    • - *
  • Uses term query for exact username matching
    • - *
  • Implements comprehensive error handling and logging
    • - *
- * - * Example usage: - * 
-        * boolean success = deleteAllRecordsForUser("john.doe");
-        * if (success) {
-        *     // Records were successfully deleted
-        * } else {
-        *     // No matching records found or operation failed
-        * }
-        *
- */ + * Deletes all resource sharing records that were created by a specific user. + * This method performs a delete-by-query operation to remove all documents where + * the created_by.user field matches the specified username. + * + *

The method executes the following steps: + *

    + *
  1. Validates the input username parameter
    2. + *
  2. Creates a delete-by-query request with term query matching
    3. + *
  3. Executes the delete operation with immediate refresh
    4. + *
  4. Returns the operation status based on number of deleted documents
    5. + *
+ * + *

Example query structure: + * 

+    * {
+    *   "query": {
+    *     "term": {
+    *       "created_by.user": "username"
+    *     }
+    *   }
+    * }
+    *
+ * + * @param name The username to match against the created_by.user field + * @return boolean indicating whether the deletion was successful: + *
    + *
  • true - if one or more documents were deleted
    • + *
  • false - if no documents were found
    • + *
  • false - if the operation failed due to an error
    • + *
+ * + * @throws IllegalArgumentException if name is null or empty + * + * + * @implNote Implementation details: + *
    + *
  • Uses DeleteByQueryRequest for efficient bulk deletion
    • + *
  • Sets refresh=true for immediate consistency
    • + *
  • Uses term query for exact username matching
    • + *
  • Implements comprehensive error handling and logging
    • + *
+ * + * Example usage: + * 
+    * boolean success = deleteAllRecordsForUser("john.doe");
+    * if (success) {
+    *     // Records were successfully deleted
+    * } else {
+    *     // No matching records found or operation failed
+    * }
+    *
+ */ public boolean deleteAllRecordsForUser(String name) { if (StringUtils.isBlank(name)) { throw new IllegalArgumentException("Username must not be null or empty");