Api v2 get customer #955
Api v2 get customer #955
Conversation
…n_alert_filter_is_not_found
…o customers_read permission
WalkthroughThis pull request refactors user context handling throughout the application by adding explicit user parameters to business logic functions, renaming customer-related functions to align terminology, restructuring access control assignment flows, and updating REST API method names from Changes
Sequence DiagramssequenceDiagram
participant User
participant API
participant Business
participant AccessControl
participant DB
rect rgb(200, 220, 240)
note right of User: Old Case Creation Flow
User->>API: POST /cases
API->>Business: cases_create(case, template_id)
Business->>AccessControl: ac_set_new_case_access(org_members, case_id)
AccessControl->>DB: [implicit user from context]
end
rect rgb(220, 240, 200)
note right of User: New Case Creation Flow
User->>API: POST /cases (with iris_current_user)
API->>Business: cases_create(iris_current_user, case, template_id)
Business->>Business: case.owner_id = user.id
Business->>AccessControl: ac_set_new_case_access(user, case_id, customer_id)
AccessControl->>DB: add_several_user_effective_access([user.id], case_id, ...)
AccessControl->>DB: set_user_case_access(user, case_id)
end
sequenceDiagram
participant Caller
participant IocRoutes
participant DB as case_iocs_db
participant AC as access_control
rect rgb(240, 220, 200)
note right of Caller: Old IoC Links Flow
Caller->>IocRoutes: GET /case/{id}/iocs/{ioc_id}/links
IocRoutes->>DB: get_ioc_links(ioc_id)
DB->>AC: [internally calls ac_get_fast_user_cases_access]
DB-->>IocRoutes: [filtered links]
end
rect rgb(200, 240, 220)
note right of Caller: New IoC Links Flow (Caller Computes Limits)
Caller->>IocRoutes: GET /case/{id}/iocs/{ioc_id}/links
IocRoutes->>AC: ac_get_fast_user_cases_access(user.id)
AC-->>IocRoutes: user_limitations
IocRoutes->>DB: get_ioc_links(ioc_id, user_limitations)
DB-->>IocRoutes: [filtered links]
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: This PR spans ~40 files with heterogeneous changes including function signature updates across multiple layers (business, data access, REST API), rename refactorings (client→customer), explicit user context threading through case/access control logic, and access control restructuring. While many changes follow consistent patterns (get→read renames, user parameter additions), each affected layer requires verification that the parameter threading is correct and access control logic remains sound. The scope, cross-layer impact, and logic density (especially in access control and case creation) warrant careful review of signature changes, call sites, and authorization implications. Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/app/datamgmt/manage/manage_case_state_db.py (1)
22-28: Fix incorrect return type annotation.The function signature declares
-> List[dict]but the implementation now returnsCaseState.query.all(), which yields a list ofCaseStateORM objects, not dictionaries. This breaks the contract and will mislead callers.Apply this diff to correct the return type:
-def get_case_states_list() -> List[dict]: +def get_case_states_list() -> List[CaseState]: """Get a list of case state Returns: - List[dict]: List of case state + List[CaseState]: List of case state """ return CaseState.query.all()Also add the import at the top of the file if not already present:
from app.models.cases import CaseStatesource/app/blueprints/rest/manage/manage_customers_routes.py (1)
309-310: Misleading activity message (“Customer” vs “Contact”).Should log “Deleted contact …”.
- track_activity(f"Deleted Customer with ID {contact_id}", ctx_less=True) + track_activity(f"Deleted contact with ID {contact_id}", ctx_less=True)
🧹 Nitpick comments (8)
source/app/business/events.py (1)
47-47: Consider consistent argument style within the file.Line 47 now uses a positional argument while line 80 uses a keyword argument (
caseid=) for the same function. For consistency and clarity, consider using the same style throughout.Apply this diff to make line 80 consistent with line 47:
- update_timeline_state(caseid=event.case_id) + update_timeline_state(event.case_id)Or alternatively, revert line 47 to use the keyword argument to match line 80:
- update_timeline_state(case_identifier) + update_timeline_state(caseid=case_identifier)source/app/blueprints/rest/manage/manage_customers_routes.py (3)
65-65: Deprecation wrapper applied correctly.Link header to /api/v2/manage/customers/{identifier} and Deprecation header are fine. Optionally add a Sunset header in endpoints.py to signal removal timeline.
85-86: Pre-flight existence checks OK.Using get_customer(client_id) before mutating contacts is fine. Consider 404 for parity with v2, but not required here.
Also applies to: 114-116
250-267: Deduplicate create side-effects via business layer.This route manually tracks activity and associates the user after create; mirror of customers_create_with_user. Centralize to avoid drift.
- from app.datamgmt.client.client_db import create_customer + from app.business.customers import customers_create_with_user @@ - create_customer(customer) + customers_create_with_user(iris_current_user, customer) @@ - track_activity(f"Added customer {customer.name}", ctx_less=True) - - # Associate the created customer with the current user - add_user_to_customer(iris_current_user.id, customer.client_id) + # side-effects handled in customers_create_with_usersource/app/blueprints/rest/v2/manage_routes/customers.py (1)
71-74: Rename handler for clarity.get_event handles a customer resource; rename to get_customer to avoid confusion.
-@customers_blueprint.get('/<int:identifier>') -@ac_api_requires(Permissions.customers_read)) -def get_event(identifier): - return customers.read(identifier) +@customers_blueprint.get('/<int:identifier>') +@ac_api_requires(Permissions.customers_read)) +def get_customer(identifier): + return customers.read(identifier)source/app/business/customers.py (2)
28-31: Return the created entity for consistency.Returning the customer aligns with customers_create_with_user and simplifies callers.
-def customers_create(customer: Client): - create_customer(customer) +def customers_create(customer: Client) -> Client: + create_customer(customer) + return customer
33-37: Mirror return value on user-aware create.Helps fluent usage and symmetry with customers_create.
-def customers_create_with_user(user, customer: Client): +def customers_create_with_user(user, customer: Client) -> Client: create_customer(customer) track_activity(f'Added customer {customer.name}', ctx_less=True) add_user_to_customer(user.id, customer.client_id) + return customersource/app/iris_engine/access_control/utils.py (1)
365-365: Clarify the distinction between local and importedset_user_case_accessfunctions.A function named
set_user_case_accessexists both locally (line 388) and inapp.business.access_controls(per relevant code snippets). The local version setsUserCaseAccesswithout setting effective access, while the imported version (if used elsewhere) callsset_case_effective_access_for_user. This naming collision can cause confusion and potential bugs if the wrong function is called.Consider renaming the local function to make its scope clear, e.g.,
_set_creator_case_accessor_create_user_case_access_record.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (36)
.github/workflows/ci.yml(4 hunks)pyproject.toml(2 hunks)source/app/blueprints/graphql/cases.py(1 hunks)source/app/blueprints/pages/manage/manage_cases_routes.py(3 hunks)source/app/blueprints/pages/manage/manage_customers_routes.py(2 hunks)source/app/blueprints/rest/alerts_routes.py(2 hunks)source/app/blueprints/rest/case/case_ioc_routes.py(2 hunks)source/app/blueprints/rest/manage/manage_case_state.py(1 hunks)source/app/blueprints/rest/manage/manage_cases_routes.py(1 hunks)source/app/blueprints/rest/manage/manage_customers_routes.py(5 hunks)source/app/blueprints/rest/v2/alerts.py(2 hunks)source/app/blueprints/rest/v2/case_routes/events.py(3 hunks)source/app/blueprints/rest/v2/cases.py(1 hunks)source/app/blueprints/rest/v2/dashboard.py(3 hunks)source/app/blueprints/rest/v2/manage_routes/customers.py(3 hunks)source/app/blueprints/rest/v2/manage_routes/groups.py(2 hunks)source/app/blueprints/rest/v2/manage_routes/users.py(0 hunks)source/app/business/access_controls.py(2 hunks)source/app/business/cases.py(4 hunks)source/app/business/customers.py(1 hunks)source/app/business/events.py(1 hunks)source/app/datamgmt/case/case_db.py(1 hunks)source/app/datamgmt/case/case_iocs_db.py(2 hunks)source/app/datamgmt/client/client_db.py(5 hunks)source/app/datamgmt/manage/manage_access_control_db.py(1 hunks)source/app/datamgmt/manage/manage_case_state_db.py(1 hunks)source/app/datamgmt/manage/manage_groups_db.py(1 hunks)source/app/datamgmt/manage/manage_users_db.py(1 hunks)source/app/iris_engine/access_control/utils.py(3 hunks)source/app/models/authorization.py(1 hunks)source/app/models/models.py(1 hunks)source/app/post_init.py(7 hunks)source/app/schema/marshables.py(2 hunks)tests/README.md(1 hunks)tests/tests_rest_comments.py(1 hunks)tests/tests_rest_customers.py(2 hunks)
💤 Files with no reviewable changes (1)
- source/app/blueprints/rest/v2/manage_routes/users.py
🧰 Additional context used
🧬 Code graph analysis (29)
source/app/blueprints/pages/manage/manage_customers_routes.py (1)
source/app/datamgmt/client/client_db.py (1)
get_customer(63-64)
source/app/datamgmt/case/case_db.py (2)
source/app/models/models.py (2)
ReviewStatus(815-819)Client(120-132)source/app/models/cases.py (1)
Cases(50-160)
source/app/blueprints/rest/manage/manage_case_state.py (2)
source/app/datamgmt/manage/manage_case_state_db.py (1)
get_case_states_list(22-28)source/app/schema/marshables.py (1)
CaseStateSchema(1699-1714)
source/app/blueprints/rest/v2/manage_routes/customers.py (3)
source/app/blueprints/rest/endpoints.py (2)
response_api_success(29-30)response_api_not_found(72-73)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/business/customers.py (2)
customers_create_with_user(33-36)customers_get(39-43)
source/app/blueprints/rest/v2/cases.py (1)
source/app/business/cases.py (1)
cases_create(110-141)
source/app/blueprints/rest/manage/manage_customers_routes.py (3)
source/app/blueprints/rest/v2/manage_routes/customers.py (1)
create_customer(67-68)source/app/datamgmt/client/client_db.py (2)
create_customer(103-105)get_customer(63-64)source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/blueprints/rest/alerts_routes.py (1)
source/app/iris_engine/access_control/utils.py (1)
ac_set_new_case_access(350-373)
source/app/blueprints/rest/case/case_ioc_routes.py (2)
source/app/iris_engine/access_control/utils.py (1)
ac_get_fast_user_cases_access(532-540)source/app/datamgmt/case/case_iocs_db.py (1)
get_ioc_links(120-142)
source/app/datamgmt/client/client_db.py (2)
source/app/models/models.py (1)
Client(120-132)source/app/blueprints/rest/v2/manage_routes/customers.py (1)
create_customer(67-68)
source/app/blueprints/graphql/cases.py (1)
source/app/business/cases.py (1)
cases_create(110-141)
source/app/datamgmt/manage/manage_users_db.py (1)
source/app/models/authorization.py (2)
CaseAccessLevel(38-45)ac_access_level_mask_from_val_list(262-270)
source/app/blueprints/rest/v2/manage_routes/groups.py (5)
source/app/blueprints/rest/v2/manage_routes/customers.py (1)
read(51-57)source/app/blueprints/rest/v2/manage_routes/users.py (1)
read(56-62)source/app/datamgmt/manage/manage_groups_db.py (1)
get_group(84-87)source/app/blueprints/access_controls.py (1)
wrap_with_permission_checks(344-376)source/app/models/authorization.py (1)
Permissions(48-65)
source/app/blueprints/rest/v2/dashboard.py (1)
source/app/business/cases.py (2)
cases_filter_by_user(83-84)cases_filter_by_reviewer(87-88)
source/app/blueprints/rest/v2/alerts.py (3)
source/app/blueprints/rest/v2/manage_routes/groups.py (1)
read(57-63)source/app/blueprints/rest/v2/cases.py (1)
read(115-124)source/app/blueprints/rest/v2/alerts_routes/comments.py (1)
read(69-78)
source/app/blueprints/pages/manage/manage_cases_routes.py (1)
source/app/schema/marshables.py (1)
CaseStateSchema(1699-1714)
source/app/business/access_controls.py (1)
source/app/datamgmt/manage/manage_access_control_db.py (1)
add_user_case_effective_access(196-204)
source/app/schema/marshables.py (2)
source/app/iris_engine/access_control/utils.py (1)
ac_get_fast_user_cases_access(532-540)source/app/datamgmt/case/case_iocs_db.py (1)
get_ioc_links(120-142)
source/app/post_init.py (6)
source/app/datamgmt/manage/manage_access_control_db.py (1)
add_several_user_effective_access(207-226)source/app/models/models.py (2)
Client(120-132)get_or_create(109-117)source/app/business/customers.py (2)
customers_get_by_name(46-50)customers_create(29-30)source/app/business/cases.py (1)
cases_get_first_with_customer(102-103)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/datamgmt/manage/manage_groups_db.py (1)
add_case_access_to_group(226-254)
source/app/datamgmt/manage/manage_groups_db.py (2)
source/app/iris_engine/access_control/utils.py (1)
ac_ldp_group_removal(169-182)source/app/models/authorization.py (2)
Group(100-112)ac_access_level_mask_from_val_list(262-270)
source/app/business/cases.py (5)
source/app/datamgmt/case/case_db.py (1)
get_first_case_with_customer(43-47)source/app/models/cases.py (1)
Cases(50-160)source/app/models/models.py (1)
Client(120-132)source/app/datamgmt/dashboard/dashboard_db.py (2)
list_user_cases(174-183)list_user_reviews(85-99)source/app/iris_engine/access_control/utils.py (1)
ac_set_new_case_access(350-373)
source/app/datamgmt/manage/manage_access_control_db.py (1)
source/app/models/authorization.py (1)
UserCaseEffectiveAccess(143-154)
source/app/datamgmt/case/case_iocs_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)IocComments(73-81)source/app/models/cases.py (1)
Cases(50-160)
source/app/blueprints/rest/v2/case_routes/events.py (5)
source/app/blueprints/access_controls.py (2)
ac_api_requires(379-383)ac_fast_check_current_user_has_case_access(583-584)source/app/blueprints/rest/v2/manage_routes/customers.py (1)
read(51-57)source/app/blueprints/rest/v2/cases.py (1)
read(115-124)source/app/blueprints/rest/v2/case_routes/evidences.py (1)
read(96-111)source/app/blueprints/rest/v2/events_routes/comments.py (1)
read(81-90)
tests/tests_rest_customers.py (2)
source/app/blueprints/rest/v2/manage_routes/customers.py (1)
create(41-49)tests/iris.py (3)
create(60-61)get(63-64)create_dummy_user(85-91)
source/app/datamgmt/manage/manage_case_state_db.py (1)
source/app/models/cases.py (1)
CaseState(211-219)
source/app/business/events.py (1)
source/app/datamgmt/states.py (1)
update_timeline_state(86-87)
source/app/iris_engine/access_control/utils.py (3)
source/app/datamgmt/manage/manage_access_control_db.py (1)
add_several_user_effective_access(207-226)source/app/models/authorization.py (4)
User(199-255)CaseAccessLevel(38-45)UserClient(184-196)UserCaseAccess(129-140)source/app/business/access_controls.py (1)
set_user_case_access(32-56)
source/app/business/customers.py (6)
source/app/blueprints/rest/v2/manage_routes/customers.py (1)
create_customer(67-68)source/app/datamgmt/client/client_db.py (3)
create_customer(103-105)get_customer(63-64)get_customer_by_name(217-218)source/app/models/models.py (1)
Client(120-132)source/app/iris_engine/utils/tracker.py (1)
track_activity(30-66)source/app/datamgmt/manage/manage_users_db.py (1)
add_user_to_customer(138-155)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)
source/app/blueprints/rest/manage/manage_cases_routes.py (1)
source/app/business/cases.py (1)
cases_create(110-141)
🪛 LanguageTool
tests/README.md
[grammar] ~32-~32: Ensure spelling is correct
Context: ...d_return_201 ``` Tip: this is a way to spped up the develop/run test loop. To restar...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
tests/README.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (4)
- GitHub Check: Database migration tests
- GitHub Check: Test API
- GitHub Check: Generate graphQL documentation
- GitHub Check: End to end tests
🔇 Additional comments (49)
source/app/datamgmt/manage/manage_groups_db.py (1)
23-23: LGTM! Import refactoring centralizes access control utilities.The relocation of
ac_access_level_mask_from_val_listfromapp.iris_engine.access_control.utilstoapp.models.authorizationconsolidates access control utilities in a single module, improving code organization.Also applies to: 28-28
source/app/datamgmt/manage/manage_users_db.py (1)
40-40: LGTM! Consistent import refactoring.The import path update aligns with the centralization of access control utilities in
app.models.authorization, matching the pattern applied in other modules.source/app/blueprints/rest/v2/manage_routes/groups.py (2)
57-57: LGTM! Method rename aligns with API consistency.The rename from
gettoreadis consistent with the same pattern applied in customers and users modules, improving API naming consistency across the v2 REST endpoints.
102-102: LGTM! Route handler correctly updated.The route registration properly references the renamed
readmethod.source/app/blueprints/rest/v2/alerts.py (2)
160-160: LGTM! Method rename aligns with v2 API patterns.The rename from
gettoreadis consistent with the broader refactoring across v2 REST endpoints.
258-258: LGTM! Route handler correctly updated.The route registration properly references the renamed
readmethod.source/app/datamgmt/manage/manage_access_control_db.py (1)
207-226: LGTM! Batch access assignment is well-implemented.The function correctly handles batch deletion and creation of effective access records. The approach of deleting existing records before adding new ones ensures a clean state.
.github/workflows/ci.yml (4)
134-135: LGTM! Environment setup simplified.The change from using
--env-fileto copying.env.tests.modelto.envstandardizes the environment initialization pattern and simplifies Docker Compose invocations.
171-172: LGTM! Consistent environment setup.The pattern is consistently applied across all CI jobs.
225-225: LGTM! Consistent environment setup.The pattern is consistently applied across all CI jobs.
275-276: LGTM! Consistent environment setup.The pattern is consistently applied across all CI jobs.
tests/tests_rest_comments.py (1)
540-540: LGTM! More descriptive test name.The rename clarifies the test precondition by explicitly stating "when there is a comment," improving test documentation.
pyproject.toml (2)
27-27: LGTM! Expanded architectural boundary enforcement.Adding
app.business.casesto the contract ensures the cases business module doesn't leak API-layer dependencies.
45-57: LGTM! Strengthened layer separation.The new contracts enforce proper architectural boundaries by preventing:
- Persistence layer from importing marshables (reducing coupling)
- Persistence layer from importing engine components (maintaining clear responsibilities)
source/app/datamgmt/client/client_db.py (5)
23-23: LGTM! Type hint improvement.Adding
Optionalimport enables more precise type annotations for nullable return values.
63-64: LGTM! Consistent terminology and improved type safety.The rename from
get_clienttoget_customeraligns with the broader terminology shift in the PR, and theOptional[Client]return type properly documents that the function may returnNone.
103-103: LGTM! Consistent rename.The function rename aligns with the terminology shift from "client" to "customer."
168-168: LGTM! Internal reference updated.The call to
get_customercorrectly uses the renamed function.
217-218: LGTM! Useful name-based lookup.The new
get_customer_by_namefunction provides convenient name-based retrieval to complement ID-based lookup.source/app/blueprints/rest/v2/case_routes/events.py (1)
86-102: LGTM - Consistent REST method naming.The rename from
gettoreadaligns with the REST API consistency improvements seen across the codebase. The logic remains unchanged, and all access control checks are properly preserved.source/app/blueprints/rest/case/case_ioc_routes.py (1)
79-80: LGTM - Proper user-scoped access control for IoC links.The addition of explicit user search limitations correctly implements per-user access filtering for IoC links. This ensures users only see links to cases they have access to, improving security and data isolation.
source/app/schema/marshables.py (1)
944-947: LGTM - Consistent user-scoped IoC link filtering.The schema's
get_linkmethod now properly filters IoC links based on the current user's case access, ensuring data isolation at the serialization layer. This aligns with the access control improvements throughout the PR.source/app/datamgmt/case/case_iocs_db.py (1)
120-142: LGTM - Secure fail-closed access control.The new
user_search_limitationsparameter implements proper access control filtering. The logic correctly defaults to an empty result set when no limitations are provided (line 124), following a secure fail-closed pattern. This ensures users cannot access IoC links for cases they don't have permission to view.source/app/business/cases.py (4)
83-84: LGTM - Explicit user context threading.The addition of the
userparameter makes the function's dependency on user context explicit, improving testability and clarity. This change aligns with the broader refactoring pattern across the PR.
87-88: LGTM - Explicit user context threading.The addition of the
userparameter makes the function's dependency on user context explicit, improving testability and clarity.
102-104: LGTM - Clear helper function for customer-based case retrieval.The new
cases_get_first_with_customerfunction provides a clean business-layer interface for retrieving the first case associated with a customer, properly delegating to the data layer.
110-133: LGTM - Explicit user ownership assignment.The
cases_createfunction now accepts an explicituserparameter and setscase.owner_id = user.id, making case ownership clear and removing implicit dependencies on global context. The user is also properly threaded through toac_set_new_case_access.source/app/blueprints/graphql/cases.py (1)
118-118: LGTM!The update correctly passes
iris_current_usertocases_create, aligning with the updated business logic signature that now requires explicit user context.source/app/blueprints/rest/alerts_routes.py (1)
609-609: LGTM!Both escalation flows now correctly pass
iris_current_usertoac_set_new_case_access, ensuring proper user-case association during case creation from alerts.Also applies to: 887-887
tests/tests_rest_customers.py (2)
24-24: Good practice using a sentinel constant.Using
_IDENTIFIER_FOR_NONEXISTENT_OBJECTimproves test maintainability.
78-87: LGTM!The tests properly verify the GET customer endpoint for both existing and non-existent customers.
source/app/blueprints/rest/v2/cases.py (1)
107-107: LGTM!The change correctly passes
iris_current_usertocases_create, consistent with the explicit user context pattern throughout this PR.source/app/blueprints/rest/v2/dashboard.py (2)
23-23: LGTM!Import added to support explicit user context in filtering operations.
43-43: LGTM!Both filtering functions now correctly receive explicit user context, aligning with the updated business logic signatures.
Also applies to: 62-62
source/app/blueprints/rest/manage/manage_case_state.py (1)
48-49: LGTM!The change correctly introduces serialization of case states using
CaseStateSchema, ensuring consistent data format in API responses.source/app/blueprints/pages/manage/manage_cases_routes.py (2)
44-44: LGTM!Import added to support case state serialization.
85-85: LGTM!Case states are now properly serialized before being passed to the template, ensuring consistent data format.
Also applies to: 97-97
source/app/blueprints/pages/manage/manage_customers_routes.py (1)
25-25: LGTM!The changes correctly update from
get_clienttoget_customer, aligning with the terminology standardization across the codebase.Also applies to: 118-118
source/app/blueprints/rest/manage/manage_customers_routes.py (1)
28-28: Import rename alignment looks good.Swapping to create_customer/get_customer matches the DAL rename; call sites in this file remain consistent.
Also applies to: 32-32
source/app/blueprints/rest/v2/manage_routes/customers.py (3)
25-26: Response helpers wired correctly.Using response_api_success/response_api_not_found matches v2 conventions.
45-46: Create path uses user-aware helper.customers_create_with_user(iris_current_user, …) is the right abstraction.
51-58: Read flow and 404 mapping look correct.ObjectNotFoundError → response_api_not_found maintains clean domain-to-HTTP mapping.
source/app/blueprints/rest/manage/manage_cases_routes.py (1)
254-255: Signature update aligns with business layer.Passing iris_current_user into cases_create is correct.
source/app/business/customers.py (2)
39-44: LGTM on not-found semantics.customers_get raising ObjectNotFoundError is appropriate for v2 mapping.
46-50: LGTM on get-by-name behavior.Consistent with customers_get; good.
source/app/post_init.py (1)
713-719: Initial customer bootstrap looks fine.Try/get-by-name then create avoids duplicate entries; simple and safe.
source/app/iris_engine/access_control/utils.py (3)
324-324: LGTM! Good refactoring to centralize bulk access assignment.Delegating to
add_several_user_effective_accessreduces code duplication and improves maintainability.
376-384: LGTM! Simple and correct query helper.The function correctly retrieves user access levels for a given customer. The TODO comment appropriately notes the planned refactoring to move this to the data management layer.
387-400: LGTM! Good refactoring to use explicit user parameter.Accepting an explicit
userparameter instead of relying oniris_current_userimproves testability and makes the function's dependencies clearer. The implementation correctly sets theUserCaseAccessrecord, with the caller responsible for setting effective access separately.
| logger.error(f'Multiple access found for user {user_id} and case {case_id}') | ||
|
|
||
| set_user_case_effective_access(access_level, case_id, user_id) | ||
| add_user_case_effective_access(user_id, case_id, access_level) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the add_user_case_effective_access implementation handles missing entries.
The parameter order change improves consistency (user → case → access_level). However, based on the code snippet from manage_access_control_db.py, the add_user_case_effective_access function appears to only update existing entries but doesn't create new ones if none exist:
def add_user_case_effective_access(user_identifier, case_identifier, access_level):
uac = UserCaseEffectiveAccess.query.where(and_(...)).first()
if uac:
uac = uac[0]
uac.access_level = access_level
db.session.commit()This could cause silent failures when setting access for new user-case combinations.
Run the following script to examine the full implementation of add_user_case_effective_access:
🏁 Script executed:
#!/bin/bash
# Find the complete implementation of add_user_case_effective_access
rg -A 15 "^def add_user_case_effective_access" --type=pyLength of output: 1401
I need to examine the calling context and related functions to fully understand the issue.
Let me search for more context about the function and its usage:Now let me search for the actual codebase context to see the complete picture:
Based on the script output provided and web research, I can now provide the final analysis:
Fix two critical bugs in add_user_case_effective_access.
The implementation has two issues:
-
Improper object indexing (bug): Line
uac = uac[0]attempts to index the result of.first(). SQLAlchemy's.first()returns the first result or None, not a tuple—attempting to index it will raise a TypeError when an entry exists. -
Silent failures on missing entries: The function only updates existing entries but doesn't create new ones. When no
UserCaseEffectiveAccessentry exists for a user-case combination, the function silently does nothing and commits an empty transaction.
Fix the implementation to either:
- Remove the erroneous
[0]indexing, or - Create a new entry if one doesn't exist, depending on the intended behavior
🏁 Script executed:
#!/bin/bash
# Get the context around line 67 in access_controls.py
sed -n '55,80p' source/app/business/access_controls.pyLength of output: 948
🏁 Script executed:
#!/bin/bash
# Look for the calling context and related functions
rg -n "add_user_case_effective_access\|add_several_user_effective_access" --type=py -A 3 -B 3Length of output: 0
I need to get more detailed context about the implementation and the database model:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the exact implementation of add_user_case_effective_access with full context
cat source/app/datamgmt/manage/manage_access_control_db.py | grep -A 20 "def add_user_case_effective_access"
Length of output: 701
🏁 Script executed:
#!/bin/bash
# Search for UserCaseEffectiveAccess model definition
rg -n "class UserCaseEffectiveAccess" --type=py -A 15Length of output: 1166
🏁 Script executed:
#!/bin/bash
# Look for where UserCaseEffectiveAccess entries might be created
rg -n "UserCaseEffectiveAccess(" --type=py -C 2Length of output: 98
🏁 Script executed:
#!/bin/bash
# Search for where UserCaseEffectiveAccess entries are created (using escaped regex)
rg -n "UserCaseEffectiveAccess\(" --type=py -C 3Length of output: 3359
🏁 Script executed:
#!/bin/bash
# Search for db.session.add patterns related to UserCaseEffectiveAccess
rg -n "db\.session\.add" --type=py -B 5 -A 2 | grep -A 7 -B 5 "UserCaseEffectiveAccess"Length of output: 1505
Fix two critical bugs in add_user_case_effective_access.
The implementation at source/app/datamgmt/manage/manage_access_control_db.py has two bugs:
-
Crashing indexing bug (critical): Line
uac = uac[0]attempts to index the result of.first(). SQLAlchemy's.first()returns a Model instance or None, not a tuple. When an entry exists,if uac:is True anduac[0]will crash withTypeError: 'UserCaseEffectiveAccess' object is not subscriptable. -
Silent failures on missing entries: The function only updates existing entries and never creates new ones. When no
UserCaseEffectiveAccessentry exists for a user-case combination, the function silently does nothing.
Remove the erroneous [0] indexing (assign uac.access_level directly), and either create missing entries or ensure entries are pre-created before calling this function.
🤖 Prompt for AI Agents
In source/app/business/access_controls.py around line 67, the call to
add_user_case_effective_access currently indexes the result of a .first() (uac =
uac[0]) which will raise TypeError and also silently does nothing when no entry
exists; remove the erroneous [0], set uac.access_level directly when uac is not
None, and when uac is None create a new UserCaseEffectiveAccess instance
populated with user_id, case_id and access_level, add it to the session and
persist (flush/commit) so missing entries are created rather than silently
ignored.
| def get_first_case_with_customer(customer_identifier) -> Optional[Cases]: | ||
| case = Cases.query.filter( | ||
| Cases.client_id == customer_identifier | ||
| ).first() | ||
| return case |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hint for the customer_identifier parameter.
The new get_first_case_with_customer function is missing a type annotation for the customer_identifier parameter. Based on the usage context and the fact it's compared with Cases.client_id, it should be typed as int.
Apply this diff:
-def get_first_case_with_customer(customer_identifier) -> Optional[Cases]:
+def get_first_case_with_customer(customer_identifier: int) -> Optional[Cases]:
case = Cases.query.filter(
Cases.client_id == customer_identifier
).first()
return case📝 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 get_first_case_with_customer(customer_identifier) -> Optional[Cases]: | |
| case = Cases.query.filter( | |
| Cases.client_id == customer_identifier | |
| ).first() | |
| return case | |
| def get_first_case_with_customer(customer_identifier: int) -> Optional[Cases]: | |
| case = Cases.query.filter( | |
| Cases.client_id == customer_identifier | |
| ).first() | |
| return case |
🤖 Prompt for AI Agents
In source/app/datamgmt/case/case_db.py around lines 43 to 47, the function
parameter customer_identifier lacks a type annotation; update the function
signature to annotate customer_identifier as int (customer_identifier: int)
while keeping the existing return type Optional[Cases], and run type checks to
ensure imports (Optional, Cases) are available and no other type errors occur.
| def add_user_case_effective_access(user_identifier, case_identifier, access_level): | ||
| uac = UserCaseEffectiveAccess.query.where(and_( | ||
| UserCaseEffectiveAccess.user_id == user_id, | ||
| UserCaseEffectiveAccess.case_id == case_id | ||
| UserCaseEffectiveAccess.user_id == user_identifier, | ||
| UserCaseEffectiveAccess.case_id == case_identifier | ||
| )).first() | ||
| if uac: | ||
| uac = uac[0] | ||
| uac.access_level = access_level | ||
| db.session.commit() |
There was a problem hiding this comment.
Fix incomplete logic in add_user_case_effective_access.
The function has multiple issues:
- Line 202 attempts to index
uac[0]whenuacis already a single object from.first(), which will cause a runtime error. - The function only updates existing records but never creates new ones when
uacisNone.
Apply this diff to fix the logic:
def add_user_case_effective_access(user_identifier, case_identifier, access_level):
uac = UserCaseEffectiveAccess.query.where(and_(
UserCaseEffectiveAccess.user_id == user_identifier,
UserCaseEffectiveAccess.case_id == case_identifier
)).first()
+
if uac:
- uac = uac[0]
uac.access_level = access_level
+ else:
+ uac = UserCaseEffectiveAccess()
+ uac.user_id = user_identifier
+ uac.case_id = case_identifier
+ uac.access_level = access_level
+ db.session.add(uac)
+
db.session.commit()📝 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 add_user_case_effective_access(user_identifier, case_identifier, access_level): | |
| uac = UserCaseEffectiveAccess.query.where(and_( | |
| UserCaseEffectiveAccess.user_id == user_id, | |
| UserCaseEffectiveAccess.case_id == case_id | |
| UserCaseEffectiveAccess.user_id == user_identifier, | |
| UserCaseEffectiveAccess.case_id == case_identifier | |
| )).first() | |
| if uac: | |
| uac = uac[0] | |
| uac.access_level = access_level | |
| db.session.commit() | |
| def add_user_case_effective_access(user_identifier, case_identifier, access_level): | |
| uac = UserCaseEffectiveAccess.query.where(and_( | |
| UserCaseEffectiveAccess.user_id == user_identifier, | |
| UserCaseEffectiveAccess.case_id == case_identifier | |
| )).first() | |
| if uac: | |
| uac.access_level = access_level | |
| else: | |
| uac = UserCaseEffectiveAccess() | |
| uac.user_id = user_identifier | |
| uac.case_id = case_identifier | |
| uac.access_level = access_level | |
| db.session.add(uac) | |
| db.session.commit() |
🤖 Prompt for AI Agents
In source/app/datamgmt/manage/manage_access_control_db.py around lines 196 to
204, the function add_user_case_effective_access wrongly indexes the result of
.first() and never creates a record when none exists; change the logic to treat
uac as the single ORM object (remove uac = uac[0]), and if uac is None
instantiate a new UserCaseEffectiveAccess with user_id=user_identifier,
case_id=case_identifier and access_level=access_level, add it to db.session,
otherwise update uac.access_level, then commit the session.
| if user.id in users: | ||
| del users[user.id] | ||
|
|
||
| users_full = User.query.with_entities(User.id).all() | ||
| users_full_access = list(set([u.id for u in users_full]) - set(users.keys())) | ||
|
|
||
| # Default users case access - Full access | ||
| ac_add_user_effective_access(users_full_access, case_id, CaseAccessLevel.deny_all.value) | ||
| add_several_user_effective_access(users_full_access, case_id, CaseAccessLevel.deny_all.value) | ||
|
|
||
| set_user_case_access(user, case_id) | ||
|
|
||
| add_several_user_effective_access([user.id], case_id, CaseAccessLevel.full_access.value) |
There was a problem hiding this comment.
Inefficient: Creator user unnecessarily receives deny_all before being granted full_access.
The creator is removed from the users dict at line 357, making them part of users_full_access at line 360. This causes them to receive deny_all at line 363, which is then overridden at line 367.
Apply this diff to exclude the creator from the deny list:
users = ac_apply_autofollow_groups_access(case_id)
if user.id in users:
del users[user.id]
users_full = User.query.with_entities(User.id).all()
-users_full_access = list(set([u.id for u in users_full]) - set(users.keys()))
+users_full_access = list(set([u.id for u in users_full]) - set(users.keys()) - {user.id})
# Default users case access - Full access
add_several_user_effective_access(users_full_access, case_id, CaseAccessLevel.deny_all.value)🤖 Prompt for AI Agents
In source/app/iris_engine/access_control/utils.py around lines 356 to 367, the
creator user is removed from the users dict but then ends up included in
users_full_access and receives a deny_all that is immediately overridden; change
the users_full_access computation to explicitly exclude the creator by
subtracting the creator's id (e.g., remove user.id from the set difference or
add it to the set of keys to exclude) so the creator does not get deny_all
before being granted full_access.
| def ac_access_level_mask_from_val_list(access_levels) -> int: | ||
| """ | ||
| Return an access level mask from a list of access levels | ||
| """ | ||
| am = 0 | ||
| for acc in access_levels: | ||
| am |= int(acc) | ||
|
|
||
| return am |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add type hints and input validation.
The function lacks a type hint for the access_levels parameter and doesn't validate inputs. This could lead to runtime errors if called with None, non-iterable values, or items that can't be converted to integers.
Apply this diff to add type hints and input validation:
+from typing import List, Union
+
-def ac_access_level_mask_from_val_list(access_levels) -> int:
+def ac_access_level_mask_from_val_list(access_levels: List[Union[int, CaseAccessLevel]]) -> int:
"""
Return an access level mask from a list of access levels
"""
+ if not access_levels:
+ return 0
+
am = 0
for acc in access_levels:
- am |= int(acc)
+ am |= int(acc.value if isinstance(acc, CaseAccessLevel) else acc)
return amCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In source/app/models/authorization.py around lines 262 to 270, the function
ac_access_level_mask_from_val_list lacks a parameter type hint and input
validation; update the signature to accept an iterable of int/str (e.g.,
access_levels: Iterable[Union[int, str]]) -> int, add a guard that raises
TypeError if access_levels is None or not iterable, then iterate over items and
for each attempt to convert to int inside a try/except and raise a ValueError
with a clear message if conversion fails, accumulating the bitmask with |= and
returning the final int.
| return True | ||
|
|
||
|
|
||
| # TODO try to remove this method: too generic |
There was a problem hiding this comment.
🧩 Analysis chain
Decide the fate of this generic helper (and document semantics).
If keeping it, document that it commits and requires kwargs to uniquely identify rows; otherwise plan a deprecation and migrate callers. Optional next step: introduce a non‑committing variant (returns instance, created_flag) and update call sites.
To scope the change, list usages and group by model:
🏁 Script executed:
#!/bin/bash
rg -nC1 '\bget_or_create\s*\(' --type=pyLength of output: 1351
I'll gather more information about the implementation and usage patterns of this method.
🏁 Script executed:
#!/bin/bash
# Get full implementation of get_or_create
sed -n '108,120p' source/app/models/models.pyLength of output: 320
🏁 Script executed:
#!/bin/bash
# Get context around usage in post_init.py
sed -n '1330,1345p' source/app/post_init.py
sed -n '1360,1375p' source/app/post_init.pyLength of output: 1585
🏁 Script executed:
#!/bin/bash
# Get context around usage in demo_builder.py
sed -n '138,150p' source/app/iris_engine/demo_builder.pyLength of output: 566
🏁 Script executed:
#!/bin/bash
# Check if there are any other imports or aliases of get_or_create
rg -n 'get_or_create' --type=py -B2 -A0 | head -30Length of output: 1648
🏁 Script executed:
#!/bin/bash
# Check for commit operations in models.py near the method
sed -n '100,130p' source/app/models/models.pyLength of output: 963
🏁 Script executed:
#!/bin/bash
# Look for db.session.add or db.session.commit near usages
rg -nC2 'get_or_create' source/app/post_init.py | head -40Length of output: 1238
🏁 Script executed:
#!/bin/bash
# Check if get_or_create is used anywhere else (beyond what the original search showed)
rg -c 'get_or_create' --type=pyLength of output: 96
🏁 Script executed:
#!/bin/bash
# Get a broader view of models.py around the method definition
sed -n '95,125p' source/app/models/models.pyLength of output: 831
🏁 Script executed:
#!/bin/bash
# Check imports in post_init.py to understand if get_or_create is imported
head -50 source/app/post_init.py | grep -A5 -B5 'get_or_create\|import'Length of output: 1383
🏁 Script executed:
#!/bin/bash
# Check complete function implementation including any commits
sed -n '109,115p' source/app/models/models.pyLength of output: 207
🏁 Script executed:
#!/bin/bash
# Verify if there's db.session.commit in post_init.py after get_or_create calls
sed -n '1334,1350p' source/app/post_init.py
sed -n '1365,1380p' source/app/post_init.pyLength of output: 1775
🏁 Script executed:
#!/bin/bash
# Check demo_builder for db.session.commit patterns
sed -n '140,160p' source/app/iris_engine/demo_builder.pyLength of output: 859
🏁 Script executed:
#!/bin/bash
# Check if there are tests for get_or_create
rg -l 'get_or_create' --type=py | grep -i testLength of output: 0
🏁 Script executed:
#!/bin/bash
# Detailed grep for all usages with filenames
rg -n 'get_or_create' --type=pyLength of output: 724
🏁 Script executed:
#!/bin/bash
# Check for any documentation or docstring on get_or_create
sed -n '105,120p' source/app/models/models.pyLength of output: 342
🏁 Script executed:
#!/bin/bash
# Verify the actual return behavior
python3 << 'EOF'
import re
with open('source/app/models/models.py', 'r') as f:
content = f.read()
# Find the function definition
match = re.search(r'def get_or_create\(.*?\):\n((?:.*?\n)*?)(?=^def |\Z)', content, re.MULTILINE)
if match:
print("=== get_or_create function ===")
print(match.group(0)[:500])
EOFLength of output: 531
🏁 Script executed:
#!/bin/bash
# Check how many times this method is called
rg '\bget_or_create\s*\(' --type=py | wc -lLength of output: 1
🏁 Script executed:
#!/bin/bash
# Look at how return values are used
rg -nA2 'get_or_create\(' --type=py | grep -A2 'get_or_create'Length of output: 1463
Document the committing behavior and address transaction management risks in call sites.
The get_or_create method at source/app/models/models.py:109 automatically commits after creating instances, which creates transaction management issues:
-
Auto-commit breaks atomicity: If multiple
get_or_createcalls should succeed together, an early commit can leave the database in a partial state if a later call fails. -
Exception handling bug in post_init.py: Lines 1340–1345 and 1366–1375 assign variables in try blocks then reference them after except blocks:
- If
gadm = get_or_create(...)at line 1340 raisesIntegrityErrorbefore completing the assignment, the except block at line 1345 references undefinedgadm. - Same issue with
ganalystsat line 1366.
- If
Recommended approach:
Either (A) add a docstring documenting the auto-commit behavior and fix exception handling to check variable existence before use, or (B) refactor to a non-committing variant that returns (instance, created) tuple and consolidate commits at function boundaries. The latter is safer for transactional consistency.
🤖 Prompt for AI Agents
In source/app/models/models.py around line 108, the get_or_create helper
currently auto-commits after creating records which breaks atomicity and causes
downstream exception-handling bugs; either (A) add a clear docstring on
get_or_create stating it auto-commits and update all call sites (e.g.,
post_init.py) to guard against assignment failures by checking variable
existence before using them in except blocks, or (B — preferred) refactor
get_or_create into a non-committing variant that returns (instance, created)
without committing, replace callers to collect results and perform a single
commit at appropriate function boundaries, and update post_init.py to use the
returned tuple and avoid referencing variables assigned inside try blocks unless
confirmed initialized.
| def get_or_create(session, model, **kwargs): | ||
| instance = session.query(model).filter_by(**kwargs).first() | ||
| if instance: | ||
| return instance | ||
| else: | ||
| instance = model(**kwargs) | ||
| session.add(instance) | ||
| session.commit() | ||
| return instance | ||
|
|
||
| instance = model(**kwargs) | ||
| session.add(instance) | ||
| session.commit() | ||
| return instance |
There was a problem hiding this comment.
🧩 Analysis chain
Make get_or_create atomic; handle race on concurrent creates.
Current get-then-create isn’t atomic; parallel requests can both miss and create duplicates. Guard with a DB unique constraint on the identifying fields and catch IntegrityError, re‑querying the row. Keeps existing commit semantics.
Apply this diff:
def get_or_create(session, model, **kwargs):
- instance = session.query(model).filter_by(**kwargs).first()
- if instance:
- return instance
-
- instance = model(**kwargs)
- session.add(instance)
- session.commit()
- return instance
+ instance = session.query(model).filter_by(**kwargs).first()
+ if instance:
+ return instance
+
+ instance = model(**kwargs)
+ session.add(instance)
+ try:
+ session.commit() # preserve current behavior
+ except IntegrityError:
+ session.rollback()
+ # Another transaction inserted it; fetch the canonical row
+ instance = session.query(model).filter_by(**kwargs).one()
+ return instanceAdd the missing import near other SQLAlchemy imports:
from sqlalchemy.exc import IntegrityErrorVerification (find call sites and ensure kwargs map to unique columns/constraints):
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Where is get_or_create used?
rg -nC2 '\bget_or_create\s*\(' --type=py
# 2) Inventory uniqueness hints in models (UniqueConstraint / unique=True)
rg -nC2 -P '(UniqueConstraint\(|\bunique\s*=\s*True\b)' --type=pyLength of output: 12699
[/skip_cloning]
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Check current imports in models.py
head -30 source/app/models/models.py
# 2) Check if IntegrityError is already imported
rg 'from sqlalchemy.*import|from sqlalchemy.exc' source/app/models/models.py -A1
# 3) Confirm Organisation and Group models have the unique constraints on the fields being used
rg -A15 'class Organisation' source/app/models/authorization.py | head -30
rg -A15 'class Group' source/app/models/authorization.py | head -30Length of output: 4072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check models.py imports and structure around get_or_create
sed -n '1,120p' source/app/models/models.py | head -50Length of output: 1766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get exact lines of get_or_create function with line numbers
sed -n '108,120p' source/app/models/models.pyLength of output: 320
Make get_or_create atomic; handle race on concurrent creates.
Current get-then-create isn't atomic; parallel requests can both miss and create duplicates. Guard with a DB unique constraint on the identifying fields and catch IntegrityError, re-querying the row. Keeps existing commit semantics.
Add the missing import to models.py (after other sqlalchemy imports, around line 37):
from sqlalchemy.exc import IntegrityErrorApply this diff to lines 109–117:
def get_or_create(session, model, **kwargs):
instance = session.query(model).filter_by(**kwargs).first()
if instance:
return instance
instance = model(**kwargs)
session.add(instance)
- session.commit()
+ try:
+ session.commit() # preserve current behavior
+ except IntegrityError:
+ session.rollback()
+ # Another transaction inserted it; fetch the canonical row
+ instance = session.query(model).filter_by(**kwargs).one()
return instanceAll call sites already pass kwargs matching unique columns (org_name, group_name, name).
🤖 Prompt for AI Agents
In source/app/models/models.py around lines 109 to 117, the get_or_create
function is not atomic and can create duplicates under concurrent calls; update
it to import IntegrityError (add "from sqlalchemy.exc import IntegrityError"
near other SQLAlchemy imports around line 37) and change the create path to:
construct and add the instance, attempt session.commit() inside a try block, on
IntegrityError call session.rollback(), re-query the instance via
session.query(model).filter_by(**kwargs).first() and return that result, and
re-raise or return the newly committed instance when commit succeeds; preserve
the existing commit semantics.
| for group in groups: | ||
| add_case_access_to_group(group=group, | ||
| cases_list=[case.case_id], | ||
| access_level=CaseAccessLevel.full_access.value) | ||
| ac_add_user_effective_access(users_list=[user.id], | ||
| case_id=1, | ||
| access_level=CaseAccessLevel.full_access.value) | ||
|
|
||
| return case | ||
| add_case_access_to_group(group, [case.case_id], CaseAccessLevel.full_access.value) | ||
| add_several_user_effective_access([user.id], 1, CaseAccessLevel.full_access.value) | ||
|
|
There was a problem hiding this comment.
Bug: hard-coded case identifier “1” when granting user access.
This assigns access to case ID 1 instead of the newly created/located case. Use case.case_id.
for group in groups:
add_case_access_to_group(group, [case.case_id], CaseAccessLevel.full_access.value)
- add_several_user_effective_access([user.id], 1, CaseAccessLevel.full_access.value)
+ add_several_user_effective_access([user.id], case.case_id, CaseAccessLevel.full_access.value)📝 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.
| for group in groups: | |
| add_case_access_to_group(group=group, | |
| cases_list=[case.case_id], | |
| access_level=CaseAccessLevel.full_access.value) | |
| ac_add_user_effective_access(users_list=[user.id], | |
| case_id=1, | |
| access_level=CaseAccessLevel.full_access.value) | |
| return case | |
| add_case_access_to_group(group, [case.case_id], CaseAccessLevel.full_access.value) | |
| add_several_user_effective_access([user.id], 1, CaseAccessLevel.full_access.value) | |
| for group in groups: | |
| add_case_access_to_group(group, [case.case_id], CaseAccessLevel.full_access.value) | |
| add_several_user_effective_access([user.id], case.case_id, CaseAccessLevel.full_access.value) |
🤖 Prompt for AI Agents
In source/app/post_init.py around lines 749 to 752, the call
add_several_user_effective_access is passing a hard-coded case ID of 1 instead
of the actual case; change the second argument from 1 to case.case_id so the
user is granted access to the newly created/located case (e.g.,
add_several_user_effective_access([user.id], case.case_id,
CaseAccessLevel.full_access.value)).
| Then start the development configuration of DFIR-IRIS server: | ||
| ``` | ||
| cp ../.env.tests.model ../.env | ||
| docker compose --file ../docker-compose.dev.yml up --detach --wait | ||
| ``` | ||
|
|
||
| Then run: | ||
| ``` | ||
| python -m unittest --verbose | ||
| ``` | ||
|
|
||
| To execute only one test, suffix with the fully qualified test name. Example: | ||
| ``` | ||
| python -m unittest tests_rest.TestsRest.test_create_asset_should_not_fail | ||
| python -m unittest tests_rest_assets.TestsRestAssets.test_create_asset_should_return_201 | ||
| ``` | ||
|
|
||
| Tip: this is a way to spped up the develop/run test loop. To restart only the `app` docker, do: | ||
| ``` | ||
| docker compose stop app && docker compose --file ../docker-compose.dev.yml start app | ||
| ``` | ||
|
|
||
| Finally, stop the development server: | ||
| ``` | ||
| docker compose down | ||
| ``` | ||
|
|
||
| Tip: if you want to clear database data: | ||
| ``` | ||
| docker volume rm iris-web_db_data | ||
| ``` |
There was a problem hiding this comment.
Fix typo and consider adding language specifiers.
Line 32 contains a typo: "spped" should be "speed".
Additionally, consider adding language specifiers to code blocks for better syntax highlighting (e.g., bash or shell).
Apply this diff to fix the typo:
-Tip: this is a way to spped up the develop/run test loop. To restart only the `app` docker, do:
+Tip: this is a way to speed up the develop/run test loop. To restart only the `app` docker, do:To add language specifiers, replace ``` with ```bash throughout the code blocks.
🧰 Tools
🪛 LanguageTool
[grammar] ~32-~32: Ensure spelling is correct
Context: ...d_return_201 ``` Tip: this is a way to spped up the develop/run test loop. To restar...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
43-43: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In tests/README.md around lines 16 to 45, correct the typo "spped" to "speed" on
line 32 and update all fenced code blocks in this section to include a
shell/bash language specifier by replacing each triple-backtick fence (```) with
```bash so the blocks use ```bash ... ``` for proper syntax highlighting.
| def test_get_customer_should_return_405_when_user_has_no_permission_to_read_customers(self): | ||
| body = {'customer_name': 'customer'} | ||
| response = self._subject.create('/api/v2/manage/customers', body).json() | ||
| identifier = response['customer_id'] | ||
|
|
||
| user = self._subject.create_dummy_user() | ||
| response = user.get(f'/api/v2/manage/customers/{identifier}') | ||
| self.assertEqual(403, response.status_code) |
There was a problem hiding this comment.
Correct the test method name.
The test name indicates HTTP 405 (Method Not Allowed), but the assertion expects HTTP 403 (Forbidden). Since 403 is the correct status for permission denial, update the test name to match.
Apply this diff:
- def test_get_customer_should_return_405_when_user_has_no_permission_to_read_customers(self):
+ def test_get_customer_should_return_403_when_user_has_no_permission_to_read_customers(self):
body = {'customer_name': 'customer'}
response = self._subject.create('/api/v2/manage/customers', body).json()
identifier = response['customer_id']📝 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 test_get_customer_should_return_405_when_user_has_no_permission_to_read_customers(self): | |
| body = {'customer_name': 'customer'} | |
| response = self._subject.create('/api/v2/manage/customers', body).json() | |
| identifier = response['customer_id'] | |
| user = self._subject.create_dummy_user() | |
| response = user.get(f'/api/v2/manage/customers/{identifier}') | |
| self.assertEqual(403, response.status_code) | |
| def test_get_customer_should_return_403_when_user_has_no_permission_to_read_customers(self): | |
| body = {'customer_name': 'customer'} | |
| response = self._subject.create('/api/v2/manage/customers', body).json() | |
| identifier = response['customer_id'] | |
| user = self._subject.create_dummy_user() | |
| response = user.get(f'/api/v2/manage/customers/{identifier}') | |
| self.assertEqual(403, response.status_code) |
🤖 Prompt for AI Agents
In tests/tests_rest_customers.py around lines 89 to 96, the test method name
incorrectly says "should_return_405" while the assertion checks for 403; rename
the method to reflect the expected 403 status (forbidden) — e.g., change the
method name to
test_get_customer_should_return_403_when_user_has_no_permission_to_read_customers
— ensuring the new name replaces the old one and no other code references break.
Implementation of
GET /api/v2/manage/customers/{identifier}GET /manage/customers/<int:client_id>.env.tests.model). Ideally, I would have prefered to use.env.dev.model, but it requires to clone the new front-end repository. Which would complexify the CI. And I believe repositories should ideally be entirely self-contained.The associated documentation is in PR#90 of iris-doc-src.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation
Tests