[WIP] Expose Insights permissions to Scalprum apps via ForemanContext #1137
+934
−40
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 Insights-style RBAC exposure and enforcement for Vulnerability and Advisor UIs by mapping Foreman permissions to Insights permissions, injecting them into the HTML for Scalprum/Chrome getUserPermissions(), and enforcing those permissions on proxied Insights/Vulnerability API calls with corresponding tests and controller handling. Sequence diagram for chrome.auth.getUserPermissions flow in ScalprumsequenceDiagram
actor User
participant Browser as Browser
participant FC as ForemanRhCloudController
participant FHP as ForemanRhCloudPermissionsHelper
participant VIEW as _permissions_data.html.erb
participant PI as PermissionsInit.js
participant GF as window.__foreman.permissions
participant SC as ScalprumContext.auth
participant FE as Insights frontend app
User->>Browser: Navigate to Insights page
Browser->>FC: HTTP GET /foreman_rh_cloud
FC->>FHP: insights_user_permissions()
FHP-->>FC: Array of {permission, resourceDefinitions}
FC->>VIEW: Render partial with data-permissions attribute
VIEW-->>Browser: HTML with div#foreman-rh-cloud-permissions
Browser->>PI: Execute on page load
PI->>PI: Read data-permissions from permissions div
PI->>GF: Parse JSON and set permissions array
User->>FE: Interact with Vulnerability/Advisor UI
FE->>SC: chrome.auth.getUserPermissions('vulnerability')
SC->>GF: getUserPermissions('vulnerability')
GF-->>SC: Filtered permissions starting with vulnerability:
SC-->>FE: Promise.resolve(filtered permissions)
FE->>FE: Enable/disable UI actions based on permissions
Sequence diagram for permission-enforced forwarding of Insights/Vulnerability API callssequenceDiagram
actor User
participant FE as Insights frontend app
participant Browser as Browser
participant UIC as UiRequestsController
participant IAF as InsightsApiForwarder
participant USER as User
participant VA as Vulnerability/Advisor/Inventory APIs
User->>FE: Trigger action (e.g. change CVE status)
FE->>Browser: XHR to Foreman proxy endpoint
Browser->>UIC: HTTP PATCH /insights_cloud/ui_request (path + method)
UIC->>IAF: forward_request(original_request, path, controller_name, user, organization, location)
IAF->>IAF: required_permission_for(path, http_method)
alt Permission required
IAF->>USER: user_has_permission?(user, permission)
alt User has permission
IAF->>VA: Forward request with tags
VA-->>IAF: API response
IAF-->>UIC: Proxied response
UIC-->>Browser: HTTP 200/2xx with data
Browser-->>FE: Resolve XHR
else User lacks permission
IAF->>UIC: Raise Foreman::Exception (permission denied)
UIC-->>Browser: HTTP 403 { message, error }
Browser-->>FE: Reject XHR (forbidden)
end
else No permission required
IAF->>VA: Forward request with tags
VA-->>IAF: API response
IAF-->>UIC: Proxied response
UIC-->>Browser: HTTP 2xx
end
Class diagram for Foreman RH Cloud RBAC mapping and enforcementclassDiagram
class ForemanRhCloudController {
+index()
+recommendations()
}
class ForemanRhCloudPermissionsHelper {
<<module>>
+PERMISSION_MAPPING
+insights_user_permissions() Array
+permissions_to_insights_format(permission Symbol) Array
}
class InsightsApiForwarder {
+SCOPED_REQUESTS
+forward_request(original_request, path, controller_name, user, organization, location)
+required_permission_for(path String, http_method String) Symbol
+user_has_permission?(user User, permission Symbol) Boolean
+http_user_agent(original_request)
+logger()
}
class UiRequestsController {
+forward_request()
}
class User {
+login
+can?(permission Symbol) Boolean
}
ForemanRhCloudController ..|> ApplicationController
ForemanRhCloudController ..> ForemanRhCloudPermissionsHelper : includes
ForemanRhCloudController ..> UiRequestsController : routes to
ForemanRhCloudPermissionsHelper ..> User : uses User.current
UiRequestsController ..> InsightsApiForwarder : calls forward_request
InsightsApiForwarder ..> User : calls user_has_permission?
InsightsApiForwarder ..> SCOPED_REQUESTS : uses for required_permission_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
InsightsApiForwarder#forward_request, the permission-denied log message unconditionally callsuser.login, which will raise ifuseris nil; consider guarding this (e.g.,user&.login || 'anonymous') or requiring a non-nil user to avoid unexpected errors. - The permission mapping between endpoints and Foreman permissions in
SCOPED_REQUESTSand the mapping from Foreman to Insights permissions inForemanRhCloudPermissionsHelper::PERMISSION_MAPPINGare tightly coupled but maintained separately; consider centralizing or reusing these definitions to reduce the risk of the mappings drifting out of sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `InsightsApiForwarder#forward_request`, the permission-denied log message unconditionally calls `user.login`, which will raise if `user` is nil; consider guarding this (e.g., `user&.login || 'anonymous'`) or requiring a non-nil user to avoid unexpected errors.
- The permission mapping between endpoints and Foreman permissions in `SCOPED_REQUESTS` and the mapping from Foreman to Insights permissions in `ForemanRhCloudPermissionsHelper::PERMISSION_MAPPING` are tightly coupled but maintained separately; consider centralizing or reusing these definitions to reduce the risk of the mappings drifting out of sync.
## Individual Comments
### Comment 1
<location> `app/services/foreman_rh_cloud/insights_api_forwarder.rb:120-123` </location>
<code_context>
def forward_request(original_request, path, controller_name, user, organization, location)
+ # Check permissions before forwarding
+ permission = required_permission_for(path, original_request.request_method)
+ if permission && !user_has_permission?(user, permission)
+ logger.warn("User #{user.login} lacks permission #{permission} for #{original_request.request_method} #{path}")
+ raise ::Foreman::Exception.new(N_("You do not have permission to perform this action"))
+ end
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Use a more specific or HTTP-aware exception type for authorization failures.
Raising a generic `Foreman::Exception` here means only `InsightsCloud::UiRequestsController` currently maps this to a 403; any other caller that doesn’t rescue it will return a 500 instead. Either use a more specific/HTTP-aware exception (e.g., one that implies 403) or handle the mapping to HTTP status at this layer so all callers get consistent authorization behavior.
</issue_to_address>
### Comment 2
<location> `app/services/foreman_rh_cloud/insights_api_forwarder.rb:232-239` </location>
<code_context>
+ # @param path [String] The request path
+ # @param http_method [String] The HTTP method (GET, POST, etc.)
+ # @return [Symbol, nil] The required permission symbol or nil if no permission required
+ def required_permission_for(path, http_method)
+ request_pattern = SCOPED_REQUESTS.find { |pattern| pattern[:test].match?(path) }
+ return nil unless request_pattern
+
+ permissions = request_pattern[:permissions]
+ return nil unless permissions
+
+ permissions[http_method]
+ end
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Relying on first regex match in SCOPED_REQUESTS makes permission resolution order-dependent.
Because `required_permission_for` returns the first `SCOPED_REQUESTS` entry whose `:test` matches the path, overlapping patterns (e.g., `api/insights/v1/ack` vs `api/insights/v1/.*`) make the permission depend on array order. A later insertion of a broader pattern above a specific one could silently change which permission is enforced. Consider either prioritizing the most specific matcher (e.g., by length or constraints) or restructuring patterns so they don’t overlap and authorization can’t be altered by reordering.
Suggested implementation:
```ruby
# Returns the required permission for the given path and HTTP method
# Resolves overlapping patterns by choosing the most specific matcher,
# defined as the one with the longest regex source that matches the path.
# This avoids permission changes caused by reordering SCOPED_REQUESTS.
# @param path [String] The request path
# @param http_method [String] The HTTP method (GET, POST, etc.)
# @return [Symbol, nil] The required permission symbol or nil if no permission required
```
```ruby
# Returns the required permission for the given path and HTTP method
# @param path [String] The request path
# @param http_method [String] The HTTP method (GET, POST, etc.)
# @return [Symbol, nil] The required permission symbol or nil if no permission required
def required_permission_for(path, http_method)
# Collect all matching patterns
matching_patterns = SCOPED_REQUESTS.select { |pattern| pattern[:test].match?(path) }
return nil if matching_patterns.empty?
# Choose the most specific pattern: longest regex source wins.
# This makes overlapping patterns deterministic and independent of array order.
request_pattern = matching_patterns.max_by { |pattern| pattern[:test].source.length }
permissions = request_pattern[:permissions]
return nil unless permissions
permissions[http_method]
end
```
</issue_to_address>
### Comment 3
<location> `webpack/common/ScalprumModule/ScalprumContext.js:58-78` </location>
<code_context>
+ * @param {boolean} _bypassCache - Optional flag to bypass cache (not used in Foreman)
+ * @returns {Promise<Array<{permission: string, resourceDefinitions: Array}>>} Array of permission objects
+ */
+const getUserPermissions = async (app, _bypassCache) => {
+ // Get all permissions from backend (already in Insights format)
+ const allPermissions = window.__foreman?.permissions || [];
+
+ // If app is specified, filter by app prefix
+ if (app) {
+ const filtered = allPermissions.filter(
+ p => p.permission && p.permission.startsWith(`${app}:`)
+ );
+ return filtered;
+ }
+
+ // Return all permissions
+ return allPermissions;
+};
+
</code_context>
<issue_to_address>
**suggestion:** Be mindful of initialization order between getUserPermissions and PermissionsInit.js.
This relies on `window.__foreman.permissions` being populated by `PermissionsInit.js`. If `chrome.auth.getUserPermissions()` is called before that script runs, `allPermissions` will be `[]` and permissions will appear missing for the session unless callers requery. Please ensure `PermissionsInit.js` is loaded before this context, or clearly document that `getUserPermissions` assumes `window.__foreman.permissions` has already been initialized.
```suggestion
* @see https://github.com/RedHatInsights/frontend-components/blob/master/docs/chrome/chrome-api.md#getuserpermissions
* @param {string} app - Optional app name to filter permissions (e.g., 'vulnerability', 'inventory')
* @param {boolean} _bypassCache - Optional flag to bypass cache (not used in Foreman)
* @returns {Promise<Array<{permission: string, resourceDefinitions: Array}>>} Array of permission objects
* @throws {Error} If called before `window.__foreman.permissions` is initialized and strict initialization is enforced.
*
* Note:
* This function assumes that `PermissionsInit.js` has already run and initialized
* `window.__foreman.permissions`. If `chrome.auth.getUserPermissions()` is invoked
* before that initialization, it will return an empty array and log a warning.
* Callers should ensure `PermissionsInit.js` is loaded first, or be prepared to
* requery after initialization.
*/
+let hasWarnedAboutMissingForemanPermissions = false;
+
+const getUserPermissions = async (app, _bypassCache) => {
+ const permissionsInitialized =
+ typeof window !== 'undefined' &&
+ window.__foreman &&
+ Array.isArray(window.__foreman.permissions);
+
+ // Warn (once) if permissions have not yet been initialized by PermissionsInit.js
+ if (!permissionsInitialized && !hasWarnedAboutMissingForemanPermissions) {
+ hasWarnedAboutMissingForemanPermissions = true;
+ // eslint-disable-next-line no-console
+ console.warn(
+ '[ScalprumContext] getUserPermissions called before PermissionsInit.js initialized window.__foreman.permissions. Returning empty permissions array.'
+ );
+ }
+
+ // Get all permissions from backend (already in Insights format) or fallback to empty array
+ const allPermissions = (permissionsInitialized && window.__foreman.permissions) || [];
+
+ // If app is specified, filter by app prefix
+ if (app) {
+ return allPermissions.filter(
+ (p) => p.permission && p.permission.startsWith(`${app}:`)
+ );
+ }
+
+ // Return all permissions
+ return allPermissions;
+};
+
```
</issue_to_address>
### Comment 4
<location> `test/unit/services/foreman_rh_cloud/insights_api_forwarder_test.rb:240-216` </location>
<code_context>
+ @forwarder.forward_request(req, 'api/inventory/v1/hosts', 'test_controller', @user, @organization, @location)
+ end
+
+ test 'should deny GET request to inventory hosts when user lacks view_vulnerability permission' do
+ user_agent = { :foo => :bar }
+ params = {}
+
+ req = ActionDispatch::Request.new(
+ 'REQUEST_URI' => '/api/inventory/v1/hosts',
+ 'REQUEST_METHOD' => 'GET',
+ 'HTTP_USER_AGENT' => user_agent,
+ 'rack.input' => ::Puma::NullIO.new,
+ 'action_dispatch.request.query_parameters' => params
+ )
+
+ @user.stubs(:can?).with(:view_vulnerability).returns(false)
+
+ assert_raises(::Foreman::Exception) do
+ @forwarder.forward_request(req, 'api/inventory/v1/hosts', 'test_controller', @user, @organization, @location)
+ end
+ end
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add expectations that `execute_cloud_request` is not called when permission checks fail
To better validate the permission enforcement, also assert that `execute_cloud_request` is never called when permissions are missing, e.g.:
```ruby
@forwarder.expects(:execute_cloud_request).never
assert_raises(::Foreman::Exception) do
@forwarder.forward_request(...)
end
```
Applying this to the other deny-tests (vulnerability and advisor endpoints) will ensure we don’t accidentally forward a request after a failed permission check.
Suggested implementation:
```ruby
test 'should deny GET request to inventory hosts when user lacks view_vulnerability permission' do
user_agent = { :foo => :bar }
params = {}
req = ActionDispatch::Request.new(
'REQUEST_URI' => '/api/inventory/v1/hosts',
'REQUEST_METHOD' => 'GET',
'HTTP_USER_AGENT' => user_agent,
'rack.input' => ::Puma::NullIO.new,
'action_dispatch.request.query_parameters' => params
)
@user.stubs(:can?).with(:view_vulnerability).returns(false)
@forwarder.expects(:execute_cloud_request).never
assert_raises(::Foreman::Exception) do
@forwarder.forward_request(req, 'api/inventory/v1/hosts', 'test_controller', @user, @organization, @location)
end
end
```
To fully implement your suggestion for all deny-tests:
1. Locate the other tests that assert denial when permissions are missing for the vulnerability and advisor endpoints (e.g., "should deny ... when user lacks view_vulnerability" / "view_advisor" or similar).
2. In each of those tests, right after the `@user.stubs(:can?).with(...).returns(false)` line, add:
`@forwarder.expects(:execute_cloud_request).never`
3. Ensure this expectation appears **before** the `assert_raises` block so that Mocha verifies `execute_cloud_request` is not called when permission checks fail.
</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
+120
to
+123
| permission = required_permission_for(path, original_request.request_method) | ||
| if permission && !user_has_permission?(user, permission) | ||
| logger.warn("User #{user.login} lacks permission #{permission} for #{original_request.request_method} #{path}") | ||
| raise ::Foreman::Exception.new(N_("You do not have permission to perform this action")) |
There was a problem hiding this comment.
issue (bug_risk): Use a more specific or HTTP-aware exception type for authorization failures.
Raising a generic Foreman::Exception here means only InsightsCloud::UiRequestsController currently maps this to a 403; any other caller that doesn’t rescue it will return a 500 instead. Either use a more specific/HTTP-aware exception (e.g., one that implies 403) or handle the mapping to HTTP status at this layer so all callers get consistent authorization behavior.
Comment on lines
+232
to
+239
| def required_permission_for(path, http_method) | ||
| request_pattern = SCOPED_REQUESTS.find { |pattern| pattern[:test].match?(path) } | ||
| return nil unless request_pattern | ||
|
|
||
| permissions = request_pattern[:permissions] | ||
| return nil unless permissions | ||
|
|
||
| permissions[http_method] |
There was a problem hiding this comment.
🚨 suggestion (security): Relying on first regex match in SCOPED_REQUESTS makes permission resolution order-dependent.
Because required_permission_for returns the first SCOPED_REQUESTS entry whose :test matches the path, overlapping patterns (e.g., api/insights/v1/ack vs api/insights/v1/.*) make the permission depend on array order. A later insertion of a broader pattern above a specific one could silently change which permission is enforced. Consider either prioritizing the most specific matcher (e.g., by length or constraints) or restructuring patterns so they don’t overlap and authorization can’t be altered by reordering.
Suggested implementation:
# Returns the required permission for the given path and HTTP method
# Resolves overlapping patterns by choosing the most specific matcher,
# defined as the one with the longest regex source that matches the path.
# This avoids permission changes caused by reordering SCOPED_REQUESTS.
# @param path [String] The request path
# @param http_method [String] The HTTP method (GET, POST, etc.)
# @return [Symbol, nil] The required permission symbol or nil if no permission required # Returns the required permission for the given path and HTTP method
# @param path [String] The request path
# @param http_method [String] The HTTP method (GET, POST, etc.)
# @return [Symbol, nil] The required permission symbol or nil if no permission required
def required_permission_for(path, http_method)
# Collect all matching patterns
matching_patterns = SCOPED_REQUESTS.select { |pattern| pattern[:test].match?(path) }
return nil if matching_patterns.empty?
# Choose the most specific pattern: longest regex source wins.
# This makes overlapping patterns deterministic and independent of array order.
request_pattern = matching_patterns.max_by { |pattern| pattern[:test].source.length }
permissions = request_pattern[:permissions]
return nil unless permissions
permissions[http_method]
end
jeremylenz
reviewed
Jan 8, 2026
| @@ -0,0 +1,4 @@ | |||
| <%# Injects user permissions for Scalprum/Chrome API integration %> | |||
| <%# Consumed by PermissionsInit.js to populate window.__foreman.permissions %> | |||
| <div id="foreman-rh-cloud-permissions" data-permissions="<%= insights_user_permissions.to_json %>" style="display:none;"></div> | |||
Collaborator
There was a problem hiding this comment.
Putting data in the DOM seems hacky and IMO should be a last resort. Have you considered registering the permissions in ForemanContext? https://github.com/theforeman/foreman/blob/4181accb7f4efd93a4aebbd84de57f4120decc9b/developer_docs/foreman-context.asciidoc#adding-a-value-to-the-context-from-a-plugin
Or another pattern we use widely is to have an include_permissions param on some API endpoint, but not sure if that's relevant here.
Collaborator
There was a problem hiding this comment.
Also, seems like this open Foreman PR is super-relevant! theforeman/foreman#10338
vkrizan
reviewed
Jan 12, 2026
| @@ -0,0 +1,4 @@ | |||
| <%# Injects user permissions for Scalprum/Chrome API integration %> | |||
| <%# Consumed by PermissionsInit.js to populate window.__foreman.permissions %> | |||
| <div id="foreman-rh-cloud-permissions" data-permissions="<%= insights_user_permissions.to_json %>" style="display:none;"></div> | |||
|
|
||
| // Return all permissions | ||
| return allPermissions; | ||
| }; |
Contributor
There was a problem hiding this comment.
Nitpick: can be written functionally with ternary (instead of procedurally)
nofaralfasi
force-pushed
the
SAT-40629
branch
2 times, most recently
from
999aaaa to
90fdf56
Compare
nofaralfasi
force-pushed
the
SAT-40629
branch
2 times, most recently
from
9167d2a to
f2c8b54
Compare
nofaralfasi
changed the title
nofaralfasi
force-pushed
the
SAT-40629
branch
from
f2c8b54 to
03d327e
Compare
nofaralfasi
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
ToDo: Improve the displayed error message on the Insights page if the user does not have the necessary permissions.
Requires #1136.
What are the changes introduced in this pull request?
Add Chrome API getUserPermissions() implementation for vulnerability-ui
and advisor apps to query user permissions and control UI accordingly.
Implements frontend RBAC for Scalprum-loaded Insights applications
(vulnerability-ui, advisor) by exposing user permissions through
the Chrome API getUserPermissions() interface.
Changes:
from ForemanContext (PR #10338) and converts them to Chrome API format
Permission mapping (Foreman → Chrome API):
view_vulnerability → inventory:hosts:read, vulnerability::read
edit_vulnerability → vulnerability::write
view_advisor → advisor::read
edit_advisor → advisor::write
This enables Insights frontend apps to check user permissions and
show/hide UI elements based on RBAC, complementing the existing
backend permission enforcement in the API forwarder.
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Prerequisites
Newer Advisor frontend that respects RBAC - The advisor-ui must be a version that queries chrome.auth.getUserPermissions() and respects the returned permissions for UI access control
Test user with view-only permissions:
Test user with edit permissions:
Summary by Sourcery
Expose Insights RBAC permissions from Foreman to Scalprum-based frontends and enforce them on forwarded Insights and Vulnerability API requests.
New Features:
Enhancements:
Tests: