Skip to content

Commit

Permalink
Adds validation for resource ownership when granting and revoking access
Browse files Browse the repository at this point in the history 
Signed-off-by: Darshit Chanpura <[email protected]>
  • Loading branch information
@DarshitChanpura
DarshitChanpura committed Dec 7, 2024
1 parent e87bb80 commit 5ad813b
Show file tree
Hide file tree
Showing 2 changed files with 85 additions and 69 deletions.
8 changes: 3 additions & 5 deletions src/main/java/org/opensearch/security/resources/ResourceAccessHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand All @@ -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) {
Expand Down
146 changes: 82 additions & 64 deletions src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ public void createResourceSharingIndexIfAbsent(Callable<Boolean> callable) {

public ResourceSharing indexResourceSharing(String resourceId, String resourceIndex, CreatedBy createdBy, ShareWith shareWith)
throws IOException {

try {
ResourceSharing entry = new ResourceSharing(resourceIndex, resourceId, createdBy, shareWith);

Expand Down Expand Up @@ -516,7 +515,6 @@ public Set<String> 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);
Expand Down Expand Up @@ -579,7 +577,6 @@ public Set<String> 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");
}
Expand Down Expand Up @@ -668,12 +665,13 @@ private void executeSearchRequest(Set<String> 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": {
Expand All @@ -685,7 +683,7 @@ private void executeSearchRequest(Set<String> 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<String, Object> shareWithMap;
try {
Expand All @@ -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 = [:];
Expand Down Expand Up @@ -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 } }
* ]
* }
* }
Expand All @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -887,12 +897,20 @@ public ResourceSharing revokeAccess(
String resourceId,
String sourceIdx,
Map<EntityType, Set<String>> revokeAccess,
Set<String> scopes
Set<String> 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 {
Expand Down Expand Up @@ -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.
*
* <p>The method executes the following steps:
* <ol>
* <li>Validates the input username parameter</li>
* <li>Creates a delete-by-query request with term query matching</li>
* <li>Executes the delete operation with immediate refresh</li>
* <li>Returns the operation status based on number of deleted documents</li>
* </ol>
*
* <p>Example query structure:
* <pre>
* {
* "query": {
* "term": {
* "created_by.user": "username"
* }
* }
* }
* </pre>
*
* @param name The username to match against the created_by.user field
* @return boolean indicating whether the deletion was successful:
* <ul>
* <li>true - if one or more documents were deleted</li>
* <li>false - if no documents were found</li>
* <li>false - if the operation failed due to an error</li>
* </ul>
*
* @throws IllegalArgumentException if name is null or empty
*
*
* @implNote Implementation details:
* <ul>
* <li>Uses DeleteByQueryRequest for efficient bulk deletion</li>
* <li>Sets refresh=true for immediate consistency</li>
* <li>Uses term query for exact username matching</li>
* <li>Implements comprehensive error handling and logging</li>
* </ul>
*
* Example usage:
* <pre>
* boolean success = deleteAllRecordsForUser("john.doe");
* if (success) {
* // Records were successfully deleted
* } else {
* // No matching records found or operation failed
* }
* </pre>
*/
* 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.
*
* <p>The method executes the following steps:
* <ol>
* <li>Validates the input username parameter</li>
* <li>Creates a delete-by-query request with term query matching</li>
* <li>Executes the delete operation with immediate refresh</li>
* <li>Returns the operation status based on number of deleted documents</li>
* </ol>
*
* <p>Example query structure:
* <pre>
* {
* "query": {
* "term": {
* "created_by.user": "username"
* }
* }
* }
* </pre>
*
* @param name The username to match against the created_by.user field
* @return boolean indicating whether the deletion was successful:
* <ul>
* <li>true - if one or more documents were deleted</li>
* <li>false - if no documents were found</li>
* <li>false - if the operation failed due to an error</li>
* </ul>
*
* @throws IllegalArgumentException if name is null or empty
*
*
* @implNote Implementation details:
* <ul>
* <li>Uses DeleteByQueryRequest for efficient bulk deletion</li>
* <li>Sets refresh=true for immediate consistency</li>
* <li>Uses term query for exact username matching</li>
* <li>Implements comprehensive error handling and logging</li>
* </ul>
*
* Example usage:
* <pre>
* boolean success = deleteAllRecordsForUser("john.doe");
* if (success) {
* // Records were successfully deleted
* } else {
* // No matching records found or operation failed
* }
* </pre>
*/
public boolean deleteAllRecordsForUser(String name) {
if (StringUtils.isBlank(name)) {
throw new IllegalArgumentException("Username must not be null or empty");
Expand Down

0 comments on commit 5ad813b

Please sign in to comment.