ROX-33402: Change permission from modify to view for view based reports#19807
ROX-33402: Change permission from modify to view for view based reports#19807ksurabhi91 wants to merge 1 commit intomasterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19807 +/- ##
=======================================
Coverage 49.60% 49.60%
=======================================
Files 2763 2763
Lines 208271 208271
=======================================
Hits 103312 103312
Misses 97292 97292
Partials 7667 7667
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 Build Images ReadyImages are ready for commit 6fe084f. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-558-g6fe084f927 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughPermission requirements for report-related endpoints are downgraded from Modify to View for WorkflowAdministration across two authorization points in the API routing and service layer, while maintaining View permission for Image resources. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
central/reports/service/v2/service_impl.go (1)
56-63:⚠️ Potential issue | 🔴 CriticalThe effective permission here is still
Modify.This table now places
apiV2.ReportService_PostViewBasedReport_FullMethodNameunder a view-level authorizer, butPostViewBasedReportstill callsworkflowSAC.WriteAllowed(ctx)on Line 476. Users who only gain the new view-level permission will still be rejected, so the RPC's actual authorization does not match this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@central/reports/service/v2/service_impl.go` around lines 56 - 63, The RPC mapping puts apiV2.ReportService_PostViewBasedReport_FullMethodName under a view-level authorizer but the handler PostViewBasedReport still enforces workflowSAC.WriteAllowed(ctx), so users with only view permissions are rejected; fix by making the authorization in PostViewBasedReport match the view-level permission (replace workflowSAC.WriteAllowed(ctx) with the appropriate view/read check such as workflowSAC.ReadAllowed(ctx) or workflowSAC.ViewAllowed(ctx) and adjust the error message/logging accordingly), or alternatively move apiV2.ReportService_PostViewBasedReport_FullMethodName back under the modify-level permissions to keep WriteAllowed — choose the option that preserves intended semantics and update any relevant tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@central/main.go`:
- Around line 959-963: The download route for "/api/reports/jobs/download"
currently requires permissions.View(resources.WorkflowAdministration) alongside
permissions.View(resources.Image), which blocks users with the target viewer
role; update the authorization on that customRoutes entry (where
NewDownloadHandler() is set) to include the target viewer permission by
replacing or adding permissions.View(resources.TargetViewer) in the
user.With(...) call so users with the target viewer role (and/or workflow admin
as appropriate) can access the download endpoint.
---
Outside diff comments:
In `@central/reports/service/v2/service_impl.go`:
- Around line 56-63: The RPC mapping puts
apiV2.ReportService_PostViewBasedReport_FullMethodName under a view-level
authorizer but the handler PostViewBasedReport still enforces
workflowSAC.WriteAllowed(ctx), so users with only view permissions are rejected;
fix by making the authorization in PostViewBasedReport match the view-level
permission (replace workflowSAC.WriteAllowed(ctx) with the appropriate view/read
check such as workflowSAC.ReadAllowed(ctx) or workflowSAC.ViewAllowed(ctx) and
adjust the error message/logging accordingly), or alternatively move
apiV2.ReportService_PostViewBasedReport_FullMethodName back under the
modify-level permissions to keep WriteAllowed — choose the option that preserves
intended semantics and update any relevant tests.
🪄 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: Pro
Run ID: 89120d04-0ebf-457c-b470-2d8d95f8bc6d
📒 Files selected for processing (2)
central/main.gocentral/reports/service/v2/service_impl.go
| customRoutes = append(customRoutes, routes.CustomRoute{ | ||
| Route: "/api/reports/jobs/download", | ||
| Authorizer: user.With(permissions.Modify(resources.WorkflowAdministration), permissions.View(resources.Image)), | ||
| Authorizer: user.With(permissions.View(resources.WorkflowAdministration), permissions.View(resources.Image)), | ||
| ServerHandler: v2Service.NewDownloadHandler(), | ||
| Compression: true, |
There was a problem hiding this comment.
This download route still excludes the target viewer role.
Line 961 still requires permissions.View(resources.WorkflowAdministration) in addition to permissions.View(resources.Image). Users who can view CVE results but lack workflow-administration access will still 403 on /api/reports/jobs/download, so the download side of the permission change is not complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@central/main.go` around lines 959 - 963, The download route for
"/api/reports/jobs/download" currently requires
permissions.View(resources.WorkflowAdministration) alongside
permissions.View(resources.Image), which blocks users with the target viewer
role; update the authorization on that customRoutes entry (where
NewDownloadHandler() is set) to include the target viewer permission by
replacing or adding permissions.View(resources.TargetViewer) in the
user.With(...) call so users with the target viewer role (and/or workflow admin
as appropriate) can access the download endpoint.
Currently starting a view based report job and downloading a view based report requires
Modify WorkflowAdministrationpermission. However report should be downloadable for users that can view CVEs on the Results page which requiresView Imagepermission only. This PR lowers the permission set for view based reports to match the API that returns CVE Results.User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!