[AAP-64918] Adds AuditableModel for Role*Assignment#956
[AAP-64918] Adds AuditableModel for Role*Assignment#956huffmanca wants to merge 2 commits intoansible:develfrom
Conversation
📝 WalkthroughWalkthroughIntroduce conditional auditing for RBAC assignment models by adding a module-level Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test_app/tests/rbac/models/test_assignment_audit_logging.py (1)
29-54: Attribute-only tests markeddjango_dband unnecessarily skipped when activitystream is absent.Two issues:
- Lines 29–54 read only class-level attributes (
getattr(RoleUserAssignment, ...)) — no DB interaction occurs, so@pytest.mark.django_dbadds overhead for no reason.- The
skipif(not ACTIVITYSTREAM_INSTALLED, ...)guard is misleading:audit_log_enabled = Trueandactivity_stream_enabled = Falseare assigned unconditionally onAssignmentBase(role.py lines 438–440) and don't depend on activitystream being present. Skipping when it's absent creates a false coverage gap.✏️ Suggested fix
-@pytest.mark.django_db -@pytest.mark.skipif(not ACTIVITYSTREAM_INSTALLED, reason="activitystream not installed") def test_role_user_assignment_has_audit_log_enabled(): """RoleUserAssignment has audit_log_enabled = True to enable audit logging.""" assert getattr(RoleUserAssignment, "audit_log_enabled", False) is True -@pytest.mark.django_db -@pytest.mark.skipif(not ACTIVITYSTREAM_INSTALLED, reason="activitystream not installed") def test_role_user_assignment_has_activity_stream_disabled(): """RoleUserAssignment has activity_stream_enabled = False to disable activity stream.""" assert getattr(RoleUserAssignment, "activity_stream_enabled", True) is False -@pytest.mark.django_db -@pytest.mark.skipif(not ACTIVITYSTREAM_INSTALLED, reason="activitystream not installed") def test_role_team_assignment_has_audit_log_enabled(): """RoleTeamAssignment has audit_log_enabled = True to enable audit logging.""" assert getattr(RoleTeamAssignment, "audit_log_enabled", False) is True -@pytest.mark.django_db -@pytest.mark.skipif(not ACTIVITYSTREAM_INSTALLED, reason="activitystream not installed") def test_role_team_assignment_has_activity_stream_disabled(): """RoleTeamAssignment has activity_stream_enabled = False to disable activity stream.""" assert getattr(RoleTeamAssignment, "activity_stream_enabled", True) is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test_app/tests/rbac/models/test_assignment_audit_logging.py` around lines 29 - 54, These tests only read class-level attributes (RoleUserAssignment.audit_log_enabled, RoleUserAssignment.activity_stream_enabled, RoleTeamAssignment.audit_log_enabled, RoleTeamAssignment.activity_stream_enabled) and do not touch the DB or depend on the activitystream package; remove the `@pytest.mark.django_db` and `@pytest.mark.skipif`(not ACTIVITYSTREAM_INSTALLED, ...) decorators from the test functions so they run unconditionally and without unnecessary DB setup, keeping the existing getattr assertions intact; you can reference AssignmentBase where those attributes are defined but no runtime change is needed beyond removing the two decorators from the four test functions.ansible_base/rbac/models/role.py (1)
437-440: Misleading comment —audit_log_enabledis set unconditionally, not conditionally.The comment
# Enable audit log for this model (only if activitystream is installed)is inaccurate. Both flags are set as unconditional class attributes onAssignmentBaseregardless of whetheransible_base.activitystreamis inINSTALLED_APPS. They're harmless when_AuditableBase = object(no signal handler consumes them), but the comment implies conditional behavior that doesn't exist.✏️ Suggested fix
- # Enable audit log for this model (only if activitystream is installed) - audit_log_enabled = True - # Disable activity stream for this model - activity_stream_enabled = False + # When activitystream is installed (_AuditableBase = AuditableModel), these flags + # override AuditableModel's defaults to enable audit logging and suppress activity stream entries. + audit_log_enabled = True + activity_stream_enabled = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ansible_base/rbac/models/role.py` around lines 437 - 440, The comment on AssignmentBase is misleading because audit_log_enabled and activity_stream_enabled are set unconditionally; update the comment near the class attributes audit_log_enabled and activity_stream_enabled to state they are unconditional class attributes (harmless when ansible_base.activitystream isn't installed) or remove the parenthetical about "only if activitystream is installed" so it accurately reflects that these flags are always present; locate the attributes audit_log_enabled and activity_stream_enabled in the AssignmentBase definition and edit the comment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ansible_base/rbac/models/role.py`:
- Around line 437-440: The comment on AssignmentBase is misleading because
audit_log_enabled and activity_stream_enabled are set unconditionally; update
the comment near the class attributes audit_log_enabled and
activity_stream_enabled to state they are unconditional class attributes
(harmless when ansible_base.activitystream isn't installed) or remove the
parenthetical about "only if activitystream is installed" so it accurately
reflects that these flags are always present; locate the attributes
audit_log_enabled and activity_stream_enabled in the AssignmentBase definition
and edit the comment accordingly.
In `@test_app/tests/rbac/models/test_assignment_audit_logging.py`:
- Around line 29-54: These tests only read class-level attributes
(RoleUserAssignment.audit_log_enabled,
RoleUserAssignment.activity_stream_enabled,
RoleTeamAssignment.audit_log_enabled,
RoleTeamAssignment.activity_stream_enabled) and do not touch the DB or depend on
the activitystream package; remove the `@pytest.mark.django_db` and
`@pytest.mark.skipif`(not ACTIVITYSTREAM_INSTALLED, ...) decorators from the test
functions so they run unconditionally and without unnecessary DB setup, keeping
the existing getattr assertions intact; you can reference AssignmentBase where
those attributes are defined but no runtime change is needed beyond removing the
two decorators from the four test functions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
ansible_base/rbac/models/role.pytest_app/tests/rbac/models/test_assignment_audit_logging.py
|
|
This is different from #945, where all of the components are using the RBAC app. I know this is a lost cause, but we ideally shouldn't be referencing settings on import. This makes the entire class structure dynamic, which is a dramatic level of debt. |
|
Since it's used by all components, then I'm assuming we can safely drop the conditional import check. Once that's done, is there a better way to meet the current requirement/proposal for having these audit logs (but no activity stream) for RoleUserAssignment and RoleTeamAssignment? I don't mind reworking the PR, but since we're moving forward (at least for immediacy + backporting to 2.5 and 2.6) with the logging controlled by AuditableModel, that's the direction I'd like to use for now. Then fix it in 2.7 with something like 951 . |
Comprehensive PR Review SummaryThank you for this well-structured implementation of audit logging for role assignments! The core functionality is solid and follows good Django patterns. However, I've identified several critical issues that should be addressed before merging, particularly around error handling and silent failure modes. 🚨 Critical Issues (Must Fix)1. Silent Security DegradationThe conditional inheritance pattern silently disables audit logging when Risk: Operators may believe audit logging is active when it's completely disabled. 2. Unprotected Signal HandlersAudit logging failures will crash database transactions instead of being handled gracefully. 3. Missing Deletion TestsNo tests verify audit logs are created when role assignments are deleted (the primary mutation after creation). 📋 Required ActionsBefore Merge:
Strongly Recommended:
🌟 Strengths
I'll add specific line-by-line comments with detailed recommendations. The security aspects are the highest priority - this feature must fail safely rather than silently. Great work overall! The implementation approach is sound, it just needs stronger defensive programming. |
Adds AuditableModel to AssignmentBase so that it can be leveraged by RoleUserAssignment and RoleTeamAssignment. This is configured to have audit logs created for these classes without updating the ActivityStream. Co-authored-by: Claude <noreply@anthropic.com>
Includes tests to confirm that audit log events are generated when Role*Assignment entries are deleted. Co-authored-by: Claude <noreply@anthropic.com>
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/rbac/models/role.py`:
- Around line 34-39: The current fallback to _AuditableBase = object silently
disables auditing; update the module so that when 'ansible_base.activitystream'
is not in settings.INSTALLED_APPS you either log a prominent warning or raise an
error instead of silently continuing. Specifically, after the fallback
assignment of _AuditableBase, import logging (or
django.core.exceptions.ImproperlyConfigured) and if the fallback is used emit a
clear logger.warning (or raise ImproperlyConfigured) stating that AuditableModel
is missing and audit logging on AssignmentBase (and models using
audit_log_enabled) will be disabled; reference _AuditableBase, AuditableModel
and AssignmentBase in the message so the location and impact are obvious.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
ansible_base/rbac/models/role.pytest_app/tests/rbac/models/test_assignment_audit_logging.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test_app/tests/rbac/models/test_assignment_audit_logging.py
3 participants
Description
Adds AuditableModel to AssignmentBase so that it can be leveraged by RoleUserAssignment and RoleTeamAssignment. This is configured to have audit logs created for these classes without updating the ActivityStream.
These are similar changes to PR 945 , just for the RoleUserAssignment / RoleTeamAssignment (through AssignmentBase).
Type of Change
Self-Review Checklist
Testing Instructions
Steps to Test
Expected Results
We should see lines similar to the following in the logs:
No ActivityStream entries should be created
Additional Context
As mentioned in the description, this is similar to the approach in PR 945 .
Summary by CodeRabbit