Skip to content

SED-4536 granular-access-control-for-resource-types #617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
david-stephan wants to merge 3 commits into
base: master
Choose a base branch
from
Open

SED-4536 granular-access-control-for-resource-types #617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff12ac3
SED-4536 Granular access control for resource types
david-stephan Mar 19, 2026
4033942
SED-4536 revert right setting for resources table
david-stephan Mar 20, 2026
61a162b
SED-4536 PR feedbacks
david-stephan Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
<maven.compiler.release>11</maven.compiler.release>
<!-- internal dependencies -->
<step-grid.version>0.0.0-MASTER-SNAPSHOT</step-grid.version>
<step-framework.version>0.0.0-MASTER-SNAPSHOT</step-framework.version>
<step-framework.version>0.0.0-SED-4536-SNAPSHOT</step-framework.version>

<!-- external, non-transitive, dependencies -->
<dep.groovy.version>5.0.4</dep.groovy.version>
Expand Down
5 changes: 5 additions & 0 deletions ...p-controller-remote-client/src/main/java/step/client/resources/RemoteResourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,11 @@ public String getResourceName() {
return resource.getResourceName();
}

@Override
public Resource getResource() {
return resource;
}

@Override
public void close() throws IOException {
// TODO Auto-generated method stub
Expand Down
5 changes: 4 additions & 1 deletion step-controller/step-controller-server/src/main/java/step/core/access/RoleProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@
package step.core.access;

import java.util.List;
import java.util.Set;

public interface RoleProvider {

public List<Role> getRoles();
List<Role> getRoles();

Set<String> getAllRights();
}
19 changes: 19 additions & 0 deletions ...oller/step-controller-server/src/main/java/step/core/deployment/AbstractStepServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@
public abstract class AbstractStepServices extends AbstractServices<User> {

public static final String SESSION = "session";
public static final String RIGHT_SEPARATOR = "-";
public static final String READ_RIGHT = "read";
public static final String WRITE_RIGHT = "write";
public static final String DELETE_RIGHT = "delete";
public static final String EXECUTE_RIGHT = "execute";


protected Configuration configuration;

Expand Down Expand Up @@ -118,6 +124,19 @@ protected void checkRightsOnBehalfOf(String right, String userOnBehalfOf) {
}
}

protected void checkRightIfDefined(String right) {
Session<User> session = getSession();
try {
if (!getAuthorizationManager().checkRightInContextIfDefined(session, right)) {
User user = session.getUser();
throw new AuthorizationException("User " + (user == null ? "" : user.getUsername()) + " has no permission on '" + right + "'");
}
} catch (NotMemberOfProjectException ex) {
// if user is not a member of the project, we want to return 'access denied' error
throw new AuthorizationException(ex.getMessage());
}
}

/**
* The ObjectHookInterceptor.aroundReadFrom can only be used for POST request passing an entity as request BODY
* This method can be used as helper for all other cases where checking if the entity is editable in given context (i.e. DELETE request...(
Expand Down
70 changes: 58 additions & 12 deletions step-controller/step-controller-server/src/main/java/step/resources/ResourceServices.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,16 @@
import java.io.InputStream;
import java.util.*;
import java.util.function.Consumer;
import java.util.stream.Collectors;

@Path("/resources")
@Tag(name = "Resources")
public class ResourceServices extends AbstractStepAsyncServices {

private static final String RESOURCE_RIGHT_NAME = "resource";
public static final String INVALID_ARGUMENTS_A_NON_NULL_RESOURCE_MUST_BE_PROVIDED = "Invalid arguments: a non null resource must be provided.";
public static final String INVALID_RESOURCE_THE_RESOURCE_HAS_NO_RESOURCE_TYPE_SET = "Invalid resource: the resource has no resourceType set.";

protected ResourceManager resourceManager;
private TableService tableService;

Expand All @@ -74,10 +79,25 @@ private void auditLog(String operation, Resource resource) {
AuditLogger.logEntityModification(getSession(), operation, "resources", resource.getId().toHexString(), entityName, attributes);
}

private void checkResourceTypeRight(String resourceType, String right) {
if (resourceType == null || resourceType.isEmpty()) {
throw new ControllerServiceException(INVALID_RESOURCE_THE_RESOURCE_HAS_NO_RESOURCE_TYPE_SET);
}
checkRightIfDefined(RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + resourceType + RIGHT_SEPARATOR + right);
}

private void checkResourceTypeRight(Resource resource, String right) {
//If a resource is null, right is granted too, it's not the role of this function to perform integrity check
Copy link
Contributor

@jeromecomte jeromecomte Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment doesn't seem to match the implementation

if (resource == null) {
throw new ControllerServiceException(INVALID_ARGUMENTS_A_NON_NULL_RESOURCE_MUST_BE_PROVIDED);
Copy link
Contributor

@jeromecomte jeromecomte Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exception might be misleading. In some case it might mean that the provided resourceId doesn't exist (61a162b#diff-3e91bc97d77a8308abd81e9cdf8a6c793ab84adaf260c3100ebf11f6959c6e12R165)

} else {
checkResourceTypeRight(resource.getResourceType(), right);
}
}
Comment on lines +89 to +96
Copy link

@gemini-code-assist gemini-code-assist bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

This method grants access if a resource has no resourceType. While the comment explains this is intentional, this "fail-open" behavior can be a security risk. If a resource is created or modified to have no type (either intentionally by a malicious user or accidentally due to a bug), granular access control is bypassed.

For improved security, consider a "fail-closed" approach where access is denied for resources without a type, or at least log a warning when such resources are accessed to detect potential issues.

Copy link
Contributor Author

@david-stephan david-stephan Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeromecomte I did this on purpose, because many of our services don't check at all if a resourceType is set or not. If you think it is fine from now on to throw an exception when a resource as no resourceType, I'll make the modification.

Copy link
Contributor

@jeromecomte jeromecomte Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@david-stephan This would be safer but we should ensure that we don't break anything. Do you know which clients could call this without resourceType?

Copy link
Contributor Author

@david-stephan david-stephan Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I was hoping you would have an idea :). Now I deep dived into the code, createResource ensure a type is set since 2019 at least. saveResource doesn't but it is apparently only called from the UI which enforce providing a type. The RemoteResourceManager does not use this service, the method saveResource is "Not implemented". The CLI/mvn plugin use the AP services directly. So apart from custom REST clients, no client is supposed to update an existing resource removing the type.


@POST
@Path("/content")
@Secured(right = "resource-write")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + WRITE_RIGHT)
@Consumes(MediaType.MULTIPART_FORM_DATA)
@Produces(MediaType.APPLICATION_JSON)
public ResourceUploadResponse createResource(@FormDataParam("file") InputStream uploadedInputStream,
Expand All @@ -91,9 +111,10 @@ public ResourceUploadResponse createResource(@FormDataParam("file") InputStream

if (uploadedInputStream == null || fileDetail == null)
throw new RuntimeException("Invalid arguments");
if (resourceType == null || resourceType.length() == 0)
if (resourceType == null || resourceType.isEmpty())
throw new RuntimeException("Missing resource type query parameter 'type'");

checkResourceTypeRight(resourceType, WRITE_RIGHT);
try {
Resource resource = resourceManager.createTrackedResource(
resourceType, isDirectory, uploadedInputStream, fileDetail.getFileName(), objectEnricher,
Expand All @@ -113,24 +134,35 @@ private ControllerServiceException uploadFileNotAnArchive() {
}

@POST
@Secured(right = "resource-write")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + WRITE_RIGHT)
@Consumes(MediaType.APPLICATION_JSON)
@Produces(MediaType.APPLICATION_JSON)
public Resource saveResource(Resource resource) throws IOException {
// Always check that we have write access to the new resource type, this automatically ensure the resource and its resourceType are not null
checkResourceTypeRight(resource, WRITE_RIGHT);
Copy link

@gemini-code-assist gemini-code-assist bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

The current implementation has a security vulnerability. A user could update a resource and remove its resourceType to bypass granular access controls. To prevent this, you must check permissions against the resource's currently stored state before applying changes.

Additionally, for consistency with createResource, saveResource should enforce that newly created resources have a resourceType.

        if (resource.getId() != null) {
            // This is an update, check against the stored resource
            Resource storedResource = resourceManager.getResource(resource.getId().toHexString());
            checkResourceTypeRight(storedResource, WRITE_RIGHT);
        } else {
            // This is a new resource, type is mandatory
            if (resource.getResourceType() == null || resource.getResourceType().isEmpty()) {
                throw new RuntimeException("Missing resourceType for new resource");
            }
        }
        checkResourceTypeRight(resource, WRITE_RIGHT);

Copy link
Contributor

@jeromecomte jeromecomte Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree

Copy link
Contributor Author

@david-stephan david-stephan Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking now access to both types (current stored in DB and new one), also ensure types is set and resource is not null

// When updating a resource, we need write access to both the original and new types
String resourceId = resource.getId().toHexString();
if (resourceManager.resourceExists(resourceId)) {
String originalResourceType = resourceManager.getResource(resourceId).getResourceType();
if (!resource.getResourceType().equals(originalResourceType)) {
checkResourceTypeRight(originalResourceType, WRITE_RIGHT);
}
}
auditLog("save", resource);
return resourceManager.saveResource(resource);
}

@POST
@Path("/{id}/content")
@Secured(right = "resource-write")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + WRITE_RIGHT)
@Consumes(MediaType.MULTIPART_FORM_DATA)
@Produces(MediaType.APPLICATION_JSON)
public ResourceUploadResponse saveResourceContent(@PathParam("id") String resourceId, @FormDataParam("file") InputStream uploadedInputStream,
@FormDataParam("file") FormDataContentDisposition fileDetail) throws Exception {
if (uploadedInputStream == null || fileDetail == null)
throw new RuntimeException("Invalid arguments");

checkResourceTypeRight(resourceManager.getResource(resourceId), WRITE_RIGHT);
try {
Resource resource = resourceManager.saveResourceContent(resourceId, uploadedInputStream, fileDetail.getFileName(), null, getSession().getUser().getUsername());
auditLog("save-content", resource);
Expand All @@ -143,31 +175,35 @@ public ResourceUploadResponse saveResourceContent(@PathParam("id") String resour
@GET
@Secured
@Path("/{id}")
@Secured(right = "resource-read")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + READ_RIGHT)
@Produces(MediaType.APPLICATION_JSON)
public Resource getResource(@PathParam("id") String resourceId) throws IOException {
try {
return resourceManager.getResource(resourceId);
Resource resource = resourceManager.getResource(resourceId);
checkResourceTypeRight(resource, READ_RIGHT);
return resource;
} catch (ResourceMissingException e) {
throw new ControllerServiceException(404, e.getMessage());
}
}

@GET
@Path("/{id}/content")
@Secured(right = "resource-read")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + READ_RIGHT)
@Produces(MediaType.APPLICATION_JSON)
public Response getResourceContent(@PathParam("id") String resourceId, @QueryParam("inline") boolean inline) throws IOException {
ResourceRevisionContent resourceContent = resourceManager.getResourceContent(resourceId);
checkResourceTypeRight(resourceContent.getResource(), READ_RIGHT);
return getResponseForResourceRevisionContent(resourceContent, inline);
}

@DELETE
@Secured
@Path("/{id}")
@Secured(right = "resource-delete")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + DELETE_RIGHT)
public void deleteResource(@PathParam("id") String resourceId) {
Resource resource = resourceManager.getResource(resourceId);
checkResourceTypeRight(resource, DELETE_RIGHT);
assertEntityIsEditableInContext(resource);
auditLog("delete", resource);
resourceManager.deleteResource(resourceId);
Expand All @@ -176,9 +212,10 @@ public void deleteResource(@PathParam("id") String resourceId) {
@DELETE
@Secured
@Path("/{id}/revisions")
@Secured(right = "resource-delete")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + DELETE_RIGHT)
public void deleteResourceRevisions(@PathParam("id") String resourceId) {
Resource resource = resourceManager.getResource(resourceId);
checkResourceTypeRight(resource, DELETE_RIGHT);
assertEntityIsEditableInContext(resource);
auditLog("delete-revisions", resource);
resourceManager.deleteResourceRevisionContent(resourceId);
Expand Down Expand Up @@ -207,19 +244,28 @@ public AsyncTaskStatus<TableBulkOperationReport> bulkDelete(TableBulkOperationRe
@GET
@Produces(MediaType.APPLICATION_JSON)
@Path("/revision/{id}/content")
@Secured(right = "resource-read")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + READ_RIGHT)
public Response getResourceRevisionContent(@PathParam("id") String resourceRevisionId, @QueryParam("inline") boolean inline) throws IOException {
ResourceRevisionContentImpl resourceContent = resourceManager.getResourceRevisionContent(resourceRevisionId);
checkResourceTypeRight(resourceContent.getResource(), READ_RIGHT);
return getResponseForResourceRevisionContent(resourceContent, inline);
}

@POST
@Path("/find")
@Produces(MediaType.APPLICATION_JSON)
@Consumes(MediaType.APPLICATION_JSON)
@Secured(right = "resource-read")
@Secured(right = RESOURCE_RIGHT_NAME + RIGHT_SEPARATOR + READ_RIGHT)
public List<Resource> findManyByCriteria(Map<String, String> criteria) {
return resourceManager.findManyByCriteria(criteria);
return resourceManager.findManyByCriteria(criteria).stream().filter(r -> {
try {
checkResourceTypeRight(r, READ_RIGHT);
return true;
} catch (Exception e) {
Copy link

@gemini-code-assist gemini-code-assist bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Catching a generic Exception is not ideal as it can hide other unexpected errors. It's better to catch the specific AuthorizationException that checkResourceTypeRight is expected to throw on a permission failure. This makes the code more robust.

            } catch (AuthorizationException e) {

//we filter out the resources for which the user has no access
return false;
}
}).collect(Collectors.toList());
}

protected Response getResponseForResourceRevisionContent(ResourceRevisionContent resourceContent, boolean inline) {
Expand Down
2 changes: 2 additions & 0 deletions step-core/src/main/java/step/resources/ResourceRevisionContent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ public interface ResourceRevisionContent {

String getResourceName();

Resource getResource();

void close() throws IOException;

}
4 changes: 4 additions & 0 deletions step-core/src/main/java/step/resources/ResourceRevisionContentImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ public String getResourceName() {
return resourceName;
}

public Resource getResource() {
return resource;
}

@Override
public void close() throws IOException {
resourceStream.close();
Expand Down