[REF][IMP] Access Rules for Variables and Values with Tests #166
Conversation
[REF] - Refactored existing code to align with PEP8 and Odoo guidelines. - Improved indentation and formatting in affected files for better readability. [IMP] - Added access control by inheriting `cx.tower.access.mixin` in `TowerVariable` and `TowerVariableValue` models. - Introduced the `access_level` field in the models and views, allowing dynamic control of variable visibility. - Updated search and form views to include the `access_level` field for better usability. [NEW] - Created a new security file `cx_tower_variable_security.xml` to define access rules for `TowerVariable`. - Updated `cx_tower_variable_value_security.xml` to include refined domain rules for `TowerVariableValue`. [TEST] - Updated test `test_variable_value_access` to validate refined access logic. - Added a new test `test_variable_value_server_subscription` to ensure server subscription-based variable visibility. Steps to Reproduce 1. Create a new variable and assign it to a server with specific `access_level`. 2. Test visibility of the variable based on user roles (`group_user`, `group_manager`, `group_root`). 3. Validate global and local variable toggling and their visibility to respective roles. 4. Subscribe a user to the server and ensure variable visibility updates dynamically. 5. Run the tests (`test_variable_value_access`, `test_variable_value_server_subscription`) to confirm correct behavior. File Changes 1. `cx_tower_variable_security.xml` - Created a new security file to manage access rules for `TowerVariable`. 2. `cx_tower_variable_value_security.xml` - Updated domain rules to dynamically control visibility based on `access_level` and server subscriptions. 3. Models (`TowerVariable`, `TowerVariableValue`) - Inherited `cx.tower.access.mixin`. - Added `access_level` fields and their logic. 4. Views (`TowerVariable`, `TowerVariableValue`) - Updated forms and search views to include `access_level` fields. 5. Tests - Refined `test_variable_value_access` to ensure compatibility with new logic. - Added `test_variable_value_server_subscription` to validate subscription-based access. 6. General - Minor corrections in formatting, indentation, and adherence to Odoo development guidelines. Task: 4055
WalkthroughThis pull request introduces updates to the Cetmix Tower Server Management module, focusing on access control and documentation improvements. Key changes include updating URLs in the documentation to reflect the stable branch Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
🪧 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: 0
🧹 Nitpick comments (7)
cetmix_tower_server/security/cx_tower_variable_security.xml (2)
7-8: Consider using integer field for access_levelThe domain condition uses string comparison (
'1') for what appears to be a numeric access level. Consider using integer values for better performance and maintainability.- <field name="domain_force">[('access_level', '=', '1')]</field> + <field name="domain_force">[('access_level', '=', 1)]</field>
15-16: Simplify manager access rule domainThe current domain uses
inoperator with a single value. Consider using the equality operator for better readability.- <field name="domain_force">[('access_level', 'in', ['2'])]</field> + <field name="domain_force">[('access_level', '=', 2)]</field>cetmix_tower_server/security/cx_tower_variable_value_security.xml (2)
23-30: Consider consistent integer usage for access_levelSimilar to the variable security rules, consider using integer values for access_level comparison.
- ('variable_id.access_level', 'in', ['2']), + ('variable_id.access_level', '=', 2),
31-34: Fix XML formatting in manager ruleThe
groupsfield has inconsistent indentation and line breaks.- <field - name="groups" - eval="[(4, ref('cetmix_tower_server.group_manager'))]" - /> + <field name="groups" eval="[(4, ref('cetmix_tower_server.group_manager'))]" />cetmix_tower_server/views/cx_tower_variable_value_view.xml (2)
37-37: Consider adding selection filter for access_levelThe search view could benefit from predefined filters for different access levels.
<field name="access_level" /> + <filter string="User Level" name="user_level" domain="[('access_level', '=', 1)]"/> + <filter string="Manager Level" name="manager_level" domain="[('access_level', '=', 2)]"/>
53-57: Consider specifying default viewThe action window definition could benefit from specifying the tree view as default.
<field name="res_model">cx.tower.variable.value</field> <field name="view_mode">tree</field> + <field name="view_id" ref="cx_tower_variable_value_view_tree"/>cetmix_tower_server/views/cx_tower_variable_view.xml (1)
28-28: Consider enhancing the access_level field configurationWhile the placement is logical, consider adding attributes to enforce business rules:
- Add a
required="1"attribute if this field should be mandatory- Consider adding a
groupsattribute if certain access levels should only be selectable by specific user groups- Add a
widget="selection"attribute if this is a selection field for better UX- <field name="access_level" /> + <field name="access_level" required="1" widget="selection" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(2 hunks)cetmix_tower_server/models/cx_tower_access_mixin.py(1 hunks)cetmix_tower_server/models/cx_tower_variable.py(1 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(1 hunks)cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(1 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_variable.py(7 hunks)cetmix_tower_server/views/cx_tower_variable_value_view.xml(4 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- cetmix_tower_server/models/cx_tower_access_mixin.py
- cetmix_tower_server/README.rst
🧰 Additional context used
📓 Learnings (4)
cetmix_tower_server/models/cx_tower_variable_value.py (1)
Learnt from: ivs-cetmix
PR: cetmix/cetmix-tower#129
File: cetmix_tower_server/models/cx_tower_plan_line.py:13-15
Timestamp: 2024-11-12T14:48:17.141Z
Learning: Adding `cx.tower.reference.mixin` to the class `CxTowerPlanLine` in `cetmix_tower_server/models/cx_tower_plan_line.py` does not require updating the `_compute_variable_ids` method.
cetmix_tower_server/models/cx_tower_variable.py (1)
Learnt from: ivs-cetmix
PR: cetmix/cetmix-tower#129
File: cetmix_tower_server/models/cx_tower_plan_line.py:13-15
Timestamp: 2024-11-12T14:48:17.141Z
Learning: Adding `cx.tower.reference.mixin` to the class `CxTowerPlanLine` in `cetmix_tower_server/models/cx_tower_plan_line.py` does not require updating the `_compute_variable_ids` method.
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
cetmix_tower_server/security/cx_tower_variable_security.xml (2)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_plan_line_security.xml:38-50
Timestamp: 2024-11-20T22:04:20.044Z
Learning: In `cetmix_tower_server/security/*.xml` files, when defining access control rules for Odoo models, if the intention is to grant full permissions (read, create, write, unlink) within the domain specified, it is acceptable to omit the `perm_*` fields and rely on default permission values.
🔇 Additional comments (14)
cetmix_tower_server/static/description/index.html (3)
472-472: Image URLs updated to point to the stable branch.
The image URLs have been updated from the development branch (14.0-dev) to the stable branch (14.0), which is the correct approach for production documentation.
Also applies to: 1027-1027, 1160-1160, 1258-1258, 1265-1265, 1295-1295
413-413: Documentation content improvements for clarity and readability.
The text content has been refined to:
- Improve sentence structure and clarity
- Fix line wrapping for better readability
- Add more detailed explanations for technical concepts
- Enhance formatting consistency
Also applies to: 485-487, 551-552, 563-564, 602-604, 694-695, 759-760, 821-822, 826-827, 839-840, 912-914, 917-918, 920-921, 964-967, 1031-1033, 1074-1075, 1127-1128, 1135-1136, 1195-1200, 1204-1205, 1214-1216, 1236-1237, 1260-1263
1282-1282: Update the GitHub issue template URL.
The GitHub issue template URL has been updated to reference the correct version and module name.
cetmix_tower_server/models/cx_tower_variable.py (1)
9-9: Ensure the new mixin is integrated correctly.
By adding "cx.tower.access.mixin" to "_inherit," you’re extending access control to TowerVariable. Verify that any new or overridden methods from the mixin do not conflict with the existing logic, and ensure all places where tower variable objects are accessed are tested accordingly.
✅ Verification successful
Let me gather information about the mixins and their usage to verify the integration.
Let me check the actual implementation of the mixins and their methods to verify potential conflicts.
Integration of cx.tower.access.mixin is properly implemented
The verification shows that the integration is correct and safe:
- The access mixin only adds a single field
access_levelwith its supporting methods - There are no method conflicts with the reference mixin or the TowerVariable class
- The mixin is already used by several other models in the codebase, indicating established pattern
- The access control is properly scoped to root and manager groups
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for the mixin definitions
ast-grep --pattern 'class $_ {
_name = "cx.tower.access.mixin"
$$$
}'
# Also search for reference mixin
ast-grep --pattern 'class $_ {
_name = "cx.tower.reference.mixin"
$$$
}'
# Look for any potential method conflicts
rg -l "cx.tower.access.mixin" -g "*.py"
# Search for usage of TowerVariable
ast-grep --pattern 'class $_ {
_name = "cx.tower.variable"
$$$
}'
Length of output: 600
Script:
#!/bin/bash
# Check the access mixin implementation
cat cetmix_tower_server/models/cx_tower_access_mixin.py
# Check the reference mixin implementation
rg -p "class.*Reference.*Mixin" -g "*.py" -A 10
# Check the TowerVariable implementation
cat cetmix_tower_server/models/cx_tower_variable.py
Length of output: 3232
cetmix_tower_server/__manifest__.py (1)
27-27: Confirm that the new security file loads in the correct sequence.
Adding "security/cx_tower_variable_security.xml" is appropriate to declare new access rules. Ensure that it does not overwrite or conflict with other security declarations. It's good practice to confirm that groups or record rules from “cx_tower_variable_security.xml” do not unintentionally restrict or grant access.
✅ Verification successful
Let me gather information about the security file and its dependencies.
Let me check the manifest file to understand the security file loading sequence.
Based on the manifest file content and security files examined, I can now provide the final response:
Security file sequence is correctly ordered and follows proper dependency chain
The security file "cx_tower_variable_security.xml" is loaded after the base security configurations (groups and access rights) and follows a logical sequence:
- Groups definition (cetmix_tower_server_groups.xml)
- Base access rights (ir.model.access.csv)
- Model-specific security rules, including the variable security file
The variable security rules align with the existing three-tier access pattern (user/manager/root) and don't conflict with other security declarations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the content of the security file and other security-related files
fd "security.*\.xml" | xargs cat
# Look for any other security-related files in the manifest
rg "security.*\.xml" -A 5 -B 5 ".*__manifest__.py"
Length of output: 16219
Script:
#!/bin/bash
# Check the manifest file content
cat cetmix_tower_server/__manifest__.py
Length of output: 2385
cetmix_tower_server/models/cx_tower_variable_value.py (1)
15-15: Double-check the side effects of adding "cx.tower.access.mixin".
Similar to TowerVariable, inheriting "cx.tower.access.mixin" can introduce new access or constraints. Verify any changes to create, read, write, or unlink operations of TowerVariableValue. Testing coverage for these operations, especially for users with restricted privileges, is recommended.
✅ Verification successful
Let me gather information about the impact of adding the access mixin by analyzing the codebase.
Let me search for more information about how this access mixin is used and its impact on operations.
Based on the analysis of the codebase, I can now provide a final response about the impact of adding the access mixin:
Adding "cx.tower.access.mixin" introduces proper access control constraints
The access mixin adds a three-level access control system (User=1, Manager=2, Root=3) with corresponding security rules:
- Records with access_level=1 are readable by users
- Records with access_level≤2 are readable by managers
- All records are accessible by root users
- Default access_level is "2" (manager)
The security rules are properly implemented through ir.rule records in security XML files, ensuring consistent access control across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the content of the access mixin file
fd "cx_tower_access_mixin.py" --exec cat {}
# Find usage and inheritance of the access mixin
ast-grep --pattern 'class $_ (models.$_, cx.tower.access.mixin)'
# Look for security-related methods or access rules in the mixin
rg -A 5 "def (check_access|_check_access|has_access)"
Length of output: 1288
Script:
#!/bin/bash
# Find files that import the access mixin
rg "from.*cx.tower.access.mixin.*import" -l
# Look for access_level field usage in security rules or methods
rg "access_level" -A 5
# Check security rules in XML files
fd ".xml" --exec rg -l "record.*model.*ir.rule" {} \;
Length of output: 63132
cetmix_tower_server/tests/test_variable.py (4)
335-385: Thorough and well-structured test for server-based subscription access.
This new test explicitly verifies that only subscribed users can read private variable values. The steps are clear and properly exercise both subscribing and unsubscribing flows. Good job ensuring that unsubscribed users trigger an AccessError.
Line range hint 390-464: Comprehensive coverage for private vs. global variable access logic.
These lines verify that unsubscribed users can’t read private variables, while global ones remain accessible. Removing all security groups from the user and then re-adding them to test different role-based permissions is a solid approach. The logic covered in lines 454–464 ensures that attempts to create variable values without the proper group raise AccessError, reinforcing security.
Line range hint 467-499: Tests for manager-level creation of new variables and values are properly structured.
Lines 467–499 confirm that a manager can create both global and private variables, reflecting the updated access logic. This scenario is crucial to ensure mid-level admins (manager group) have the correct level of privileges, without requiring the root group.
504-515: Validation that unsubscribing removes private variable access.
This block shows how unsubscribing from a server leads to an AccessError when trying to read private variable values again, confirming that your access control logic is enforced. Adding the user to the root group afterward further ensures that root can always access relevant variables.
cetmix_tower_server/security/cx_tower_variable_security.xml (1)
23-24: LGTM: Root access rule is correctly implemented
The root access rule provides full access using the standard Odoo pattern [(1, '=', 1)].
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
8-15: LGTM: User access rule domain is well structured
The domain correctly combines:
- Global access check
- Access level restriction
- Server subscription verification
cetmix_tower_server/views/cx_tower_variable_value_view.xml (1)
Line range hint 8-26: LGTM: Tree view is well structured
The tree view implementation is solid with:
- Appropriate use of optional and decoration attributes
- Well-defined readonly conditions using attrs
- Clear field organization
cetmix_tower_server/views/cx_tower_variable_view.xml (1)
92-92: LGTM! Search view changes look good
The addition of the access_level field to the search view is well-placed and aligns with the PR's objective of enhancing access control mechanisms.
… data usage
This commit addresses comments by improving the creation and usage of test data in the following ways:
- Replaced demo data usage:
- Removed references to `demo` data (e.g., `self.env.ref("cetmix_tower_server.tag_staging")`) in test cases.
- Introduced locally created test records for tags with unique names such as `"Test Staging"` and `"Test Production"`.
- Improved reusability:
- Created reusable `self.Tag` model in `setUp` for tag creation in tests.
- Moved common test data (e.g., tags) to `setUp` for easier maintenance and consistency.
- Aligned with TL recommendations:
- Ensured test data names are unique to avoid conflicts with existing data.
- Simplified test cases by reusing common test setup.
Why?
- Ensures independence of test cases from demo data.
- Makes tests more maintainable and aligned with best practices.
- Resolves all comments raised in the PR review.
Task: 4212
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cetmix_tower_server/tests/test_plan.py (1)
316-316: Tag usage consistency.Again, consistently switching to
[self.tag_test_staging.id]is a good move. Ensure that any leftover references totag_stagingin other tests are also updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cetmix_tower_server/tests/common.py(2 hunks)cetmix_tower_server/tests/test_plan.py(3 hunks)cetmix_tower_server/tests/test_server.py(2 hunks)
🔇 Additional comments (7)
cetmix_tower_server/tests/common.py (2)
18-22: Well-structured tag creation logic.
This approach creates test-specific tags directly, avoiding reliance on external references. This makes the tests more self-contained and reliable. Great job!
158-158: Ensure tag references align with intended test scenarios.
Using [self.tag_test_staging.id] is consistent with the newly introduced tags. Verify all references in other tests are also updated to guarantee consistency across the test suite.
cetmix_tower_server/tests/test_server.py (1)
Line range hint 10-42: Tag assignment consistency and clarity.
Referencing self.tag_test_production.id ensures the server instance is associated with the correct tag in a self-contained way rather than relying on environment references. This change aligns with the tagging approach introduced in common.py. Good job!
cetmix_tower_server/tests/test_plan.py (4)
277-277: Tag ID assignment best practice.
Replacing environment references with [self.tag_test_staging.id] improves maintainability by ensuring that the test uses self-defined tags. Make sure related references in other steps remain in sync.
430-430: Unified tag reference approach.
Using [self.tag_test_staging.id] strengthens test independence from environment data.
435-435: Production tag alignment.
This maintains consistency with the newly introduced production tag. Looks good!
440-440: Consistent staging tag usage.
All references to staging tags now rely on the same creation logic in common.py, which is a good approach for clarity and maintenance.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cetmix_tower_server/tests/test_variable.py (3)
336-390: LGTM! Well-structured test with comprehensive coverage.The test method effectively validates the subscription-based access control mechanism through multiple scenarios. Consider these minor improvements:
- Split long lines to comply with PEP8 (88 characters):
- with self.assertRaises(AccessError, msg="User should not have access to private variable"): + with self.assertRaises( + AccessError, + msg="User should not have access to private variable" + ):🧰 Tools
🪛 Ruff (0.8.2)
368-368: Line too long (99 > 88)
(E501)
387-387: Line too long (105 > 88)
(E501)
336-338: Enhance test documentation for better maintainability.Consider adding more detailed docstring that explains:
- The purpose of subscription-based access control
- The test scenarios being covered
- Expected behavior for each scenario
Example:
- """Test variable access based on server subscription""" + """Test variable access control based on server subscription status. + + Test scenarios: + 1. Verify access is denied without subscription + 2. Verify access is granted after subscribing to server + 3. Verify access is revoked after unsubscribing from server + """
339-348: Consider improving test isolation with setup methods.Both test methods create similar test data (private variables and values). Consider moving this common setup to a helper method or the
setUpmethod to improve maintainability and reduce code duplication.Example:
def _create_private_variable(self, server): """Helper method to create a private variable and its value.""" variable = self.Variable.create({"name": "Private Variable"}) return self.VariableValue.create({ "variable_id": variable.id, "server_id": server.id, "value_char": "Private Value", })Also applies to: 395-404
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cetmix_tower_server/tests/test_variable.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
cetmix_tower_server/tests/test_variable.py
368-368: Line too long (99 > 88)
(E501)
387-387: Line too long (105 > 88)
(E501)
🔇 Additional comments (1)
cetmix_tower_server/tests/test_variable.py (1)
Line range hint 394-513: LGTM! Clear and comprehensive test coverage.
The modifications improve readability through better comments and variable names while maintaining thorough test coverage of access control scenarios.
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
b1233c3 to
727ca6c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cetmix_tower_server/tests/test_variable.py (3)
335-396: Consider enhancing test coverage and removing CI/CD workaround.The test method could be improved in several ways:
- Test multiple users with different access levels
- Test multiple variables with different access settings
- Remove the CI/CD workaround and fix the underlying issue
Consider refactoring the test to be more comprehensive:
def test_variable_value_server_subscription(self): """Test variable access based on server subscription""" server = self.server_test_1 + # Test with multiple users + users = { + 'basic_user': self.user_bob, + 'manager': self.user_manager, + } - # Creating a private variable - variable_private = self.Variable.create({"name": "Private Variable"}) - variable_private_value = self.VariableValue.create( - { - "variable_id": variable_private.id, - "server_id": server.id, - "value_char": "Private Value", - } - ) + # Create multiple variables with different access levels + variables = { + 'private': self.Variable.create({ + "name": "Private Variable", + "access_level": "private" + }), + 'restricted': self.Variable.create({ + "name": "Restricted Variable", + "access_level": "restricted" + }) + } + + values = { + 'private': self.VariableValue.create({ + "variable_id": variables['private'].id, + "server_id": server.id, + "value_char": "Private Value" + }), + 'restricted': self.VariableValue.create({ + "variable_id": variables['restricted'].id, + "server_id": server.id, + "value_char": "Restricted Value" + }) + } - # Block for safe execution on CICD - try: - self.assertEqual( - variable_private_value_as_bob.value_char, - "Private Value", - msg="User should have access to private variables after subscription", - ) - except AccessError: - # If the test is run on CI/CD, we bypass the limitation - variable_private_value_as_bob_sudo = variable_private_value_as_bob.sudo() - self.assertEqual( - variable_private_value_as_bob_sudo.value_char, - "Private Value", - msg="User should have access to private variables after subscription", - ) + # Test access for each user and variable combination + for user_type, user in users.items(): + for var_type, value in values.items(): + with self.subTest(user=user_type, variable=var_type): + value_as_user = value.with_user(user) + + # Test: No access without subscription + with self.assertRaises(AccessError): + value_as_user.read([]) + + # Subscribe user + server.message_subscribe([user.partner_id.id]) + value_as_user.invalidate_cache() + + # Test: Access granted after subscription + self.assertEqual( + value_as_user.value_char, + f"{var_type.title()} Value", + f"{user_type} should have access after subscription" + ) + + # Unsubscribe user + server.message_unsubscribe([user.partner_id.id]) + value_as_user.invalidate_cache() + + # Test: Access revoked after unsubscribe + with self.assertRaises(AccessError): + value_as_user.read([])
Line range hint
400-519: Consider splitting the test method into smaller, focused test methods.The test method is quite long and tests multiple scenarios. Consider splitting it into smaller, focused test methods for better maintainability and clarity:
test_global_variable_accesstest_private_variable_accesstest_variable_creation_permissionstest_subscription_based_accessHere's an example of how to split the first scenario:
def test_global_variable_access(self): """Test access rules for global variables""" # Create global variable variable_global = self.Variable.create({"name": "Global Variable"}) variable_global_value = self.VariableValue.create({ "variable_id": variable_global.id, "is_global": True, "value_char": "Global Value" }) # Setup user user_bob = self.user_bob self.remove_from_group(user_bob, [ "cetmix_tower_server.group_user", "cetmix_tower_server.group_manager", "cetmix_tower_server.group_root" ]) self.add_to_group(user_bob, "cetmix_tower_server.group_user") # Test access variable_global_value_as_bob = variable_global_value.with_user(user_bob) self.assertEqual( variable_global_value_as_bob.value_char, "Global Value", msg="Users should always have access to global variables" )Additionally, consider creating helper methods for common operations like cache invalidation and group management to reduce code duplication.
Line range hint
335-519: Overall test coverage looks good but could be enhanced.The test coverage for variable access control is comprehensive, testing various scenarios including:
- Server subscription-based access
- Global vs private variable access
- User permission levels
- Variable creation permissions
However, consider adding tests for:
- Edge cases in access control (e.g., concurrent subscription changes)
- Access level field mentioned in PR objectives
- Performance implications of cache invalidation
Consider creating a base test class
TestTowerVariableAccessthat provides common setup and helper methods for all variable access-related tests. This would improve code reuse and maintainability.
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
727ca6c to
9800c1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cetmix_tower_server/tests/test_variable.py (4)
Line range hint
362-372: Consider adding assertions for group removal.While the group management setup is correct, consider adding assertions to verify that the user is actually removed from all groups before proceeding with the tests.
self.remove_from_group( user_bob, [ "cetmix_tower_server.group_user", "cetmix_tower_server.group_manager", "cetmix_tower_server.group_root", ], ) +# Verify user is not in any of the groups +for group in ["group_user", "group_manager", "group_root"]: + self.assertFalse( + user_bob.has_group(f"cetmix_tower_server.{group}"), + f"User should not be in {group}" + )
389-401: Consider adding error message validation.While the subscription-based access testing is thorough, consider validating the error message when access is denied to ensure the correct error is being raised.
with self.assertRaises(AccessError): - variable_private_value_as_bob.read([]) + with self.assertRaisesRegex(AccessError, "Access Denied"): + variable_private_value_as_bob.read([])
Line range hint
414-452: Consider adding update and delete test cases.While the creation permissions are well tested for the manager group, consider adding test cases for updating and deleting variables to ensure complete CRUD operation coverage.
self.assertEqual( variable_value_new_private_as_bob.value_char, "New Private Value", "Must return New Private Value", ) +# Test update permissions +variable_value_new_private_as_bob.write({"value_char": "Updated Private Value"}) +self.assertEqual( + variable_value_new_private_as_bob.value_char, + "Updated Private Value", + "Must return Updated Private Value" +) + +# Test delete permissions +variable_value_new_private_as_bob.unlink() +self.assertFalse( + self.VariableValue.search([("id", "=", variable_value_new_private_as_bob.id)]), + "Variable value should be deleted" +)
459-465: Consider adding complete CRUD testing for root access.While basic read access is tested for the root group, consider adding complete CRUD operation testing to ensure root users have full access.
self.assertEqual(variable_new_global_value_as_bob.value_char, "Global Value 1") + +# Test root can update any variable +variable_private_value_as_bob.write({"value_char": "Root Updated Value"}) +self.assertEqual( + variable_private_value_as_bob.value_char, + "Root Updated Value", + "Root should be able to update any variable" +) + +# Test root can delete any variable +variable_private_value_as_bob.unlink() +self.assertFalse( + self.VariableValue.search([("id", "=", variable_private_value_as_bob.id)]), + "Root should be able to delete any variable" +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cetmix_tower_server/tests/test_variable.py(7 hunks)
🔇 Additional comments (5)
cetmix_tower_server/tests/test_variable.py (5)
Line range hint 339-350: LGTM! Clear test setup for private variables.
The test setup properly creates a private variable and assigns it to a server, providing a good foundation for testing access controls.
Line range hint 351-361: LGTM! Good test coverage for global variables.
The test setup correctly creates a global variable for comparison testing, which will help validate the difference in access rules between global and private variables.
375-388: LGTM! Comprehensive access control testing for global variables.
The test properly validates both positive (global variable access) and negative (private variable access) scenarios for users in the basic user group.
Line range hint 403-413: LGTM! Good coverage of creation restrictions.
The test properly validates that users cannot create new variable values without appropriate permissions.
453-458: LGTM! Proper validation of access removal.
The test correctly verifies that unsubscribing removes access to private variables.
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
9800c1e to
7d161db
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cetmix_tower_server/tests/test_variable.py (3)
740-767: Enhance test documentation and coverage.The test method effectively verifies read access for different user groups, but could be improved in the following areas:
- Document the meaning of access levels in the docstring (1=private, 2=manager, 3=root)
- Add test cases for variable creation permissions per access level
def test_variable_access_rules(self): - """Test access rules for `cx_tower_variable`.""" + """Test access rules for `cx_tower_variable`. + + Access Levels: + 1: Private - Accessible by all users + 2: Manager - Accessible by managers and root users + 3: Root - Accessible only by root users + """Also, consider adding these test cases:
# Test variable creation permissions with self.assertRaises(AccessError): self.Variable.with_user(user_bob).create({"name": "Test", "access_level": "2"})🧰 Tools
🪛 Ruff (0.8.2)
742-742: Line too long (98 > 88)
(E501)
743-743: Line too long (98 > 88)
(E501)
744-744: Line too long (92 > 88)
(E501)
748-748: Line too long (113 > 88)
(E501)
753-753: Line too long (92 > 88)
(E501)
754-754: Line too long (99 > 88)
(E501)
755-755: Line too long (93 > 88)
(E501)
760-760: Line too long (95 > 88)
(E501)
761-761: Line too long (96 > 88)
(E501)
768-817: Enhance test coverage for variable value permissions.The test method effectively verifies read access, but could be improved in the following areas:
- Add test cases for variable value modification permissions
- Add cleanup of test data to prevent interference with other tests
Add these test cases:
# Test modification permissions with self.assertRaises(AccessError): variable_private_value_as_bob.write({"value_char": "Modified Value"}) # Test creation permissions with self.assertRaises(AccessError): self.VariableValue.with_user(user_bob).create({ "variable_id": variable_private.id, "server_id": server.id, "value_char": "New Value" })Add cleanup in tearDown:
def tearDown(self): """Clean up test data.""" super().tearDown() self.VariableValue.search([]).unlink() self.Variable.search([]).unlink()🧰 Tools
🪛 Ruff (0.8.2)
773-773: Line too long (98 > 88)
(E501)
775-775: Line too long (103 > 88)
(E501)
778-778: Line too long (96 > 88)
(E501)
780-780: Line too long (96 > 88)
(E501)
785-785: Line too long (113 > 88)
(E501)
791-791: Line too long (110 > 88)
(E501)
802-802: Line too long (113 > 88)
(E501)
809-809: Line too long (116 > 88)
(E501)
816-816: Line too long (109 > 88)
(E501)
742-816: Fix line length violations.Multiple lines exceed the 88-character limit. Consider breaking them into multiple lines for better readability.
Example fixes:
- self.remove_from_group(user_bob, ["cetmix_tower_server.group_manager", "cetmix_tower_server.group_root"]) + self.remove_from_group( + user_bob, + ["cetmix_tower_server.group_manager", "cetmix_tower_server.group_root"] + ) - variable_private = self.Variable.create({"name": "Private Variable", "access_level": "1"}) + variable_private = self.Variable.create({ + "name": "Private Variable", + "access_level": "1" + })🧰 Tools
🪛 Ruff (0.8.2)
742-742: Line too long (98 > 88)
(E501)
743-743: Line too long (98 > 88)
(E501)
744-744: Line too long (92 > 88)
(E501)
748-748: Line too long (113 > 88)
(E501)
753-753: Line too long (92 > 88)
(E501)
754-754: Line too long (99 > 88)
(E501)
755-755: Line too long (93 > 88)
(E501)
760-760: Line too long (95 > 88)
(E501)
761-761: Line too long (96 > 88)
(E501)
773-773: Line too long (98 > 88)
(E501)
775-775: Line too long (103 > 88)
(E501)
778-778: Line too long (96 > 88)
(E501)
780-780: Line too long (96 > 88)
(E501)
785-785: Line too long (113 > 88)
(E501)
791-791: Line too long (110 > 88)
(E501)
802-802: Line too long (113 > 88)
(E501)
809-809: Line too long (116 > 88)
(E501)
816-816: Line too long (109 > 88)
(E501)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/tests/test_variable.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server/security/cx_tower_variable_security.xml
🧰 Additional context used
🪛 Ruff (0.8.2)
cetmix_tower_server/tests/test_variable.py
742-742: Line too long (98 > 88)
(E501)
743-743: Line too long (98 > 88)
(E501)
744-744: Line too long (92 > 88)
(E501)
748-748: Line too long (113 > 88)
(E501)
753-753: Line too long (92 > 88)
(E501)
754-754: Line too long (99 > 88)
(E501)
755-755: Line too long (93 > 88)
(E501)
760-760: Line too long (95 > 88)
(E501)
761-761: Line too long (96 > 88)
(E501)
773-773: Line too long (98 > 88)
(E501)
775-775: Line too long (103 > 88)
(E501)
778-778: Line too long (96 > 88)
(E501)
780-780: Line too long (96 > 88)
(E501)
785-785: Line too long (113 > 88)
(E501)
791-791: Line too long (110 > 88)
(E501)
802-802: Line too long (113 > 88)
(E501)
809-809: Line too long (116 > 88)
(E501)
816-816: Line too long (109 > 88)
(E501)
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
7d161db to
595d32a
Compare
… data usage
This commit addresses comments by improving the creation and usage of test data in the following ways:
- Replaced demo data usage:
- Removed references to `demo` data (e.g., `self.env.ref("cetmix_tower_server.tag_staging")`) in test cases.
- Introduced locally created test records for tags with unique names such as `"Test Staging"` and `"Test Production"`.
- Improved reusability:
- Created reusable `self.Tag` model in `setUp` for tag creation in tests.
- Moved common test data (e.g., tags) to `setUp` for easier maintenance and consistency.
- Aligned with TL recommendations:
- Ensured test data names are unique to avoid conflicts with existing data.
- Simplified test cases by reusing common test setup.
Why?
- Ensures independence of test cases from demo data.
- Makes tests more maintainable and aligned with best practices.
- Resolves all comments raised in the PR review.
Task: 4212
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
595d32a to
58b5bbf
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cetmix_tower_server/tests/test_variable.py (3)
Line range hint
340-471: Good test coverage, but could benefit from better organization.The test method thoroughly covers access control scenarios, but could be improved by:
- Adding docstring sections for setup, test scenarios, and expected outcomes
- Using subtests to better organize different test scenarios
- Adding more descriptive assertion messages
Consider refactoring like this:
def test_variable_value_access(self): - """Test access rules for variable values""" + """Test access rules for variable values. + + Test cases: + 1. Global variable access for basic user + 2. Private variable access with/without subscription + 3. Manager access to create variables + 4. Root access to all variables + + For each case, verifies: + - Read access to values + - Create/Write permissions + - Access level restrictions + """ server = self.server_test_1 # Create variables with different access levels variable_private = self.Variable.create( {"name": "Private Variable", "access_level": "1"} ) + # Use subtests for better organization + with self.subTest("Test global variable access for basic user"): variable_private_value = self.VariableValue.create({ "variable_id": variable_private.id, "server_id": server.id, "value_char": "Private Value", })
753-799: Add edge cases and improve variable naming.While the test covers basic access rules well, consider these improvements:
- Test edge cases (e.g., changing access levels after creation)
- Use more descriptive variable names (e.g.,
variable_access_level_privateinstead ofvariable_private)- Add tests for variable modification attempts
Consider adding these test cases:
# Test changing access level with self.assertRaises(AccessError): variable_private.with_user(user_bob).write({"access_level": "2"}) # Test variable modification with self.assertRaises(AccessError): variable_manager.with_user(user_bob).write({"name": "Modified Name"})
10-10: Add type hints and test constants.Consider these overall improvements to the test file:
- Define constants for access levels
- Add type hints for better code maintainability
Add at the top of the file:
from typing import List, Dict, Any # Access level constants ACCESS_LEVEL_PRIVATE = "1" ACCESS_LEVEL_MANAGER = "2" ACCESS_LEVEL_ROOT = "3" class TestTowerVariable(TestTowerCommon): """Testing variables and variable values.""" def setUp(self) -> None: super().setUp()
🛑 Comments failed to post (1)
cetmix_tower_server/tests/test_variable.py (1)
800-872: 🛠️ Refactor suggestion
Reduce code duplication and add cleanup.
The test method overlaps significantly with
test_variable_value_access. Consider:
- Creating shared setup methods for common test data
- Adding cleanup to prevent test interference
- Focusing this test on unique scenarios not covered in
test_variable_value_accessConsider refactoring like this:
def setUp(self): super().setUp() self.test_server = self.server_test_1 self.test_variables = self._create_test_variables() self.test_user = self._create_test_user() def tearDown(self): # Clean up created records self.test_variables.unlink() self.test_user.unlink() super().tearDown() def _create_test_variables(self): return self.Variable.create([ { "name": "Private Variable", "access_level": "1" }, { "name": "Global Variable", "access_level": "1" } ])
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cetmix_tower_server/tests/test_variable.py (3)
339-342: Consider using constants for access levels instead of magic stringsReplace hardcoded access level values with meaningful constants to improve maintainability and prevent typos.
+# At the top of the file +ACCESS_LEVEL_PRIVATE = "1" +ACCESS_LEVEL_MANAGER = "2" +ACCESS_LEVEL_ROOT = "3" - {"name": "Private Variable", "access_level": "1"} + {"name": "Private Variable", "access_level": ACCESS_LEVEL_PRIVATE}
374-399: Consider using subtests for better test organizationThe test method could be better organized using subtests to improve readability and error reporting.
- # Add user_bob to group_user - self.add_to_group(user_bob, "cetmix_tower_server.group_user") - - # Check access to global values for group_user - variable_global_value_as_bob = variable_global_value.with_user(user_bob) - self.assertEqual( - variable_global_value_as_bob.value_char, - "Global Value", - msg="User must be able to access global values", - ) + with self.subTest("Test access for group_user"): + self.add_to_group(user_bob, "cetmix_tower_server.group_user") + + # Check access to global values + variable_global_value_as_bob = variable_global_value.with_user(user_bob) + self.assertEqual( + variable_global_value_as_bob.value_char, + "Global Value", + msg="User in group_user should have access to global values", + )
754-762: Use the same access level constants hereFor consistency, use the same access level constants as suggested earlier.
variable_private = self.Variable.create( - {"name": "Private Variable", "access_level": "1"} + {"name": "Private Variable", "access_level": ACCESS_LEVEL_PRIVATE} ) variable_manager = self.Variable.create( - {"name": "Manager Variable", "access_level": "2"} + {"name": "Manager Variable", "access_level": ACCESS_LEVEL_MANAGER} ) variable_root = self.Variable.create( - {"name": "Root Variable", "access_level": "3"} + {"name": "Root Variable", "access_level": ACCESS_LEVEL_ROOT} )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/tests/test_variable.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server/security/cx_tower_variable_security.xml
🔇 Additional comments (1)
cetmix_tower_server/tests/test_variable.py (1)
Line range hint 339-871: Overall implementation looks good!
The test implementation provides comprehensive coverage of access control scenarios for both variables and variable values. The test methods are well-structured and cover important cases including:
- Access levels for different user groups
- Server subscription effects on access
- Global vs. private variable access
- Creation and modification permissions
While there are some suggestions for improvement, the core implementation is solid and reliable.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cetmix_tower_server/tests/test_variable.py (1)
799-871: 🛠️ Refactor suggestionConsider consolidating duplicate test logic.
This test method shares similar setup and test logic with
test_variable_value_access. Consider:
- Extracting common setup into helper methods
- Focusing this test on unique scenarios not covered by
test_variable_value_access
🧹 Nitpick comments (3)
cetmix_tower_server/tests/test_variable.py (3)
Line range hint
339-470: Consider enhancing test coverage with edge cases.The test could be more comprehensive by including:
- Testing access with invalid access levels
- Testing access when a user belongs to multiple groups
- Testing access when server subscription is removed and re-added
755-761: Use constants for access levels and consider adding setup methods.
- Replace hard-coded access levels with constants
- Consider extracting common setup code into a setUp method
- {"name": "Private Variable", "access_level": "1"} - {"name": "Manager Variable", "access_level": "2"} - {"name": "Root Variable", "access_level": "3"} + {"name": "Private Variable", "access_level": ACCESS_LEVEL_PRIVATE} + {"name": "Manager Variable", "access_level": ACCESS_LEVEL_MANAGER} + {"name": "Root Variable", "access_level": ACCESS_LEVEL_ROOT}
931-952: Optimize dictionary comparison and improve test readability.Consider these improvements:
- Use
assertDictEqualfor comparing the entire dictionary at once- Make comments more concise by removing redundant information
- used_variables = { - "User Variable": variable_user_value.value_char, - "Manager Variable": variable_manager_value.value_char, - "Root Variable": variable_root_value.value_char, - } - - # Let's make sure that all variables have been used - self.assertEqual( - used_variables["User Variable"], - "User Value", - "User variable must be used during execution", - ) - self.assertEqual( - used_variables["Manager Variable"], - "Manager Value", - "Manager variable must be used during execution", - ) - self.assertEqual( - used_variables["Root Variable"], - "Root Value", - "Root variable must be used during execution", - ) + expected_values = { + "User Variable": "User Value", + "Manager Variable": "Manager Value", + "Root Variable": "Root Value", + } + actual_values = { + "User Variable": variable_user_value.value_char, + "Manager Variable": variable_manager_value.value_char, + "Root Variable": variable_root_value.value_char, + } + self.assertDictEqual(actual_values, expected_values, "All variables must be used during execution")
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
cetmix_tower_server/tests/test_variable.py (4)
Line range hint
365-400: Consider adding edge cases to strengthen test coverage.While the current test coverage is good, consider adding these edge cases:
- Test access after removing subscription while keeping group membership
- Test access with multiple subscriptions
- Test access with invalid/deleted variables
Line range hint
412-449: Add validation for error cases in manager access testing.Consider adding tests for these scenarios:
- Manager attempting to create a root-level variable (should fail)
- Manager attempting to modify a root-level variable (should fail)
- Manager attempting to delete variables of different access levels
452-471: Enhance root access testing coverage.Consider adding these test cases:
- Root creating variables at all access levels
- Root modifying variables at all access levels
- Root deleting variables at all access levels
- Root managing subscriptions and verifying impact
753-953: Enhance test method documentation.While the test methods are well-structured, consider improving documentation by:
- Adding docstring parameters and return values
- Documenting test prerequisites and assumptions
- Adding comments explaining complex assertions
- Including examples of expected behavior in docstrings
Example improvement:
def test_variable_access_rules(self): - """Test access rules for `cx_tower_variable`.""" + """Test access rules for `cx_tower_variable`. + + Prerequisites: + - User exists in the system + - Security groups are properly configured + + Test cases: + 1. User level access (access_level=1) + - Can view private variables + - Cannot view manager/root variables + 2. Manager level access (access_level=2) + - Can view private/manager variables + - Cannot view root variables + 3. Root level access (access_level=3) + - Can view all variables + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(2 hunks)cetmix_tower_server/models/cx_tower_variable.py(1 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(1 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_variable.py(8 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- cetmix_tower_server/manifest.py
- cetmix_tower_server/models/cx_tower_variable.py
- cetmix_tower_server/models/cx_tower_variable_value.py
- cetmix_tower_server/static/description/index.html
- cetmix_tower_server/README.rst
- cetmix_tower_server/views/cx_tower_variable_view.xml
There was a problem hiding this comment.
access_levelfield in thecx.tower.variable.valuemodel currently is not used.- Variable values are used in several models, we need to add rules for all of them. Check this PR as a reference: https://github.com/cetmix/cetmix-tower/pull/98/files
- There is no solution on how a user who is a member of "User" group can render a command or a file that contains variables of "Manager" or "Root" access level.
cetmix_tower_server/security/cx_tower_variable_value_security.xml
Outdated
Show resolved
Hide resolved
cetmix_tower_server/security/cx_tower_variable_value_security.xml
Outdated
Show resolved
Hide resolved
cetmix_tower_server/security/cx_tower_variable_value_security.xml
Outdated
Show resolved
Hide resolved
@ivs-cetmix |
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
96838e7 to
c8ebc37
Compare
…eature Signed-off-by: Dmitry Meita <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cetmix_tower_server/tests/test_variable.py (1)
Line range hint
340-400: Fix AccessError in test_variable_value_access.The test is failing with AccessError on line 381. This suggests that the user access rule in
cx_tower_variable_value_security.xmlmight be too restrictive. Ensure that the access rule allows users to read global values without requiring server subscription.Update the user access rule to handle global values:
<field name="domain_force"> - [('server_id.message_partner_ids', 'in', [user.partner_id.id])] + ['|', + ('is_global', '=', True), + ('server_id.message_partner_ids', 'in', [user.partner_id.id])] </field>🧰 Tools
🪛 GitHub Actions: tests
[error] 381-381: AccessError: Due to security restrictions, not allowed to access 'Cetmix Tower Variable Values' (cx.tower.variable.value) records in test_variable_value_access
🧹 Nitpick comments (1)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
26-28: Fix XML formatting.The
nameattribute should be on the same line for consistency.Apply this diff to fix the formatting:
- <field - name="groups" - eval="[(4, ref('cetmix_tower_server.group_manager'))]" + <field name="groups" eval="[(4, ref('cetmix_tower_server.group_manager'))]"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(2 hunks)cetmix_tower_server/models/cx_tower_access_mixin.py(1 hunks)cetmix_tower_server/models/cx_tower_variable.py(1 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(1 hunks)cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(1 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/tests/test_variable.py(8 hunks)cetmix_tower_server/views/cx_tower_variable_value_view.xml(4 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- cetmix_tower_server/manifest.py
- cetmix_tower_server/models/cx_tower_access_mixin.py
- cetmix_tower_server/models/cx_tower_variable.py
- cetmix_tower_server/models/cx_tower_variable_value.py
- cetmix_tower_server/static/description/index.html
- cetmix_tower_server/views/cx_tower_variable_view.xml
- cetmix_tower_server/security/cx_tower_variable_security.xml
- cetmix_tower_server/views/cx_tower_variable_value_view.xml
- cetmix_tower_server/README.rst
🧰 Additional context used
📓 Learnings (2)
cetmix_tower_server/tests/test_variable.py (2)
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:55:14.571Z
Learning: In the cetmix-tower project, the numeric access levels "1", "2", and "3" used within `cx.tower.access.mixin` are considered standard and are not seen as magic numbers. Replacing them with constants would introduce unnecessary abstraction.
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:53:58.513Z
Learning: The numeric access levels ("1", "2", "3") for `cx.tower.access.mixin` are standard across this codebase. Replacing them with constants is not desired, as it would create an unnecessary abstraction layer and deviate from existing project conventions.
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
🪛 GitHub Actions: tests
cetmix_tower_server/tests/test_variable.py
[error] 381-381: AccessError: Due to security restrictions, not allowed to access 'Cetmix Tower Variable Values' (cx.tower.variable.value) records in test_variable_value_access
[error] 838-838: AccessError: Due to security restrictions, not allowed to access 'Cetmix Tower Variable Values' (cx.tower.variable.value) records in test_variable_value_access_rules
🔇 Additional comments (4)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
32-38: LGTM!The root access rule correctly implements unrestricted access for root users.
cetmix_tower_server/tests/test_variable.py (3)
Line range hint
412-449: LGTM!The test correctly validates manager permissions for creating variables and values.
457-471: LGTM!The test correctly validates root access to all variable values.
874-953: LGTM! Addresses PR comment concern.The test correctly validates that variables of all access levels are used during execution, addressing the concern about how users can render commands containing variables with higher access levels.
cetmix_tower_server/security/cx_tower_variable_value_security.xml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
8-11:⚠️ Potential issueAdd access level condition to user rule.
The user access rule should also check the
access_levelfield to ensure users can only access variables with appropriate access levels.Apply this diff to add the access level check:
<field name="domain_force"> - [('server_id.message_partner_ids', 'in', [user.partner_id.id])] + ['&', + ('access_level', '=', '1'), + ('server_id.message_partner_ids', 'in', [user.partner_id.id])] </field>
🧹 Nitpick comments (1)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
26-29: Fix XML formatting and indentation.The
groupsfield has inconsistent indentation and line breaks.Apply this diff to fix the formatting:
- <field - name="groups" - eval="[(4, ref('cetmix_tower_server.group_manager'))]" - /> + <field name="groups" eval="[(4, ref('cetmix_tower_server.group_manager'))]" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/security/cx_tower_variable_security.xml(1 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cetmix_tower_server/security/cx_tower_variable_security.xml
🧰 Additional context used
📓 Learnings (1)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
🔇 Additional comments (2)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (2)
19-24: LGTM! Manager access rule is well structured.The domain correctly:
- Allows access to global variables
- Restricts access to variables with access levels 1 or 2
- Checks server subscription for non-global variables
32-37: LGTM! Root access rule provides appropriate permissions.The rule correctly grants full access to root users with the domain
[(1, '=', 1)].
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
2 times, most recently
from
c04ea3f to
2097b29
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/security/cx_tower_variable_value_security.xml(1 hunks)cetmix_tower_server/tests/test_variable.py(8 hunks)
🧰 Additional context used
📓 Learnings (2)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
cetmix_tower_server/tests/test_variable.py (2)
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:55:14.571Z
Learning: In the cetmix-tower project, the numeric access levels "1", "2", and "3" used within `cx.tower.access.mixin` are considered standard and are not seen as magic numbers. Replacing them with constants would introduce unnecessary abstraction.
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:53:58.513Z
Learning: The numeric access levels ("1", "2", "3") for `cx.tower.access.mixin` are standard across this codebase. Replacing them with constants is not desired, as it would create an unnecessary abstraction layer and deviate from existing project conventions.
🪛 GitHub Actions: tests
cetmix_tower_server/tests/test_variable.py
[error] 397-397: Test failure in TestTowerVariable.test_variable_value_access: AccessError - Due to security restrictions, unable to access 'Cetmix Tower Variable Values' records
[error] 838-838: Test failure in TestTowerVariable.test_variable_value_access_rules: AccessError - Due to security restrictions, unable to access 'Cetmix Tower Variable Values' records
🔇 Additional comments (5)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (2)
16-32: LGTM! Manager access rule is well-structured.The rule correctly implements:
- Access to global values
- Access to values with access_level <= 2 for subscribed servers
34-40: LGTM! Root access rule provides appropriate permissions.The rule correctly grants full access to root users.
cetmix_tower_server/tests/test_variable.py (3)
753-799: LGTM! Comprehensive test coverage for variable access rules.The test method thoroughly validates access control for variables across different user roles:
- User can only see private variables (access_level = 1)
- Manager can see private and manager variables (access_level <= 2)
- Root can see all variables
800-872: LGTM! Comprehensive test coverage for variable value access rules.The test method thoroughly validates access control for variable values:
- User can access global values and private values after subscription
- Manager can access private values
- Root can access all values
🧰 Tools
🪛 GitHub Actions: tests
[error] 838-838: Test failure in TestTowerVariable.test_variable_value_access_rules: AccessError - Due to security restrictions, unable to access 'Cetmix Tower Variable Values' records
874-953: LGTM! Well-structured test for variable rendering.The test method effectively validates that variables of all access levels are correctly rendered during execution, regardless of user access level. This is crucial for ensuring that commands work correctly even when they contain variables with higher access levels than the executing user.
cetmix_tower_server/security/cx_tower_variable_value_security.xml
Outdated
Show resolved
Hide resolved
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
2097b29 to
a86db46
Compare
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
a86db46 to
1f194df
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
cetmix_tower_server/tests/test_variable.py (4)
Line range hint
353-361: Consider setting access_level for global value.For consistency with the private variable value above, consider setting the access_level for the global value as well.
variable_global_value = self.VariableValue.create( { "variable_id": variable_global.id, "is_global": True, "value_char": "Global Value", + "access_level": "1", } )
379-398: Add a descriptive comment block for the test section.Consider adding a descriptive comment block to clearly outline the test scenarios being covered in this section:
+ # Test scenarios: + # 1. User in group_user cannot access global values + # 2. User in group_user cannot access private values without subscription + # 3. User in group_user can access private values after server subscription
450-469: Consider adding edge cases for root access.The root access tests are good, but consider adding these edge cases:
- Root creating variables with different access levels
- Root modifying variables created by other users
Would you like me to provide example test cases for these scenarios?
798-868: Consider consolidating with test_variable_value_access.This test method has significant overlap with
test_variable_value_access. Consider:
- Merging the common test cases
- Keeping only the unique scenarios in each method
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
28-31: Improve XML formatting for better readability.The
groupsfield attributes are not consistently indented.- <field - name="groups" - eval="[(4, ref('cetmix_tower_server.group_manager'))]" - /> + <field name="groups" eval="[(4, ref('cetmix_tower_server.group_manager'))]" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cetmix_tower_server/security/cx_tower_variable_value_security.xml(1 hunks)cetmix_tower_server/tests/test_variable.py(7 hunks)
🧰 Additional context used
📓 Learnings (2)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
cetmix_tower_server/tests/test_variable.py (2)
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:55:14.571Z
Learning: In the cetmix-tower project, the numeric access levels "1", "2", and "3" used within `cx.tower.access.mixin` are considered standard and are not seen as magic numbers. Replacing them with constants would introduce unnecessary abstraction.
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:53:58.513Z
Learning: The numeric access levels ("1", "2", "3") for `cx.tower.access.mixin` are standard across this codebase. Replacing them with constants is not desired, as it would create an unnecessary abstraction layer and deviate from existing project conventions.
🔇 Additional comments (8)
cetmix_tower_server/tests/test_variable.py (5)
340-349: LGTM! Variable creation with private access level.The implementation correctly sets up a private variable and its value with access_level "1".
Line range hint
366-376: LGTM! Clean user group setup.Good practice to ensure a clean initial state by removing the user from all security groups before testing.
Line range hint
400-447: LGTM! Comprehensive manager access tests.Good coverage of manager-level permissions, testing both global and private variable creation capabilities.
751-797: Previous suggestion about reducing test duplication applies here.
870-949: LGTM! Addresses the PR comment concern.This test effectively addresses the concern raised in the PR comments about rendering variables with different access levels. It verifies that a user can execute commands containing variables of any access level, even if they can't directly access those variables.
cetmix_tower_server/security/cx_tower_variable_value_security.xml (3)
34-39: LGTM! Root access rule is correctly implemented.The rule maintains consistent naming with other rules and correctly provides unrestricted access to root users.
Line range hint
1-43: Verify consistency with TowerVariable security rules.Since there's a new security file
cx_tower_variable_security.xmlfor TowerVariable model, we should ensure that access rules are consistent between both models.✅ Verification successful
Security rules are consistent with TowerVariable model
The verification shows that both TowerVariable and TowerVariableValue models follow the same security pattern:
- User level (access_level = 1)
- Manager level (access_level <= 2)
- Root level (full access with domain [(1, '=', 1)])
The only difference is that TowerVariableValue has additional conditions:
- User/Manager rules check server_id.message_partner_ids
- Manager rule includes is_global flag
These additional conditions make sense as TowerVariableValue needs to be linked to specific servers and supports global values, while TowerVariable defines the variable templates themselves.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Compare access rules between TowerVariable and TowerVariableValue echo "Searching for TowerVariable security rules..." rg -A 10 "model_cx_tower_variable" --glob "*security.xml" echo "Comparing access_level conditions..." rg "access_level" --glob "*security.xml"Length of output: 7729
8-12:⚠️ Potential issueAdd access to global values and clarify higher access level handling.
The current domain is too restrictive and doesn't allow users to access global values. Additionally, there's a concern in the PR comments about how users can render commands/files containing variables with higher access levels.
- Add access to global values:
<field name="domain_force"> - ['&', - ('access_level', '=', '1'), - ('server_id.message_partner_ids', 'in', [user.partner_id.id])] + ['|', + ('is_global', '=', True), + '&', + ('access_level', '=', '1'), + ('server_id.message_partner_ids', 'in', [user.partner_id.id])] </field>
- Let's verify if there are any commands or files that might break with this access rule:
✅ Verification successful
Let me gather more information about how variables are used in commands and templates.
Let me check how variables are used in commands and templates, particularly focusing on access levels.
Let me try a different approach to understand how variables are handled in the system.
Based on the analysis of the codebase, I can now provide a final response regarding the variable access control concern.
Access to global values is safe and properly handled
The review comment suggesting to add access to global values is correct, but the concern about users accessing variables with higher access levels is already handled by the system:
The variable value model has proper access control mechanisms:
- Values are uniquely constrained through
_constraint_global_unique- Global values are computed and managed through
_compute_is_globaland_inverse_is_global- Variable values are properly validated before use in commands/templates
The system safely handles variable access:
- Variables are extracted and validated before use (
get_variables_from_code)- Values are retrieved with proper access checks (
get_variable_values)- Template rendering is done with validated variable values only
Therefore, the suggested modification to allow access to global values is safe:
<field name="domain_force"> - ['&', - ('access_level', '=', '1'), - ('server_id.message_partner_ids', 'in', [user.partner_id.id])] + ['|', + ('is_global', '=', True), + '&', + ('access_level', '=', '1'), + ('server_id.message_partner_ids', 'in', [user.partner_id.id])] </field>🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to variables in command/file templates rg -A 5 "variable" --glob "*.xml" | grep -i "template\|command"Length of output: 6803
Script:
#!/bin/bash # Search for variable-related models and their fields ast-grep --pattern 'model="cx.tower.$_"' | grep -i "variable\|command\|template" # Search for Python files that handle variables rg -l "variable" --glob "*.py"Length of output: 2235
Script:
#!/bin/bash # Check how variables are used in commands and templates rg -A 5 "access_level" cetmix_tower_server/models/cx_tower_command.py cetmix_tower_server/models/cx_tower_file_template.py # Check variable access handling in the variable model rg -A 5 "access_level" cetmix_tower_server/models/cx_tower_variable.py # Check for any validation of variable access levels rg -A 5 "validate|check.*access" cetmix_tower_server/models/cx_tower_variable_value.pyLength of output: 282
Script:
#!/bin/bash # Search for variable value model definition and fields ast-grep --pattern 'class $_VariableValue(models.Model): $$$ ' # Check for any access control or validation methods in variable value model rg -A 10 "def" cetmix_tower_server/models/cx_tower_variable_value.py # Look for variable usage in templates and commands rg -A 5 "get_variable" --glob "*.py"Length of output: 19301
Yes of course. Otherwise variables are barely usable. |
cetmix_tower_server/security/cx_tower_variable_value_security.xml
Outdated
Show resolved
Hide resolved
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
561395d to
a70ce93
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cetmix_tower_server/models/cx_tower_variable_value.py (1)
187-215: Simplify the error message construction.The error message construction using nested dictionary access is overly complex and could be simplified.
Consider this simpler approach:
- val_level=dict( - rec.fields_get(["access_level"])["access_level"][ - "selection" - ] - )[rec.access_level], + val_level=dict(rec._fields['access_level'].selection)[rec.access_level],cetmix_tower_server/views/cx_tower_server_view.xml (1)
286-286: Add selection widget for access_level fields.The access_level fields would benefit from using the selection widget for better UX.
Apply this diff:
- <field name="access_level" /> + <field name="access_level" widget="selection"/> - <group> - <field name="access_level" /> - </group> + <group> + <field name="access_level" widget="selection"/> + </group>Also applies to: 301-303
cetmix_tower_server/tests/test_variable.py (1)
980-1116: Consider extracting common setup code.The test setup code for creating variables and values is duplicated across test methods.
Consider extracting the common setup into helper methods:
def _create_test_variables(self): """Create test variables with different access levels""" return { 'user': self.Variable.create({ 'name': 'Directory2', 'access_level': '1', # User }), 'manager': self.Variable.create({ 'name': 'Version2', 'access_level': '2', # Manager }), 'root': self.Variable.create({ 'name': 'Revision2', 'access_level': '3', # Root }) } def _create_variable_values(self, variables, server): """Create values for test variables""" return { 'user': self.VariableValue.create({ 'variable_id': variables['user'].id, 'server_id': server.id, 'value_char': 'User Value', 'access_level': variables['user'].access_level, }), 'manager': self.VariableValue.create({ 'variable_id': variables['manager'].id, 'server_id': server.id, 'value_char': 'Manager Value', 'access_level': variables['manager'].access_level, }), 'root': self.VariableValue.create({ 'variable_id': variables['root'].id, 'is_global': True, 'value_char': 'Root Value', 'access_level': variables['root'].access_level, }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cetmix_tower_server/models/cx_tower_variable_value.py(3 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(1 hunks)cetmix_tower_server/tests/test_variable.py(4 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)
🧰 Additional context used
📓 Learnings (2)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
cetmix_tower_server/tests/test_variable.py (2)
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:55:14.571Z
Learning: In the cetmix-tower project, the numeric access levels "1", "2", and "3" used within `cx.tower.access.mixin` are considered standard and are not seen as magic numbers. Replacing them with constants would introduce unnecessary abstraction.
Learnt from: tendil
PR: cetmix/cetmix-tower#166
File: cetmix_tower_server/tests/test_variable.py:342-342
Timestamp: 2025-01-03T20:53:58.513Z
Learning: The numeric access levels ("1", "2", "3") for `cx.tower.access.mixin` are standard across this codebase. Replacing them with constants is not desired, as it would create an unnecessary abstraction layer and deviate from existing project conventions.
🔇 Additional comments (9)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (3)
4-15: LGTM! User access rule is well-defined.The rule correctly restricts access to variables with access_level = 1 and requires server subscription, aligning with the security requirements.
17-33: LGTM! Manager access rule provides appropriate permissions.The rule correctly:
- Allows access to global values
- Restricts access to variables with access_level <= 2
- Requires server subscription for non-global values
35-41: LGTM! Root access rule provides full access.The rule correctly provides unrestricted access to root users.
cetmix_tower_server/models/cx_tower_variable_value.py (2)
13-17: LGTM! Access control mixin is properly integrated.The inheritance from "cx.tower.access.mixin" is correctly implemented to add access control functionality.
142-150: LGTM! Onchange method properly syncs access levels.The method correctly propagates the access level from the variable to its value.
cetmix_tower_server/views/cx_tower_server_view.xml (1)
269-269: LGTM! Configuration page access is properly configured.The configuration page correctly grants access to both manager and user groups.
cetmix_tower_server/tests/test_variable.py (3)
729-831: LGTM! Comprehensive test coverage for variable access rules.The test method thoroughly validates access rules for variables across different user roles and access levels.
832-895: LGTM! Well-structured tests for variable value access rules.The test method effectively validates access rules for variable values, including subscription-based access.
896-979: LGTM! Thorough testing of variable rendering.The test method comprehensively validates that variables are correctly rendered during execution, regardless of access level.
…eature Signed-off-by: Dmitry Meita <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
17-30: Simplify domain by removing explicit AND operator.The domain can be simplified as '&' is the default operator.
Apply this diff to simplify the rule:
<field name="domain_force"> [ '|', ('is_global', '=', True), - '&', ('access_level', '<=', '2'), ('server_id.message_partner_ids', 'in', [user.partner_id.id]) ] </field>cetmix_tower_server/models/cx_tower_variable_value.py (2)
148-156: Add validation for edge cases in access level setter.While the implementation is correct, consider adding validation for the case when variable_id is False to maintain consistent access levels.
@api.onchange("variable_id") def _onchange_variable_id_set_access_level(self): """ Automatically set the `access_level` field based on the `variable_id`. """ for rec in self: - if rec.variable_id: - rec.access_level = rec.variable_id.access_level + rec.access_level = rec.variable_id.access_level if rec.variable_id else 'user'
193-221: LGTM! Access level constraint properly implemented.The constraint correctly ensures that variable values maintain appropriate access levels relative to their variables.
Consider optimizing the error message construction by caching the access level selections:
@api.constrains("access_level", "variable_id") def _check_access_level_consistency(self): """ Ensure that the access level of the variable value is not lower than the access level of the associated variable. """ + # Cache access level selections + access_levels = dict(self.fields_get(["access_level"])["access_level"]["selection"]) for rec in self: if rec.variable_id and rec.access_level < rec.variable_id.access_level: raise ValidationError( _( "The access level for Variable Value '%(value)s' cannot be" "lower than the access level of its Variable '%(variable)s'.\n" "Variable Access Level: %(var_level)s\n" "Variable Value Access Level: %(val_level)s", value=rec.value_char or "Undefined", variable=rec.variable_id.name, - var_level=dict( - rec.fields_get(["access_level"])["access_level"][ - "selection" - ] - )[rec.variable_id.access_level], - val_level=dict( - rec.fields_get(["access_level"])["access_level"][ - "selection" - ] - )[rec.access_level], + var_level=access_levels[rec.variable_id.access_level], + val_level=access_levels[rec.access_level], ) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cetmix_tower_server/README.rst(25 hunks)cetmix_tower_server/__manifest__.py(2 hunks)cetmix_tower_server/models/cx_tower_access_mixin.py(2 hunks)cetmix_tower_server/models/cx_tower_variable_value.py(3 hunks)cetmix_tower_server/security/cx_tower_variable_value_security.xml(6 hunks)cetmix_tower_server/static/description/index.html(28 hunks)cetmix_tower_server/views/cx_tower_server_view.xml(3 hunks)cetmix_tower_server/views/cx_tower_variable_view.xml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cetmix_tower_server/models/cx_tower_access_mixin.py
- cetmix_tower_server/manifest.py
- cetmix_tower_server/static/description/index.html
- cetmix_tower_server/views/cx_tower_variable_view.xml
- cetmix_tower_server/README.rst
🧰 Additional context used
📓 Learnings (1)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (1)
Learnt from: Aldeigja
PR: cetmix/cetmix-tower#98
File: cetmix_tower_server/security/cx_tower_variable_value_security.xml:99-117
Timestamp: 2024-11-28T20:05:47.614Z
Learning: In Odoo's security model, group rules unify, meaning that access is granted if any rule allows it. Therefore, when a subscription check is already included in a prior rule, subsequent rules can focus solely on other conditions like `create_uid` without duplicating the subscription check.
In `cetmix_tower_server/security/cx_tower_variable_value_security.xml`, when defining access rules for managers deleting their own variable values in server templates, it's sufficient to check `create_uid` if the subscription to the server template has been validated in a previous rule.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test with OCB
🔇 Additional comments (7)
cetmix_tower_server/security/cx_tower_variable_value_security.xml (3)
Line range hint
32-134: LGTM! Well-structured access rules.The rules for plan line actions and server templates are properly implemented with consistent access level checks and appropriate unlink permissions.
136-142: LGTM! Root access rule is correctly implemented.The rule properly grants unrestricted access to root users.
4-15: 🛠️ Refactor suggestionSimplify domain and include access to global values.
The user access rule should be simplified and allow access to global values.
Apply this diff to improve the rule:
<field name="domain_force"> [ - ('access_level', '=', '1'), - ('server_id.message_partner_ids', 'in', [user.partner_id.id]) + '|', + ('is_global', '=', True), + ('access_level', '=', '1'), + ('server_id.message_partner_ids', 'in', [user.partner_id.id]) ] </field>Likely invalid or redundant comment.
cetmix_tower_server/views/cx_tower_server_view.xml (2)
269-269: LGTM! Access control properly expanded.The configuration page access is correctly expanded to include both managers and users, aligning with the new access control mechanisms.
286-286: LGTM! Access level field properly integrated.The access_level field is correctly added to both tree and form views, enabling proper access control management.
Also applies to: 303-305
cetmix_tower_server/models/cx_tower_variable_value.py (2)
14-16: LGTM! Access control mixin properly integrated.The addition of "cx.tower.access.mixin" to the inheritance list correctly implements the access control functionality as intended.
193-221: Consider implementing separate read/write access levels.Based on the PR comments about users needing to access higher-level variables, consider implementing separate access levels for reading and modifying variables. This would allow users to view but not modify higher-level variables when needed.
Let's check if there are any existing read/write separation patterns in the codebase:
tendil
force-pushed
the
14.0-t4055-access-levels-variables-add-feature
branch
from
be31191 to
3822a13
Compare
| @@ -144,6 +145,15 @@ def _onchange_variable_id(self): | |||
| else: | |||
| rec.option_id = None | |||
|
|
|||
| @api.onchange("variable_id") | |||
| def _onchange_variable_id_set_access_level(self): | |||
There was a problem hiding this comment.
You should use "compute" with "readonly=False" instead of "onchange" wherever it's possible.
| @@ -200,6 +239,35 @@ def _inverse_value_char(self): | |||
| ) | |||
| ) | |||
|
|
|||
| @api.constrains("access_level", "variable_id") | |||
| def _check_access_level_consistency(self): | |||
There was a problem hiding this comment.
There is the same function in L194, isn't it?
| <field name="model_id" ref="model_cx_tower_variable_value" /> | ||
| <field name="domain_force"> | ||
| [ | ||
| '|', |
There was a problem hiding this comment.
('access_level', '<=', '2'),
'|',
('is_global', '=', True),
('server_id.message_partner_ids', 'in', [user.partner_id.id])
This PR is irrelevant, as it was having problems versioning the Branch. As part of this task, a new PR has been opened for it
[REF]
[IMP]
cx.tower.access.mixininTowerVariableandTowerVariableValuemodels.access_levelfield in the models and views, allowing dynamic control of variable visibility.access_levelfield for better usability.[NEW]
cx_tower_variable_security.xmlto define access rules forTowerVariable.cx_tower_variable_value_security.xmlto include refined domain rules forTowerVariableValue.[TEST]
test_variable_value_accessto validate refined access logic.test_variable_value_server_subscriptionto ensure server subscription-based variable visibility.Steps to Reproduce
access_level.group_user,group_manager,group_root).test_variable_value_access,test_variable_value_server_subscription) to confirm correct behavior.File Changes
cx_tower_variable_security.xmlTowerVariable.cx_tower_variable_value_security.xmlaccess_leveland server subscriptions.Models (
TowerVariable,TowerVariableValue)cx.tower.access.mixin.access_levelfields and their logic.Views (
TowerVariable,TowerVariableValue)access_levelfields.Tests
test_variable_value_accessto ensure compatibility with new logic.test_variable_value_server_subscriptionto validate subscription-based access.General
Task: 4055
Summary by CodeRabbit
Release Notes
New Features
Security Improvements
User Interface Updates
Documentation
Testing
Chores