Implement permission-scoped forwarding for Insights API requests #1136
+823
−3
Conversation
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
Reviewer's GuideImplements permission-scoped forwarding for Insights API requests by enforcing Foreman-level Vulnerability and Advisor permissions in the InsightsApiForwarder and exposing a helper that maps those permissions to Insights Chrome API permission strings for frontend access control, with comprehensive unit tests for both the forwarder and the helper plus plugin permission wiring. Sequence diagram for permission-scoped Insights API forwardingsequenceDiagram
actor User
participant ForemanController
participant InsightsApiForwarder
participant UserModel as User
participant InsightsCloud
User ->> ForemanController: HTTP request (method, path)
ForemanController ->> InsightsApiForwarder: forward_request(original_request, path, controller_name, user, organization, location)
InsightsApiForwarder ->> InsightsApiForwarder: required_permission_for(path, original_request.request_method)
alt Permission required
InsightsApiForwarder ->> InsightsApiForwarder: user_has_permission?(user, permission)
alt User has permission
InsightsApiForwarder ->> InsightsApiForwarder: scope_request?(original_request, path)
alt Scoped request
InsightsApiForwarder ->> TagsAuth: new(user, organization, location, logger)
TagsAuth ->> TagsAuth: update_tag
end
InsightsApiForwarder ->> InsightsApiForwarder: prepare_forward_params(original_request, path, user, organization, location)
InsightsApiForwarder ->> InsightsCloud: Forward proxied request
InsightsCloud -->> InsightsApiForwarder: Response
InsightsApiForwarder -->> ForemanController: Response
ForemanController -->> User: HTTP response
else User lacks permission
InsightsApiForwarder ->> InsightsApiForwarder: logger.warn("User login lacks permission ...")
InsightsApiForwarder ->> ForemanController: Raise Foreman::Exception (Forbidden)
ForemanController -->> User: 403 Forbidden
end
else No permission required
InsightsApiForwarder ->> InsightsApiForwarder: scope_request?(original_request, path)
alt Scoped request
InsightsApiForwarder ->> TagsAuth: new(user, organization, location, logger)
TagsAuth ->> TagsAuth: update_tag
end
InsightsApiForwarder ->> InsightsCloud: Forward proxied request
InsightsCloud -->> InsightsApiForwarder: Response
InsightsApiForwarder -->> ForemanController: Response
ForemanController -->> User: HTTP response
end
Updated class diagram for InsightsApiForwarder and permissions helperclassDiagram
class ForemanRhCloud_InsightsApiForwarder {
<<service>>
SCOPED_REQUESTS
+forward_request(original_request, path, controller_name, user, organization, location)
+scope_request?(original_request, path)
+prepare_forward_params(original_request, path, user, organization, location)
+http_user_agent(original_request)
+logger()
+required_permission_for(path, http_method)
+user_has_permission?(user, permission)
}
class ForemanRhCloudPermissionsHelper {
<<module>>
PERMISSION_MAPPING
+insights_user_permissions()
+permissions_to_insights_format(permission)
}
class User {
+login : String
+can?(permission) Bool
}
class TagsAuth {
+TagsAuth(user, organization, location, logger)
+update_tag()
}
class ForemanRhCloudPlugin {
<<plugin>>
+register()
rh_cloud_permissions : Array~Symbol~
insights_permissions : Array~Symbol~
plugin_permissions : Array~Symbol~
}
ForemanRhCloud_InsightsApiForwarder --> User : uses_for_permission_checks
ForemanRhCloud_InsightsApiForwarder --> TagsAuth : creates_and_uses
ForemanRhCloudPermissionsHelper --> User : uses_current_user
ForemanRhCloudPlugin ..> ForemanRhCloud_InsightsApiForwarder : defines_permissions_for
ForemanRhCloudPlugin ..> ForemanRhCloudPermissionsHelper : defines_permissions_for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
forward_request, the log lineUser #{user.login}...will raise ifuseris ever nil (whileuser_has_permission?explicitly supports a nil user); consider guarding or using safe navigation to avoid aNoMethodError. - The permission-enforcement tests build many similar
ActionDispatch::Requestobjects inline; extracting a small helper/factory for common cases (GET with query params, POST/PATCH with body, etc.) would make the tests shorter and easier to maintain. - Tests currently rely on the
RAW_POST_DATAkey when setting up request bodies, which is deprecated in modern Rack/Rails; consider switching to writing the body torack.input(e.g.,StringIO.new(post_data)) to better match real request handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `forward_request`, the log line `User #{user.login}...` will raise if `user` is ever nil (while `user_has_permission?` explicitly supports a nil user); consider guarding or using safe navigation to avoid a `NoMethodError`.
- The permission-enforcement tests build many similar `ActionDispatch::Request` objects inline; extracting a small helper/factory for common cases (GET with query params, POST/PATCH with body, etc.) would make the tests shorter and easier to maintain.
- Tests currently rely on the `RAW_POST_DATA` key when setting up request bodies, which is deprecated in modern Rack/Rails; consider switching to writing the body to `rack.input` (e.g., `StringIO.new(post_data)`) to better match real request handling.
## Individual Comments
### Comment 1
<location> `app/services/foreman_rh_cloud/insights_api_forwarder.rb:12-21` </location>
<code_context>
+ # view_vulnerability | GET /api/vulnerability/v1/*
</code_context>
<issue_to_address>
**🚨 issue (security):** Permission table comments for vulnerability GET endpoints don't match the actual enforcement logic.
The header states that `view_vulnerability` is required for all `GET /api/vulnerability/v1/*`, but `SCOPED_REQUESTS` only applies permissions to specific POST/PATCH routes and otherwise only tags these requests. As a result, vulnerability GET endpoints (e.g. CVE list, reports) are not actually gated by `required_permission_for('view_vulnerability')`. Either add an explicit pattern to enforce `view_vulnerability` on these GET routes (similar to `view_advisor` for `GET /api/insights/v1/.*`) or update the comment to describe the narrower enforcement that currently exists.
</issue_to_address>
### Comment 2
<location> `app/services/foreman_rh_cloud/insights_api_forwarder.rb:28-31` </location>
<code_context>
SCOPED_REQUESTS = [
- { test: %r{api/vulnerability/v1/vulnerabilities/cves}, tag_name: :tags },
+ # Inventory hosts - requires view_vulnerability for GET
+ {
+ test: %r{api/inventory/v1/hosts(/.*)?$},
+ tag_name: :tags,
+ permissions: {
+ 'GET' => :view_vulnerability,
+ },
</code_context>
<issue_to_address>
**issue (bug_risk):** GET requests to ack/hostack endpoints bypass the view_advisor check due to pattern ordering.
Because `required_permission_for` matches the specific `ack`/`hostack` patterns first, `GET /api/insights/v1/ack/...` and `GET /api/insights/v1/hostack/...` end up with `permissions[http_method] == nil`, so no permission is enforced, and they never fall through to the generic `api/insights/v1/.*` rule that requires `view_advisor`. If these GETs are intended to require `view_advisor`, either add `'GET' => :view_advisor` to the `ack`/`hostack` patterns or change `required_permission_for` so that it prefers patterns that define permissions for the current method.
</issue_to_address>
### Comment 3
<location> `test/unit/services/foreman_rh_cloud/insights_api_forwarder_test.rb:342-358` </location>
<code_context>
+ end
+
+ # PATCH /api/vulnerability/v1/systems/opt_out requires edit_vulnerability
+ test 'should allow PATCH request to systems opt_out when user has edit_vulnerability permission' do
+ patch_data = '{"opt_out": true}'
+ req = ActionDispatch::Request.new(
+ 'REQUEST_URI' => '/api/vulnerability/v1/systems/opt_out',
+ 'REQUEST_METHOD' => 'PATCH',
+ 'rack.input' => ::Puma::NullIO.new,
+ 'RAW_POST_DATA' => patch_data,
+ "action_dispatch.request.path_parameters" => { :format => "json" }
+ )
+
+ @user.stubs(:can?).with(:edit_vulnerability).returns(true)
+ @forwarder.expects(:execute_cloud_request).returns(true)
+
+ @forwarder.forward_request(req, 'api/vulnerability/v1/systems/opt_out', 'test_controller', @user, @organization, @location)
+ end
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative test case for PATCH systems opt_out when the user lacks edit_vulnerability
Please add a complementary test that stubs `@user.can?(:edit_vulnerability)` to `false` and expects a `Foreman::Exception`, matching the pattern used for other protected endpoints to verify the permission check is enforced.
```suggestion
# PATCH /api/vulnerability/v1/systems/opt_out requires edit_vulnerability
test 'should allow PATCH request to systems opt_out when user has edit_vulnerability permission' do
patch_data = '{"opt_out": true}'
req = ActionDispatch::Request.new(
'REQUEST_URI' => '/api/vulnerability/v1/systems/opt_out',
'REQUEST_METHOD' => 'PATCH',
'rack.input' => ::Puma::NullIO.new,
'RAW_POST_DATA' => patch_data,
"action_dispatch.request.path_parameters" => { :format => "json" }
)
@user.stubs(:can?).with(:edit_vulnerability).returns(true)
@forwarder.expects(:execute_cloud_request).returns(true)
@forwarder.forward_request(req, 'api/vulnerability/v1/systems/opt_out', 'test_controller', @user, @organization, @location)
end
test 'should deny PATCH request to systems opt_out when user lacks edit_vulnerability permission' do
patch_data = '{"opt_out": true}'
req = ActionDispatch::Request.new(
'REQUEST_URI' => '/api/vulnerability/v1/systems/opt_out',
'REQUEST_METHOD' => 'PATCH',
'rack.input' => ::Puma::NullIO.new,
'RAW_POST_DATA' => patch_data,
"action_dispatch.request.path_parameters" => { :format => "json" }
)
@user.stubs(:can?).with(:edit_vulnerability).returns(false)
assert_raises(Foreman::Exception) do
@forwarder.forward_request(req, 'api/vulnerability/v1/systems/opt_out', 'test_controller', @user, @organization, @location)
end
end
```
</issue_to_address>
### Comment 4
<location> `test/unit/helpers/foreman_rh_cloud_permissions_helper_test.rb:143-140` </location>
<code_context>
+ test 'insights_user_permissions should return all permissions when user has all' do
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test to ensure no duplicate Chrome permissions are emitted
Since `insights_user_permissions` aggregation looks good, it would still be helpful to assert that the resulting `permission_strings` list contains no duplicates (either in this test or a dedicated one). That way, if `PERMISSION_MAPPING` changes in the future (e.g., overlapping Foreman permissions), we’ll catch any accidental multiple mappings of the same Insights permission.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+12
to
+21
| # view_vulnerability | GET /api/vulnerability/v1/* | ||
| # | POST /api/vulnerability/v1/vulnerabilities/cves | ||
| # edit_vulnerability | PATCH /api/vulnerability/v1/status | ||
| # edit_vulnerability | PATCH /api/vulnerability/v1/cves/status | ||
| # | PATCH /api/vulnerability/v1/cves/business_risk | ||
| # edit_vulnerability | PATCH /api/vulnerability/v1/systems/opt_out | ||
| # | ||
| # view_advisor | GET /api/insights/v1/* | ||
| # edit_advisor | POST /api/insights/v1/ack/ | ||
| # | DELETE /api/insights/v1/ack/{rule_id}/ |
There was a problem hiding this comment.
🚨 issue (security): Permission table comments for vulnerability GET endpoints don't match the actual enforcement logic.
The header states that view_vulnerability is required for all GET /api/vulnerability/v1/*, but SCOPED_REQUESTS only applies permissions to specific POST/PATCH routes and otherwise only tags these requests. As a result, vulnerability GET endpoints (e.g. CVE list, reports) are not actually gated by required_permission_for('view_vulnerability'). Either add an explicit pattern to enforce view_vulnerability on these GET routes (similar to view_advisor for GET /api/insights/v1/.*) or update the comment to describe the narrower enforcement that currently exists.
Collaborator
There was a problem hiding this comment.
We do need to make sure these tables match reality (I think this will be easier with AI). Or another option is to remove them, but I do think they are nice to have..
| assert_equal 1, permissions.length | ||
|
|
||
| permission_strings = permissions.map { |p| p[:permission] } | ||
| assert_includes permission_strings, 'advisor:disable-recommendations:write' |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test to ensure no duplicate Chrome permissions are emitted
Since insights_user_permissions aggregation looks good, it would still be helpful to assert that the resulting permission_strings list contains no duplicates (either in this test or a dedicated one). That way, if PERMISSION_MAPPING changes in the future (e.g., overlapping Foreman permissions), we’ll catch any accidental multiple mappings of the same Insights permission.
jeremylenz
reviewed
Jan 8, 2026
Collaborator
jeremylenz
left a comment
jeremylenz
left a comment
There was a problem hiding this comment.
Thanks @nofaralfasi !
some initial minor comments below, haven't tested yet
Comment on lines
+12
to
+21
| # view_vulnerability | GET /api/vulnerability/v1/* | ||
| # | POST /api/vulnerability/v1/vulnerabilities/cves | ||
| # edit_vulnerability | PATCH /api/vulnerability/v1/status | ||
| # edit_vulnerability | PATCH /api/vulnerability/v1/cves/status | ||
| # | PATCH /api/vulnerability/v1/cves/business_risk | ||
| # edit_vulnerability | PATCH /api/vulnerability/v1/systems/opt_out | ||
| # | ||
| # view_advisor | GET /api/insights/v1/* | ||
| # edit_advisor | POST /api/insights/v1/ack/ | ||
| # | DELETE /api/insights/v1/ack/{rule_id}/ |
Collaborator
There was a problem hiding this comment.
We do need to make sure these tables match reality (I think this will be easier with AI). Or another option is to remove them, but I do think they are nice to have..
nofaralfasi
force-pushed
the
SAT-40631
branch
3 times, most recently
from
2b1c088 to
05a9904
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Requires #1135.
What are the changes introduced in this pull request?
Add permission enforcement for Insights API forwarding.
Implement permission checks in InsightsApiForwarder before proxying
requests to Insights cloud. Users without appropriate view/edit
permissions for Vulnerability or Advisor apps receive 403 Forbidden.
Add ForemanRhCloudPermissionsHelper to map Foreman permissions to
Insights Chrome API format for frontend UI access control.
Includes comprehensive tests for all protected endpoints.
Considerations taken when implementing this change?
This is Part 2 of a 3-part PR series.
What are the testing steps for this pull request?
This PR should be tested together with the related PRs. For now, please ensure that all Ruby tests pass.
Summary by Sourcery
Enforce Foreman role-based permissions when forwarding Insights-related API requests and expose corresponding Insights Chrome permissions for the frontend.
Enhancements:
Tests: