refactor: access and roles management #483
Conversation
thicham43
force-pushed
the
3031_refactor_access_management
branch
2 times, most recently
from
06f3f08 to
e6155ef
Compare
thicham43
force-pushed
the
3031_refactor_access_management
branch
from
e6155ef to
7c39899
Compare
WalkthroughThis update overhauls the rights and roles management system, consolidating and renaming roles and rights for clarity, removing obsolete reading and export rights, and introducing a more structured schema. The codebase, migrations, and documentation are updated to reflect these changes, with tests and permissions logic adapted accordingly. The project version advances to "3.28.0-rc". Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccessesService
participant Role
participant RightsService
User->>AccessesService: Request to manage role/access
AccessesService->>Role: Check required rights for target role
Role->>AccessesService: Return required management rights
AccessesService->>RightsService: Verify user has required rights
RightsService-->>AccessesService: Rights check result
AccessesService-->>User: Allow or deny management action
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (33)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (25)
🧰 Additional context used🧬 Code Graph Analysis (2)accesses/tests/test_view_role.py (2)
accesses/services/roles.py (3)
🪛 LanguageTool.docs/rights_schema.md[style] ~177-~177: ‘With respect to’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_WITH_RESPECT_TO) [style] ~189-~189: Unless you want to emphasize “not”, use “Cannot” which is more common. (CAN_NOT_PREMIUM) 🪛 markdownlint-cli2 (0.17.2).docs/rights_schema.md53-53: Heading levels should only increment by one level at a time (MD001, heading-increment) 53-53: Trailing punctuation in heading (MD026, no-trailing-punctuation) 60-60: Trailing punctuation in heading (MD026, no-trailing-punctuation) 73-73: Trailing punctuation in heading (MD026, no-trailing-punctuation) 78-78: Trailing punctuation in heading (MD026, no-trailing-punctuation) 79-79: Emphasis style (MD049, emphasis-style) 79-79: Emphasis style (MD049, emphasis-style) 86-86: Trailing punctuation in heading (MD026, no-trailing-punctuation) 87-87: Emphasis style (MD049, emphasis-style) 87-87: Emphasis style (MD049, emphasis-style) 94-94: Trailing punctuation in heading (MD026, no-trailing-punctuation) 100-100: Trailing punctuation in heading (MD026, no-trailing-punctuation) 108-108: Trailing punctuation in heading (MD026, no-trailing-punctuation) 155-155: Trailing punctuation in heading (MD026, no-trailing-punctuation) 157-157: Unordered list style (MD004, ul-style) 158-158: Unordered list style (MD004, ul-style) 164-164: Unordered list style (MD004, ul-style) 165-165: Unordered list style (MD004, ul-style) 171-171: Unordered list style (MD004, ul-style) 172-172: Unordered list style (MD004, ul-style) 179-179: Unordered list style (MD004, ul-style) 179-179: Unordered list indentation (MD007, ul-indent) 180-180: Unordered list style (MD004, ul-style) 180-180: Unordered list indentation (MD007, ul-indent) 181-181: Unordered list style (MD004, ul-style) 181-181: Unordered list indentation (MD007, ul-indent) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (16)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 4
🧹 Nitpick comments (5)
accesses/migrations/0019_remove_multiple_rights.py (1)
15-21: Note: This update appears to be temporary.The SQL sets
right_read_accesses_above_levels = True, but according to the next migration (0020_remove_role_right_read_accesses_above_levels), this field is removed immediately after. Consider if this intermediate step is necessary or if the migrations could be consolidated.Also applies to: 92-92
accesses/models/role.py (1)
66-89: Well-structured role management categorization methods.The three new methods provide clear categorization of roles based on their management requirements. The inline comments effectively document the required rights.
Consider adding the
@lru_cache(maxsize=None)decorator to these methods for consistency with other methods in the class, as role rights are immutable after creation:+ @lru_cache(maxsize=None) def requires_full_admin_right_to_be_managed(self):accesses/services/roles.py (1)
75-108: Good refactoring with clear validation logic.The helper functions improve readability and the new validation preventing IPP search without nominative rights is a valuable security check.
For consistency, rename the helper function to match the pattern:
- def allow_manage_accesses(d: dict) -> bool: + def allows_manage_accesses(d: dict) -> bool:.docs/rights_schema.md (1)
1-189: Excellent documentation restructuring with comprehensive coverage.The new documentation provides clear role definitions, management rules, and practical examples. The class diagram effectively visualizes the role hierarchy.
Consider addressing the markdown formatting issues flagged by the linters:
- Use consistent heading levels (line 53 should be h3 not h4)
- Remove trailing colons from headings
- Use consistent list markers (dashes instead of asterisks)
- Fix the grammatical issue on line 117: "The rules of managing roles"
accesses/services/accesses.py (1)
311-317: Consider removing redundant adapter methods.These static methods now simply delegate to the role's methods. Consider calling the role methods directly to reduce indirection.
Instead of:
if self.role_requires_full_admin_right_to_be_read(role): # ...Use:
if role.requires_full_admin_right_to_be_managed(): # ...This would make the code more direct and remove unnecessary abstraction layers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.docs/rights_schema.md(1 hunks)accesses/migrations/0011_add_new_rights_and_rename_others.py(1 hunks)accesses/migrations/0019_remove_multiple_rights.py(1 hunks)accesses/migrations/0020_remove_role_right_read_accesses_above_levels.py(1 hunks)accesses/migrations/data/new_rights.py(1 hunks)accesses/models/right.py(0 hunks)accesses/models/role.py(3 hunks)accesses/permissions.py(2 hunks)accesses/q_expressions.py(1 hunks)accesses/serializers.py(2 hunks)accesses/services/accesses.py(9 hunks)accesses/services/perimeters.py(1 hunks)accesses/services/rights.py(1 hunks)accesses/services/roles.py(3 hunks)accesses/services/shared.py(2 hunks)accesses/tests/base.py(2 hunks)accesses/tests/test_view_access.py(7 hunks)accesses/tests/test_view_perimeters.py(10 hunks)accesses/tests/test_view_role.py(6 hunks)accesses/views/perimeter.py(1 hunks)accesses/views/profile.py(1 hunks)accesses/views/right.py(0 hunks)accesses/views/role.py(3 hunks)admin_cohort/permissions.py(3 hunks)admin_cohort/tests/tests_auth.py(1 hunks)cohort/services/cohort_rights.py(2 hunks)content_management/tests/tests_view_content.py(2 hunks)exporters/tests/base_test.py(1 hunks)exports/permissions.py(1 hunks)exports/services/rights_checker.py(2 hunks)exports/tests/base_test.py(1 hunks)exports/tests/test_view_export_request.py(3 hunks)pyproject.toml(1 hunks)
💤 Files with no reviewable changes (2)
- accesses/views/right.py
- accesses/models/right.py
🧰 Additional context used
🧬 Code Graph Analysis (9)
accesses/migrations/0020_remove_role_right_read_accesses_above_levels.py (2)
accesses/migrations/0011_add_new_rights_and_rename_others.py (1)
Migration(20-94)accesses/migrations/0019_remove_multiple_rights.py (1)
Migration(34-93)
accesses/services/rights.py (1)
accesses/models/right.py (1)
Right(6-15)
exports/permissions.py (3)
accesses/permissions.py (4)
can_user_read_datalabs(55-56)can_user_manage_datalabs(59-60)can_user_make_export_csv_xlsx_nomi(51-52)can_user_make_export_jupyter_nomi(43-44)exports/apps.py (1)
ExportsConfig(6-20)exporters/enums.py (1)
ExportTypes(7-24)
admin_cohort/permissions.py (3)
accesses/permissions.py (8)
can_user_manage_users(35-36)has_object_permission(19-20)has_object_permission(76-77)has_object_permission(86-87)has_permission(15-17)has_permission(64-67)has_permission(71-74)has_permission(81-84)accesses/services/accesses.py (1)
user_is_full_admin(32-34)cohort_job_server/permissions.py (3)
has_object_permission(18-19)has_permission(8-10)has_permission(14-16)
exports/tests/test_view_export_request.py (2)
accesses/models/role.py (1)
Role(11-89)accesses/models/access.py (1)
Access(17-53)
cohort/services/cohort_rights.py (2)
admin_cohort/models/user.py (1)
User(7-27)cohort/models/cohort_result.py (1)
CohortResult(9-21)
accesses/tests/test_view_role.py (2)
accesses/models/role.py (1)
Role(11-89)admin_cohort/tests/tests_tools.py (3)
ListCase(165-188)new_user_and_profile(81-95)check_list_case(458-479)
accesses/migrations/0019_remove_multiple_rights.py (3)
accesses/serializers.py (1)
RightSerializer(13-30)accesses/migrations/0020_remove_role_right_read_accesses_above_levels.py (1)
Migration(6-19)accesses/migrations/0011_add_new_rights_and_rename_others.py (1)
Migration(20-94)
accesses/services/accesses.py (5)
accesses/q_expressions.py (3)
q_allow_manage_accesses_on_same_level(21-23)q_allow_manage_accesses_on_inf_levels(27-29)q_impact_inferior_levels(33-35)accesses/models/perimeter.py (4)
above_levels(41-48)q_all_parents(65-66)Perimeter(14-66)is_child_of(61-62)accesses/models/access.py (1)
Access(17-53)admin_cohort/models/user.py (1)
User(7-27)accesses/models/role.py (4)
requires_full_admin_right_to_be_managed(66-71)requires_admin_accesses_managing_right_to_be_managed(73-79)requires_data_accesses_managing_right_to_be_managed(81-89)Role(11-89)
🪛 LanguageTool
.docs/rights_schema.md
[grammar] ~51-~51: Use proper spacing conventions.
Context: ...ders --> Users ``` --- ## Possible roles #### Full_Admin role: This is a *Super User...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~54-~54: Use singular and plural nouns appropriately.
Context: ...ding everything, including managing all kind of accesses. This role has the follow...
(QB_NEW_EN_OTHER_ERROR_IDS_000012)
[grammar] ~54-~54: Use proper spacing conventions.
Context: ...including managing all kind of accesses. This role has the following right enable...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~56-~56: Use proper spacing conventions.
Context: ...ing right enabled: - right_full_admin >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~58-~58: Use proper spacing conventions.
Context: ... enabled: - right_full_admin >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~58-~58: Use proper spacing conventions.
Context: ...Admin role is expected to be configured. #### Patient_Data_Reader role: It allows read...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~61-~61: Use proper spacing conventions.
Context: ... in nominative or in pseudonymized mode. It must have at least one of the followi...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~64-~64: Use proper spacing conventions.
Context: ...ive* - right_read_patient_pseudonymized Furthermore, this role could be enhanced...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~71-~71: Use proper spacing conventions.
Context: ...e* - right_export_jupyter_pseudonymized #### Unlimited_Data_Reader role: A role havin...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~74-~74: Use proper spacing conventions.
Context: ... in all perimeters without restrictions. Only the Full_Admin role is allowed to a...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~75-~75: There might be a mistake here.
Context: ...allowed to assign such a role to a user: - right_search_patients_unlimited ####...
(QB_NEW_EN_OTHER)
[grammar] ~76-~76: Place a period at the end of declarative sentences.
Context: ...er: - right_search_patients_unlimited #### Administrator_Of_Patient_Data_Readers ro...
(QB_NEW_EN_OTHER_ERROR_IDS_000178)
[grammar] ~79-~79: There might be a mistake here.
Context: ...Of_Patient_Data_Readers role: It allows to manage accesses associated with Patient_Data...
(QB_NEW_EN_OTHER)
[grammar] ~82-~82: Use proper spacing conventions.
Context: ...s_inferior_levels* - right_manage_users >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~84-~84: Use proper spacing conventions.
Context: ...ior_levels* - right_manage_users >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~84-~84: Use correct prepositions.
Context: ...ghts granted by this role are dependent to the perimeters' hierarchy #### Manager...
(QB_NEW_EN_OTHER_ERROR_IDS_000010)
[grammar] ~84-~84: Place a period at the end of declarative sentences.
Context: ...e dependent to the perimeters' hierarchy #### Manager_Of_Administrators role: It allow...
(QB_NEW_EN_OTHER_ERROR_IDS_000178)
[grammar] ~87-~87: There might be a mistake here.
Context: ...nager_Of_Administrators role: It allows to manage accesses associated with _Administrator...
(QB_NEW_EN_OTHER)
[grammar] ~87-~87: Use proper spacing conventions.
Context: ...istrator_Of_Patient_Data_Readers_ roles. - right_manage_admin_accesses_same_level ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~90-~90: Use proper spacing conventions.
Context: ...s_inferior_levels* - right_manage_users >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~92-~92: Use proper spacing conventions.
Context: ...ior_levels* - right_manage_users >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~92-~92: Use correct prepositions.
Context: ...ghts granted by this role are dependent to the perimeters' hierarchy #### CSV_Exc...
(QB_NEW_EN_OTHER_ERROR_IDS_000010)
[grammar] ~92-~92: Place a period at the end of declarative sentences.
Context: ...e dependent to the perimeters' hierarchy #### CSV_Excel_Exports role: It allows to exp...
(QB_NEW_EN_OTHER_ERROR_IDS_000178)
[grammar] ~95-~95: There might be a mistake here.
Context: ... #### CSV_Excel_Exports role: It allows to export data in CSV and XLSX formats. - *righ...
(QB_NEW_EN_OTHER)
[grammar] ~95-~95: Use proper spacing conventions.
Context: ... to export data in CSV and XLSX formats. - right_export_csv_xlsx_nominative >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~96-~96: Use proper spacing conventions.
Context: ...s. - right_export_csv_xlsx_nominative >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~98-~98: Use proper spacing conventions.
Context: ...right_export_csv_xlsx_nominative >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~98-~98: Use commas after Latin abbreviations.
Context: ...rmat is only allowed in nominative mode (i.e. must have the **right_read_patient_nomi...
(QB_NEW_EN_OTHER_ERROR_IDS_000037)
[grammar] ~98-~98: Place a period at the end of declarative sentences.
Context: ...right_read_patient_nominative** enabled) #### Jupyter_Exports role: It allows to expor...
(QB_NEW_EN_OTHER_ERROR_IDS_000178)
[grammar] ~101-~101: There might be a mistake here.
Context: ...) #### Jupyter_Exports role: It allows to export data in Hive format to a Jupyter datala...
(QB_NEW_EN_OTHER)
[grammar] ~101-~101: Use proper spacing conventions.
Context: ...ormat to a Jupyter datalab or workspace. - right_export_jupyter_nominative - *righ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~103-~103: Use proper spacing conventions.
Context: ...e* - right_export_jupyter_pseudonymized >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~105-~105: Use proper spacing conventions.
Context: ...ight_export_jupyter_pseudonymized* >
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~105-~105: Place a period at the end of declarative sentences.
Context: ..._patient_nominative** is enabled as well #### Datalabs role: It allows you to manage/r...
(QB_NEW_EN_OTHER_ERROR_IDS_000178)
[grammar] ~109-~109: Use proper spacing conventions.
Context: ...you to manage/read the list of Datalabs. - right_manage_datalabs - *right_read_dat...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~111-~111: Use proper spacing conventions.
Context: ..._manage_datalabs* - right_read_datalabs --- ## Roles management The rules of managing ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~115-~115: Use proper spacing conventions.
Context: ...read_datalabs* --- ## Roles management The rules of managing role are defined a...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~117-~117: Use singular and plural nouns appropriately.
Context: ...Roles management The rules of managing role are defined according to the following ...
(QB_NEW_EN_OTHER_ERROR_IDS_000012)
[grammar] ~117-~117: Use proper spacing conventions.
Context: ...according to the following dependencies: | If a Role has one of these rights ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~127-~127: Use proper spacing conventions.
Context: ...ses_inferior_levels | --- ## Use cases Given this perimeters hierarchy tree: ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~129-~129: Use proper spacing conventions.
Context: ...s Given this perimeters hierarchy tree: mermaid graph TD APHP --> P0 APHP --> P1 APHP --> P2 P0 --> P3 P0 --> P4 P0 --> P5 P1 --> P6 P1 --> P7 P2 --> P8 P2 --> P9 P2 --> P10 P4 --> P11 P4 --> P12 P10 --> P13 P10 --> P14 ### Basic examples: 1. On perimeter P1: ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~157-~157: There might be a mistake here.
Context: ...Access Manager on inferior perimeters only" * User Y has a role of "Data Acces...
(QB_NEW_EN_OTHER)
[grammar] ~158-~158: There might be a problem here.
Context: ...Access Manager on inferior perimeters only" > User X can manage accesses of user Y onl...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~160-~160: Use articles correctly.
Context: ...meters only**" > User X can manage accesses of user Y only if they are defined with...
(QB_NEW_EN_OTHER_ERROR_IDS_000004)
[grammar] ~160-~160: Use proper spacing conventions.
Context: ...y are defined within inferior levels of P1 2. On perimeter P1: * User X has "A...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~164-~164: There might be a mistake here.
Context: ...dmin Access Manager on same perimeter only" * User Y has "Data Access Manager ...
(QB_NEW_EN_OTHER)
[grammar] ~165-~165: There might be a problem here.
Context: ...Data Access Manager on same perimeter only" > User X can manage accesses of user Y as ...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~167-~167: Place a period at the end of declarative sentences.
Context: ...ey are defined within the same perimeter P1 3. mix: * User X has "Admin Access Mana...
(QB_NEW_EN_OTHER_ERROR_IDS_000178)
[grammar] ~171-~171: There might be a mistake here.
Context: ...inferior perimeters only**" on perimeter P1 * User Y has "Data Access Manag...
(QB_NEW_EN_OTHER)
[grammar] ~172-~172: Place a period at the end of declarative sentences.
Context: ...on same perimeter only" on perimeter P7 > User X can manage accesses of user Y as ...
(QB_NEW_EN_OTHER_ERROR_IDS_000178)
[grammar] ~174-~174: Use articles correctly.
Context: ...perimeter P7 > User X can manage accesses of user Y as they are defined within an...
(QB_NEW_EN_OTHER_ERROR_IDS_000004)
[grammar] ~174-~174: Use proper spacing conventions.
Context: ...defined within an inferior perimeter of P1 ### Listing user accesses With respect to t...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~176-~176: Use proper spacing conventions.
Context: ...meter of P1 ### Listing user accesses With respect to the hierarchy tree above...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[style] ~177-~177: ‘With respect to’ might be wordy. Consider a shorter alternative.
Context: ...ter of P1 ### Listing user accesses With respect to the hierarchy tree above, if user Y has...
(EN_WORDINESS_PREMIUM_WITH_RESPECT_TO)
[grammar] ~180-~180: Use proper spacing conventions.
Context: ...1: Data reader nominative * On P4`: Admin accesses manager (on same level +...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~181-~181: Use proper spacing conventions.
Context: ...ses manager (on same level + inf levels) | X roles | Res...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~187-~187: There might be a mistake here.
Context: ...efined on P4 is listed as readonly | | Data Access Manager on APHP | Can...
(QB_NEW_EN_OTHER)
[grammar] ~188-~188: Use proper spacing conventions.
Context: ...** | | Data Access Manager on APHP | Can manage access defined within `...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[style] ~189-~189: Unless you want to emphasize “not”, use “Cannot” which is more common.
Context: ...| | Data reader nominative on APHP | Can not read any of user Y's accesses ...
(CAN_NOT_PREMIUM)
[grammar] ~189-~189: Use proper spacing conventions.
Context: ... | Can not read any of user Y's accesses |
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
🪛 markdownlint-cli2 (0.17.2)
.docs/rights_schema.md
53-53: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
53-53: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
60-60: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
73-73: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
78-78: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
79-79: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
79-79: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
86-86: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
87-87: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
87-87: Emphasis style
Expected: asterisk; Actual: underscore
(MD049, emphasis-style)
94-94: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
100-100: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
108-108: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
155-155: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
157-157: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
158-158: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
164-164: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
165-165: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
171-171: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
172-172: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
179-179: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
179-179: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
180-180: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
180-180: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
181-181: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
181-181: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (63)
pyproject.toml (1)
3-3: Ensure version bump is reflected everywhere & CI accepts the-rcsuffixThe switch from a
-devto an-rcsuffix looks fine here, but please double-check that:
- Any hard-coded
__version__attributes (e.g., inpackage/__init__.py) are bumped in sync.- Release workflows (Docker tags, PyPI publish jobs, Helm charts, etc.) don’t filter out pre-releases and will correctly pick up
3.28.0-rc.- CHANGELOG / docs mention the release-candidate status so downstream teams know it’s not GA yet.
content_management/tests/tests_view_content.py (1)
9-9: LGTM: Consistent renaming of role definition.The import and usage have been consistently updated to use the new
full_admin_role_definitionnaming convention, maintaining test functionality while aligning with the refactored role definitions.Also applies to: 27-27
exporters/tests/base_test.py (1)
20-20: LGTM: Updated to use consolidated export right.The change correctly updates the test to use the new
right_export_csv_xlsx_nominativeflag, which consolidates the previous separate CSV and XLSX export rights into a single permission. This aligns with the broader rights consolidation effort.exports/tests/base_test.py (1)
34-34: LGTM: Consistent update to consolidated export right.The test correctly uses the new
right_export_csv_xlsx_nominativepermission, maintaining consistency with the consolidated export rights schema across the test suite.accesses/migrations/0020_remove_role_right_read_accesses_above_levels.py (1)
1-20: LGTM: Clean removal of deprecated right field.The migration correctly removes the
right_read_accesses_above_levelsfield from the Role model and cleans up corresponding database entries. The dependency on the previous migration (0019) is appropriate.Note: The migration is irreversible due to the
noopreverse operations, which is acceptable for this rights simplification effort.accesses/migrations/0011_add_new_rights_and_rename_others.py (1)
16-16: LGTM! Correct fix for rights attribute setting.This change correctly adapts to the updated
all_rightsmethod that now returns a list of right names (strings) instead of Right objects. Usingrightdirectly instead ofright.nameis the proper fix.admin_cohort/tests/tests_auth.py (1)
12-12: LGTM! Appropriate cleanup of unused imports.The removal of
RoleandAccessimports aligns with the elimination of role and access creation in the test setup, reflecting the broader refactoring of access rights management.accesses/services/rights.py (2)
7-8: LGTM! Correct refactoring of all_rights method.Returning a flat list of right names instead of Right objects is consistent with the migration usage and simplifies the interface for callers that only need the names.
10-13: LGTM! Proper conversion to static method.Converting to static method and using
Right.objects.all()directly is correct since:
- The method no longer needs instance state
- It requires full Right objects to access category, label, and depends_on fields
- This maintains functionality while adapting to the
all_rightsmethod changesexports/tests/test_view_export_request.py (2)
85-93: LGTM! Correct consolidation of export roles.The replacement of separate
"EXP_CSV_PSEUDO"and"EXP_CSV_NOMI"roles with a single"EXP_CSV_XLSX_NOMI"role properly reflects the consolidation of export rights mentioned in the PR objectives.
103-109: LGTM! Consistent access object updates.The access objects are correctly updated to use the new consolidated export role, maintaining test coverage while adapting to the new rights schema.
Also applies to: 153-159
accesses/services/perimeters.py (1)
202-206: LGTM! Improved logic ensures data consistency.Iterating over all perimeters instead of only those in the
updatesdictionary ensures that perimeters with zero users also get theircount_allowed_usersfield properly updated to 0. This maintains better data consistency across all perimeter records.accesses/views/role.py (3)
12-12: LGTM: Good consolidation of rights management.The import of
all_rightscentralizes rights definitions, which improves maintainability and consistency across the codebase.
131-131: Parameter renaming improves code clarity.The parameter name change from
querysettoall_rolesbetter reflects the actual data being passed and improves code readability.
61-67: Ensureall_rightsvalues align withRoleBooleanField namesThe view builds
databy iterating over each name inall_rightsand falling back togetattr(role, right, False). This works correctly only if everyRight.nameexactly matches aRoleattribute; otherwise you’ll silently getFalsefor unrecognized rights.• accesses/services/rights.py:
all_rights = rights_service.all_rights()should return strings like
"right_full_admin","right_manage_users", etc.
• accesses/models/role.py: verify that each of these names corresponds to a declaredBooleanField.If your
Right.namevalues differ (for example, missing the"right_"prefix), either
– adjust how you store/derive those names in theRightmodel/service, or
– normalize them in the view before merging intodata, and/or raise an explicit error on missing attributes to catch mismatches early.accesses/views/profile.py (3)
6-6: LGTM: Simplified imports align with permission model changes.The removal of
can_user_read_usersimport is consistent with the broader refactor that eliminates read-only rights.
17-17: Simplified serializer usage is consistent with permission refactor.The fixed
serializer_class = ProfileSerializersimplifies the code by removing conditional logic, which aligns with the broader removal of read permission checks.
11-11: Verify sensitive data exposure in ProfileSerializer
Please review thefieldsdefinitions inaccesses/serializers.pyto ensure that switching fromReducedProfileSerializertoProfileSerializerdoesn’t now leak any private or sensitive attributes (e.g., email addresses, phone numbers, internal identifiers) to clients that previously received only the reduced view.• File: accesses/serializers.py
–class ReducedProfileSerializer(Meta.fields around line 55)
–class ProfileSerializer(Meta.fields around line 73)If any sensitive fields appear in
ProfileSerializerthat weren’t inReducedProfileSerializer, either re-introduce conditional serializer logic or add appropriate permission checks.accesses/serializers.py (2)
27-30: LGTM: Cleaned up serializer fields align with model changes.The removal of read access fields from the
extra_kwargsis consistent with the broader refactor that eliminates read-only rights from the role management system.
204-204: Export rights consolidation simplifies permission model.The replacement of separate
right_export_csv_nominativeandright_export_csv_pseudonymizedwith a singleright_export_csv_xlsx_nominativefield effectively consolidates export permissions and aligns with the broader refactor.exports/permissions.py (3)
6-7: LGTM: Import consolidation aligns with unified export permissions.The updated import statement correctly uses the consolidated
can_user_make_export_csv_xlsx_nomifunction, which unifies CSV and XLSX export permission checks.
15-16: Verify the consolidation correctly handles both CSV and XLSX formats.The logic now uses a single permission function for both CSV and XLSX formats, which is consistent with the broader refactor. The condition
output_format in (ExportTypes.CSV, ExportTypes.XLSX)correctly covers both formats.
23-24: Confirm Intentional Removal of Pseudonymized CSV/XLSX ExportsFile: exports/permissions.py (lines 23–24)
if output_format in (ExportTypes.CSV, ExportTypes.XLSX): raise PermissionDenied("Vous n'avez pas le droit d'export CSV/Excel pseudonymisé")Please verify that:
- Disabling pseudonymized CSV/XLSX exports aligns with current business requirements.
- No existing workflows or automated tests depend on this functionality.
- User-facing messages and documentation clearly communicate this change.
If this behavior is unintentional, consider restoring the original permission check or introducing a new permission that allows pseudonymized exports for authorized users.
accesses/services/shared.py (2)
17-17: LGTM: DataRight field consolidation aligns with export rights refactor.The consolidation of CSV export rights into a single
right_export_csv_xlsx_nominativefield simplifies the data structure and is consistent with the broader refactor.
28-28: Method correctly updated to use consolidated export right.The
acquire_extra_global_rightsmethod properly uses the new consolidated field name while maintaining the same logical behavior (OR operation).cohort/services/cohort_rights.py (4)
9-10: Import consolidation looks good.The consolidation of CSV export rights into a unified CSV/XLSX export right is well-implemented. The removal of
q_allow_export_csv_pseudoaligns with the refactoring to eliminate pseudonymized CSV exports.
18-18: Constant renaming is consistent.The constant
EXPORT_CSV_XLSX_NOMIproperly reflects the unified CSV/XLSX export right, maintaining consistency with the new naming convention.
26-26: Rights dictionary update is correct.The
all_true_rightsfunction correctly uses the new unified export right constant, maintaining consistency with the refactored permissions structure.
77-77: Query expression update is properly implemented.The method now correctly uses the unified
q_allow_export_csv_xlsx_nominativequery expression, removing the need for separate CSV and XLSX logic.exports/services/rights_checker.py (2)
15-15: Right constant renaming is consistent.The constant
right_csv_xlsx_nomiproperly reflects the unified CSV/XLSX export right, maintaining consistency with the broader refactoring.
28-28: Verify unified CSV/XLSX permission covers both nominative and pseudonymized exportsPlease confirm that granting
right_export_csv_xlsx_nominativefor all CSV and XLSX exports (regardless of nominative vs. pseudonymized) is acceptable under our security requirements. If pseudonymized exports should remain less-privileged, you’ll need to restore conditional logic.Consider also renaming the field to remove the “_nominative” suffix for clarity (e.g.
right_export_csv_xlsx), since it now governs both export modes.Affected locations to review:
- exports/services/rights_checker.py:28
- accesses/models/role.py (field definition at line 20)
- accesses/q_expressions.py:14
- accesses/permissions.py:52
accesses/q_expressions.py (1)
14-14: Query expression renaming is consistent.The renaming from
q_allow_export_csv_nominativetoq_allow_export_csv_xlsx_nominativeproperly reflects the unified CSV/XLSX export permission, maintaining consistency with the broader refactoring.admin_cohort/permissions.py (3)
5-5: Import simplification is consistent with refactoring.The removal of read-only permission functions aligns with the broader refactoring to eliminate read-only rights and simplify the permission model.
28-28: Permission logic update is more restrictive.The change from
can_user_read_logstoaccesses_service.user_is_full_adminmakes the logs permission more restrictive, requiring full admin rights instead of specific read permissions. This aligns with the removal of read-only rights.
42-42: Object permission simplification is consistent.The delegation to
has_permissionremoves the previous object-level ownership check, simplifying the permission model in line with the broader refactoring.accesses/migrations/data/new_rights.py (3)
1-106: Rights definition is well-structured and comprehensive.The rights list provides a clear hierarchy with proper categorization and scope flags. The separation of data access management from admin access management is well-designed, and the impact flags properly indicate inheritance behavior.
108-116: Dependent rights structure is properly implemented.The dependent rights mechanism provides a clean way to handle rights that automatically inherit from other rights. The
right_read_datalabsdepending onright_manage_datalabsfollows the principle of read access being implied by manage access.
88-93: Consolidation of CSV/XLSX export rights is consistent
Confirmed that legacy CSV-only rights only appear in migrations (for data cleanup) and that all application logic, serializers, permissions checks and tests now reference the unifiedright_export_csv_xlsx_nominative. No unintended permission broadening detected.accesses/tests/test_view_access.py (6)
48-48: LGTM!The role name change is consistent with the PR's consolidation of export rights.
326-334: Role-based access control logic is correctly implemented.The updated perimeter filtering and editability checks properly reflect the hierarchical management rules where admin access managers can manage data accesses but not admin accesses at the same level.
337-344: Access control hierarchy is properly enforced.The logic correctly restricts data access managers from editing admin-level accesses while allowing them to manage data accesses.
391-391: Role updates are consistent with the refactoring objectives.All role references have been properly updated to use the consolidated XLSX export role and the new admin accesses manager role.
Also applies to: 405-405, 440-443
454-457: Rights structure correctly reflects the new schema.The updated rights dictionary properly sets permissions for the data reader role with the consolidated XLSX export right.
462-483: Comprehensive test coverage for perimeter-based rights.The new test cases effectively validate rights aggregation across different perimeter levels.
accesses/permissions.py (3)
51-52: Export permission function correctly updated.The function name and permission check have been properly updated to use the consolidated CSV/XLSX export right.
77-77: Object permission check is appropriate.The permission check correctly validates management rights for specific access objects.
84-84: Verify the removal of read permission checks is intentional.The
has_permissionmethod now allows all authenticated users to perform GET requests on profiles. Please confirm this aligns with the intended access control model after removing read-only rights.Also applies to: 87-87
accesses/tests/base.py (2)
61-137: Well-structured role definitions with clear separation of concerns.The new role definitions provide excellent clarity with:
- Explicit naming conventions
- Granular separation between same-level and inferior-level management rights
- Proper consolidation of export rights
This structure will make test maintenance easier and more intuitive.
199-200: Good refactoring to static method.Since
create_new_access_for_userdoesn't use instance state, converting it to a static method is the correct approach.accesses/migrations/0019_remove_multiple_rights.py (1)
41-89: Migration operations are comprehensive and well-structured.The field renaming and removals properly implement the rights schema refactoring while preserving critical data through the rename operation.
accesses/tests/test_view_role.py (3)
51-51: Export right references consistently updated.All occurrences of the CSV export right have been properly updated to use the consolidated CSV/XLSX export right.
Also applies to: 56-56, 61-61, 68-68, 118-118, 165-165
250-287: Excellent test coverage for granular role management.The new test cases effectively validate the hierarchical role assignment rules:
- Same-level managers cannot assign roles to child perimeters
- Inferior-level managers can assign appropriate roles to child perimeters
This ensures the access control hierarchy is properly enforced.
236-244: Comprehensive role lists for assignability tests.The expected assignable roles correctly include all data-level roles and the new granular management role variants, ensuring complete test coverage.
Also applies to: 274-282, 297-305
accesses/tests/test_view_perimeters.py (4)
133-138: Role naming updates look good.The updated role names align with the new explicit naming convention documented in the rights schema.
151-156: Correct addition of P1 read rights.The new
PerimeterReadRightfor P1 with all rights enabled correctly reflects therole_data_reader_full_accessassignment.Also applies to: 172-172
445-446: Appropriate use of level-specific manager roles.The updated roles correctly test the scenario where a user has management rights but no data reading rights.
324-324: No changes needed for opposed patient lookup flagThe
allow_lookup_opposed_patientsvalue is driven by the user’sright_search_opposed_patientssetting (default False inaccesses/models/role.pyand exposed viaaccesses_service.can_user_read_opposed_patient_data). The updated tests now correctly expect False for roles without that right and still cover the True case where a role grants it.accesses/models/role.py (1)
19-20: Good consolidation of CSV/Excel export rights.Merging the export rights into a single
right_export_csv_xlsx_nominativefield simplifies permission management while maintaining clarity.accesses/services/roles.py (1)
69-70: Logical simplification of read access permissions.Making read access implicit in manage access aligns with the principle that managers need to see what they manage.
accesses/services/accesses.py (4)
59-66: Appropriate simplification of access filtering logic.Removing the separate read access check and treating all non-manageable accesses as readonly aligns with the simplified permission model.
78-85: Correct updates to perimeter access logic.The changes appropriately reflect the shift from read-based to manage-based permissions for accessing child perimeters.
290-308: Excellent refactoring of access management logic.The new implementation effectively uses the role's categorization methods and provides clearer logic for determining management permissions. The OR operation correctly allows admin access managers to manage data accesses.
145-145: Consistent application of consolidated export rights.All export right references have been properly updated to use the single
right_export_csv_xlsx_nominativefield.Also applies to: 159-159, 173-173, 221-221, 230-230
| @@ -125,7 +125,7 @@ def check_read_patient_data_rights(self, request, *args, **kwargs): | |||
| serializer = RightReadPatientDataSerializer(data={"allow_read_patient_data_nomi": True, | |||
| "allow_lookup_opposed_patients": True, | |||
| "allow_read_patient_without_perimeter_limit": True, | |||
| "allow_read_administrative_data": True, | |||
| "allow_read_administrative_data": True, # todo: remove this ? used in FHIR ? | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Resolve the TODO comment about administrative data rights.
The TODO comment indicates uncertainty about whether allow_read_administrative_data should be removed, particularly in relation to FHIR usage. This technical debt should be addressed to avoid confusion and maintain clean code.
Would you like me to help investigate the FHIR usage of this flag or create an issue to track this cleanup task?
Let me search for FHIR-related usage of this administrative data flag:
🏁 Script executed:
#!/bin/bash
# Search for usage of allow_read_administrative_data in FHIR-related code
rg -A 3 -B 3 "allow_read_administrative_data" --type py
# Search for FHIR-related code that might use administrative data
rg -A 5 -B 5 "fhir|FHIR" --type py | rg -A 5 -B 5 "administrative"Length of output: 13629
Remove or clarify the outdated TODO on allow_read_administrative_data
The TODO mentioning FHIR is outdated—there are no FHIR references in the codebase. You should either remove this comment (and the field if unused) or update it to reflect the real intent of the flag.
- accesses/views/perimeter.py (line 128): Remove the
# todo: remove this ? used in FHIR ?comment. - accesses/serializers.py: Review whether
allow_read_administrative_datais still required; if not, remove it from the serializer. - accesses/tests/test_view_perimeters.py: Update or remove all test assertions related to this flag.
Let me know if you’d like assistance creating an issue or a migration to clean this up.
🤖 Prompt for AI Agents
In accesses/views/perimeter.py at line 128, remove the outdated TODO comment
about FHIR from the "allow_read_administrative_data" field. Then, check
accesses/serializers.py to determine if this field is still needed; if it is
unused, remove it from the serializer. Finally, update or remove any related
test assertions in accesses/tests/test_view_perimeters.py to reflect these
changes.
| (request.method == "GET" or | ||
| (request.method in ("POST", "PATCH") and can_user_manage_users(request.user))) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the security implications of allowing unrestricted GET requests.
The logic now allows any authenticated user to make GET requests without checking read permissions. This is a significant change that could potentially expose sensitive user data.
🏁 Script executed:
#!/bin/bash
# Description: Check what data is exposed through GET requests in the users endpoint
# Expected: Understand the security implications of allowing unrestricted GET access
echo "Searching for user-related view classes and their GET methods..."
rg -n "class.*User.*View" --type py -A 10
echo "Searching for serializers that handle user data..."
rg -n "class.*User.*Serializer" --type py -A 10Length of output: 13761
🏁 Script executed:
#!/bin/bash
echo "Extracting UsersPermission class implementation..."
rg -n "class UsersPermission" -A 40 admin_cohort/permissions.py
echo
echo "Locating can_user_manage_users function..."
rg -n "def can_user_manage_users" -A 20 admin_cohort/permissions.pyLength of output: 1366
🏁 Script executed:
#!/bin/bash
echo "Searching for the definition of can_user_manage_users across the repository..."
rg -n "def can_user_manage_users" -A 20 .
echo
echo "Searching for its import statements to locate the module..."
rg -n "can_user_manage_users" -n -C 2 .Length of output: 2801
Restrict unrestricted GET on User endpoints
The current UsersPermission implementation in admin_cohort/permissions.py (lines 37–39) allows any authenticated user to perform GET on the UserViewSet, and admin_cohort/views/users.py uses queryset = User.objects.all() with a serializer that exposes username, firstname, lastname, and email. This effectively lets any logged-in user list or retrieve all user records.
• Files to review:
– admin_cohort/permissions.py (UsersPermission.has_permission)
– admin_cohort/views/users.py (UserViewSet; default get_queryset and serializer fields)
Suggestions:
- Confirm whether all authenticated users should see other users’ details.
- If not, restrict GET to users with
can_user_manage_users(request.user). - Alternatively, introduce a read-only serializer that omits sensitive fields or implement object-level permissions for GET.
🤖 Prompt for AI Agents
In admin_cohort/permissions.py around lines 38 to 39, the current permission
allows any authenticated user to perform GET requests on UserViewSet, exposing
all user details. To fix this, restrict GET access by requiring
can_user_manage_users(request.user) to be true for GET requests as well, or
alternatively implement a read-only serializer with limited fields or
object-level permissions to prevent unrestricted access to user data.
| delete_old_rights = "DELETE FROM accesses_right WHERE True;" | ||
|
|
There was a problem hiding this comment.
Consider the risks of deleting all rights data.
The migration deletes all existing rights before loading new ones. This approach has risks:
- If the migration fails after deletion but before loading, you'll lose all rights data
- The error handling in
load_new_rightsonly logs errors without failing the migration
Consider:
- Adding a database transaction wrapper to ensure atomicity
- Making the migration fail if rights loading encounters errors
- Creating a backup of the rights table before deletion
Also applies to: 24-31, 90-91
🤖 Prompt for AI Agents
In accesses/migrations/0019_remove_multiple_rights.py around lines 13-14, 24-31,
and 90-91, the migration deletes all rights data without transactional safety or
proper error handling, risking data loss if the process fails mid-way. Wrap the
deletion and rights loading steps inside a single database transaction to ensure
atomicity, modify the error handling in load_new_rights to raise exceptions and
fail the migration on errors, and implement a backup mechanism for the
accesses_right table before deletion to allow recovery if needed.
There was a problem hiding this comment.
@thicham43 Understood! I appreciate you confirming that the migration approach is safe in your context. You likely have additional safeguards or considerations in place that ensure the data integrity during the migration process.
| roles = [self.role_data_accesses_manager, | ||
| self.role_admin_accesses_reader, | ||
| self.role_admin_accesses_manager, | ||
| self.role_export_accesses_manager, | ||
| self.role_admin_accesses_manager, | ||
| self.role_data_accesses_manager] |
There was a problem hiding this comment.
Fix duplicate role assignment.
The roles list contains self.role_admin_accesses_manager twice, which doesn't align with the comment indicating different access levels (S+I) for different perimeters.
Based on the pattern in other test methods and the hierarchy diagram, this should likely use different role variants:
roles = [self.role_data_accesses_manager,
self.role_admin_accesses_manager,
- self.role_admin_accesses_manager,
+ self.role_admin_accesses_manager_same_level, # or _inf_levels based on the test scenario
self.role_data_accesses_manager]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| roles = [self.role_data_accesses_manager, | |
| self.role_admin_accesses_reader, | |
| self.role_admin_accesses_manager, | |
| self.role_export_accesses_manager, | |
| self.role_admin_accesses_manager, | |
| self.role_data_accesses_manager] | |
| roles = [self.role_data_accesses_manager, | |
| self.role_admin_accesses_manager, | |
| self.role_admin_accesses_manager_same_level, # or _inf_levels based on the test scenario | |
| self.role_data_accesses_manager] |
🤖 Prompt for AI Agents
In accesses/tests/test_view_perimeters.py around lines 59 to 62, the roles list
has a duplicate entry of self.role_admin_accesses_manager, which conflicts with
the intended different access levels for perimeters. Replace the second
occurrence of self.role_admin_accesses_manager with the appropriate distinct
role variant, such as self.role_admin_accesses_manager_inherited or another
relevant role, to reflect the correct access hierarchy and match the pattern
used in other test methods.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
admin_cohort/apps.py (1)
4-4: Consider removing the intermediate variable.The
envvariable is only used twice and doesn't provide significant value. You could useos.environ.get()directly.-env = os.environThen update the usage:
- auth_hooks = env.get("USER_AUTHENTICATION_HOOKS") - user_identity_hooks = env.get("USER_IDENTITY_HOOKS") + auth_hooks = os.environ.get("USER_AUTHENTICATION_HOOKS") + user_identity_hooks = os.environ.get("USER_IDENTITY_HOOKS")cohort_job_server/cohort_creator.py (1)
43-45: Improve job status handling with consistent logging.The explicit handling of failed and other terminal job statuses provides clearer logging and better observability. The use of
_logger_err.errorfor failures and_logger.infofor other statuses is appropriate.However, consider using the
is_end_stateproperty from theJobStatusenum to make the logic more maintainable:if job_status in (JobStatus.finished, JobStatus.failed): data["request_job_duration"] = str(timezone.now() - cohort.created_at) if job_status == JobStatus.failed: data["request_job_fail_msg"] = data.pop(ERR_MESSAGE, None) _logger_err.error(f"CohortResult[{cohort.uuid}] - Failed") - else: + elif job_status.is_end_state: _logger.info(f"CohortResult[{cohort.uuid}] - Ended with status: {job_status}")cohort_job_server/cohort_counter.py (1)
73-77: Improve job status handling with explicit failure logging.The explicit handling of failed job statuses with error logging provides better observability and debugging capabilities. The else branch for other job statuses ensures all terminal states are properly logged.
Similar to the cohort_creator.py suggestion, consider using the
is_end_stateproperty from theJobStatusenum for consistency:if job_status == JobStatus.finished: # ... existing logic ... _logger.info(f"DatedMeasure[{dm.uuid}] successfully updated from Query Executor") elif job_status == JobStatus.failed: data["request_job_fail_msg"] = data.pop(ERR_MESSAGE, None) _logger_err.error(f"DatedMeasure[{dm.uuid}] - Failed") - else: + elif job_status.is_end_state: _logger.info(f"DatedMeasure[{dm.uuid}] - Ended with status: {job_status}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
admin_cohort/apps.py(1 hunks)admin_cohort/management/commands/load_initial_data.py(3 hunks)cohort/serializers.py(1 hunks)cohort/services/dated_measure.py(1 hunks)cohort_job_server/cohort_counter.py(2 hunks)cohort_job_server/cohort_creator.py(3 hunks)cohort_job_server/query_executor_api/cohort_requests/base_cohort_request.py(1 hunks)cohort_job_server/query_executor_api/query_executor_client.py(1 hunks)cohort_job_server/query_executor_api/query_executor_requester.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- cohort_job_server/query_executor_api/cohort_requests/base_cohort_request.py
- cohort_job_server/query_executor_api/query_executor_client.py
- cohort_job_server/query_executor_api/query_executor_requester.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
cohort/serializers.py (1)
cohort/models/request.py (1)
dated_measures(20-21)
cohort_job_server/cohort_counter.py (1)
admin_cohort/types.py (1)
JobStatus(35-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (11)
cohort/services/dated_measure.py (1)
65-65: Approve with caution: validatesnapshot_idhandling and access implicationsThe addition of
snapshot_idto the WebSocket payload gives clients valuable context, but please double-check:
- That
dm.request_query_snapshot_idcan’t beNone(or is safely serialized asnull) so front-end handlers won’t break.- That exposing this internal ID aligns with your access/roles refactor and doesn’t leak unauthorized snapshot information.
Location to review:
- cohort/services/dated_measure.py, in
ws_send_to_clientat the line"snapshot_id": dm.request_query_snapshot_id,admin_cohort/apps.py (2)
1-1: LGTM: Proper import for environment variable access.The
osimport is necessary and correctly placed for accessing environment variables.
10-14: Consider adding hook validation for security.The current implementation trusts environment variables without validation, which could lead to runtime errors or security issues if invalid hook paths are provided.
Consider adding validation to ensure hook paths are valid and secure:
def validate_hook_path(hook_path): """Validate that hook path is a valid Python import path.""" if not hook_path or not isinstance(hook_path, str): return False # Add validation logic for your specific hook format return True # Then use in the HOOKS configuration: auth_hooks_list = [hook.strip() for hook in auth_hooks.split(",") if validate_hook_path(hook.strip())] if auth_hooks else []You should verify what format these hooks should have and implement appropriate validation.
cohort/serializers.py (1)
276-276: Good addition of clarifying comment.The comment helps explain the purpose of the filter condition, improving code readability.
cohort_job_server/cohort_creator.py (2)
10-10: LGTM: Proper error logger import.Adding
_logger_errimport is appropriate for the new error logging functionality introduced in the failure handling logic below.
61-61: LGTM: Consistent log message formatting.The change from "QUERY_EXECUTOR" to "Query Executor" aligns with the standardization effort mentioned in the AI summary and improves readability.
admin_cohort/management/commands/load_initial_data.py (2)
19-19: LGTM: Environment variable for query executor username.Adding the environment variable allows for configuration flexibility while maintaining a sensible default.
31-31: LGTM: Method call integration.The call to
setup_query_executor_user_account()is properly placed after the admin user setup.cohort_job_server/cohort_counter.py (3)
72-72: LGTM: Consistent log message formatting.The change from "QUERY_EXECUTOR" to "Query Executor" maintains consistency with the logging standardization across the codebase.
95-99: LGTM: Consistent failure handling pattern.The explicit handling of failed job statuses in the feasibility study method follows the same pattern established in the dated measure handler, ensuring consistency across the codebase.
101-101: LGTM: Appropriate logging level change.Changing from
_logger_err.exceptionto_logger_err.erroris appropriate here since the exception is being re-raised and will be handled upstream. This avoids duplicate stack traces in logs while maintaining error visibility.
admin_cohort/apps.py
Outdated
| HOOKS = {"USER_AUTHENTICATION": auth_hooks and auth_hooks.split(",") or [], | ||
| "USER_IDENTITY": user_identity_hooks and user_identity_hooks.split(",") or [], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle whitespace and improve readability.
The current implementation doesn't handle whitespace around commas in environment variables, which could lead to malformed hook names.
- HOOKS = {"USER_AUTHENTICATION": auth_hooks and auth_hooks.split(",") or [],
- "USER_IDENTITY": user_identity_hooks and user_identity_hooks.split(",") or [],
+ HOOKS = {
+ "USER_AUTHENTICATION": [hook.strip() for hook in auth_hooks.split(",")] if auth_hooks else [],
+ "USER_IDENTITY": [hook.strip() for hook in identity_hooks.split(",")] if identity_hooks else [],
+ }This change:
- Strips whitespace from individual hook names
- Uses more readable conditional logic
- Improves formatting consistency
- Handles empty strings properly
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In admin_cohort/apps.py around lines 13 to 14, the code splits environment
variable strings by commas but does not strip whitespace from each hook name,
which can cause malformed entries. Update the code to split the strings by
commas, then strip whitespace from each resulting hook name. Also, refactor the
conditional logic to be clearer and handle empty strings properly, ensuring the
HOOKS dictionary values are lists of clean, trimmed hook names or empty lists if
the variables are not set.
admin_cohort/apps.py
Outdated
| auth_hooks = env.get("USER_AUTHENTICATION_HOOKS") | ||
| user_identity_hooks = env.get("USER_IDENTITY_HOOKS") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve consistency and add input validation.
The variable naming is inconsistent (auth_hooks vs user_identity_hooks), and there's no validation of the environment variable values.
- auth_hooks = env.get("USER_AUTHENTICATION_HOOKS")
- user_identity_hooks = env.get("USER_IDENTITY_HOOKS")
+ auth_hooks = os.environ.get("USER_AUTHENTICATION_HOOKS", "")
+ identity_hooks = os.environ.get("USER_IDENTITY_HOOKS", "")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In admin_cohort/apps.py around lines 10 to 11, the variable names auth_hooks and
user_identity_hooks are inconsistent in style and there is no validation for the
environment variable values. Rename the variables to use a consistent naming
pattern, such as both using snake_case with similar prefixes or suffixes, and
add checks to validate that the retrieved environment variables are not None or
empty before using them, raising an appropriate error or handling the case if
validation fails.
| def setup_query_executor_user_account(self) -> None: | ||
| _ = self.create_profile(username=QUERY_EXECUTOR_USERNAME, | ||
| firstname='Query', | ||
| lastname='EXECUTOR', | ||
| email='[email protected]', | ||
| password='1234' | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Address security concerns with hardcoded credentials.
The method creates a system user for query executor operations but has several security concerns:
- Hardcoded password: The password
'1234'is hardcoded and weak - Hardcoded email: Using
'[email protected]'may not be appropriate for production - No role assignment: Unlike the admin user, no role or permissions are assigned to this user
Apply this diff to improve security:
def setup_query_executor_user_account(self) -> None:
+ query_executor_password = env.get('QUERY_EXECUTOR_PASSWORD', '1234')
+ query_executor_email = env.get('QUERY_EXECUTOR_EMAIL', '[email protected]')
+
_ = self.create_profile(username=QUERY_EXECUTOR_USERNAME,
firstname='Query',
lastname='EXECUTOR',
- email='[email protected]',
- password='1234'
+ email=query_executor_email,
+ password=query_executor_password
)Additionally, consider whether this user needs specific roles or permissions for its intended purpose.
I need to verify how create_profile is implemented and whether an env utility is available before suggesting the diff. Let me gather that:
🏁 Script executed:
#!/bin/bash
# Locate and preview the management command file
fd load_initial_data.py
sed -n '1,50p' admin_cohort/management/commands/load_initial_data.py
# Find where create_profile is defined and check its parameters
rg -n "def create_profile" .
# Check for any existing env utility or import
rg -n "import .*env" .
rg -n "env\." .Length of output: 7894
I’d like to see how the admin account setup handles role assignment to mirror that for the query executor. Gathering that now:
🏁 Script executed:
#!/bin/bash
# Show the setup_admin_user_account implementation to check role assignment
rg -n "def setup_admin_user_account" -A 30 admin_cohort/management/commands/load_initial_data.pyLength of output: 1306
Externalize Query Executor credentials and assign permissions
To secure the query executor account, replace the hardcoded email and password with environment-backed defaults and mirror the admin’s role‐and‐access setup:
• File to update: admin_cohort/management/commands/load_initial_data.py
• Location: inside setup_query_executor_user_account (around lines 57–63)
Suggested diff:
def setup_query_executor_user_account(self) -> None:
- _ = self.create_profile(username=QUERY_EXECUTOR_USERNAME,
- firstname='Query',
- lastname='EXECUTOR',
- email='[email protected]',
- password='1234'
- )
+ # Read credentials from environment (fallback to defaults)
+ query_executor_email = env.get('QUERY_EXECUTOR_EMAIL', '[email protected]')
+ query_executor_password = env.get('QUERY_EXECUTOR_PASSWORD', '1234')
+
+ profile = self.create_profile(
+ username=QUERY_EXECUTOR_USERNAME,
+ firstname='Query',
+ lastname='EXECUTOR',
+ email=query_executor_email,
+ password=query_executor_password
+ )
+
+ # TODO: assign minimal role and perimeter access as needed
+ # e.g., create a “Query Executor” Role and link it to the appropriate perimeter:
+ # rights = {f.name: False for f in Role._meta.fields if f.name.startswith("right_")}
+ # query_role = Role.objects.create(name="Query Executor", **rights)
+ # Access.objects.create(
+ # profile=profile,
+ # role=query_role,
+ # perimeter=<desired_perimeter>,
+ # start_datetime=timezone.now(),
+ # end_datetime=timezone.now() + timedelta(days=365)
+ # )• Environment utility (env = os.environ) is already imported.
• Ensure you define and scope the “Query Executor” role and perimeter to enforce least privilege.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def setup_query_executor_user_account(self) -> None: | |
| _ = self.create_profile(username=QUERY_EXECUTOR_USERNAME, | |
| firstname='Query', | |
| lastname='EXECUTOR', | |
| email='[email protected]', | |
| password='1234' | |
| ) | |
| def setup_query_executor_user_account(self) -> None: | |
| # Read credentials from environment (fallback to defaults) | |
| query_executor_email = env.get('QUERY_EXECUTOR_EMAIL', '[email protected]') | |
| query_executor_password = env.get('QUERY_EXECUTOR_PASSWORD', '1234') | |
| profile = self.create_profile( | |
| username=QUERY_EXECUTOR_USERNAME, | |
| firstname='Query', | |
| lastname='EXECUTOR', | |
| email=query_executor_email, | |
| password=query_executor_password | |
| ) | |
| # TODO: assign minimal role and perimeter access as needed | |
| # e.g., create a “Query Executor” Role and link it to the appropriate perimeter: | |
| # rights = {f.name: False for f in Role._meta.fields if f.name.startswith("right_")} | |
| # query_role = Role.objects.create(name="Query Executor", **rights) | |
| # Access.objects.create( | |
| # profile=profile, | |
| # role=query_role, | |
| # perimeter=<desired_perimeter>, | |
| # start_datetime=timezone.now(), | |
| # end_datetime=timezone.now() + timedelta(days=365) | |
| # ) |
🤖 Prompt for AI Agents
In admin_cohort/management/commands/load_initial_data.py around lines 57 to 63,
replace the hardcoded email and password in setup_query_executor_user_account
with values read from environment variables using the existing env utility.
Additionally, assign the query executor user the appropriate role and
permissions that mirror the admin's access but scoped to enforce least
privilege, defining and applying a specific "Query Executor" role and perimeter
as needed.
d6cb4c2 to
ce02a8d
Compare
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores