chore: rbac v1 cleanup#2385
Merged
Merged
Conversation
Reviewer's GuideRemoves legacy RBAC v1 support and centralizes all RBAC concepts in the v2 (Kessel) manager, while ensuring responses consistently include empty permission metadata and updating handlers, configuration, mocks, and tests accordingly. 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 manager/rbac_exceptions.RbacException,
super().__init__(self)should probably besuper().__init__(message)so that the base Exception stores the human-readable message rather than the instance object. - In manager/rbac_v2_manager.RbacPermission.eq, consider adding an
isinstancecheck (or returning NotImplemented) before accessingother.app/resource/actionto avoid AttributeError when compared with non-RbacPermission objects.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In manager/rbac_exceptions.RbacException, `super().__init__(self)` should probably be `super().__init__(message)` so that the base Exception stores the human-readable message rather than the instance object.
- In manager/rbac_v2_manager.RbacPermission.__eq__, consider adding an `isinstance` check (or returning NotImplemented) before accessing `other.app/resource/action` to avoid AttributeError when compared with non-RbacPermission objects.
## Individual Comments
### Comment 1
<location path="manager/rbac_exceptions.py" line_range="4-9" />
<code_context>
+"""RBAC-related exceptions"""
+
+
+class RbacException(Exception):
+ """Represents RBAC-related exception"""
+
+ def __init__(self, message):
+ self.message = message
+ super().__init__(self)
</code_context>
<issue_to_address>
**issue (bug_risk):** RbacException should pass the message to Exception instead of `self`.
`super().__init__(self)` sets the base `Exception` argument to the instance instead of the message, which changes `str(e)`, logging output, and `e.args`. Please pass `message` to the base class (`super().__init__(message)`) and consider removing `self.message` if it's unnecessary.
</issue_to_address>
### Comment 2
<location path="manager/rbac_v2_manager.py" line_range="78-82" />
<code_context>
- def __repr__(self):
- return self.__str__()
-
- def __eq__(self, other):
- app = self.app == other.app
- resource = self.resource == other.resource or (self.resource == RbacResource.ANY) or (other.resource == RbacResource.ANY)
- action = self.action == other.action or (self.action == RbacAction.ANY) or (other.action == RbacAction.ANY)
- return app and resource and action
-
- def to_kessel_permission(self):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** RbacPermission.__eq__ should guard against comparisons with non-RbacPermission objects.
The implementation assumes `other` has `.app`, `.resource`, and `.action` attributes, so comparing to a different type (including `None`) will raise `AttributeError` instead of returning `False`. Adding an `isinstance(other, RbacPermission)` guard or returning `NotImplemented` for non-matching types would avoid this and make equality behave predictably.
```suggestion
def __eq__(self, other):
if not isinstance(other, RbacPermission):
return NotImplemented
app = self.app == other.app
resource = (
self.resource == other.resource
or (self.resource == RbacResource.ANY)
or (other.resource == RbacResource.ANY)
)
action = (
self.action == other.action
or (self.action == RbacAction.ANY)
or (other.action == RbacAction.ANY)
)
return app and resource and action
```
</issue_to_address>
### Comment 3
<location path="tests/manager_tests/test_report_handler.py" line_range="27" />
<code_context>
},
"top_cves": [],
"top_rules": [],
- "meta": {"cache_used": False, "permissions": ["vulnerability:*:*", "inventory:hosts:read"]},
+ "meta": {"cache_used": False, "permissions": []},
}
</code_context>
<issue_to_address>
**suggestion (testing):** Extend the report handler tests to assert the presence and type of the `permissions` field rather than only its exact value.
The expected response now uses `"permissions": []`, which matches the new behavior. To make the tests more resilient, consider asserting that `permissions` exists and is a list (e.g. `assert isinstance(result["meta"]["permissions"], list)`), and optionally keep a separate assertion for its contents (currently an empty list).
Suggested implementation:
```python
},
"top_cves": [],
"top_rules": [],
"meta": {"cache_used": False, "permissions": []},
}
```
To fully implement the suggestion, update the test that checks this response structure (where this expected dict is used) as follows:
1. After obtaining the actual result (e.g. `result = ...` or `response.json()`), add an assertion that verifies the existence and type of the `permissions` field:
```python
assert "meta" in result
assert "permissions" in result["meta"]
assert isinstance(result["meta"]["permissions"], list)
```
2. Optionally keep or add a separate assertion for the specific contents, which currently should be an empty list:
```python
assert result["meta"]["permissions"] == []
```
3. If the test currently performs a full deep equality comparison between the result and the expected dict shown in the snippet (e.g. `assert result == EXPECTED_RESPONSE`), you can keep that assertion, since `"permissions": []` still matches the new behavior; the new type-focused assertions will make the test resilient if the precise contents of `permissions` change in the future while remaining a list.
Place these new assertions in the relevant test(s) immediately after you obtain `result`, before or alongside any existing equality checks.
</issue_to_address>
### Comment 4
<location path="manager/rbac_v2_manager.py" line_range="64" />
<code_context>
- ANY = "*"
-
-
-class RbacPermission:
- """Class represents single permission"""
-
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new RBAC permission enums, mappings, and route/filter permission structures to make wildcard behavior, permission translation, and configuration data more explicit and readable while preserving current behavior.
You can keep the new behavior but reduce complexity in a few focused spots.
### 1. `RbacPermission.to_kessel_permission` string mangling
Right now `StrEnum` values are treated as strings and transformed via `replace`, which couples behavior to the literal string and makes the enum abstraction thin.
You can make this more explicit and safer by:
* Using `.value` instead of bare enum instances
* Replacing ad‑hoc `.replace` calls with small mappings
```python
class RbacApp(StrEnum):
VULNERABILITY = "vulnerability"
INVENTORY = "inventory"
ANY = "all" # optional, if this matches Kessel semantics
```
```python
class RbacPermission:
# ...
def to_kessel_permission(self) -> str:
"""Convert RBACv1 permission to Kessel v2 permission string format."""
action_mapping = {
RbacAction.READ: "view",
RbacAction.WRITE: "edit",
RbacAction.ANY: "all",
}
app_mapping = {
RbacApp.VULNERABILITY: "vulnerability",
RbacApp.INVENTORY: "inventory",
RbacApp.ANY: "all",
}
resource_mapping = {
RbacResource.VULNERABILITY_RESULTS: "vulnerability_results",
RbacResource.CVE_BUSINESS_RISK_AND_STATUS: "cve_business_risk_and_status",
RbacResource.SYSTEM_CVE_STATUS: "system_cve_status",
RbacResource.ADVANCED_REPORT: "advanced_report",
RbacResource.SYSTEM_OPT_OUT: "system_opt_out",
RbacResource.EXPORT_AND_REPORT: "report_and_export",
RbacResource.HOSTS: "hosts",
RbacResource.GROUPS: "groups",
RbacResource.TOGGLE_CVES_WITHOUT_ERRATA: "toggle_cves_without_errata",
RbacResource.ANY: "all",
}
app = app_mapping[self.app]
resource = resource_mapping[self.resource]
action = action_mapping[self.action]
return f"{app}_{resource}_{action}"
```
This keeps functionality but makes the mapping explicit and decoupled from the literal enum string format.
### 2. Wildcard matching vs equality
Overloading `__eq__` to do wildcard matching is surprising. You can keep the behavior but introduce a dedicated method and start moving callers over when convenient, without breaking anything now:
```python
class RbacPermission:
# ...
def matches(self, other: "RbacPermission") -> bool:
app = self.app == other.app
resource = (
self.resource == other.resource
or self.resource == RbacResource.ANY
or other.resource == RbacResource.ANY
)
action = (
self.action == other.action
or self.action == RbacAction.ANY
or other.action == RbacAction.ANY
)
return app and resource and action
def __eq__(self, other: object) -> bool:
if not isinstance(other, RbacPermission):
return NotImplemented
# Keep current behavior for now
return self.matches(other)
```
Then in any new code (or when you touch existing call sites), you can use `perm.matches(other_perm)` instead of `==`, making intent clearer.
### 3. Route permission nesting readability
The `[[perm, INVENTORY_READ]]` pattern is not self‑describing. A tiny helper clarifies semantics without changing behavior:
```python
def _perm_with_inventory(*perms: RbacPermission) -> list[list[RbacPermission]]:
# OR-of-AND: single group requiring given perms and inventory read
return [[*perms, RbacRoutePermissions.INVENTORY_READ]]
```
```python
class RbacRoutePermissions:
VULNERABILITY_ANY = RbacPermission(RbacApp.VULNERABILITY, RbacResource.ANY, RbacAction.ANY)
INVENTORY_READ = RbacPermission(RbacApp.INVENTORY, RbacResource.HOSTS, RbacAction.READ)
BASE_OPT_OUT_READ = RbacPermission(RbacApp.VULNERABILITY, RbacResource.SYSTEM_OPT_OUT, RbacAction.READ)
VULNERABILITY_RESULTS = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.VULNERABILITY_RESULTS, RbacAction.READ)
)
CVE_BR_AND_STATUS_EDIT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.CVE_BUSINESS_RISK_AND_STATUS, RbacAction.WRITE)
)
SYSTEM_CVE_STATUS_EDIT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.SYSTEM_CVE_STATUS, RbacAction.WRITE)
)
SYSTEM_OPT_OUT_READ = _perm_with_inventory(BASE_OPT_OUT_READ)
SYSTEM_OPT_OUT_EDIT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.SYSTEM_OPT_OUT, RbacAction.WRITE)
)
ADVANCED_REPORTING = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.ADVANCED_REPORT, RbacAction.READ)
)
REPORT_AND_EXPORT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.EXPORT_AND_REPORT, RbacAction.READ)
)
TOGGLE_CVES_WITHOUT_ERRATA = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.TOGGLE_CVES_WITHOUT_ERRATA, RbacAction.WRITE)
)
```
This preserves the nested list shape but makes the “AND with inventory read” intent obvious.
### 4. `RbacFilterRoutePermissions` dict construction
The `copy()` + `update()` cascade makes it harder to see what each endpoint actually allows. You can keep the shared pieces but use literal unpacking for clarity:
```python
class RbacFilterRoutePermissions:
OPT_OUT_VALUE = {"excluded": (RbacRoutePermissions.SYSTEM_OPT_OUT_READ, True)}
CSV_FORMAT_VALUE = {"data_format": (RbacRoutePermissions.REPORT_AND_EXPORT, "csv")}
REPORT_VALUE = {"report": (RbacRoutePermissions.REPORT_AND_EXPORT, True)}
ADVANCED_REPORT_VALUE = {"advanced_report": (RbacRoutePermissions.ADVANCED_REPORTING, True)}
CSV_FORMAT_ENDPOINTS = {
**CSV_FORMAT_VALUE,
}
REPORTABLE_ENDPOINTS = {
**CSV_FORMAT_VALUE,
**REPORT_VALUE,
}
VULNERABILITIES_CVES_ENDPOINT = {
**CSV_FORMAT_VALUE,
**REPORT_VALUE,
**ADVANCED_REPORT_VALUE,
}
SYSTEMS_ENDPOINT_VALUES = {
**CSV_FORMAT_VALUE,
**REPORT_VALUE,
**OPT_OUT_VALUE,
}
```
Same resulting dicts, but each one’s content is visible at the point of definition.
### 5. Make “constants” clearly immutable
If these route/filter permission collections are intended as static configuration, consider using tuples for nested permission lists to avoid accidental mutation:
```python
from typing import Tuple
Permission = RbacPermission
PermissionGroup = Tuple[Permission, ...]
PermissionAlternatives = Tuple[PermissionGroup, ...]
class RbacRoutePermissions:
# ...
VULNERABILITY_RESULTS: PermissionAlternatives = (
(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.VULNERABILITY_RESULTS, RbacAction.READ),
INVENTORY_READ,
),
)
```
You don’t have to change all call sites immediately, but even just changing the inner lists to tuples removes one class of accidental mutations while keeping behavior the same.
</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
+78
to
+82
| def __eq__(self, other): | ||
| app = self.app == other.app | ||
| resource = self.resource == other.resource or (self.resource == RbacResource.ANY) or (other.resource == RbacResource.ANY) | ||
| action = self.action == other.action or (self.action == RbacAction.ANY) or (other.action == RbacAction.ANY) | ||
| return app and resource and action |
There was a problem hiding this comment.
suggestion (bug_risk): RbacPermission.eq should guard against comparisons with non-RbacPermission objects.
The implementation assumes other has .app, .resource, and .action attributes, so comparing to a different type (including None) will raise AttributeError instead of returning False. Adding an isinstance(other, RbacPermission) guard or returning NotImplemented for non-matching types would avoid this and make equality behave predictably.
Suggested change
| def __eq__(self, other): | |
| app = self.app == other.app | |
| resource = self.resource == other.resource or (self.resource == RbacResource.ANY) or (other.resource == RbacResource.ANY) | |
| action = self.action == other.action or (self.action == RbacAction.ANY) or (other.action == RbacAction.ANY) | |
| return app and resource and action | |
| def __eq__(self, other): | |
| if not isinstance(other, RbacPermission): | |
| return NotImplemented | |
| app = self.app == other.app | |
| resource = ( | |
| self.resource == other.resource | |
| or (self.resource == RbacResource.ANY) | |
| or (other.resource == RbacResource.ANY) | |
| ) | |
| action = ( | |
| self.action == other.action | |
| or (self.action == RbacAction.ANY) | |
| or (other.action == RbacAction.ANY) | |
| ) | |
| return app and resource and action |
| ANY = "*" | ||
|
|
||
|
|
||
| class RbacPermission: |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the new RBAC permission enums, mappings, and route/filter permission structures to make wildcard behavior, permission translation, and configuration data more explicit and readable while preserving current behavior.
You can keep the new behavior but reduce complexity in a few focused spots.
1. RbacPermission.to_kessel_permission string mangling
Right now StrEnum values are treated as strings and transformed via replace, which couples behavior to the literal string and makes the enum abstraction thin.
You can make this more explicit and safer by:
- Using
.valueinstead of bare enum instances - Replacing ad‑hoc
.replacecalls with small mappings
class RbacApp(StrEnum):
VULNERABILITY = "vulnerability"
INVENTORY = "inventory"
ANY = "all" # optional, if this matches Kessel semanticsclass RbacPermission:
# ...
def to_kessel_permission(self) -> str:
"""Convert RBACv1 permission to Kessel v2 permission string format."""
action_mapping = {
RbacAction.READ: "view",
RbacAction.WRITE: "edit",
RbacAction.ANY: "all",
}
app_mapping = {
RbacApp.VULNERABILITY: "vulnerability",
RbacApp.INVENTORY: "inventory",
RbacApp.ANY: "all",
}
resource_mapping = {
RbacResource.VULNERABILITY_RESULTS: "vulnerability_results",
RbacResource.CVE_BUSINESS_RISK_AND_STATUS: "cve_business_risk_and_status",
RbacResource.SYSTEM_CVE_STATUS: "system_cve_status",
RbacResource.ADVANCED_REPORT: "advanced_report",
RbacResource.SYSTEM_OPT_OUT: "system_opt_out",
RbacResource.EXPORT_AND_REPORT: "report_and_export",
RbacResource.HOSTS: "hosts",
RbacResource.GROUPS: "groups",
RbacResource.TOGGLE_CVES_WITHOUT_ERRATA: "toggle_cves_without_errata",
RbacResource.ANY: "all",
}
app = app_mapping[self.app]
resource = resource_mapping[self.resource]
action = action_mapping[self.action]
return f"{app}_{resource}_{action}"This keeps functionality but makes the mapping explicit and decoupled from the literal enum string format.
2. Wildcard matching vs equality
Overloading __eq__ to do wildcard matching is surprising. You can keep the behavior but introduce a dedicated method and start moving callers over when convenient, without breaking anything now:
class RbacPermission:
# ...
def matches(self, other: "RbacPermission") -> bool:
app = self.app == other.app
resource = (
self.resource == other.resource
or self.resource == RbacResource.ANY
or other.resource == RbacResource.ANY
)
action = (
self.action == other.action
or self.action == RbacAction.ANY
or other.action == RbacAction.ANY
)
return app and resource and action
def __eq__(self, other: object) -> bool:
if not isinstance(other, RbacPermission):
return NotImplemented
# Keep current behavior for now
return self.matches(other)Then in any new code (or when you touch existing call sites), you can use perm.matches(other_perm) instead of ==, making intent clearer.
3. Route permission nesting readability
The [[perm, INVENTORY_READ]] pattern is not self‑describing. A tiny helper clarifies semantics without changing behavior:
def _perm_with_inventory(*perms: RbacPermission) -> list[list[RbacPermission]]:
# OR-of-AND: single group requiring given perms and inventory read
return [[*perms, RbacRoutePermissions.INVENTORY_READ]]class RbacRoutePermissions:
VULNERABILITY_ANY = RbacPermission(RbacApp.VULNERABILITY, RbacResource.ANY, RbacAction.ANY)
INVENTORY_READ = RbacPermission(RbacApp.INVENTORY, RbacResource.HOSTS, RbacAction.READ)
BASE_OPT_OUT_READ = RbacPermission(RbacApp.VULNERABILITY, RbacResource.SYSTEM_OPT_OUT, RbacAction.READ)
VULNERABILITY_RESULTS = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.VULNERABILITY_RESULTS, RbacAction.READ)
)
CVE_BR_AND_STATUS_EDIT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.CVE_BUSINESS_RISK_AND_STATUS, RbacAction.WRITE)
)
SYSTEM_CVE_STATUS_EDIT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.SYSTEM_CVE_STATUS, RbacAction.WRITE)
)
SYSTEM_OPT_OUT_READ = _perm_with_inventory(BASE_OPT_OUT_READ)
SYSTEM_OPT_OUT_EDIT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.SYSTEM_OPT_OUT, RbacAction.WRITE)
)
ADVANCED_REPORTING = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.ADVANCED_REPORT, RbacAction.READ)
)
REPORT_AND_EXPORT = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.EXPORT_AND_REPORT, RbacAction.READ)
)
TOGGLE_CVES_WITHOUT_ERRATA = _perm_with_inventory(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.TOGGLE_CVES_WITHOUT_ERRATA, RbacAction.WRITE)
)This preserves the nested list shape but makes the “AND with inventory read” intent obvious.
4. RbacFilterRoutePermissions dict construction
The copy() + update() cascade makes it harder to see what each endpoint actually allows. You can keep the shared pieces but use literal unpacking for clarity:
class RbacFilterRoutePermissions:
OPT_OUT_VALUE = {"excluded": (RbacRoutePermissions.SYSTEM_OPT_OUT_READ, True)}
CSV_FORMAT_VALUE = {"data_format": (RbacRoutePermissions.REPORT_AND_EXPORT, "csv")}
REPORT_VALUE = {"report": (RbacRoutePermissions.REPORT_AND_EXPORT, True)}
ADVANCED_REPORT_VALUE = {"advanced_report": (RbacRoutePermissions.ADVANCED_REPORTING, True)}
CSV_FORMAT_ENDPOINTS = {
**CSV_FORMAT_VALUE,
}
REPORTABLE_ENDPOINTS = {
**CSV_FORMAT_VALUE,
**REPORT_VALUE,
}
VULNERABILITIES_CVES_ENDPOINT = {
**CSV_FORMAT_VALUE,
**REPORT_VALUE,
**ADVANCED_REPORT_VALUE,
}
SYSTEMS_ENDPOINT_VALUES = {
**CSV_FORMAT_VALUE,
**REPORT_VALUE,
**OPT_OUT_VALUE,
}Same resulting dicts, but each one’s content is visible at the point of definition.
5. Make “constants” clearly immutable
If these route/filter permission collections are intended as static configuration, consider using tuples for nested permission lists to avoid accidental mutation:
from typing import Tuple
Permission = RbacPermission
PermissionGroup = Tuple[Permission, ...]
PermissionAlternatives = Tuple[PermissionGroup, ...]
class RbacRoutePermissions:
# ...
VULNERABILITY_RESULTS: PermissionAlternatives = (
(
RbacPermission(RbacApp.VULNERABILITY, RbacResource.VULNERABILITY_RESULTS, RbacAction.READ),
INVENTORY_READ,
),
)You don’t have to change all call sites immediately, but even just changing the inner lists to tuples removes one class of accidental mutations while keeping behavior the same.
Removed rbac v1 and its leftovers, kept shared parts RHINENG-26714
pindo696
force-pushed
the
cleanup-rbac-v1
branch
from
June 17, 2026 09:25
9c4ec8b to
f9ae579
Compare
jdobes
approved these changes
Jun 17, 2026
14 tasks
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
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.
Good luck with PR
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Remove legacy RBAC v1 usage in favor of Kessel-based RBAC v2 and centralize RBAC exceptions.
Bug Fixes:
Enhancements:
Build:
Tests: