Analytics access #8509
Analytics access #8509
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve the addition of a new boolean property, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
cvat/apps/engine/migrations/0085_profile_has_analytics_access.py (1)
13-21: LGTM: Field definition is appropriate and follows best practices.The
has_analytics_accessfield is well-defined:
- BooleanField is suitable for a permission flag.
- Default value of False aligns with the principle of least privilege.
- Help text and verbose name provide clear explanations.
Consider adding a comment explaining why this permission is separate from the regular user permissions system, if there's a specific reason.
cvat/apps/log_viewer/rules/analytics.rego (2)
42-44: New rule added, but consider additional checksThe new rule successfully implements the PR objective of granting access based on the
has_analytics_accessfield. However, consider the following suggestions to enhance security and completeness:
This rule might be too permissive as it doesn't check for any other conditions (like scope or visibility). Consider adding additional checks, e.g.:
allow if { input.auth.user.has_analytics_access input.scope == utils.VIEW }There's no explicit handling for the case when
has_analytics_accessis false. While the defaultallow := falsecovers this, it might be worth adding an explicit deny rule for clarity:deny if { not input.auth.user.has_analytics_access }Consider adding a comment explaining the purpose of this new rule for better documentation.
Would you like me to propose a more comprehensive implementation addressing these points?
Frontend is missing the analytics access checkbox implementation
The frontend does not include a checkbox for analytics access, which is necessary for users to set the
has_analytics_accessfield. Please implement the checkbox to allow users to enable or disable analytics access accordingly.🔗 Analysis chain
Line range hint
1-44: Summary: Implementation correct, consider frontend implicationsThe changes successfully implement the PR objective of adding a checkbox for analytics access. The new field and rule have been added correctly. However, consider the following next steps:
- Implement the suggested improvements to the new rule for better security and clarity.
- Ensure that the frontend correctly sets the
has_analytics_accessfield based on the checkbox state.- Add unit tests to verify the behavior of the new rule under different conditions.
- Update the documentation to reflect this new access control mechanism.
To verify the frontend implementation, you can run the following script:
This script will help identify if the frontend has been updated to include the new checkbox and if it's correctly handling the
has_analytics_accessfield.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of the analytics access checkbox in frontend code # Test: Search for checkbox related to analytics access rg --type js --type ts -i 'checkbox.*analytics.*access' # Test: Search for the field name in API calls or state management rg --type js --type ts 'has_analytics_access'Length of output: 326
Script:
#!/bin/bash # Description: Search for UI components controlling 'has_analytics_access' in frontend code # Search in common frontend component file types for references to 'has_analytics_access' rg --type jsx --type tsx --type vue --type html --type jsx -i 'has_analytics_access' # Search for checkbox inputs that might toggle 'has_analytics_access' rg --type jsx --type tsx --type vue --type html -i '<input[^>]*type=["\']checkbox["\'][^>]*has_analytics_access' # Search for labels or text associated with the analytics access checkbox rg --type jsx --type tsx --type vue --type html -i 'analytics access'Length of output: 206
Script:
#!/bin/bash # Description: Search for UI components controlling 'has_analytics_access' in frontend code # Search in common frontend component file types for references to 'has_analytics_access' rg 'has_analytics_access' --type js --type ts --type vue --type html # Search for checkbox inputs that might toggle 'has_analytics_access' rg '<input[^>]*type=["'\'']checkbox["'\''][^>]*has_analytics_access' --type js --type ts --type vue --type html # Search for labels or text associated with the analytics access checkbox rg 'analytics access' --type js --type ts --type vue --type htmlLength of output: 333
Script:
#!/bin/bash # Description: Search for UI components controlling 'has_analytics_access' in frontend code # Search in common frontend component file extensions for references to 'has_analytics_access' rg 'has_analytics_access' --glob '*.js' --glob '*.ts' --glob '*.html' --glob '*.vue' # Search for checkbox inputs that might toggle 'has_analytics_access' rg '<input[^>]*type=["'\''"]checkbox["'\''"][^>]*has_analytics_access' --glob '*.js' --glob '*.ts' --glob '*.html' --glob '*.vue' # Search for labels or text associated with the analytics access checkbox rg 'analytics access' --glob '*.js' --glob '*.ts' --glob '*.html' --glob '*.vue'Length of output: 522
cvat/apps/iam/admin.py (1)
13-18: LGTM: ProfileInline class implementation.The ProfileInline class is well-structured and correctly implements the inline admin functionality for the Profile model. The inclusion of the 'has_analytics_access' field aligns with the PR objective of adding a checkbox for analytics access.
Consider adding a docstring to the ProfileInline class to describe its purpose and relationship to the User model. For example:
class ProfileInline(admin.StackedInline): """Inline admin interface for User's Profile model.""" model = Profile # ... rest of the codecvat-ui/src/components/header/header.tsx (1)
Line range hint
516-528: LGTM! Consider a minor improvement for consistency.The changes look good and align with the PR objective of expanding access to the analytics feature. The implementation is correct and consistent with the rest of the component.
A minor suggestion for consistency:
Consider extracting the condition
user.isSuperuser || user.hasAnalyticsAccessinto a variable for better readability and potential reuse. For example:+ const hasAnalyticsAccess = user.isSuperuser || user.hasAnalyticsAccess; - {isAnalyticsPluginActive && (user.isSuperuser || user.hasAnalyticsAccess) ? ( + {isAnalyticsPluginActive && hasAnalyticsAccess ? (This change would make the code more readable and easier to maintain if the access condition needs to be used elsewhere in the future.
cvat/apps/iam/permissions.py (2)
103-103: Document the new class attributehas_analytics_accessFor clarity and maintainability, consider adding a docstring or comment to document the purpose of the
has_analytics_accessattribute in theOpenPolicyAgentPermissionclass. This will help other developers understand its role in permission checks.
Missing
load_app_permissionsCalls in AppConfig.ready() MethodsThe following
AppConfigclasses do not callload_app_permissionswithin theirready()methods:
cvat/apps/dataset_manager/apps.py:DatasetManagerConfigcvat/apps/engine/apps.py:EngineConfigcvat/apps/health/apps.py:HealthConfigcvat/apps/iam/apps.py:IAMConfigcvat/apps/lambda_manager/apps.py:LambdaManagerConfigcvat/apps/log_viewer/apps.py:LogViewerConfigcvat/apps/organizations/apps.py:OrganizationsConfigcvat/apps/quality_control/apps.py:QualityControlConfigPlease ensure that
load_app_permissionsis called within theready()method of eachAppConfigclass to properly load permissions and OPA rules.🔗 Analysis chain
Line range hint
280-289: Ensureload_app_permissionsis called appropriatelyThe
load_app_permissionsfunction is intended to be called from theAppConfig.ready()method of each app that defines a permissions module. Please verify that this function is being called in the appropriate place to ensure that permissions and OPA rules are loaded correctly.Run the following script to check if
load_app_permissionsis called in theready()method of each app'sAppConfig:This script lists all
AppConfigclasses and checks ifload_app_permissionsis called within theirready()methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that load_app_permissions is called in AppConfig.ready() # Find all AppConfig classes and check for load_app_permissions call fd --type f --name "*.py" | xargs grep -E "class.*AppConfig" # Check if load_app_permissions is called within ready() methods fd --type f --name "*.py" | xargs grep -A 5 -E "def ready\(self\):" | grep "load_app_permissions"Length of output: 662
Script:
#!/bin/bash # Description: Verify that load_app_permissions is called in AppConfig.ready() # Find all AppConfig classes fd --type f -e py | xargs grep -E "class\s+.*AppConfig" # Check if load_app_permissions is called within ready() methods fd --type f -e py | xargs grep -A 5 -E "def ready\(self\):" | grep "load_app_permissions"Length of output: 1321
cvat/apps/engine/serializers.py (1)
236-236: Addhas_analytics_accesstoread_only_fieldsSince
has_analytics_accessis a read-only field, it should be added to theread_only_fieldsin the serializer'sMetaclass to prevent write attempts.Apply this diff:
read_only_fields = ('last_login', 'date_joined') + read_only_fields = ('last_login', 'date_joined', 'has_analytics_access')cvat/apps/engine/models.py (1)
1094-1098: Use named arguments for clarity and refine user-facing stringsIn the
has_analytics_accessfield definition, using named arguments likeverbose_nameandhelp_textenhances readability. Additionally, consider rephrasing theverbose_nameandhelp_textto avoid using slashes in user-facing strings.Here's a suggested change:
has_analytics_access = models.BooleanField( - _("has /analytics access"), + verbose_name=_("Has analytics access"), default=False, - help_text=_("Designates whether the user can access /analytics."), + help_text=_("Indicates whether the user can access the analytics endpoint."), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
cvat/apps/log_viewer/rules/tests/configs/analytics.csvis excluded by!**/*.csv
📒 Files selected for processing (9)
- cvat-core/src/user.ts (4 hunks)
- cvat-ui/src/components/header/header.tsx (1 hunks)
- cvat/apps/engine/migrations/0085_profile_has_analytics_access.py (1 hunks)
- cvat/apps/engine/models.py (2 hunks)
- cvat/apps/engine/serializers.py (4 hunks)
- cvat/apps/iam/admin.py (1 hunks)
- cvat/apps/iam/permissions.py (8 hunks)
- cvat/apps/log_viewer/rules/analytics.rego (2 hunks)
- cvat/apps/log_viewer/rules/tests/generators/analytics_test.gen.rego.py (5 hunks)
🔇 Additional comments (22)
cvat/apps/engine/migrations/0085_profile_has_analytics_access.py (2)
1-10: LGTM: Migration structure and dependencies are correct.The migration file follows the standard Django structure and correctly specifies its dependency on the previous migration "0084_honeypot_support".
1-22: Verify implementation of UI changes and access control logic.This migration successfully adds the database field needed for the analytics access checkbox mentioned in the PR objectives. However, to fully implement this feature:
- Ensure that corresponding changes are made in the user interface to display and manage this checkbox.
- Implement the necessary logic in views or middleware to enforce this permission when accessing the
/analyticsendpoint.- Update user documentation to explain this new permission.
To check for related changes, run:
Would you like assistance in implementing any of these additional changes?
cvat/apps/log_viewer/rules/analytics.rego (2)
12-13: LGTM: New field added correctlyThe addition of the
has_analytics_accessboolean field to theinput.auth.userstructure is consistent with the PR objective of introducing a checkbox for analytics access. The comment structure is maintained, which is good for readability.
41-41: LGTM: Improved readabilityThe addition of a blank line here improves the readability of the code by clearly separating different rule blocks.
cvat/apps/iam/admin.py (3)
10-10: LGTM: Import statement for Profile model.The import statement for the Profile model is correctly placed and necessary for the new ProfileInline class.
Line range hint
1-67: Summary: Implementation aligns with PR objectives.The changes in this file successfully implement the checkbox feature for granting access to the
/analyticsendpoint, as outlined in the PR objectives. The modifications to the Django admin interface are well-structured and follow best practices for customizing the admin site.Key points:
- The new ProfileInline class correctly adds the 'has_analytics_access' field to the User admin interface.
- The CustomUserAdmin class is properly modified to include the ProfileInline.
- The implementation allows for easy management of analytics access permissions through the admin interface.
To ensure full compatibility with the existing codebase:
- Verify that the Profile model in
cvat/apps/engine/models.pyhas been updated to include the 'has_analytics_access' field.- Check if any views or permissions in the project need to be updated to respect this new access control.
- Ensure that appropriate unit and integration tests have been added to cover this new functionality.
These verifications will help maintain the integrity and security of the analytics access feature.
22-22: LGTM: CustomUserAdmin modification.The addition of ProfileInline to the inlines attribute of CustomUserAdmin is correct and aligns with the PR objective. This change allows editing of Profile fields, including the new 'has_analytics_access' checkbox, directly in the User admin interface.
To ensure the changes are properly integrated, please verify the admin interface by running the following commands:
These commands will help confirm that the changes are correctly implemented across the relevant files.
✅ Verification successful
Verified: CustomUserAdmin modification.
The
ProfileInlinehas been correctly added to theinlinesattribute ofCustomUserAdmin, and theProfilemodel includes thehas_analytics_accessfield. Additionally, there are no conflicting admin registrations for theUsermodel.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of ProfileInline in CustomUserAdmin # Test 1: Check if ProfileInline is imported and used in CustomUserAdmin rg --type python "class CustomUserAdmin.*ProfileInline" cvat/apps/iam/admin.py # Test 2: Verify the Profile model has the 'has_analytics_access' field rg --type python "class Profile.*has_analytics_access" cvat/apps/engine/models.py # Test 3: Check if there are any conflicting admin registrations for User model rg --type python "admin\.site\.register.*User" cvatLength of output: 311
Script:
#!/bin/bash # Description: Verify the integration of ProfileInline in CustomUserAdmin without relying on --type flag # Test 1: Check if ProfileInline is imported and used in CustomUserAdmin rg "class CustomUserAdmin.*ProfileInline" cvat/apps/iam/admin.py # Test 2: Verify the Profile model has the 'has_analytics_access' field rg "class Profile.*has_analytics_access" cvat/apps/engine/models.py # Test 3: Check if there are any conflicting admin registrations for User model rg "admin\.site\.register.*User" cvatLength of output: 242
Script:
#!/bin/bash # Description: Further verify the integration of ProfileInline in CustomUserAdmin and the 'has_analytics_access' field # Test 1: Check if inlines are defined with ProfileInline in CustomUserAdmin rg "inlines\s*=\s*\(.*ProfileInline.*\)" cvat/apps/iam/admin.py # Test 2: Verify the Profile model has the 'has_analytics_access' field rg "has_analytics_access" cvat/apps/engine/models.pyLength of output: 197
cvat-core/src/user.ts (5)
21-21: LGTM: New property added correctly.The addition of the
hasAnalyticsAccessproperty is well-implemented. It's correctly defined as readonly and follows the existing naming conventions. This aligns with the PR objective of introducing a checkbox feature for analytics access.
37-37: LGTM: Data object updated correctly.The addition of the
has_analytics_accessproperty to thedataobject is consistent with the existing code structure. It's correctly initialized tonulland uses the appropriate naming convention.
85-87: LGTM: Getter for hasAnalyticsAccess implemented correctly.The new getter for
hasAnalyticsAccessis well-implemented. It follows the existing pattern in the class, provides appropriate read-only access to the data, and maintains consistent naming conventions.
106-106: LGTM: Serialization updated correctly.The
has_analytics_accessproperty has been correctly added to theserializemethod. This ensures that the new property is included when the User object is serialized, maintaining consistency with the rest of the object's properties.
Line range hint
21-106: Verify the usage of hasAnalyticsAccess property.The implementation of the
hasAnalyticsAccessproperty is complete and consistent with the existing code. However, it would be beneficial to verify how this property is set and used in other parts of the codebase to ensure it fulfills the PR objective of introducing a checkbox feature for analytics access.To verify the usage of this new property, we can run the following script:
Would you like me to run this script or assist in any other way to ensure the proper implementation and usage of this new feature?
cvat-ui/src/components/header/header.tsx (2)
Line range hint
1-728: Summary: Changes look good, awaiting additional contextThe changes in this file are well-implemented and align with the PR objectives. The "Analytics" button is now conditionally rendered based on both superuser status and a new
hasAnalyticsAccessproperty, which expands access to the analytics feature as intended.A minor suggestion for code readability has been provided, and we've requested additional information about related changes and potential security implications. Once these points are addressed, and assuming no issues are found in the related changes, this part of the implementation looks ready for approval.
Line range hint
1-728: Verify related changes and consider security implicationsThe changes in this file look good, but they seem to be part of a larger feature implementation. To ensure a comprehensive review:
Could you provide information about related changes in other files, particularly regarding the checkbox feature mentioned in the PR objectives?
Have there been any changes to the user model or permissions system to support the new
hasAnalyticsAccessproperty?Are there any security implications of expanding access to the analytics feature that we should consider?
To verify the implementation and its impact, please run the following script:
This script will help us identify related changes and potential security implications across the codebase.
cvat/apps/log_viewer/rules/tests/generators/analytics_test.gen.rego.py (6)
46-46: Good addition ofHAS_ANALYTICS_ACCESSconstantDefining
HAS_ANALYTICS_ACCESS = [True, False]is appropriate for generating test cases with different analytics access permissions.
81-81: Consistently includehas_analytics_accessinget_datafunctionUpdating
get_datato accepthas_analytics_accessensures that user data accurately reflects the analytics access status, which is essential for generating correct test cases.
85-89: Properly integratehas_analytics_accessinto user dataIncluding
"has_analytics_access": has_analytics_accesswithin the user dictionary correctly represents the user's permissions and aligns with the updated data structure.
132-132: Updateget_nameto includehas_analytics_accessfor clarity in test namesBy adding
has_analytics_accessto theget_namefunction parameters, generated test names will reflect whether analytics access is granted, improving test identification and debugging.
148-149: Expand test case generation withHAS_ANALYTICS_ACCESSIncorporating
HAS_ANALYTICS_ACCESSinto the product of parameters ensures that test cases cover scenarios both with and without analytics access, enhancing test coverage.
155-157: Passhas_analytics_accessconsistently in function callsEnsuring that
has_analytics_accessis passed toget_data,get_name, andeval_rulemaintains consistency across function calls and prevents potential mismatches or errors in test case generation.cvat/apps/iam/permissions.py (1)
230-233: Type VariableTand Functionis_public_objThe introduction of the generic type variable
Tbound toModeland theis_public_objfunction helps in type safety and clarity. Good use of typing to enhance code reliability.cvat/apps/engine/models.py (1)
19-29: LGTM!The import statements are appropriate and necessary for the additions made to the code.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8509 +/- ##
===========================================
+ Coverage 74.26% 74.28% +0.02%
===========================================
Files 403 403
Lines 43287 43316 +29
Branches 3914 3914
===========================================
+ Hits 32147 32179 +32
+ Misses 11140 11137 -3
|
Eldies
force-pushed
the
dl/analytics-bool-field-v3
branch
from
0b954f1
to
0a3da3b
Compare
|
|
||
| @receiver(m2m_changed, sender=User.groups.through, dispatch_uid=__name__ + ".m2m_user_groups_change_handler") | ||
| def __m2m_user_groups_change_handler(sender, instance: User, action: str, **kwargs): | ||
| if action == 'post_add': |
There was a problem hiding this comment.
Do we need to handle also post_remove?
There was a problem hiding this comment.
Its possible to give access to analytics to not-admin users, therefore I think analytics access should not be revoked with admin group
There was a problem hiding this comment.
I meant handling cases when a user had the admin group and then he was excluded from this group.
Could be discussed and fixed later I guess.
| { | ||
| "model": "engine.segment", | ||
| "pk": 38, | ||
| "fields": { | ||
| "task": 29, | ||
| "start_frame": 0, | ||
| "stop_frame": 7, | ||
| "chunks_updated_date": "2024-10-02T08:13:16.623Z", | ||
| "type": "range", | ||
| "frames": "[]" | ||
| } | ||
| }, | ||
| { | ||
| "model": "engine.segment", | ||
| "pk": 39, | ||
| "fields": { | ||
| "task": 29, | ||
| "start_frame": 8, | ||
| "stop_frame": 15, | ||
| "chunks_updated_date": "2024-10-02T08:13:16.623Z", | ||
| "type": "range", | ||
| "frames": "[]" | ||
| } | ||
| }, | ||
| { | ||
| "model": "engine.segment", | ||
| "pk": 40, | ||
| "fields": { | ||
| "task": 29, | ||
| "start_frame": 16, | ||
| "stop_frame": 22, | ||
| "chunks_updated_date": "2024-10-02T08:13:16.623Z", | ||
| "type": "range", | ||
| "frames": "[]" | ||
| } | ||
| }, | ||
| { | ||
| "model": "engine.segment", | ||
| "pk": 41, | ||
| "fields": { | ||
| "task": 29, | ||
| "start_frame": 23, | ||
| "stop_frame": 28, | ||
| "chunks_updated_date": "2024-10-02T08:13:16.623Z", | ||
| "type": "range", | ||
| "frames": "[]" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Could you please explain why you deleted these segments?
There was a problem hiding this comment.
It is the result of docker exec test_cvat_server_1 python manage.py dumpdata --indent 2 --natural-foreign --exclude=auth.permission --exclude=contenttypes --exclude=django_rq > tests/python/shared/assets/cvat_db/data.json. I guess it happened because these segments were duplicated - there still are segments with pk 38-41 in data.json
cvat/apps/log_viewer/permissions.py
Outdated
| @classmethod | ||
| def create_scope_list(cls, request, iam_context=None): | ||
| if not iam_context and request: | ||
| iam_context = super().create_scope_list(request, iam_context=iam_context) | ||
| return cls( | ||
| **iam_context, | ||
| scope='list', | ||
| has_analytics_access=request.user.profile.has_analytics_access, | ||
| ) |
There was a problem hiding this comment.
I don't see this method being used anywhere + there is no scope list in LogViewerPermission (only view). So, could you please clarify why you added it? (or delete unused code).
| "conditions, is_allow", | ||
| [ | ||
| (dict(privilege="admin"), True), | ||
| (dict(privilege="business"), True), |
There was a problem hiding this comment.
Should you update has_analytics_access in the test db/assets for users with business privilege too?
There was a problem hiding this comment.
Or sorry, it is my bad. Probably it will be better to add a comment that users with business privilege should have analytics access only when the cvat server is configured with RESTRICTIONS['analytics_visibility'] = True. Perhaps we should test that too when there will be a pipeline for testing different server configurations.
There was a problem hiding this comment.
I also see a problem that existed before and has not been fixed here:
If the server is configured with RESTRICTIONS['analytics_visibility'] = True then a user with the business group will not see the analytics tab on the CVAT UI. Probably need to think about it or discuss this moment one more time with the team. @azhavoro, Could you please remind me why it was implemented so and do we still need this server configuration or can it be replaced with a new approach(with has_analytics_access)?
There was a problem hiding this comment.
Please, fix 2 comments:
https://github.com/cvat-ai/cvat/pull/8509/files/6334e688306033d36b2ff3e4ad9ac4dc0e18b414#diff-463535b27f491d7d8a1df109300bc277a70d1737ce2feae515c26292394964c0
https://github.com/cvat-ai/cvat/pull/8509/files/6334e688306033d36b2ff3e4ad9ac4dc0e18b414#diff-37aaff6063fcb28edfadeecc9f4adca12aed1b490b1ada24752eb9b54fd6c593
Otherwise LGTM
|
Motivation and context
Checkbox for granting access to /analytics
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
Release Notes
New Features
hasAnalyticsAccess, in user profiles to manage access to analytics features.Bug Fixes
hasAnalyticsAccess, enhancing access control for analytics features.Documentation
hasAnalyticsAccessfield.These updates enhance user experience by providing better access management and visibility of analytics features.