chore: improve the rbac migrations #28123
Conversation
zlwaterfield
force-pushed
the
zach/rbac/14
branch
from
c1b480e to
9f53766
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Based on the provided files and context, I'll summarize the key changes in this pull request:
This PR improves the feature flag RBAC migration by enhancing access control handling and coverage. Here are the key changes:
- Replaces
PROJECT_BASED_PERMISSIONINGwithADVANCED_PERMISSIONSacross the codebase for unified access control - Adds comprehensive handling for organization-wide feature flag permissions:
- Default view-only access for all feature flags
- Role-based edit access when view-only is enabled
- Specific per-flag role access controls
- Implements atomic migration within transactions to safely convert existing permissions
- Adds extensive test coverage for new access control scenarios including organization-wide and role-specific permissions
The changes appear well-tested and maintain backward compatibility while preparing for enhanced RBAC features.
Potential concerns:
- Migration should be monitored for large organizations with many feature flags
- Need to ensure proper cleanup of old FeatureFlagRoleAccess records
- Consider performance impact of additional access control queries
18 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| access_level=21, # view only | ||
| ) |
There was a problem hiding this comment.
style: magic number 21 used for access level - consider using a constant or enum for better maintainability
| # Should create viewer access for all flags | ||
| viewer_access = AccessControl.objects.filter( | ||
| resource="feature_flag", | ||
| access_level="viewer", | ||
| role__isnull=True, | ||
| ) | ||
| self.assertEqual(viewer_access.count(), 3) |
There was a problem hiding this comment.
logic: test verifies viewer access but not the cleanup of old FeatureFlagRoleAccess entries - add assertion for FeatureFlagRoleAccess.objects.count() == 0
| # Should create viewer access for all flags | |
| viewer_access = AccessControl.objects.filter( | |
| resource="feature_flag", | |
| access_level="viewer", | |
| role__isnull=True, | |
| ) | |
| self.assertEqual(viewer_access.count(), 3) | |
| # Should create viewer access for all flags | |
| viewer_access = AccessControl.objects.filter( | |
| resource="feature_flag", | |
| access_level="viewer", | |
| role__isnull=True, | |
| ) | |
| self.assertEqual(viewer_access.count(), 3) | |
| self.assertEqual(FeatureFlagRoleAccess.objects.count(), 0) # Verify old entries are cleaned up |
| organization_resource_access = OrganizationResourceAccess.objects.filter( | ||
| organization_id=organization_id, | ||
| resource="feature_flag", | ||
| access_level=21, | ||
| ) |
There was a problem hiding this comment.
style: Magic number 21 used for access_level. Should be defined as a constant or enum to improve maintainability and prevent errors
|
|
||
| # Then we want to look at all roles where the feature flag role is edit level to apply those to all flags, we need | ||
| # to do this if the organization resource access is view only | ||
| editor_roles = Role.objects.filter(organization_id=organization_id, feature_flags_access_level=37) |
There was a problem hiding this comment.
style: Magic number 37 used for feature_flags_access_level. Should be defined as a constant or enum
| for feature_flag in FeatureFlag.objects.filter(team__organization_id=organization_id): | ||
| AccessControl.objects.create( | ||
| # Note: no user or role so it's project wide | ||
| team=feature_flag.team, | ||
| access_level="viewer", | ||
| resource="feature_flag", | ||
| resource_id=feature_flag.id, | ||
| ) |
There was a problem hiding this comment.
logic: No error handling for AccessControl.objects.create() - could fail silently if duplicate entries exist
| for feature_flag in FeatureFlag.objects.filter(team__organization_id=organization_id): | ||
| AccessControl.objects.create( | ||
| team=feature_flag.team, | ||
| access_level="editor", | ||
| resource="feature_flag", | ||
| resource_id=feature_flag.id, | ||
| role=role, | ||
| ) |
There was a problem hiding this comment.
style: Potential N+1 query issue with nested loops over roles and feature flags. Consider using bulk_create
| for feature_flag in FeatureFlag.objects.filter(team__organization_id=organization_id): | |
| AccessControl.objects.create( | |
| team=feature_flag.team, | |
| access_level="editor", | |
| resource="feature_flag", | |
| resource_id=feature_flag.id, | |
| role=role, | |
| ) | |
| access_controls = [ | |
| AccessControl( | |
| team=feature_flag.team, | |
| access_level="editor", | |
| resource="feature_flag", | |
| resource_id=feature_flag.id, | |
| role=role, | |
| ) | |
| for feature_flag in FeatureFlag.objects.filter(team__organization_id=organization_id) | |
| ] | |
| AccessControl.objects.bulk_create(access_controls) |
zlwaterfield
changed the title
📸 UI snapshots have been updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
|
Size Change: +5 B (0%) Total Size: 1.16 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated6 snapshot changes in total. 0 added, 6 modified, 0 deleted:
Triggered by this commit. |
Changes
Follow along for RBAC: #24512
This is just improving the feature flag RBAC migration to make sure all cases are covers
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
It doesn't have an impact.
How did you test this code?
Added more to the existing tests