{{ message }}
Combined test/rbacrep#21405
Draft
ksurabhi91 wants to merge 10 commits into
Draft
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughVulnerability report authorization and routing now use image-and-deployment access checks. Backend report routes and RPC methods were updated, the report page and tab layout use permission-gated routes, and workload pages changed their view-based report gating. ChangesVulnerability reports authorization and routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@central/reports/service/v2/service_impl.go`:
- Around line 67-70: RunReport and DeleteReport are too broadly authorized under
the View Image + View Deployment rule in the service authorizer. Update the
authorization mapping in ReportServiceImpl so both RPCs remain behind
WorkflowAdministration, and handle any user-owned delete allowance with a
method-level ownership check inside DeleteReport rather than weakening the
coarse RPC-level permissions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 027cdd7a-e090-4756-b800-a11d50d7a3f1
📒 Files selected for processing (10)
central/main.gocentral/reports/service/v2/service_impl.gocentral/reports/service/v2/service_impl_test.goui/apps/platform/src/Containers/MainPage/Body.tsxui/apps/platform/src/Containers/Vulnerabilities/VulnerablityReporting/VulnReportingPage.tsxui/apps/platform/src/Containers/Vulnerabilities/VulnerablityReporting/VulnReports/VulnReportingLayout.tsxui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/Deployment/DeploymentPage.tsxui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/Image/ImagePage.tsxui/apps/platform/src/Containers/Vulnerabilities/WorkloadCves/Overview/WorkloadCvesOverviewPage.tsxui/apps/platform/src/routePaths.ts
Comment on lines
+67
to
70
| // view permissions are enough if user is deleting a job created by the user | ||
| user.With(permissions.View(resources.Image), permissions.View(resources.Deployment)): { | ||
| apiV2.ReportService_RunReport_FullMethodName, | ||
| apiV2.ReportService_CancelReport_FullMethodName, | ||
| apiV2.ReportService_DeleteReport_FullMethodName, |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep collection report lifecycle methods behind WorkflowAdministration.
RunReport and DeleteReport are currently grouped under View Image + View Deployment, but the new auth tests expect both to reject viewImageDeployment without WorkflowAdministration. This also over-broadens collection report actions; if user-owned deletion should be allowed, add that as a method-level ownership check instead of lowering the coarse RPC authorizer.
Suggested authorization alignment
- // view permissions are enough if user is deleting a job created by the user
- user.With(permissions.View(resources.Image), permissions.View(resources.Deployment)): {
+ user.With(permissions.Modify(resources.WorkflowAdministration), permissions.View(resources.Image), permissions.View(resources.Deployment)): {
apiV2.ReportService_RunReport_FullMethodName,
apiV2.ReportService_DeleteReport_FullMethodName,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // view permissions are enough if user is deleting a job created by the user | |
| user.With(permissions.View(resources.Image), permissions.View(resources.Deployment)): { | |
| apiV2.ReportService_RunReport_FullMethodName, | |
| apiV2.ReportService_CancelReport_FullMethodName, | |
| apiV2.ReportService_DeleteReport_FullMethodName, | |
| // view permissions are enough if user is deleting a job created by the user | |
| user.With(permissions.Modify(resources.WorkflowAdministration), permissions.View(resources.Image), permissions.View(resources.Deployment)): { | |
| apiV2.ReportService_RunReport_FullMethodName, | |
| apiV2.ReportService_DeleteReport_FullMethodName, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@central/reports/service/v2/service_impl.go` around lines 67 - 70, RunReport
and DeleteReport are too broadly authorized under the View Image + View
Deployment rule in the service authorizer. Update the authorization mapping in
ReportServiceImpl so both RPCs remain behind WorkflowAdministration, and handle
any user-owned delete allowance with a method-level ownership check inside
DeleteReport rather than weakening the coarse RPC-level permissions.
Contributor
🚀 Build Images ReadyImages are ready for commit 1734777. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-316-g1734777b9c |
ksurabhi91
force-pushed
the
combined-test/rbacrep
branch
from
8aedf7f to
1734777
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@central/reports/scheduler/v2/scheduler_impl.go`:
- Around line 563-570: The context elevation added for view-based report
persistence is only applied in the insert path, while the resubmission path
still uses the original ctx when calling UpdateReportSnapshot. Update the
reSubmission branch in scheduler_impl.go to use the same conditional
sac.WithAllAccess(ctx) logic based on snapshot.GetViewBasedVulnReportFilters(),
so both AddReportSnapshot and UpdateReportSnapshot run with the privileged
context for view-based reports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bd08e4d6-296b-48c5-bc7c-f224bc8cb335
📒 Files selected for processing (2)
central/reports/scheduler/v2/scheduler_impl.goui/apps/platform/src/routePaths.ts
💤 Files with no reviewable changes (1)
- ui/apps/platform/src/routePaths.ts
Comment on lines
+563
to
+570
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether view-based report snapshots can reach validateAndPersistSnapshot with reSubmission=true.
# Expectation: no call path combines ViewBasedVulnReportFilters with reSubmission=true under a user ctx
# lacking WorkflowAdministration write, or the update branch should also use the privileged persist context.
rg -n -C4 '\bvalidateAndPersistSnapshot\s*\(' central/reports central/main.go
rg -n -C6 'reSubmission|resubmission|ViewBasedVulnReportFilters|VIEW_BASED|PostViewBasedReport' central/reports central/main.goRepository: stackrox/stackrox
Length of output: 43484
Ensure context elevation applies to the resubmission (update) path.
The fix correctly elevates context for the initial insert of view-based reports, but the reSubmission=true branch at line 572 still calls UpdateReportSnapshot(ctx, snapshot) using the non-elevated user context. Since UpdateReportSnapshot enforces the same WorkflowAdministration write gate as AddReportSnapshot, any future scenario where a view-based report is resubmitted will fail at the datastore layer.
Apply the same conditional context elevation logic to the resubmission block:
} else {
- snapshot.ReportId, err = s.reportSnapshotStore.UpdateReportSnapshot(ctx, snapshot)
+ persistCtx := ctx
+ if snapshot.GetViewBasedVulnReportFilters() != nil {
+ persistCtx = sac.WithAllAccess(ctx)
+ }
+ snapshot.ReportId, err = s.reportSnapshotStore.UpdateReportSnapshot(persistCtx, snapshot)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@central/reports/scheduler/v2/scheduler_impl.go` around lines 563 - 570, The
context elevation added for view-based report persistence is only applied in the
insert path, while the resubmission path still uses the original ctx when
calling UpdateReportSnapshot. Update the reSubmission branch in
scheduler_impl.go to use the same conditional sac.WithAllAccess(ctx) logic based
on snapshot.GetViewBasedVulnReportFilters(), so both AddReportSnapshot and
UpdateReportSnapshot run with the privileged context for view-based reports.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!