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>

Please address the comments from this code review:

## Overall Comments
- The frontend and backend permission mappings are currently duplicated (PERMISSION_MAPPING in PermissionsHooks.js vs SCOPED_REQUESTS in InsightsApiForwarder); consider centralizing these mappings or generating one from the other to avoid drift when endpoints or permissions change.
- forward_request now raises a generic Foreman::Exception for RBAC failures which is caught and converted to 403 in the UI controller; it may be clearer and safer to introduce a dedicated permission error type so other call sites can distinguish RBAC failures from other exceptional conditions.
- required_permission_for relies on the ordering of SCOPED_REQUESTS (e.g., inventory hosts and specific insights endpoints before broader regexes); adding a brief comment near SCOPED_REQUESTS about this ordering requirement would help future maintainers avoid subtle bugs when extending the list.

## Individual Comments

### Comment 1
<location> `app/services/foreman_rh_cloud/insights_api_forwarder.rb:120-122` </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&.can?(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):** Guard against nil `user` when logging missing permission

The permission guard uses `user&.can?`, but the log line calls `user.login` unconditionally. If `user` is nil, this will raise `NoMethodError` instead of returning a 403. Use something like `user&.login || 'anonymous'` (or similar) in the log message.
</issue_to_address>

### Comment 2
<location> `app/controllers/insights_cloud/ui_requests_controller.rb:21-24` </location>
<code_context>
           @organization,
           @location
         )
+      rescue ::Foreman::Exception => e
+        logger.warn("Permission denied for forwarding request: #{e}")
+        message = e.message
+        return render json: { message: message, error: message }, status: :forbidden
       rescue RestClient::Exceptions::Timeout => e
         response_obj = e.response.presence || e.exception
</code_context>

<issue_to_address>
**issue (bug_risk):** Rescuing all `Foreman::Exception` as 403 may mask unrelated errors

`Foreman::Exception` is now used for permission issues, but other (current or future) call paths in `forward_request` may also raise it for non-RBAC reasons. Rescuing the base class and always returning 403 risks misclassifying real server errors as authorization failures. Consider rescuing a more specific subclass (e.g. a dedicated permission error) or checking the exception details to distinguish permission failures from other cases.
</issue_to_address>