SED-4417 grid-layout-for-executions-report by david-stephan · Pull Request #603 · exense/step

david-stephan · 2026-03-09T13:38:00Z

No description provided.

gemini-code-assist · 2026-03-09T13:38:20Z

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request delivers a new capability for users to manage and share custom report layouts. It establishes the foundational components for this feature, from data persistence and retrieval to API endpoints and security mechanisms. The changes ensure that report layouts can be created, modified, and accessed with appropriate permissions, enhancing the flexibility and collaborative potential of reporting functionalities. Additionally, the system now tracks user IDs for entity modifications, providing more granular audit trails.

Highlights

New Report Layout Feature: Introduced a comprehensive system for managing report layouts, including a new data model (ReportLayout), data access layer (ReportLayoutAccessor), plugin integration (ReportLayoutPlugin), and RESTful services (ReportLayoutServices). This allows for defining, saving, and retrieving custom report layouts.
Enhanced User Tracking: Modified AbstractEntityServices to store creationUserId and lastModificationUserId alongside usernames, improving auditing capabilities for entity creation and modification.
Access Control for Report Layouts: Implemented robust access control within ReportLayoutServices to manage permissions for saving, deleting, sharing, and unsharing report layouts, distinguishing between owner-specific and shared layouts.

Changelog

pom.xml
- Updated the step-framework.version property to 0.0.0-SED-4417-SNAPSHOT.
step-controller/step-controller-server/src/main/java/step/controller/services/entities/AbstractEntityServices.java
- Imported the step.core.access.User class.
- Modified the trackEntityIfApplicable method to retrieve the User object and store creationUserId and lastModificationUserId.
step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayout.java
- Added a new class ReportLayout extending AbstractTrackedObject to represent report layout entities, including fields for layout (Map) and shared (boolean).
step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutAccessor.java
- Added a new accessor class ReportLayoutAccessor for ReportLayout entities, providing a method to retrieve accessible report layouts based on ownership or shared status.
step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutPlugin.java
- Added a new plugin ReportLayoutPlugin to initialize and register the ReportLayoutAccessor, ReportLayout entity, and ReportLayoutServices during server startup.
step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutServices.java
- Added a new RESTful service ReportLayoutServices for managing ReportLayout entities, including endpoints for listing, saving, deleting, cloning, restoring, locking, sharing, and unsharing layouts, with integrated access control logic.
step-core-model/src/main/java/step/core/entities/EntityConstants.java
- Added a new constant reportLayouts to define the entity name for report layouts.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a new ReportLayout entity to manage layouts, complete with access controls for shared and private settings. However, critical security vulnerabilities have been identified. Specifically, the ReportLayoutServices is vulnerable to IDOR due to missing ownership checks on resource retrieval endpoints, a flaw in the base AbstractEntityServices allows for ownership takeover of entities during updates, and the table registration for report layouts lacks proper filtering, leading to unauthorized information exposure of layout metadata. Additionally, a critical bug prevents the creation of new layouts, and a minor encapsulation issue was found. Addressing these points is crucial to ensure the privacy and integrity of user-defined layouts and the overall stability of the feature.

gemini-code-assist · 2026-03-09T13:40:30Z

...ontroller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutServices.java

+@Tag(name = "ReportLayout")
+@Tag(name = "Entity=ReportLayout")
+@SecuredContext(key = "entity", value = ReportLayoutServices.REPORT_LAYOUT_RIGHT)
+public class ReportLayoutServices extends AbstractEntityServices<ReportLayout> {


The ReportLayoutServices class inherits several data retrieval methods from AbstractEntityServices (such as get, findByIds, findNamesByIds, and findManyByAttributes) but does not override them to implement ownership or sharing checks. While the getAllReportLayouts method correctly filters the list of layouts, an attacker with the global reportLayout-read permission can still access any private layout by its ID or by searching for its attributes. This constitutes an Insecure Direct Object Reference (IDOR) vulnerability.

All critical servies are now overriden

gemini-code-assist · 2026-03-09T13:40:30Z

...ontroller-server/src/main/java/step/controller/services/entities/AbstractEntityServices.java

            if (sourceEntity == null) {
                newTrackedEntity.setCreationDate(lastModificationDate);
                newTrackedEntity.setCreationUser(username);
+                newTrackedEntity.setCreationUserId(userId);
            }


In the trackEntityIfApplicable method, the creationUser and creationUserId fields are only set when a new entity is created (sourceEntity == null). During an update, these fields are not overwritten with the values from the existing database record. Since the entity object is deserialized from the request body, an attacker can provide their own creationUserId in the JSON payload to take over ownership of an existing entity. This allows for privilege escalation and unauthorized modification of resources.

if (sourceEntity == null) { newTrackedEntity.setCreationDate(lastModificationDate); newTrackedEntity.setCreationUser(username); newTrackedEntity.setCreationUserId(userId); } else { AbstractTrackedObject sourceTrackedEntity = (AbstractTrackedObject) sourceEntity; newTrackedEntity.setCreationDate(sourceTrackedEntity.getCreationDate()); newTrackedEntity.setCreationUser(sourceTrackedEntity.getCreationUser()); newTrackedEntity.setCreationUserId(sourceTrackedEntity.getCreationUserId()); }

Good point, this is now fixed.

gemini-code-assist · 2026-03-09T13:40:30Z

...ontroller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutServices.java

+    public ReportLayout save(ReportLayout reportLayout) {
+        //Only check additional specific rights when updating layout
+        Optional.ofNullable(get(reportLayout.getId().toHexString())).ifPresent(entity -> checkLayoutRight(entity, WRITE_RIGHT));
+        return super.save(reportLayout);
+    }


The save method can throw a NullPointerException. For a new ReportLayout entity, reportLayout.getId() will be null, and calling .toHexString() on it will cause an NPE. This prevents new layouts from being saved. You should add a null check for the ID before attempting to check rights on an existing entity.

public ReportLayout save(ReportLayout reportLayout) { //Only check additional specific rights when updating layout if (reportLayout.getId() != null) { Optional.ofNullable(get(reportLayout.getId().toHexString())).ifPresent(entity -> checkLayoutRight(entity, WRITE_RIGHT)); } return super.save(reportLayout); }

No it cannot, id is always set even for a new object, this is ensured by the deserializer

gemini-code-assist · 2026-03-09T13:40:30Z

...-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutPlugin.java

+        tableRegistry.register(EntityConstants.reportLayouts, new Table<>(reportLayoutCollection, ReportLayoutServices.REPORT_LAYOUT_RIGHT + "-read", false).withResultItemTransformer((reportLayout, session) -> {
+            reportLayout.layout = Map.of();
+            return reportLayout;
+        }));


The table for reportLayouts is registered without any default filters to restrict access based on ownership or sharing status. This allows any user with the reportLayout-read permission to view the metadata (such as name, owner, and attributes) of all layouts in the system via the /table endpoint, bypassing the ownership filtering implemented in the /list endpoint. While the layout field itself is cleared, the exposure of metadata for private layouts is still a security concern.

The table is created with restriction for the report layout read right, only authenticated user with this specific right can access the list of layout (stripped from the actual layout content). I don't see it as an issue in this case.

gemini-code-assist · 2026-03-09T13:40:30Z

step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayout.java

+    Map<String, Object> layout;
+    boolean shared = false;


The fields layout and shared have package-private visibility. To enforce proper encapsulation and improve maintainability, they should be declared as private. This change will require updating any direct field access in other classes (e.g., in ReportLayoutPlugin) to use the public setter methods.

Suggested change

Map<String, Object> layout;

boolean shared = false;

private Map<String, Object> layout;

private boolean shared = false;

Or be public without getter

Switch to public and Json creator/property annotations

jeromecomte · 2026-03-09T19:56:16Z

...ontroller-server/src/main/java/step/controller/services/entities/AbstractEntityServices.java

-            String username = getSession().getUser().getUsername();
+            User user = getSession().getUser();
+            String username = user.getUsername();
+            ObjectId userId = user.getId();


As mentioned in the framework PR, I don't think we support the update of the username

We actually can rename any Step users including the "admin" user in the user's admin setting view, that's why I introduced this. For now I couldn't see other cases where renaming the user could cause a functional issue (this is also nothing new), as in most cases it is only used as displayed info or at execution time.

jeromecomte · 2026-03-09T19:57:16Z

step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayout.java

+    Map<String, Object> layout;
+    boolean shared = false;


Or be public without getter

jeromecomte · 2026-03-09T19:57:56Z

step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayout.java

+    public static final String FIELD_IS_SHARED = "shared";
+
+    @NotNull
+    Map<String, Object> layout;


Should we prefer JsonObject?

Replaced indeed with JsonObject which makes more sense.

jeromecomte · 2026-03-09T20:08:24Z

...-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutPlugin.java

+        //Register entity
+        context.getEntityManager().register(new Entity<>(EntityConstants.reportLayouts, reportLayoutAbstractAccessor, ReportLayout.class));
+        //Register Table, table only return layout metadata not the layout itself
+        tableRegistry.register(EntityConstants.reportLayouts, new Table<>(reportLayoutCollection, ReportLayoutServices.REPORT_LAYOUT_RIGHT + "-read", false).withResultItemTransformer((reportLayout, session) -> {


Why do we need a table?

Simply to be coherent with other entities and be ready if a managed layouts view is required in the future. Do you see any issue keeping it?

Not really. If we'll anyway need it in the future it doesn't hurt. Concerning the gemini comment below, it is not a security issue as we remove the content

jeromecomte · 2026-03-09T20:16:55Z

...ontroller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutServices.java

+    private void checkLayoutRight(ReportLayout reportLayout, String right) {
+        //If user is the owner, he is always allowed (if he has the base access right role)
+        User currentUser = this.getSession().getUser();
+        if (!reportLayout.getCreationUserId().equals(currentUser.getId())) {


Will "preset" layouts have a creationUserId?

Well preset layout was still an open point, i.e. are they handled by the BE or FE? We now decided to define them BE side, the model and the access right check have been modified accordingly. Presets won't have any creation/modification metadata but that should not impact the access right check.

# Conflicts: # pom.xml

jeromecomte · 2026-03-11T09:49:34Z

step-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayout.java

-    public void setShared(boolean shared) {
-        this.shared = shared;
+    @JsonCreator
+    public ReportLayout(@JsonProperty("layout") JsonObject layout, @JsonProperty(value = "visibility", defaultValue = "Private") ReportLayoutVisibility visibility) {


We could use the constant declared above for "visibility"

We define 2 default values: one here for deserialization, one below for null values.

jeromecomte · 2026-03-11T09:50:33Z

...ontroller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutAccessor.java


-    public List<ReportLayout> getAccessibleReportLayoutsDefinitions(String username) {
-        Or ownerOrShared = Filters.or(List.of(Filters.equals(FIELD_IS_SHARED, true), Filters.equals("creationUser", username)));
+    public List<ReportLayout> getAccessibleReportLayoutsDefinitions(String userId) {


Change to username as discussed

jeromecomte · 2026-03-11T09:50:43Z

...ontroller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutAccessor.java

-    public List<ReportLayout> getAccessibleReportLayoutsDefinitions(String username) {
-        Or ownerOrShared = Filters.or(List.of(Filters.equals(FIELD_IS_SHARED, true), Filters.equals("creationUser", username)));
+    public List<ReportLayout> getAccessibleReportLayoutsDefinitions(String userId) {
+        Or ownerOrShared = Filters.or(List.of(Filters.equals(FIELD_VISIBILITY, Preset.name()), Filters.equals(FIELD_VISIBILITY, Shared.name()), Filters.equals("creationUserId", userId)));


jeromecomte · 2026-03-11T09:52:55Z

...-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutPlugin.java

+        //Register entity
+        context.getEntityManager().register(new Entity<>(EntityConstants.reportLayouts, reportLayoutAbstractAccessor, ReportLayout.class));
+        //Register Table, table only return layout metadata not the layout itself
+        tableRegistry.register(EntityConstants.reportLayouts, new Table<>(reportLayoutCollection, ReportLayoutServices.REPORT_LAYOUT_RIGHT + "-read", false).withResultItemTransformer((reportLayout, session) -> {


Not really. If we'll anyway need it in the future it doesn't hurt. Concerning the gemini comment below, it is not a security issue as we remove the content

jeromecomte · 2026-03-11T09:54:03Z

...-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutPlugin.java

+    @Override
+    public void initializeData(GlobalContext context) throws Exception {
+        super.initializeData(context);
+        ReportLayoutAccessor reportLayoutAccessor = context.require(ReportLayoutAccessor.class);


Could be accessed as field (detail)...

jeromecomte · 2026-03-11T10:04:29Z

...-controller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutPlugin.java

+        super.initializeData(context);
+        ReportLayoutAccessor reportLayoutAccessor = context.require(ReportLayoutAccessor.class);
+        ReportLayout existingDefaultLayout = reportLayoutAccessor.getReportLayoutPresetIfExists(DEFAULT_REPORT_LAYOUT);
+        try (InputStream resourceAsStream = this.getClass().getResourceAsStream("presets/DefaultLayout.json");


Not sure to understand this logic. We'll we now always have a preset in the DB for the default layout? The content of the DefaultLayout.json seems to be dummy. My understanding was that the default layout is just a pointer to an existing layout (either "preset" or "shared"). No?

Update: just saw the comment in the ticket. Would propose to discuss this

…system

jeromecomte · 2026-03-12T15:06:32Z

...ep-controller-backend/src/test/resources/reporting/layouts/presets/StepMainReportLayout.json

+  "layout": {
+    "widgets": [
+      {
+        "id": "c4460688-41d9-4765-a260-d3bed46c4d50",


As discussed with Tim, these id are not required and should be removed

jeromecomte · 2026-03-12T15:12:21Z

...ontroller/step-controller-server/src/main/java/step/core/reporting/ReportLayoutServices.java

+    @Produces(MediaType.APPLICATION_JSON)
+    @Secured(right = "{entity}-read")
+    public ReportLayoutJson exportLayout(@PathParam("id") String id) {
+        ReportLayout reportLayout = getEntity(id);


Not sure but I have the impression that this bypasses the checkLayoutRight. Shouldn't we use get()?

Good catch I prefer to use getEntity which throw a clean error if the ID does not exists, but added an explicit call to checkLayoutRight now

david-stephan added 2 commits March 6, 2026 16:32

SED-4417 Grid layout for Executions Report

41023f3

SED-4417 adding and using user IDs to verify the owner

e643bba

david-stephan requested a review from jeromecomte March 9, 2026 13:38

gemini-code-assist bot reviewed Mar 9, 2026

View reviewed changes

jeromecomte requested changes Mar 9, 2026

View reviewed changes

david-stephan added 5 commits March 10, 2026 15:01

SED-4417 PR feedbacks

12b4113

SED-4417 adding FE default layout

c68fcdb

SED-4417 adding FE default layout

bf259ca

Merge branch 'master' into SED-4417-grid-layout-for-executions-report

34bb57c

# Conflicts: # pom.xml

SED-4417 merging master code reformatting

75bf5bc

jeromecomte requested changes Mar 11, 2026

View reviewed changes

david-stephan added 2 commits March 12, 2026 14:30

SED-4417 Refactoring, relaying on username, loading presets from file…

1828216

…system

SED-4417 fixing test layout id

125297c

jeromecomte requested changes Mar 12, 2026

View reviewed changes

SED-4417 PR feedbacks

a6acf75

jeromecomte approved these changes Mar 16, 2026

View reviewed changes

Conversation

david-stephan commented Mar 9, 2026

Uh oh!

gemini-code-assist bot commented Mar 9, 2026

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromecomte Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

jeromecomte Mar 11, 2026 •

edited

Loading