fix: composite role update#298
Conversation
📝 WalkthroughWalkthroughThree behavioral changes across two modules: ChangesUser Management: Role Protection and Resource Cleanup
Superset Auth: Missing data_filters Guard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wavefront/server/modules/auth_module/auth_module/controllers/superset_controller.py (1)
59-65: ⚡ Quick winConsider aligning HTTP status codes for consistency.
The new 403 FORBIDDEN is semantically correct for an authorization issue. However, the earlier dashboard check at line 49 returns 400 BAD_REQUEST for a similar authorization concern ("User does not have access to any dashboard"). Both situations represent access/permission issues and should ideally use the same status code.
Since 403 FORBIDDEN more accurately represents authorization failures, consider updating the dashboard check (lines 47-53) to also use
status.HTTP_403_FORBIDDENfor consistency.📝 Suggested alignment for consistency
if not dashboards: return JSONResponse( - status_code=status.HTTP_400_BAD_REQUEST, + status_code=status.HTTP_403_FORBIDDEN, content=response_formatter.buildErrorResponse( 'User does not have access to any dashboard' ), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wavefront/server/modules/auth_module/auth_module/controllers/superset_controller.py` around lines 59 - 65, The HTTP status codes for authorization failures are inconsistent. The data_filters check correctly uses status.HTTP_403_FORBIDDEN, but the earlier dashboard access check uses status.HTTP_400_BAD_REQUEST for the message "User does not have access to any dashboard". Both represent authorization/permission failures and should use the same status code. Update the dashboard access check to use status.HTTP_403_FORBIDDEN instead of status.HTTP_400_BAD_REQUEST to align with the data_filters check and properly represent authorization failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/access_controller.py`:
- Around line 535-567: Before executing the delete(Role) statement that removes
roles where Role.id.in_(orphaned_role_ids), add a check to query the UserRole
table to see if any users are assigned to those orphaned roles. If user-role
associations exist for the orphaned_role_ids, either prevent the deletion from
occurring or add appropriate logging to explicitly document that users are
losing role assignments, making the behavior transparent rather than silently
cascading the deletion.
---
Nitpick comments:
In
`@wavefront/server/modules/auth_module/auth_module/controllers/superset_controller.py`:
- Around line 59-65: The HTTP status codes for authorization failures are
inconsistent. The data_filters check correctly uses status.HTTP_403_FORBIDDEN,
but the earlier dashboard access check uses status.HTTP_400_BAD_REQUEST for the
message "User does not have access to any dashboard". Both represent
authorization/permission failures and should use the same status code. Update
the dashboard access check to use status.HTTP_403_FORBIDDEN instead of
status.HTTP_400_BAD_REQUEST to align with the data_filters check and properly
represent authorization failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ba203f2-45a7-460e-ae2b-48c55f1ddb85
📒 Files selected for processing (3)
wavefront/server/modules/auth_module/auth_module/controllers/superset_controller.pywavefront/server/modules/user_management_module/tests/test_access_controller.pywavefront/server/modules/user_management_module/user_management_module/controllers/access_controller.py
|
|
||
| async with resource_repository.session() as session: | ||
| async with session.begin(): | ||
| linked_role_ids = ( | ||
| await session.scalars( | ||
| select(RoleResource.role_id).where( | ||
| RoleResource.resource_id == resource_id | ||
| ) | ||
| ) | ||
| ).all() | ||
|
|
||
| # Deleting the resource cascades its role_resource links. | ||
| await resource_repository.delete_all(id=resource_id, session=session) | ||
|
|
||
| if linked_role_ids: | ||
| remaining_role_ids = set( | ||
| ( | ||
| await session.scalars( | ||
| select(RoleResource.role_id).where( | ||
| RoleResource.role_id.in_(linked_role_ids) | ||
| ) | ||
| ) | ||
| ).all() | ||
| ) | ||
| orphaned_role_ids = [ | ||
| role_id | ||
| for role_id in linked_role_ids | ||
| if role_id not in remaining_role_ids | ||
| ] | ||
| if orphaned_role_ids: | ||
| await session.execute( | ||
| delete(Role).where(Role.id.in_(orphaned_role_ids)) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how User-Role relationship is defined and whether cascading is handled
ast-grep --pattern $'class User($_) {
$$$
role$_
$$$
}'
# Also search for any user_role join table or relationship definition
rg -n "user.*role|role.*user" --type py -g '*/models/*' -C 3Repository: rootflo/wavefront
Length of output: 210
🏁 Script executed:
# Find the repository structure
git ls-files | grep -E "(model|user|role)" | head -20Repository: rootflo/wavefront
Length of output: 1033
🏁 Script executed:
# Look for Python files that might contain User and Role class definitions
fd -e py -type f | grep -iE "(model|user|role)" | head -20Repository: rootflo/wavefront
Length of output: 232
🏁 Script executed:
# Check the directory structure of the user_management_module
find wavefront/server/modules/user_management_module -type f -name "*.py" | head -30Repository: rootflo/wavefront
Length of output: 2213
🏁 Script executed:
# Examine the resource model file which likely contains Role and RoleResource
cat -n wavefront/server/modules/user_management_module/user_management_module/models/resource.pyRepository: rootflo/wavefront
Length of output: 2401
🏁 Script executed:
# Examine the user schema file which likely contains User model
cat -n wavefront/server/modules/user_management_module/user_management_module/models/user_schema.pyRepository: rootflo/wavefront
Length of output: 6512
🏁 Script executed:
# Search for where Role model is defined and check for user_role relationship
rg -n "class Role\b|class RoleResource\b" wavefront/server/modules/user_management_module --type py -A 20Repository: rootflo/wavefront
Length of output: 2446
🏁 Script executed:
# Search for SQLAlchemy imports and model definitions
rg -n "from sqlalchemy|import.*Column|class.*db\.|declarative_base" wavefront/server/modules/user_management_module --type py | head -20Repository: rootflo/wavefront
Length of output: 2692
🏁 Script executed:
# Search specifically for RoleResource class definition
rg -n "class RoleResource" wavefront/server/modules/user_management_module --type py -A 15Repository: rootflo/wavefront
Length of output: 43
🏁 Script executed:
# Look in the entire user_management_module for all Python files to understand structure
find wavefront/server/modules/user_management_module -name "*.py" -type f | xargs grep -l "sqlalchemy\|declarative_base\|Column"Repository: rootflo/wavefront
Length of output: 665
🏁 Script executed:
# Check imports in access_controller.py to find where models come from
head -50 wavefront/server/modules/user_management_module/user_management_module/controllers/access_controller.pyRepository: rootflo/wavefront
Length of output: 1821
🏁 Script executed:
# Check conftest to see how models are loaded
cat -n wavefront/server/modules/user_management_module/tests/conftest.pyRepository: rootflo/wavefront
Length of output: 12171
🏁 Script executed:
# Search for all Python files containing RoleResource or Role ORM model in the broader codebase
find wavefront -name "*.py" -type f | xargs grep -l "class RoleResource\|class.*Role.*:" | grep -v __pycache__ | head -20Repository: rootflo/wavefront
Length of output: 437
🏁 Script executed:
# Examine the Role ORM model
cat -n wavefront/server/modules/db_repo_module/db_repo_module/models/role.pyRepository: rootflo/wavefront
Length of output: 1104
🏁 Script executed:
# Examine the RoleResource ORM model
cat -n wavefront/server/modules/db_repo_module/db_repo_module/models/role_resource.pyRepository: rootflo/wavefront
Length of output: 621
🏁 Script executed:
# Examine the User-Role relationship model
cat -n wavefront/server/modules/db_repo_module/db_repo_module/models/user_role.pyRepository: rootflo/wavefront
Length of output: 605
🏁 Script executed:
# Also check if there's a User model to see all relationships
find wavefront/server/modules/db_repo_module -name "*.py" -type f | xargs grep -l "class User" | head -5Repository: rootflo/wavefront
Length of output: 206
The orphan role deletion is safe regarding foreign-key constraints but may silently remove users' role assignments.
The UserRole table has ondelete='CASCADE' configured (ForeignKey on role_id), so deleting a role automatically cleans up user-role associations at the database level without causing FK violations. However, users assigned to orphaned roles will lose their role assignments silently, with no warning or compensation. If role deletion should be prevented when users are assigned, add a check to validate this before deletion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/access_controller.py`
around lines 535 - 567, Before executing the delete(Role) statement that removes
roles where Role.id.in_(orphaned_role_ids), add a check to query the UserRole
table to see if any users are assigned to those orphaned roles. If user-role
associations exist for the orphaned_role_ids, either prevent the deletion from
occurring or add appropriate logging to explicitly document that users are
losing role assignments, making the behavior transparent rather than silently
cascading the deletion.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests