Api v2 create object comment #914
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a dedicated comments model and association tables, implements v2 comment creation endpoints with access checks and creation helpers, centralizes comment creation/deletion in business/datamgmt layers, refactors access-control helpers, marks legacy add-comment routes deprecated, captures CI container logs, and extends tests for comment flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as "REST v2 /{entity}/{id}/comments"
participant Ops as "CommentsOperations.create"
participant Loader as "_get_{entity}(id, access_levels)"
participant Biz as "business.comments_create_for_{entity}"
participant DB as DB
participant Hooks as "Module Hooks / History"
Client->>API: POST /api/v2/{entity}/{id}/comments {comment_text}
API->>Ops: create(id)
Ops->>Loader: _get_{entity}(id, required_access)
alt not found / no access
Loader-->>Ops: ObjectNotFoundError
Ops-->>Client: 404 Not Found
else found
Loader-->>Ops: {entity_object}
Ops->>Biz: comments_create_for_{entity}(iris_current_user, entity_object, comment)
Biz->>DB: insert Comments + association row
DB-->>Biz: created comment
Biz->>Hooks: on_postload_{entity}_commented
Ops-->>Client: 201 Created {serialized comment}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (16)
source/app/blueprints/rest/case/case_tasks_routes.py (1)
179-213: Make task comment creation atomic and UTC-safeReplace the early commit with a flush and use UTC timestamps to avoid orphan comments and TZ drift.
Apply this diff:
@@ def case_comment_task_add(cur_id: int, caseid: int): - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() + db.session.add(comment) + db.session.flush() # ensure comment_id is available without committing @@ - db.session.commit() + db.session.commit()source/app/blueprints/rest/case/case_notes_routes.py (1)
406-441: Make note comment creation atomic, validate case ownership, and use UTCPrevents orphan comments and ensures the note belongs to the provided case.
Apply this diff:
@@ def case_comment_note_add(cur_id, caseid): - comment.comment_case_id = caseid + # Ensure the note belongs to the case + if getattr(note, 'note_case_id', caseid) != caseid: + return response_error('Invalid note ID for this case') + + comment.comment_case_id = caseid comment.comment_user_id = iris_current_user.id - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() + db.session.add(comment) + db.session.flush() @@ - db.session.commit() + db.session.commit()source/app/blueprints/rest/case/case_timeline_routes.py (1)
127-161: Make event comment creation atomic, validate case ownership, and use UTCPrevents orphan comments and cross-case writes.
Apply this diff:
@@ def case_comment_add(cur_id, caseid): - if not event: + if not event: return response_error('Invalid event ID') + if getattr(event, 'case_id', caseid) != caseid: + return response_error('Invalid event ID for this case') @@ - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() + db.session.add(comment) + db.session.flush() @@ - db.session.commit() + db.session.commit()source/app/blueprints/rest/case/case_assets_routes.py (1)
351-383: Make asset comment creation atomic, validate case ownership, and use UTCSame pattern as other objects.
Apply this diff:
@@ def case_comment_asset_add(cur_id, caseid): - if not asset: + if not asset: return response_error('Invalid asset ID') + if getattr(asset, 'case_id', caseid) != caseid: + return response_error('Invalid asset ID for this case') @@ - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() + db.session.add(comment) + db.session.flush() @@ - db.session.commit() + db.session.commit()source/app/blueprints/rest/case/case_ioc_routes.py (1)
261-297: Make IOC comment creation atomic, enforce case match, and use UTCAligns with other endpoints and avoids orphan comments/mismatched case IDs.
Apply this diff:
@@ def case_comment_ioc_add(cur_id, caseid): - ioc = iocs_get(cur_id) + ioc = iocs_get(cur_id) + if getattr(ioc, 'case_id', caseid) != caseid: + return response_error('Invalid ioc ID for this case') @@ - comment = comment_schema.load(request.get_json()) - comment.comment_case_id = ioc.case_id + comment = comment_schema.load(request.get_json()) + comment.comment_case_id = caseid comment.comment_user_id = iris_current_user.id - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() + db.session.add(comment) + db.session.flush() @@ - db.session.commit() + db.session.commit()source/app/datamgmt/case/case_iocs_db.py (2)
286-291: Bug: case-scoped branch drops the return value.When caseid is provided, the filtered query result is computed but not returned.
-def get_ioc_by_value(ioc_value, caseid=None): - if caseid: - Ioc.query.filter(Ioc.ioc_value == ioc_value, Ioc.case_id == caseid).first() - return Ioc.query.filter(Ioc.ioc_value == ioc_value).first() +def get_ioc_by_value(ioc_value, caseid=None): + if caseid: + return Ioc.query.filter(Ioc.ioc_value == ioc_value, Ioc.case_id == caseid).first() + return Ioc.query.filter(Ioc.ioc_value == ioc_value).first()
85-94: Do not unconditionally delete Comments rows — other comment-association tables reference them.source/app/models/comments.py defines event_comments, task_comments, ioc_comments, asset_comments, evidence_comments, note_comments. The delete in source/app/datamgmt/case/case_iocs_db.py (lines 85–94) will remove Comments rows that may still be referenced; only delete Comments after confirming no other associations (e.g., NOT EXISTS), or use/ensure per-association FK ON DELETE CASCADE, or delete association rows first.
source/app/datamgmt/case/case_assets_db.py (1)
132-138: get_asset now returns Optional — either raise ObjectNotFoundError or guard call sitesMost callers already handle a missing asset (e.g., business.assets.assets_get or routes that check
if not asset), but these do not — fix them:
- source/app/blueprints/pages/case/case_assets_routes.py — asset_view_modal: dereferences
asset(asset.asset_name, etc.) without a None check; add a guard or return an error.- source/app/datamgmt/case/case_assets_db.py — update_asset(...): calls
asset = get_asset(asset_id)then updates fields without checking; add a guard or raise.Optional: annotate parameter type (e.g.
def get_asset(asset_id: int) -> Optional[CaseAssets]:) for clarity.source/app/blueprints/rest/case/case_evidences_routes.py (1)
202-207: Use 'evidences' (not 'tasks') when updating an evidence commentcase_comment_evidence_edit currently passes 'tasks' to case_comment_update; change it to 'evidences' so hooks/track_activity target evidences.
-def case_comment_evidence_edit(cur_id, com_id, caseid): - return case_comment_update(com_id, 'tasks', caseid) +def case_comment_evidence_edit(cur_id, com_id, caseid): + return case_comment_update(com_id, 'evidences', caseid)source/app/blueprints/rest/alerts_routes.py (1)
1018-1018: Wrong object_type passed to comment updater (should be 'alerts', not 'events').This breaks hook names and activity messages for alert comments.
Apply:
- return case_comment_update(com_id, 'events', None) + return case_comment_update(com_id, 'alerts', None)source/app/datamgmt/case/case_rfiles_db.py (1)
92-111: Risk:commit()insidebegin_nested()can prematurely commit the outer transaction.Calling
db.session.commit()while in abegin_nested()block can commit the whole session, breaking atomic case-level cascades. Either remove the nested block or defer committing to the caller.Option A (simplify; keep function-scoped commit, no nested tx):
-def delete_rfile(evidence: CaseReceivedFile): - with db.session.begin_nested(): - com_ids = EvidencesComments.query.with_entities( +def delete_rfile(evidence: CaseReceivedFile): + com_ids = EvidencesComments.query.with_entities( EvidencesComments.comment_id - ).filter( - EvidencesComments.comment_evidence_id == evidence.id - ).all() + ).filter(EvidencesComments.comment_evidence_id == evidence.id).all() @@ - db.session.delete(evidence) - - update_evidences_state(caseid=evidence.case_id) - - db.session.commit() + db.session.delete(evidence) + update_evidences_state(caseid=evidence.case_id) + db.session.commit()Option B (keep nested atomicity; let caller commit):
- with db.session.begin_nested(): + with db.session.begin_nested(): @@ - db.session.commit() + db.session.flush()Please pick one pattern and apply it consistently across similar delete helpers.
source/app/datamgmt/manage/manage_cases_db.py (1)
433-436: Bug: Deletes all IOC-asset links due to tautological filter.
asset.asset_id == asset.asset_idis always true, so this bulk delete can wipe every IocAssetLink.- for asset in da: - IocAssetLink.query.filter(asset.asset_id == asset.asset_id).delete() + for asset in da: + IocAssetLink.query.filter(IocAssetLink.asset_id == asset.asset_id).delete(synchronize_session=False)source/app/iris_engine/access_control/utils.py (3)
577-604: Makeac_set_case_access_for_useroptionally non-committing to reduce N commitsFrequent commits inside a per-user loop are slow and risk partial writes across multi-call flows.
Apply:
-def ac_set_case_access_for_user(user_id, case_id, access_level: int): +def ac_set_case_access_for_user(user_id, case_id, access_level: int, commit: bool = True): @@ - db.session.commit() + if commit: + db.session.commit()Update callers that batch changes to pass
commit=Falseand commit once at the end (see previous comment).
145-149: Bitwise combine group permissions uses AND; should be OR
ac_get_effective_permissions_from_groupsstarts at 0 and ANDs each group’s permissions, which will stay 0. Use OR to accumulate.Apply:
- final_perm = 0 - for group in groups: - final_perm &= group.group_permissions + final_perm = 0 + for group in groups: + final_perm |= group.group_permissions
568-575: Do not silently escalate current user; batch commits once
Current logic in ac_set_case_access_for_users sets iris_current_user to full access while logging that they were "excluded" — remove the ac_set_case_access_for_user(...) call for the current user (truly skip) or change the log to accurately reflect an intentional escalation. File: source/app/iris_engine/access_control/utils.py (ac_set_case_access_for_users, ~lines 558–574).
ac_set_case_access_for_user commits on every call (db.session.commit()); remove per-call commits and perform a single db.session.commit() after the loop. Implement by adding a commit flag to the helper or by moving commits into the caller. Files: source/app/iris_engine/access_control/utils.py (ac_set_case_access_for_user, ~lines 577–593; ac_set_case_access_for_users, ~lines 558–574).
source/app/business/comments.py (1)
225-232: Make_create_commentflush instead of commit; use UTCFlush to get
comment_idwithout committing early; keep a single commit at the service boundary.- comment.comment_case_id = case_identifier - comment.comment_user_id = current_user.id - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + comment.comment_case_id = case_identifier + comment.comment_user_id = current_user.id + now = datetime.utcnow() + comment.comment_date = now + comment.comment_update_date = now + db.session.add(comment) + db.session.flush()
🧹 Nitpick comments (30)
source/app/business/access_controls.py (1)
1-18: Empty module: either add a module docstring or remove the fileThis file only contains the license header; consider adding a short docstring describing its purpose or removing it to avoid dead modules.
source/app/blueprints/rest/case/case_tasks_routes.py (1)
179-181: Deprecation headers: consider adding Sunset and consistent Link usageendpoint_deprecated sets Deprecation=True and Link to the alternative path. Consider also setting a Sunset header (date/version), and ensure Link includes rel="deprecation" in addition to rel="alternate" in endpoints.py. Also standardize placeholders (either {id} or int:id) across all deprecation messages.
Would you like me to propose a small patch in source/app/blueprints/rest/endpoints.py to add:
- Link: rel="deprecation"
- Sunset: version/date
- Deprecation: "true" (string)?
source/app/models/comments.py (2)
36-45: Add created/updated defaults and indexes for hot paths.
- Set server-side defaults for timestamps; auto-update comment_update_date.
- Add indexes on FK columns to speed common queries (by case, user, alert).
Apply:
-from sqlalchemy import Column +from sqlalchemy import Column +from sqlalchemy import func +from sqlalchemy import Index @@ - comment_text = Column(Text) - comment_date = Column(DateTime) - comment_update_date = Column(DateTime) + comment_text = Column(Text) + comment_date = Column(DateTime, server_default=text("now()"), nullable=False) + comment_update_date = Column(DateTime, server_default=text("now()"), onupdate=func.now(), nullable=False)Outside the class (same module), define indexes:
# indexes Index('ix_comments_case_id', Comments.comment_case_id) Index('ix_comments_user_id', Comments.comment_user_id) Index('ix_comments_alert_id', Comments.comment_alert_id)Note: migrating defaults/indexes requires a DB migration.
51-115: Prefer DB-level cascades and dedup constraints on association tables.
- Without ON DELETE CASCADE, deleting a parent can orphan rows unless every code path cleans them up.
- Prevent duplicates per (comment_id, <parent_id>) pair.
Example pattern (repeat for each association):
- comment_id = Column(ForeignKey('comments.comment_id')) - comment_event_id = Column(ForeignKey('cases_events.event_id')) + comment_id = Column(ForeignKey('comments.comment_id', ondelete='CASCADE'), nullable=False) + comment_event_id = Column(ForeignKey('cases_events.event_id', ondelete='CASCADE'), nullable=False)And add unique constraints:
from sqlalchemy import UniqueConstraint class EventComments(db.Model): __tablename__ = "event_comments" __table_args__ = (UniqueConstraint('comment_id', 'comment_event_id', name='uq_event_comments_comment_event'),) ...These changes also require schema migration.
source/app/datamgmt/comments.py (1)
21-22: Import relocation looks good; prefer parentheses over backslash.Backslash continuations are fragile. Switching to parentheses avoids style/linelength pitfalls.
-from app.models.comments import Comments, EventComments, TaskComments, IocComments, AssetComments, EvidencesComments, \ - NotesComments +from app.models.comments import ( + Comments, + EventComments, + TaskComments, + IocComments, + AssetComments, + EvidencesComments, + NotesComments, +)source/app/datamgmt/case/case_events_db.py (1)
176-187: Tighten bulk-delete helper: use a subquery + synchronize_session=False; caller commitsAvoid materializing IDs in Python and make bulk deletes efficient/safe; this helper is called from manage_cases_db._delete_events and manage_cases_db.delete_case performs db.session.commit() later (see manage_cases_db.py — commit around line 429).
-def delete_events_comments_in_case(case_identifier): - com_ids = EventComments.query.with_entities( - EventComments.comment_id - ).join(CasesEvent).filter( - EventComments.comment_event_id == CasesEvent.event_id, - CasesEvent.case_id == case_identifier - ).all() - - com_ids = [c.comment_id for c in com_ids] - EventComments.query.filter(EventComments.comment_id.in_(com_ids)).delete() - Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() +def delete_events_comments_in_case(case_identifier): + subq = (EventComments.query.with_entities(EventComments.comment_id) + .join(CasesEvent) + .filter(EventComments.comment_event_id == CasesEvent.event_id, + CasesEvent.case_id == case_identifier) + ).subquery() + EventComments.query.filter(EventComments.comment_id.in_(subq)).delete(synchronize_session=False) + Comments.query.filter(Comments.comment_id.in_(subq)).delete(synchronize_session=False) + # Commit here if not wrapped by a larger transaction. + # db.session.commit()source/app/blueprints/rest/case/case_evidences_routes.py (1)
169-170: Use UTC for timestamps to match rest of codebase.Elsewhere you use datetime.utcnow(); prefer consistency.
- comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow()source/app/blueprints/rest/alerts_routes.py (2)
1036-1053: Use UTC timestamps and fix permission message; avoid double commit.
- Prefer UTC for comment timestamps for consistency with the rest of the codebase.
- Message should reflect write permission.
- One commit at the end is sufficient here.
- if not user_has_client_access(iris_current_user.id, alert.alert_customer_id): - return response_error('User not entitled to read alerts for the client', status=403) + if not user_has_client_access(iris_current_user.id, alert.alert_customer_id): + return response_error('User not entitled to write alerts for the client', status=403) @@ - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() - db.session.add(comment) - db.session.commit() + db.session.add(comment) @@ - db.session.commit() + db.session.commit()Also applies to: 1041-1043, 1049-1051, 1056-1056
1022-1022: Consider adding Sunset header support to deprecation flow.If you have an EOL date, extend endpoint_deprecated to optionally set a RFC-compliant Sunset header and use a string 'true' for Deprecation.
Additional change in source/app/blueprints/rest/endpoints.py (for maintainers’ reference):
- result.headers['Deprecation'] = True + result.headers['Deprecation'] = 'true' + if sunset := kwargs.get('sunset'): + result.headers['Sunset'] = sunset # e.g., RFC 7231 IMF-fixdatesource/app/datamgmt/case/case_notes_db.py (1)
100-111: Make deletes more robust and efficient (distinct + synchronize_session=False + empty-guard).Avoid duplicate IDs, skip no-op IN (), and speed up bulk deletes.
def delete_notes_comments_in_case(case_identifier): - com_ids = NotesComments.query.with_entities( - NotesComments.comment_id - ).join(Notes).filter( + com_ids = NotesComments.query.with_entities( + NotesComments.comment_id + ).join(Notes).filter( NotesComments.comment_note_id == Notes.note_id, Notes.note_case_id == case_identifier - ).all() - - com_ids = [c.comment_id for c in com_ids] - NotesComments.query.filter(NotesComments.comment_id.in_(com_ids)).delete() - Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() + ).distinct().all() + + com_ids = [c.comment_id for c in com_ids] + if com_ids: + NotesComments.query.filter(NotesComments.comment_id.in_(com_ids)).delete(synchronize_session=False) + Comments.query.filter(Comments.comment_id.in_(com_ids)).delete(synchronize_session=False)source/app/business/alerts.py (1)
64-71: Add type hints for clarity.Minor nit to improve readability and static analysis.
-from app.datamgmt.manage.manage_access_control_db import user_has_client_access +from app.datamgmt.manage.manage_access_control_db import user_has_client_access +from typing import Optional @@ -def _get(current_user, identifier): +def _get(current_user, identifier: int) -> Optional[Alert]:source/app/blueprints/rest/case/case_routes.py (1)
246-257: Use isinstance for numeric validation.type(x) is int is overly strict; isinstance is the conventional check.
- if user.id is None or type(user.id) is not int: + if not isinstance(user.id, int): return response_error('Invalid user id') - if case_identifier is None or type(case_identifier) is not int: + if not isinstance(case_identifier, int): return response_error('Invalid case id') - if access_level is None or type(access_level) is not int: + if not isinstance(access_level, int): return response_error('Invalid access level')source/app/blueprints/rest/case_comments.py (1)
33-36: Validate presence/type of comment_text before update.Avoids updating to None or non-strings.
rq_t = request.get_json() - comment_text = rq_t.get('comment_text') + comment_text = rq_t.get('comment_text') + if not isinstance(comment_text, str) or comment_text.strip() == '': + return response_error('Invalid comment text')source/app/datamgmt/case/case_tasks_db.py (2)
318-329: Avoid loading IDs into Python; guard empty set and use a subquery delete.Current approach loads all
comment_ids, and.in_([])can be backend-sensitive. Prefer subqueries and an early return.def delete_tasks_comments_in_case(case_identifier): - com_ids = TaskComments.query.with_entities( - TaskComments.comment_id - ).join(CaseTasks).filter( - TaskComments.comment_task_id == CaseTasks.id, - CaseTasks.task_case_id == case_identifier - ).all() - - com_ids = [c.comment_id for c in com_ids] - TaskComments.query.filter(TaskComments.comment_id.in_(com_ids)).delete() - Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() + subq = (TaskComments.query.with_entities(TaskComments.comment_id) + .join(CaseTasks, TaskComments.comment_task_id == CaseTasks.id) + .filter(CaseTasks.task_case_id == case_identifier) + .subquery()) + # No-op if subq is empty + TaskComments.query.filter(TaskComments.comment_id.in_(subq)).delete(synchronize_session=False) + Comments.query.filter(Comments.comment_id.in_(subq)).delete(synchronize_session=False)
318-329: Clarify transaction/commit semantics for bulk deletes.delete_tasks_comments_in_case (source/app/datamgmt/case/case_tasks_db.py:318) executes bulk .delete() without a transaction block; its only callsite is _delete_tasks (source/app/datamgmt/manage/manage_cases_db.py:391–392), which is run inside delete_case that commits later. Choose one policy and apply consistently:
- Option A: wrap the helper in with db.session.begin_nested() to match delete_task (safer if ever called standalone).
- Option B (recommended here): keep commits caller-managed for grouped bulk deletions and add a short comment in the helper documenting that callers must manage the transaction/commit.
tests/iris.py (3)
136-143: Break early once the module is found.Saves unnecessary iterations and avoids last-match surprises.
- for module in response['data']: - if module['module_human_name'] == module_name: - module_identifier = module['id'] - return module_identifier + for module in response['data']: + if module['module_human_name'] == module_name: + return module['id'] + return None
145-156: Handle empty/invalid modification history safely.Defensive checks reduce flakiness when payloads are unexpected.
- modification_history = response['modification_history'] + modification_history = response.get('modification_history') or {} + if not isinstance(modification_history, dict): + return None
157-168: Make polling configurable.Expose
max_attemptsandsleep_secondsfor slower CI hosts; keeps default behavior unchanged.- def wait_for_module_task(self): + def wait_for_module_task(self, max_attempts=20, sleep_seconds=1): @@ - while len(response['data']) == 0: - sleep(1) + while len(response['data']) == 0: + sleep(sleep_seconds) @@ - if attempts > 20: + if attempts > max_attempts:source/app/blueprints/rest/v2/events_routes/comments.py (1)
21-21: Unify ValidationError import across modules.Elsewhere (alerts) uses
from marshmallow.exceptions import ValidationError. Prefer the canonicalfrom marshmallow import ValidationErrorin all v2 comment routes for consistency.source/app/blueprints/rest/v2/alerts_routes/comments.py (2)
21-21: Use consistent ValidationError import.Switch to
from marshmallow import ValidationErrorto match other modules.-from marshmallow.exceptions import ValidationError +from marshmallow import ValidationError
42-49: Naming consistency: prefergetoversearch.Other v2 comment handlers use
get(...). Consider aligning for uniformity.source/app/datamgmt/case/case_rfiles_db.py (2)
112-123: Mirror the tasks helper: use subquery delete and guard empties.Avoid Python-side ID materialization and backend-specific empty IN behavior.
def delete_evidences_comments_in_case(case_identifier): - com_ids = EvidencesComments.query.with_entities( - EvidencesComments.comment_id - ).join(CaseReceivedFile).filter( - EvidencesComments.comment_evidence_id == CaseReceivedFile.id, - CaseReceivedFile.case_id == case_identifier - ).all() - - com_ids = [c.comment_id for c in com_ids] - EvidencesComments.query.filter(EvidencesComments.comment_id.in_(com_ids)).delete() - Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() + subq = (EvidencesComments.query.with_entities(EvidencesComments.comment_id) + .join(CaseReceivedFile, EvidencesComments.comment_evidence_id == CaseReceivedFile.id) + .filter(CaseReceivedFile.case_id == case_identifier) + .subquery()) + EvidencesComments.query.filter(EvidencesComments.comment_id.in_(subq)).delete(synchronize_session=False) + Comments.query.filter(Comments.comment_id.in_(subq)).delete(synchronize_session=False)
112-123: Consider centralizing “delete comments for case” logic.Several modules now implement near-identical helpers. A single persistence-layer utility would reduce duplication and drift.
source/app/blueprints/rest/v2/assets_routes/comments.py (1)
21-21: Unify ValidationError import for consistency.Other v2 comment routes import ValidationError from marshmallow. Mirror that here.
-from marshmallow.exceptions import ValidationError +from marshmallow import ValidationErrorsource/app/datamgmt/manage/manage_cases_db.py (1)
442-447: Clarify comment and variable naming (assets, not alerts).Minor readability nit; rename to reflect intent.
- # Get all alerts associated with assets in the case - alerts_to_update = db.session.query(CaseAssets).filter(CaseAssets.case_id == case_id) - - # Update case_id for the alerts - alerts_to_update.update({CaseAssets.case_id: None}, synchronize_session='fetch') + # Detach remaining assets in the case (those referenced by alerts) from the case + assets_to_detach = CaseAssets.query.filter(CaseAssets.case_id == case_id) + assets_to_detach.update({CaseAssets.case_id: None}, synchronize_session='fetch')source/app/iris_engine/access_control/utils.py (1)
318-320: Type clarity forac_fast_check_current_user_has_case_accessParameter name
access_levelnow represents a collection of levels; consider renaming and annotating for readability.Example:
-def ac_fast_check_current_user_has_case_access(cid, access_level): - return ac_fast_check_user_has_case_access(iris_current_user.id, cid, access_level) +def ac_fast_check_current_user_has_case_access(cid: int, expected_access_levels: list[CaseAccessLevel]): + return ac_fast_check_user_has_case_access(iris_current_user.id, cid, expected_access_levels)source/app/blueprints/rest/v2/evidences_routes/comments.py (1)
45-51: Access helper looks good; minor type hint improvement
_get_evidencecorrectly abstracts object lookup + access check and maps unauthorized to 404. Consider annotating thepossible_case_access_levelsasSequence[CaseAccessLevel]for flexibility.tests/tests_rest_comments.py (2)
239-241: Nit: avoid passing body to GET
get('/api/v2/alerts/{id}', body)passes an unused dict as query params. Drop the extra arg for clarity.- response = self._subject.get(f'/api/v2/alerts/{object_identifier}', body).json() + response = self._subject.get(f'/api/v2/alerts/{object_identifier}').json()
324-333: Nit: use {} for comment creation in RBAC negative testsIn the “read-only access” tests you post the RBAC payload as the comment body. It works because authz fails first, but clearer to post
{}.- response = user.create(f'/api/v2/assets/{object_identifier}/comments', body) + response = user.create(f'/api/v2/assets/{object_identifier}/comments', {})- response = user.create(f'/api/v2/evidences/{object_identifier}/comments', body) + response = user.create(f'/api/v2/evidences/{object_identifier}/comments', {})Also applies to: 360-369
source/app/business/comments.py (1)
109-121: Use UTC once; preserve single commit boundarySet timestamps with a single UTC value; you already commit at the end. This keeps audit times consistent.
- comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() + now = datetime.utcnow() + comment.comment_date = now + comment.comment_update_date = now
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.github/workflows/ci.yml(1 hunks)source/app/blueprints/rest/alerts_routes.py(1 hunks)source/app/blueprints/rest/case/case_assets_routes.py(1 hunks)source/app/blueprints/rest/case/case_evidences_routes.py(1 hunks)source/app/blueprints/rest/case/case_ioc_routes.py(1 hunks)source/app/blueprints/rest/case/case_notes_routes.py(1 hunks)source/app/blueprints/rest/case/case_routes.py(1 hunks)source/app/blueprints/rest/case/case_tasks_routes.py(1 hunks)source/app/blueprints/rest/case/case_timeline_routes.py(1 hunks)source/app/blueprints/rest/case_comments.py(1 hunks)source/app/blueprints/rest/search_routes.py(1 hunks)source/app/blueprints/rest/v2/alerts_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/assets_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/events_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/evidences_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/iocs_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/notes_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/tasks_routes/comments.py(3 hunks)source/app/business/access_controls.py(1 hunks)source/app/business/alerts.py(1 hunks)source/app/business/comments.py(4 hunks)source/app/business/evidences.py(1 hunks)source/app/datamgmt/alerts/alerts_db.py(2 hunks)source/app/datamgmt/case/case_assets_db.py(3 hunks)source/app/datamgmt/case/case_comments.py(1 hunks)source/app/datamgmt/case/case_events_db.py(2 hunks)source/app/datamgmt/case/case_iocs_db.py(3 hunks)source/app/datamgmt/case/case_notes_db.py(2 hunks)source/app/datamgmt/case/case_rfiles_db.py(3 hunks)source/app/datamgmt/case/case_tasks_db.py(2 hunks)source/app/datamgmt/comments.py(1 hunks)source/app/datamgmt/manage/manage_access_control_db.py(3 hunks)source/app/datamgmt/manage/manage_cases_db.py(6 hunks)source/app/datamgmt/manage/manage_users_db.py(0 hunks)source/app/datamgmt/reporter/report_db.py(1 hunks)source/app/iris_engine/access_control/iris_user.py(2 hunks)source/app/iris_engine/access_control/utils.py(7 hunks)source/app/models/comments.py(1 hunks)source/app/models/models.py(0 hunks)source/app/schema/marshables.py(1 hunks)tests/iris.py(3 hunks)tests/tests_rest_assets.py(2 hunks)tests/tests_rest_comments.py(4 hunks)tests/tests_rest_miscellaneous.py(1 hunks)tests/tests_rest_notes_directories.py(1 hunks)
💤 Files with no reviewable changes (2)
- source/app/models/models.py
- source/app/datamgmt/manage/manage_users_db.py
🧰 Additional context used
🧬 Code graph analysis (40)
source/app/blueprints/rest/case/case_timeline_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/schema/marshables.py (1)
source/app/models/comments.py (1)
Comments(33-48)
source/app/blueprints/rest/case/case_ioc_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/datamgmt/case/case_notes_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)NotesComments(106-114)source/app/models/models.py (1)
Notes(465-483)
source/app/blueprints/rest/case/case_notes_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/blueprints/rest/case/case_assets_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/blueprints/rest/v2/assets_routes/comments.py (7)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_asset(62-63)comments_create_for_asset(129-142)source/app/business/assets.py (1)
assets_get(70-75)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/blueprints/rest/v2/evidences_routes/comments.py (2)
get(52-62)create(64-75)
source/app/business/evidences.py (1)
source/app/datamgmt/case/case_rfiles_db.py (1)
add_rfile(62-76)
source/app/datamgmt/case/case_assets_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)AssetComments(84-92)source/app/models/models.py (1)
CaseAssets(152-180)
source/app/datamgmt/comments.py (1)
source/app/models/comments.py (7)
Comments(33-48)EventComments(51-59)TaskComments(62-70)IocComments(73-81)AssetComments(84-92)EvidencesComments(95-103)NotesComments(106-114)
source/app/blueprints/rest/search_routes.py (1)
source/app/models/comments.py (1)
Comments(33-48)
source/app/blueprints/rest/case/case_routes.py (5)
source/app/blueprints/responses.py (2)
response_error(48-54)response_success(57-63)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)source/app/datamgmt/manage/manage_users_db.py (1)
set_user_case_access(448-472)source/app/iris_engine/utils/tracker.py (1)
track_activity(30-66)source/app/util.py (1)
add_obj_history_entry(49-79)
source/app/blueprints/rest/alerts_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/blueprints/rest/case/case_tasks_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/datamgmt/reporter/report_db.py (1)
source/app/models/comments.py (1)
Comments(33-48)
source/app/blueprints/rest/case/case_evidences_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
tests/iris.py (4)
source/app/blueprints/rest/v2/assets_routes/comments.py (1)
get(51-60)source/app/blueprints/rest/v2/case_objects/events.py (1)
get(86-101)tests/rest_api.py (1)
get(49-54)tests/docker_compose.py (1)
extract_logs(28-30)
source/app/business/alerts.py (4)
source/app/datamgmt/alerts/alerts_db.py (1)
get_alert_by_id(317-332)source/app/datamgmt/manage/manage_access_control_db.py (1)
user_has_client_access(158-176)source/app/models/alerts.py (1)
Alert(46-82)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)
source/app/blueprints/rest/v2/notes_routes/comments.py (6)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/business/comments.py (2)
comments_get_filtered_by_note(74-75)comments_create_for_note(175-188)source/app/business/notes.py (1)
notes_get(68-73)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/blueprints/rest/v2/iocs_routes/comments.py (2)
get(51-59)create(61-73)
tests/tests_rest_notes_directories.py (1)
tests/iris.py (1)
get_latest_activity(169-171)
tests/tests_rest_miscellaneous.py (1)
tests/iris.py (5)
get_module_identifier_by_name(136-142)create(56-57)create_dummy_case(98-106)delete(65-66)wait_for_module_task(157-167)
source/app/blueprints/rest/v2/evidences_routes/comments.py (7)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_evidence(66-67)comments_create_for_evidence(145-157)source/app/models/models.py (1)
CaseReceivedFile(551-573)source/app/business/evidences.py (1)
evidences_get(60-64)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/datamgmt/case/case_tasks_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)TaskComments(62-70)source/app/models/models.py (1)
CaseTasks(585-608)
tests/tests_rest_assets.py (1)
tests/iris.py (1)
get_most_recent_object_history_entry(145-155)
source/app/blueprints/rest/v2/alerts_routes/comments.py (7)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/models/authorization.py (1)
Permissions(48-65)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/business/comments.py (2)
comments_get_filtered_by_alert(55-59)comments_create_for_alert(109-126)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)
source/app/datamgmt/manage/manage_cases_db.py (8)
source/app/models/comments.py (3)
Comments(33-48)IocComments(73-81)AssetComments(84-92)source/app/datamgmt/case/case_rfiles_db.py (1)
delete_evidences_comments_in_case(112-122)source/app/datamgmt/case/case_notes_db.py (1)
delete_notes_comments_in_case(100-110)source/app/datamgmt/case/case_tasks_db.py (1)
delete_tasks_comments_in_case(318-328)source/app/datamgmt/case/case_events_db.py (1)
delete_events_comments_in_case(176-186)source/app/models/iocs.py (1)
Ioc(51-76)source/app/models/models.py (10)
CaseAssets(152-180)CaseReceivedFile(551-573)NotesGroupLink(529-539)NotesGroup(513-526)NoteRevisions(486-498)Notes(465-483)NoteDirectory(501-510)CaseTasks(585-608)TaskAssignee(640-650)CaseEventCategory(309-317)source/app/datamgmt/states.py (1)
delete_case_states(80-83)
source/app/blueprints/rest/v2/tasks_routes/comments.py (9)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_task(78-79)comments_create_for_task(191-204)source/app/business/tasks.py (1)
tasks_get(83-87)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/blueprints/rest/v2/events_routes/comments.py (2)
get(52-61)create(63-74)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/blueprints/rest/v2/events_routes/comments.py (10)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_event(82-83)comments_create_for_event(207-222)source/app/business/events.py (1)
events_get(70-74)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/models/cases.py (1)
CasesEvent(173-208)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/blueprints/rest/v2/tasks_routes/comments.py (2)
get(51-60)create(62-74)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/blueprints/rest/case_comments.py (1)
source/app/business/comments.py (1)
comments_update_for_case(86-106)
tests/tests_rest_comments.py (10)
tests/iris.py (9)
create_dummy_case(98-106)create(56-57)create_dummy_user(81-82)get(59-60)get_most_recent_object_history_entry(145-155)get_module_identifier_by_name(136-142)wait_for_module_task(157-167)get_latest_activity(169-171)delete(65-66)source/app/blueprints/rest/v2/events_routes/comments.py (2)
create(63-74)get(52-61)source/app/blueprints/rest/v2/tasks_routes/comments.py (2)
create(62-74)get(51-60)source/app/blueprints/rest/v2/notes_routes/comments.py (2)
create(62-74)get(51-60)source/app/blueprints/rest/v2/assets_routes/comments.py (2)
create(62-73)get(51-60)source/app/blueprints/rest/v2/evidences_routes/comments.py (2)
create(64-75)get(52-62)source/app/blueprints/rest/v2/iocs_routes/comments.py (2)
create(61-73)get(51-59)source/app/blueprints/rest/v2/alerts_routes/comments.py (1)
create(50-59)source/app/blueprints/rest/v2/case_objects/assets.py (3)
create(82-98)get(100-112)delete(139-152)source/app/blueprints/rest/v2/alerts.py (3)
create(135-157)get(159-166)delete(202-209)
source/app/blueprints/rest/v2/iocs_routes/comments.py (7)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_ioc(70-71)comments_create_for_ioc(160-172)source/app/business/iocs.py (1)
iocs_get(47-51)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/blueprints/rest/v2/notes_routes/comments.py (2)
get(51-60)create(62-74)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/datamgmt/case/case_comments.py (1)
source/app/models/comments.py (1)
Comments(33-48)
source/app/datamgmt/case/case_events_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)EventComments(51-59)source/app/models/cases.py (1)
CasesEvent(173-208)
source/app/datamgmt/alerts/alerts_db.py (2)
source/app/datamgmt/filtering.py (2)
combine_conditions(90-105)apply_custom_conditions(108-150)source/app/models/comments.py (1)
Comments(33-48)
source/app/datamgmt/case/case_iocs_db.py (3)
source/app/models/cases.py (2)
Cases(50-160)CasesEvent(173-208)source/app/models/models.py (2)
Client(119-131)CaseAssets(152-180)source/app/models/comments.py (2)
Comments(33-48)IocComments(73-81)
source/app/models/comments.py (1)
source/app/blueprints/responses.py (1)
default(68-99)
source/app/iris_engine/access_control/utils.py (2)
source/app/datamgmt/manage/manage_access_control_db.py (2)
check_ua_case_client(80-107)get_case_effective_access(110-121)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/datamgmt/case/case_rfiles_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)EvidencesComments(95-103)source/app/models/models.py (1)
CaseReceivedFile(551-573)
source/app/business/comments.py (16)
source/app/business/alerts.py (1)
alerts_get(73-77)source/app/datamgmt/case/case_assets_db.py (1)
add_comment_to_asset(326-332)source/app/datamgmt/case/case_rfiles_db.py (1)
add_comment_to_evidence(136-142)source/app/datamgmt/case/case_iocs_db.py (1)
add_comment_to_ioc(230-236)source/app/datamgmt/case/case_notes_db.py (1)
add_comment_to_note(335-341)source/app/datamgmt/case/case_tasks_db.py (1)
add_comment_to_task(237-243)source/app/datamgmt/case/case_events_db.py (1)
add_comment_to_event(189-195)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(493-581)source/app/iris_engine/utils/tracker.py (1)
track_activity(30-66)source/app/models/comments.py (1)
Comments(33-48)source/app/models/models.py (4)
CaseAssets(152-180)CaseReceivedFile(551-573)Notes(465-483)CaseTasks(585-608)source/app/models/iocs.py (1)
Ioc(51-76)source/app/models/cases.py (1)
CasesEvent(173-208)source/app/util.py (1)
add_obj_history_entry(49-79)source/app/datamgmt/comments.py (6)
get_filtered_asset_comments(38-45)get_filtered_evidence_comments(48-55)get_filtered_ioc_comments(58-65)get_filtered_note_comments(68-75)get_filtered_task_comments(78-85)get_filtered_event_comments(88-95)source/app/datamgmt/case/case_comments.py (1)
get_case_comment(23-32)
source/app/datamgmt/manage/manage_access_control_db.py (2)
source/app/models/authorization.py (3)
UserCaseEffectiveAccess(143-154)CaseAccessLevel(38-45)UserClient(184-196)source/app/models/cases.py (1)
Cases(50-160)
⏰ 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: Build docker app
| def get_case_effective_access(user_identifier, case_identifier) -> Optional[int]: | ||
| row = UserCaseEffectiveAccess.query.with_entities( | ||
| UserCaseEffectiveAccess.access_level | ||
| ).filter( | ||
| UserCaseEffectiveAccess.user_id == user_identifier, | ||
| UserCaseEffectiveAccess.case_id == case_identifier | ||
| ).first() | ||
|
|
||
| if not row: | ||
| return None | ||
|
|
||
| return row[0] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure uniqueness of effective-access rows and handle ambiguity.
- If multiple rows exist for (user_id, case_id), .first() returns an arbitrary row, risking incorrect authorization.
- The model currently shows UniqueConstraint('case_id','user_id') not bound to table_args, so uniqueness may not be enforced.
Apply in this file (safer query):
-def get_case_effective_access(user_identifier, case_identifier) -> Optional[int]:
- row = UserCaseEffectiveAccess.query.with_entities(
- UserCaseEffectiveAccess.access_level
- ).filter(
- UserCaseEffectiveAccess.user_id == user_identifier,
- UserCaseEffectiveAccess.case_id == case_identifier
- ).first()
+def get_case_effective_access(user_identifier: int, case_identifier: int) -> Optional[int]:
+ q = (
+ UserCaseEffectiveAccess.query
+ .with_entities(UserCaseEffectiveAccess.access_level)
+ .filter(
+ UserCaseEffectiveAccess.user_id == user_identifier,
+ UserCaseEffectiveAccess.case_id == case_identifier
+ )
+ )
+ row = q.one_or_none()
if not row:
return None
- return row[0]
+ return row[0]And in app.models.authorization.UserCaseEffectiveAccess (outside this file), bind the unique constraint:
# in app/models/authorization.py
class UserCaseEffectiveAccess(db.Model):
__tablename__ = "user_case_effective_access"
__table_args__ = (
UniqueConstraint('case_id', 'user_id', name='uq_user_case_effective_access'),
)Optionally, validate values are in CaseAccessLevel.has_value before returning.
| CaseAssetsAlias = aliased(CaseAssets) | ||
|
|
||
| # Query for CaseAssets that are not referenced in alerts and match the case_id | ||
| assets_to_delete = db.session.query(CaseAssets).filter( | ||
| and_( | ||
| CaseAssets.case_id == case_identifier, | ||
| ~db.session.query(alert_assets_association).filter( | ||
| alert_assets_association.c.asset_id == CaseAssetsAlias.asset_id | ||
| ).exists() | ||
| ) | ||
| ) | ||
| # Delete the assets | ||
| assets_to_delete.delete(synchronize_session='fetch') | ||
|
|
There was a problem hiding this comment.
Fix uncorrelated subquery when selecting assets to delete.
CaseAssetsAlias isn’t correlated to the outer query; result is undefined and may compile incorrectly. Use relationship .alerts.any() or correlate on CaseAssets.asset_id.
- CaseAssetsAlias = aliased(CaseAssets)
-
- # Query for CaseAssets that are not referenced in alerts and match the case_id
- assets_to_delete = db.session.query(CaseAssets).filter(
- and_(
- CaseAssets.case_id == case_identifier,
- ~db.session.query(alert_assets_association).filter(
- alert_assets_association.c.asset_id == CaseAssetsAlias.asset_id
- ).exists()
- )
- )
+ # Delete assets in this case that are not referenced by any alert
+ assets_to_delete = CaseAssets.query.filter(
+ CaseAssets.case_id == case_identifier,
+ ~CaseAssets.alerts.any()
+ )📝 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.
| CaseAssetsAlias = aliased(CaseAssets) | |
| # Query for CaseAssets that are not referenced in alerts and match the case_id | |
| assets_to_delete = db.session.query(CaseAssets).filter( | |
| and_( | |
| CaseAssets.case_id == case_identifier, | |
| ~db.session.query(alert_assets_association).filter( | |
| alert_assets_association.c.asset_id == CaseAssetsAlias.asset_id | |
| ).exists() | |
| ) | |
| ) | |
| # Delete the assets | |
| assets_to_delete.delete(synchronize_session='fetch') | |
| # Delete assets in this case that are not referenced by any alert | |
| assets_to_delete = CaseAssets.query.filter( | |
| CaseAssets.case_id == case_identifier, | |
| ~CaseAssets.alerts.any() | |
| ) | |
| # Delete the assets | |
| assets_to_delete.delete(synchronize_session='fetch') |
🤖 Prompt for AI Agents
In source/app/datamgmt/manage/manage_cases_db.py around lines 356 to 369, the
subquery uses CaseAssetsAlias which is not correlated to the outer CaseAssets
query causing undefined behavior; replace the uncorrelated subquery by either
using the relationship-based filter CaseAssets.alerts.any(...) to check for
referenced alerts, or build a correlated subquery that compares
alert_assets_association.c.asset_id directly to CaseAssets.asset_id (no alias)
and use .exists(); then delete the resulting assets_to_delete. Ensure you remove
the unused alias and use synchronize_session='fetch' as before.
| NoteRevisions.query.filter( | ||
| and_( | ||
| Notes.note_case_id == case_identifier, | ||
| NoteRevisions.note_id == Notes.note_id | ||
| ) | ||
| ).delete() | ||
| Notes.query.filter(Notes.note_case_id == case_identifier).delete() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid cross-table conditions in bulk delete (SQLAlchemy won’t apply relationship cascades).
This delete references Notes without a join, which is fragile. Use a subquery for note_ids in the case.
- NoteRevisions.query.filter(
- and_(
- Notes.note_case_id == case_identifier,
- NoteRevisions.note_id == Notes.note_id
- )
- ).delete()
+ NoteRevisions.query.filter(
+ NoteRevisions.note_id.in_(
+ db.session.query(Notes.note_id).filter(Notes.note_case_id == case_identifier)
+ )
+ ).delete(synchronize_session=False)📝 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.
| NoteRevisions.query.filter( | |
| and_( | |
| Notes.note_case_id == case_identifier, | |
| NoteRevisions.note_id == Notes.note_id | |
| ) | |
| ).delete() | |
| Notes.query.filter(Notes.note_case_id == case_identifier).delete() | |
| NoteRevisions.query.filter( | |
| NoteRevisions.note_id.in_( | |
| db.session.query(Notes.note_id).filter(Notes.note_case_id == case_identifier) | |
| ) | |
| ).delete(synchronize_session=False) | |
| Notes.query.filter(Notes.note_case_id == case_identifier).delete() |
🤖 Prompt for AI Agents
In source/app/datamgmt/manage/manage_cases_db.py around lines 381 to 387, the
bulk delete uses a cross-table condition referencing Notes from the
NoteRevisions.delete() call which prevents SQLAlchemy from applying relationship
cascades; change to first build a subquery that selects note_id from Notes
filtered by note_case_id == case_identifier, then delete NoteRevisions where
note_id is in that subquery, and finally delete Notes for the case_identifier;
perform deletes as bulk operations and set the appropriate synchronize_session
option (e.g. 'fetch' or False) to avoid session state inconsistencies.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
source/app/blueprints/rest/case/case_evidences_routes.py (2)
129-129: Wrong deprecation target path (assets vs evidences) on deleteThe alternative URL should reference evidences, not assets.
-@endpoint_deprecated('DELETE', '/api/v2/cases/{case_identifier}/assets/{identifier}') +@endpoint_deprecated('DELETE', '/api/v2/cases/{case_identifier}/evidences/{identifier}')
202-207: Incorrect object type passed to updater (‘tasks’ instead of ‘evidences’)This breaks evidence comment edits.
-def case_comment_evidence_edit(cur_id, com_id, caseid): - return case_comment_update(com_id, 'tasks', caseid) +def case_comment_evidence_edit(cur_id, com_id, caseid): + return case_comment_update(com_id, 'evidences', caseid)source/app/iris_engine/access_control/utils.py (1)
577-604: Fix: create-or-update bug (len==0 path does nothing) and redundant commits.When no UCEA row exists, the function commits without creating one. Also, this always-commit function is called inside loops causing commit storms.
Apply:
def ac_set_case_access_for_user(user_id, case_id, access_level: int): @@ - uac = UserCaseEffectiveAccess.query.where(and_( + uac = UserCaseEffectiveAccess.query.where(and_( UserCaseEffectiveAccess.user_id == user_id, UserCaseEffectiveAccess.case_id == case_id )).all() @@ - if len(uac) > 1: + if len(uac) > 1: logger.error(f'Multiple access found for user {user_id} and case {case_id}') for u in uac: db.session.delete(u) db.session.commit() uac = UserCaseEffectiveAccess() uac.user_id = user_id uac.case_id = case_id uac.access_level = access_level db.session.add(uac) - elif len(uac) == 1: + elif len(uac) == 1: uac = uac[0] uac.access_level = access_level + else: + # len == 0 -> create + uac = UserCaseEffectiveAccess() + uac.user_id = user_id + uac.case_id = case_id + uac.access_level = access_level + db.session.add(uac) db.session.commit()Follow-ups:
- Consider enforcing a DB unique constraint on (user_id, case_id) to prevent duplicates and remove the clean-up branch.
- Optionally accept a commit=False flag (default True) to batch updates and reduce commits in loops.
source/app/blueprints/rest/case/case_ioc_routes.py (1)
267-291: Missing case/IOC consistency check and mass-assignment risk when creating comments
- No verification that ioc.case_id matches the route’s caseid (can let a user with access to case A write on an IOC from case B).
- CommentSchema.load() accepts FK/PK fields; a client can supply comment_user_id, comment_case_id, comment_id, etc. You overwrite some, but not all are protected prior to insertion. Use a restricted schema (only=['comment_text']) and set server-controlled fields explicitly.
- Use utcnow for timestamps (consistent with comments_update_for_case) and a single transaction (flush to get PK, then one commit). Add rollbacks on failures.
Apply this diff:
def case_comment_ioc_add(cur_id, caseid): try: ioc = iocs_get(cur_id) - comment_schema = CommentSchema() + # Ensure the IOC belongs to the routed case context + if ioc.case_id != caseid: + return response_error('Invalid ioc ID') + + # Only allow the text to be client-supplied + comment_schema = CommentSchema(only=['comment_text']) - comment = comment_schema.load(request.get_json()) - comment.comment_case_id = ioc.case_id - comment.comment_user_id = iris_current_user.id - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + payload = request.get_json() or {} + comment = comment_schema.load(payload) + comment.comment_case_id = ioc.case_id + comment.comment_user_id = iris_current_user.id + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() + db.session.add(comment) + # assign PK without committing the transaction + db.session.flush() add_comment_to_ioc(ioc.ioc_id, comment.comment_id) - db.session.commit() + db.session.commit() hook_data = { 'comment': comment_schema.dump(comment), 'ioc': IocSchema().dump(ioc) } call_modules_hook('on_postload_ioc_commented', data=hook_data, caseid=ioc.case_id) track_activity(f'ioc "{ioc.ioc_value}" commented', caseid=ioc.case_id) return response_success('IOC commented', data=comment_schema.dump(comment)) except marshmallow.exceptions.ValidationError as e: + db.session.rollback() return response_error(msg='Data error', data=e.normalized_messages()) except ObjectNotFoundError: + db.session.rollback() return response_error('Invalid ioc ID')source/app/blueprints/rest/alerts_routes.py (1)
1036-1069: Harden alert comment creation against mass assignment; prefer UTC and single transaction
- Restrict client payload to comment_text only; set server-controlled fields explicitly.
- Use datetime.utcnow() for consistency with updates.
- Flush to get PK, then commit once; rollback on exceptions.
- comment_schema = CommentSchema() - - comment = comment_schema.load(request.get_json()) - comment.comment_alert_id = alert_id - comment.comment_user_id = iris_current_user.id - comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() - db.session.add(comment) - db.session.commit() + comment_schema = CommentSchema(only=['comment_text']) + payload = request.get_json() or {} + comment = comment_schema.load(payload) + comment.comment_alert_id = alert_id + comment.comment_user_id = iris_current_user.id + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow() + db.session.add(comment) + db.session.flush()And add rollbacks:
- except ValidationError as e: - return response_error(msg="Data error", data=e.normalized_messages()) + except ValidationError as e: + db.session.rollback() + return response_error(msg="Data error", data=e.normalized_messages())source/app/datamgmt/case/case_iocs_db.py (1)
286-291: Bug: caseid branch result is ignoredFunction always returns the unfiltered query; the branch lacks a return. Fix or remove if truly unused.
def get_ioc_by_value(ioc_value, caseid=None): - if caseid: - Ioc.query.filter(Ioc.ioc_value == ioc_value, Ioc.case_id == caseid).first() - - return Ioc.query.filter(Ioc.ioc_value == ioc_value).first() + q = Ioc.query.filter(Ioc.ioc_value == ioc_value) + if caseid: + q = q.filter(Ioc.case_id == caseid) + return q.first()source/app/datamgmt/manage/manage_cases_db.py (1)
433-436: Data-loss bug: filter condition is always True; deletes all IocAssetLink rows
asset.asset_id == asset.asset_idevaluates to True before SQL is built, so every row is deleted. You likely meant to filter by the model column.- for asset in da: - IocAssetLink.query.filter(asset.asset_id == asset.asset_id).delete() + for asset in da: + IocAssetLink.query.filter(IocAssetLink.asset_id == asset.asset_id).delete(synchronize_session=False)
🧹 Nitpick comments (48)
source/app/datamgmt/manage/manage_case_templates_db.py (2)
61-70: Return type changed to Optional — update docstringDocstring still states CaseTemplate; adjust to Optional[CaseTemplate] for consistency.
def get_case_template_by_id(cur_id: int) -> Optional[CaseTemplate]: """Get a case template @@ - Returns: - CaseTemplate: Case template + Returns: + Optional[CaseTemplate]: Case template if found, otherwise None """ return CaseTemplate.query.filter_by(id=cur_id).first()
96-102: Duplicate author-length validationAuthor length is checked twice; keep one.
- # We check that author length is not above 128 chars - if "author" in data and len(data["author"]) > 128: - return "Author cannot be longer than 128 characters." - - # We check that author length is not above 128 chars - if "author" in data and len(data["author"]) > 128: - return "Author cannot be longer than 128 characters." + # We check that author length is not above 128 chars + if "author" in data and len(data["author"]) > 128: + return "Author cannot be longer than 128 characters."source/app/business/alerts.py (1)
64-71: Centralized helper _get — LGTM; consider adding typingGood consolidation. Optional: annotate return as Optional[Alert] and parameter types.
source/app/datamgmt/manage/manage_access_control_db.py (1)
80-108: Docstring: return type is Optional[int] — update and callers verified
- Replace "UserClient" in the Returns docstring with "Optional[int]" and clarify "access level integer or None".
- Call sites checked: source/app/iris_engine/access_control/utils.py:301 (assigned to variable
access_level); source/app/blueprints/rest/alerts_routes.py:672, 732, 789 (used in truthiness checks). No callers access a.access_levelattribute.- No caller changes required; update the docstring only.
source/app/iris_engine/access_control/utils.py (2)
538-538: Add DB uniqueness constraint for UserCaseEffectiveAccess.You’re cleaning up duplicates at runtime. Prefer a unique index (user_id, case_id) to prevent them.
I can draft an Alembic migration if desired.
564-572: Confirm intentional self-escalation and adjust message.Setting the acting user to full_access unconditionally (Line 568) is a policy choice; confirm it’s desired for all callers. The returned string “I can’t let you do that, Dave” is user-facing and may be confusing/humorous.
Proposed tweaks:
- logs = "It's done, but I excluded you from the list of users to update, Dave" + logs = "Actor excluded from downgrade; kept full access to prevent lockout"If self-escalation isn’t always desired, gate it behind a parameter.
source/app/blueprints/rest/case/case_routes.py (2)
246-257: Use isinstance for input validation (more robust).type(x) is int fails for bools and subclasses; prefer isinstance.
- if user.id is None or type(user.id) is not int: + if user.id is None or not isinstance(user.id, int): return response_error('Invalid user id') - if case_identifier is None or type(case_identifier) is not int: + if case_identifier is None or not isinstance(case_identifier, int): return response_error('Invalid case id') - if access_level is None or type(access_level) is not int: + if access_level is None or not isinstance(access_level, int): return response_error('Invalid access level')
262-264: Avoid double commits and return consistent data.set_user_case_access commits internally; you commit again on Line 262. Consider removing the extra commit. Also, you return a message but no data; optionally include normalized payload for clients.
- db.session.commit() - return response_success(msg=f'Case access set to {access_level} for user {user.id}') + return response_success( + msg='Case access updated', + data={'user_id': user.id, 'case_id': case_identifier, 'access_level': access_level} + )source/app/business/comments.py (2)
113-115: Normalize timestamps to UTC.Use datetime.utcnow() consistently (update already uses utcnow in comments_update_for_case).
- comment.comment_date = datetime.now() - comment.comment_update_date = datetime.now() + comment.comment_date = datetime.utcnow() + comment.comment_update_date = datetime.utcnow()Apply the same change in _create_comment.
Also applies to: 228-229
121-126: Pass case context to hook (if available).Other wrappers pass caseid to call_modules_hook; alerts path omits it. If Alert has a case context, pass it for parity; if not, ignore.
- call_modules_hook('on_postload_alert_commented', hook_data) + # If alert has case_id, pass it; otherwise keep ctx-less + call_modules_hook('on_postload_alert_commented', hook_data, caseid=getattr(alert, 'case_id', None))source/app/datamgmt/comments.py (1)
33-36: Align alert comment pagination with other helpersUse _get_filtered_comments for ordering/pagination consistency across assets/iocs/notes/tasks/events.
-def get_filtered_alert_comments(alert_identifier: int, pagination_parameters: PaginationParameters) -> Pagination: - query = Comments.query.filter(Comments.comment_alert_id == alert_identifier) - return query.paginate(page=pagination_parameters.get_page(), per_page=pagination_parameters.get_per_page()) +def get_filtered_alert_comments(alert_identifier: int, pagination_parameters: PaginationParameters) -> Pagination: + query = Comments.query.filter(Comments.comment_alert_id == alert_identifier) + return _get_filtered_comments(query, pagination_parameters)source/app/blueprints/rest/alerts_routes.py (1)
1041-1043: Tweak error message to match write permissionThis is a write route; message says “read alerts”. Suggest “write alerts”.
- return response_error('User not entitled to read alerts for the client', status=403) + return response_error('User not entitled to write alerts for the client', status=403)tests/tests_rest_miscellaneous.py (1)
73-75: Assert module discovery before useGuard against get_module_identifier_by_name returning None to reduce test flakiness.
- module_identifier = self._subject.get_module_identifier_by_name('IrisCheck') + module_identifier = self._subject.get_module_identifier_by_name('IrisCheck') + self.assertIsNotNone(module_identifier)source/app/datamgmt/case/case_comments.py (1)
18-33: Type clarity improved; consider annotating parameters tooReturn type Optional[Comments] is good. Add types to parameters for completeness.
-from typing import Optional +from typing import Optional ... -def get_case_comment(comment_id, caseid) -> Optional[Comments]: +def get_case_comment(comment_id: int, caseid: Optional[int]) -> Optional[Comments]:source/app/business/evidences.py (1)
50-50: Prefer keyword args to avoid param-order couplingCall
add_rfilewith keywords to make the call resilient to future signature changes.- crf = add_rfile(evidence, case_identifier, iris_current_user.id) + crf = add_rfile(evidence=evidence, caseid=case_identifier, user_id=iris_current_user.id)tests/tests_rest_assets.py (2)
80-82: Nice centralization via Iris helper; add a guard assertAdd a quick non-null check before asserting on fields to improve failure diagnostics.
- modification = self._subject.get_most_recent_object_history_entry(response) + modification = self._subject.get_most_recent_object_history_entry(response) + self.assertIsNotNone(modification)
150-152: Mirror the non-null check hereSame rationale as above for update flow.
- modification = self._subject.get_most_recent_object_history_entry(response) + modification = self._subject.get_most_recent_object_history_entry(response) + self.assertIsNotNone(modification)source/app/datamgmt/case/case_assets_db.py (1)
132-137: Type-hint the parameter for clarityMinor readability touch-up.
-def get_asset(asset_id) -> Optional[CaseAssets]: +def get_asset(asset_id: int) -> Optional[CaseAssets]:source/app/datamgmt/case/case_events_db.py (1)
176-187: Avoid loading IDs into Python; delete via subqueryDelete directly using a subquery to reduce memory round-trips and speed up large-case cascades. Consider also disabling session synchronization for bulk deletes.
-def delete_events_comments_in_case(case_identifier): - com_ids = EventComments.query.with_entities( - EventComments.comment_id - ).join(CasesEvent).filter( - EventComments.comment_event_id == CasesEvent.event_id, - CasesEvent.case_id == case_identifier - ).all() - - com_ids = [c.comment_id for c in com_ids] - EventComments.query.filter(EventComments.comment_id.in_(com_ids)).delete() - Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() +def delete_events_comments_in_case(case_identifier): + subq = ( + db.session.query(EventComments.comment_id) + .join(CasesEvent, EventComments.comment_event_id == CasesEvent.event_id) + .filter(CasesEvent.case_id == case_identifier) + .subquery() + ) + EventComments.query.filter(EventComments.comment_id.in_(subq)).delete(synchronize_session=False) + Comments.query.filter(Comments.comment_id.in_(subq)).delete(synchronize_session=False)Please confirm the caller wraps these deletions in a transaction/commit as needed.
source/app/datamgmt/case/case_rfiles_db.py (2)
62-76: Use UTC timestamps for consistency across modulesOther creation paths use UTC (e.g., assets/events). Align evidence timestamps to UTC.
-def add_rfile(evidence: CaseReceivedFile, caseid, user_id): +def add_rfile(evidence: CaseReceivedFile, caseid, user_id): - evidence.date_added = datetime.datetime.now() + evidence.date_added = datetime.datetime.utcnow() evidence.case_id = caseid evidence.user_id = user_id
112-123: Subquery-based bulk delete for evidence commentsMirror the events/tasks approach: avoid materializing IDs; use a subquery and disable session sync.
-def delete_evidences_comments_in_case(case_identifier): - com_ids = EvidencesComments.query.with_entities( - EvidencesComments.comment_id - ).join(CaseReceivedFile).filter( - EvidencesComments.comment_evidence_id == CaseReceivedFile.id, - CaseReceivedFile.case_id == case_identifier - ).all() - - com_ids = [c.comment_id for c in com_ids] - EvidencesComments.query.filter(EvidencesComments.comment_id.in_(com_ids)).delete() - Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() +def delete_evidences_comments_in_case(case_identifier): + subq = ( + db.session.query(EvidencesComments.comment_id) + .join(CaseReceivedFile, EvidencesComments.comment_evidence_id == CaseReceivedFile.id) + .filter(CaseReceivedFile.case_id == case_identifier) + .subquery() + ) + EvidencesComments.query.filter(EvidencesComments.comment_id.in_(subq)).delete(synchronize_session=False) + Comments.query.filter(Comments.comment_id.in_(subq)).delete(synchronize_session=False)source/app/datamgmt/case/case_tasks_db.py (1)
318-329: Streamline deletion via subquery; clarify transaction ownershipUse a subquery to avoid fetching IDs into memory and disable session sync for bulk ops. Ensure the caller commits/rolls back the transaction.
-def delete_tasks_comments_in_case(case_identifier): - com_ids = TaskComments.query.with_entities( - TaskComments.comment_id - ).join(CaseTasks).filter( - TaskComments.comment_task_id == CaseTasks.id, - CaseTasks.task_case_id == case_identifier - ).all() - - com_ids = [c.comment_id for c in com_ids] - TaskComments.query.filter(TaskComments.comment_id.in_(com_ids)).delete() - Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() +def delete_tasks_comments_in_case(case_identifier): + subq = ( + db.session.query(TaskComments.comment_id) + .join(CaseTasks, TaskComments.comment_task_id == CaseTasks.id) + .filter(CaseTasks.task_case_id == case_identifier) + .subquery() + ) + TaskComments.query.filter(TaskComments.comment_id.in_(subq)).delete(synchronize_session=False) + Comments.query.filter(Comments.comment_id.in_(subq)).delete(synchronize_session=False)source/app/iris_engine/access_control/iris_user.py (2)
36-47: Avoid double-proxy and handle missing g.auth_user safelyUnwrap Flask-Login’s proxy and prefer getattr to avoid treating a present-but-None g.auth_user as truthy.
def _get_current_user(): """ Returns a compatible user object for both session and token auth For token auth, uses data from g.auth_user For session auth, returns Flask current_user """ - if has_request_context(): - if hasattr(g, 'auth_user'): - return TokenUser(g.auth_user) - return current_user - - return None + if not has_request_context(): + return None + auth = getattr(g, 'auth_user', None) + if auth: + return TokenUser(auth) + # unwrap flask-login LocalProxy to avoid double-proxying + try: + return current_user._get_current_object() + except Exception: + return current_user
50-50: Minor: simplify LocalProxy constructionPass the callable directly instead of wrapping in a lambda.
-iris_current_user = LocalProxy(lambda: _get_current_user()) +iris_current_user = LocalProxy(_get_current_user)tests/iris.py (4)
136-143: Early-return for clarityReturn as soon as the module is found.
def get_module_identifier_by_name(self, module_name): response = self.get('/manage/modules/list').json() - module_identifier = None - for module in response['data']: - if module['module_human_name'] == module_name: - module_identifier = module['id'] - return module_identifier + for module in response['data']: + if module['module_human_name'] == module_name: + return module['id'] + return None
145-156: Simplify most-recent history selection and handle empty inputUse max() and guard against missing/empty history.
@staticmethod def get_most_recent_object_history_entry(response): - modification_history = response['modification_history'] - current_timestamp = 0 - result = None - for timestamp_as_string, modification in modification_history.items(): - timestamp = float(timestamp_as_string) - if timestamp < current_timestamp: - continue - result = modification - current_timestamp = timestamp - return result + modification_history = response.get('modification_history') or {} + if not modification_history: + return None + return max( + ((float(ts), mod) for ts, mod in modification_history.items()), + key=lambda x: x[0] + )[1]
157-168: Timeout ergonomics and resilienceExpose timeout attempts as a parameter and guard KeyErrors when 'data' is missing.
-def wait_for_module_task(self): - response = self.get('/dim/tasks/list/1').json() - attempts = 0 - while len(response['data']) == 0: - sleep(1) - response = self.get('/dim/tasks/list/1').json() - attempts += 1 - if attempts > 20: - logs = self.extract_logs('worker') - raise TimeoutError(f'Timed out with logs: {logs}') - return response['data'][0] +def wait_for_module_task(self, max_attempts: int = 20): + response = self.get('/dim/tasks/list/1').json() + attempts = 0 + while not response.get('data'): + sleep(1) + response = self.get('/dim/tasks/list/1').json() + attempts += 1 + if attempts > max_attempts: + logs = self.extract_logs('worker') + raise TimeoutError(f'Timed out with logs: {logs}') + return response['data'][0]
169-171: Guard against empty activities listPrevent IndexError when no activities exist.
def get_latest_activity(self): - activities = self.get('/activities/list-all').json() - return activities['data'][0] + activities = self.get('/activities/list-all').json() + data = activities.get('data') or [] + return data[0] if data else Nonesource/app/blueprints/rest/v2/alerts_routes/comments.py (1)
50-60: Harden JSON parsing and return consistent validation errorsUse silent JSON parsing to avoid a 400 BadRequest exception before marshmallow can format errors.
def create(self, alert_identifier): try: - comment = self._schema.load(request.get_json()) + payload = request.get_json(silent=True) or {} + comment = self._schema.load(payload) comments_create_for_alert(iris_current_user, comment, alert_identifier) result = self._schema.dump(comment) return response_api_created(result) except ValidationError as e: return response_api_error('Data error', data=e.normalized_messages()) except ObjectNotFoundError: return response_api_not_found()source/app/datamgmt/case/case_iocs_db.py (1)
93-94: Guard empty IN and use bulk-delete optionsAvoid emitting “IN ()” and speed up by disabling session synchronization.
- Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() + if com_ids: + Comments.query.filter(Comments.comment_id.in_(com_ids)).delete(synchronize_session=False)source/app/blueprints/rest/v2/assets_routes/comments.py (3)
21-21: Standardize ValidationError import for consistencyOther v2 comment routes import from
marshmallow. Align this file.-from marshmallow.exceptions import ValidationError +from marshmallow import ValidationError
45-49: Make access check explicit over multiple allowed levels
ac_fast_check_current_user_has_case_accessappears to accept a single level; here a list is passed. To avoid relying on implicit behavior, iterate explicitly. Also confirms intent (“any of”).- def _get_asset(asset_identifier, possible_case_access_levels): - asset = assets_get(asset_identifier) - if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels): - raise ObjectNotFoundError() - return asset + def _get_asset(asset_identifier, possible_case_access_levels): + asset = assets_get(asset_identifier) + has_access = any( + ac_fast_check_current_user_has_case_access(asset.case_id, level) + for level in possible_case_access_levels + ) + if not has_access: + raise ObjectNotFoundError() + return asset
65-66: Harden JSON parsing to tolerate empty bodiesAllow empty payloads (tests post
{}) and avoid 400 on missing/invalid JSON.- comment = self._schema.load(request.get_json()) + payload = request.get_json(silent=True) or {} + comment = self._schema.load(payload)tests/tests_rest_comments.py (3)
104-106: Stop using deprecated “/comments/add” in testsSeed comments via the new v2 POST endpoint to avoid reliance on deprecated routes.
- self._subject.create(f'/alerts/{object_identifier}/comments/add', {'comment_text': 'comment1'}) - self._subject.create(f'/alerts/{object_identifier}/comments/add', {'comment_text': 'comment2'}) + self._subject.create(f'/api/v2/alerts/{object_identifier}/comments', {'comment_text': 'comment1'}) + self._subject.create(f'/api/v2/alerts/{object_identifier}/comments', {'comment_text': 'comment2'})
239-241: Avoid passing creation body as query params in GETNoise in request; fetch alert without extra params.
- response = self._subject.get(f'/api/v2/alerts/{object_identifier}', body).json() + response = self._subject.get(f'/api/v2/alerts/{object_identifier}').json()
327-333: Use a dedicated payload for comment creationYou’re reusing the ACL update body when posting a comment. Send
{}(or a comment-specific body) for clarity.- body = { - 'access_level': IRIS_CASE_ACCESS_LEVEL_READ_ONLY, - 'cases_list': [case_identifier] - } - self._subject.create(f'/manage/users/{user_identifier}/cases-access/update', body) - response = user.create(f'/api/v2/assets/{object_identifier}/comments', body) + acl_body = { + 'access_level': IRIS_CASE_ACCESS_LEVEL_READ_ONLY, + 'cases_list': [case_identifier] + } + self._subject.create(f'/manage/users/{user_identifier}/cases-access/update', acl_body) + response = user.create(f'/api/v2/assets/{object_identifier}/comments', {})source/app/blueprints/rest/v2/iocs_routes/comments.py (2)
45-49: Clarify multi-level access checkMirror explicit “any-of” evaluation to avoid depending on the checker accepting lists.
- def _get_ioc(ioc_identifier, possible_case_access_levels): - ioc = iocs_get(ioc_identifier) - if not ac_fast_check_current_user_has_case_access(ioc.case_id, possible_case_access_levels): - raise ObjectNotFoundError() - return ioc + def _get_ioc(ioc_identifier, possible_case_access_levels): + ioc = iocs_get(ioc_identifier) + has_access = any( + ac_fast_check_current_user_has_case_access(ioc.case_id, level) + for level in possible_case_access_levels + ) + if not has_access: + raise ObjectNotFoundError() + return ioc
65-66: Tolerate empty/absent JSONAlign with other routes and tests posting
{}.- comment = self._schema.load(request.get_json()) + payload = request.get_json(silent=True) or {} + comment = self._schema.load(payload)source/app/blueprints/rest/v2/evidences_routes/comments.py (2)
46-50: Iterate access levels explicitlyEnsure compatibility regardless of the access-checker’s parameter contract.
- def _get_evidence(evidence_identifier, possible_case_access_levels) -> CaseReceivedFile: - evidence = evidences_get(evidence_identifier) - if not ac_fast_check_current_user_has_case_access(evidence.case_id, possible_case_access_levels): - raise ObjectNotFoundError() - return evidence + def _get_evidence(evidence_identifier, possible_case_access_levels) -> CaseReceivedFile: + evidence = evidences_get(evidence_identifier) + has_access = any( + ac_fast_check_current_user_has_case_access(evidence.case_id, level) + for level in possible_case_access_levels + ) + if not has_access: + raise ObjectNotFoundError() + return evidence
67-68: Graceful JSON parsingUse a safe default for empty bodies.
- comment = self._schema.load(request.get_json()) + payload = request.get_json(silent=True) or {} + comment = self._schema.load(payload)source/app/blueprints/rest/v2/notes_routes/comments.py (2)
45-49: Explicit any-of access evaluationSame rationale as other resources.
- def _get_note(note_identifier, possible_case_access_levels): - note = notes_get(note_identifier) - if not ac_fast_check_current_user_has_case_access(note.note_case_id, possible_case_access_levels): - raise ObjectNotFoundError() - return note + def _get_note(note_identifier, possible_case_access_levels): + note = notes_get(note_identifier) + has_access = any( + ac_fast_check_current_user_has_case_access(note.note_case_id, level) + for level in possible_case_access_levels + ) + if not has_access: + raise ObjectNotFoundError() + return note
66-67: Safe JSON handlingAccept
{}and ignore unknowns per schema config.- comment = self._schema.load(request.get_json()) + payload = request.get_json(silent=True) or {} + comment = self._schema.load(payload)source/app/blueprints/rest/v2/tasks_routes/comments.py (1)
62-75: Nit: be resilient to missing/invalid JSON payloadsUse silent=True or default to {} so marshmallow gets a dict instead of None on bad Content-Type.
- comment = self._schema.load(request.get_json()) + payload = request.get_json(silent=True) or {} + comment = self._schema.load(payload)source/app/datamgmt/manage/manage_cases_db.py (3)
391-397: Batch deletes would be faster than per-row loopsYou can delete TaskAssignee and CaseTasks with CASE-scoped filters to reduce round trips.
- tasks = CaseTasks.query.filter(CaseTasks.task_case_id == case_identifier).all() - for task in tasks: - TaskAssignee.query.filter(TaskAssignee.task_id == task.id).delete() - CaseTasks.query.filter(CaseTasks.id == task.id).delete() + task_ids = [t.id for t in CaseTasks.query.with_entities(CaseTasks.id) + .filter(CaseTasks.task_case_id == case_identifier)] + if task_ids: + TaskAssignee.query.filter(TaskAssignee.task_id.in_(task_ids)).delete(synchronize_session=False) + CaseTasks.query.filter(CaseTasks.task_case_id == case_identifier).delete(synchronize_session=False)
442-447: Nit: comment/var name mismatchComment says “alerts,” but you’re updating CaseAssets.case_id. Consider renaming the variable or comment for clarity.
407-461: Consider single-transaction delete_caseMultiple commits make the operation non-atomic. Wrapping the whole flow in a single transaction reduces partial-deletion risk.
source/app/models/comments.py (1)
51-115: Optional: add indexes and ON DELETE cascades on FKsHigh-churn tables (comments and link tables) benefit from:
- Indexes on comment_user_id, comment_case_id, and each link’s (comment_id, <object_id>).
- DB-level ON DELETE CASCADE for link tables’ comment_id to simplify cleanup.
source/app/blueprints/rest/v2/events_routes/comments.py (1)
63-75: Nit: handle None/invalid JSON gracefullyMarshmallow handles validation, but defaulting payload helps avoid 400 from Flask JSON parsing.
- comment = self._schema.load(request.get_json()) + payload = request.get_json(silent=True) or {} + comment = self._schema.load(payload)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
.github/workflows/ci.yml(1 hunks)source/app/blueprints/rest/alerts_routes.py(1 hunks)source/app/blueprints/rest/case/case_assets_routes.py(1 hunks)source/app/blueprints/rest/case/case_evidences_routes.py(1 hunks)source/app/blueprints/rest/case/case_ioc_routes.py(1 hunks)source/app/blueprints/rest/case/case_notes_routes.py(1 hunks)source/app/blueprints/rest/case/case_routes.py(1 hunks)source/app/blueprints/rest/case/case_tasks_routes.py(1 hunks)source/app/blueprints/rest/case/case_timeline_routes.py(1 hunks)source/app/blueprints/rest/case_comments.py(1 hunks)source/app/blueprints/rest/search_routes.py(1 hunks)source/app/blueprints/rest/v2/alerts_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/assets_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/events_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/evidences_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/iocs_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/notes_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/tasks_routes/comments.py(3 hunks)source/app/business/access_controls.py(1 hunks)source/app/business/alerts.py(1 hunks)source/app/business/comments.py(4 hunks)source/app/business/evidences.py(1 hunks)source/app/datamgmt/alerts/alerts_db.py(2 hunks)source/app/datamgmt/case/case_assets_db.py(3 hunks)source/app/datamgmt/case/case_comments.py(1 hunks)source/app/datamgmt/case/case_events_db.py(2 hunks)source/app/datamgmt/case/case_iocs_db.py(3 hunks)source/app/datamgmt/case/case_notes_db.py(2 hunks)source/app/datamgmt/case/case_rfiles_db.py(3 hunks)source/app/datamgmt/case/case_tasks_db.py(2 hunks)source/app/datamgmt/comments.py(1 hunks)source/app/datamgmt/manage/manage_access_control_db.py(3 hunks)source/app/datamgmt/manage/manage_case_templates_db.py(2 hunks)source/app/datamgmt/manage/manage_cases_db.py(6 hunks)source/app/datamgmt/manage/manage_users_db.py(0 hunks)source/app/datamgmt/reporter/report_db.py(1 hunks)source/app/iris_engine/access_control/iris_user.py(2 hunks)source/app/iris_engine/access_control/utils.py(7 hunks)source/app/models/comments.py(1 hunks)source/app/models/models.py(0 hunks)source/app/schema/marshables.py(1 hunks)tests/iris.py(3 hunks)tests/tests_rest_assets.py(2 hunks)tests/tests_rest_comments.py(4 hunks)tests/tests_rest_miscellaneous.py(1 hunks)tests/tests_rest_notes_directories.py(1 hunks)
💤 Files with no reviewable changes (2)
- source/app/datamgmt/manage/manage_users_db.py
- source/app/models/models.py
🧰 Additional context used
🧬 Code graph analysis (41)
source/app/blueprints/rest/case/case_tasks_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/blueprints/rest/search_routes.py (1)
source/app/models/comments.py (1)
Comments(33-48)
source/app/blueprints/rest/alerts_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/schema/marshables.py (1)
source/app/models/comments.py (1)
Comments(33-48)
source/app/datamgmt/reporter/report_db.py (1)
source/app/models/comments.py (1)
Comments(33-48)
tests/tests_rest_assets.py (1)
tests/iris.py (1)
get_most_recent_object_history_entry(145-155)
source/app/blueprints/rest/case/case_assets_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/datamgmt/manage/manage_case_templates_db.py (1)
source/app/models/models.py (1)
CaseTemplate(215-242)
source/app/blueprints/rest/v2/alerts_routes/comments.py (6)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/business/comments.py (2)
comments_get_filtered_by_alert(55-59)comments_create_for_alert(109-126)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/blueprints/rest/v2/alerts.py (1)
create(135-157)
source/app/blueprints/rest/case/case_ioc_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/datamgmt/manage/manage_access_control_db.py (2)
source/app/models/authorization.py (3)
UserCaseEffectiveAccess(143-154)CaseAccessLevel(38-45)UserClient(184-196)source/app/models/cases.py (1)
Cases(50-160)
source/app/blueprints/rest/case/case_timeline_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
source/app/models/comments.py (1)
source/app/blueprints/responses.py (1)
default(68-99)
source/app/blueprints/rest/v2/notes_routes/comments.py (9)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_note(74-75)comments_create_for_note(175-188)source/app/business/notes.py (1)
notes_get(68-73)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/blueprints/rest/v2/iocs_routes/comments.py (2)
get(51-59)create(61-73)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/blueprints/rest/case/case_notes_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
tests/tests_rest_notes_directories.py (1)
tests/iris.py (1)
get_latest_activity(169-171)
source/app/blueprints/rest/case/case_evidences_routes.py (1)
source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)
tests/tests_rest_miscellaneous.py (1)
tests/iris.py (5)
get_module_identifier_by_name(136-142)create(56-57)create_dummy_case(98-106)delete(65-66)wait_for_module_task(157-167)
source/app/blueprints/rest/v2/events_routes/comments.py (10)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_event(82-83)comments_create_for_event(207-222)source/app/business/events.py (1)
events_get(70-74)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/models/cases.py (1)
CasesEvent(173-208)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/blueprints/rest/v2/tasks_routes/comments.py (2)
get(51-60)create(62-74)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/blueprints/rest/case_comments.py (1)
source/app/business/comments.py (1)
comments_update_for_case(86-106)
source/app/blueprints/rest/v2/iocs_routes/comments.py (7)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_ioc(70-71)comments_create_for_ioc(160-172)source/app/business/iocs.py (1)
iocs_get(47-51)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)
source/app/datamgmt/alerts/alerts_db.py (2)
source/app/datamgmt/filtering.py (2)
combine_conditions(90-105)apply_custom_conditions(108-150)source/app/models/comments.py (1)
Comments(33-48)
tests/tests_rest_comments.py (8)
tests/iris.py (9)
create_dummy_case(98-106)create(56-57)create_dummy_user(81-82)get(59-60)get_most_recent_object_history_entry(145-155)get_module_identifier_by_name(136-142)wait_for_module_task(157-167)get_latest_activity(169-171)delete(65-66)source/app/blueprints/rest/v2/events_routes/comments.py (2)
create(63-74)get(52-61)source/app/blueprints/rest/v2/tasks_routes/comments.py (2)
create(62-74)get(51-60)source/app/blueprints/rest/v2/notes_routes/comments.py (2)
create(62-74)get(51-60)source/app/blueprints/rest/v2/assets_routes/comments.py (2)
create(62-73)get(51-60)source/app/blueprints/rest/v2/evidences_routes/comments.py (2)
create(64-75)get(52-62)source/app/blueprints/rest/v2/iocs_routes/comments.py (2)
create(61-73)get(51-59)source/app/blueprints/rest/v2/alerts_routes/comments.py (1)
create(50-59)
source/app/blueprints/rest/case/case_routes.py (5)
source/app/blueprints/responses.py (2)
response_error(48-54)response_success(57-63)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)source/app/datamgmt/manage/manage_users_db.py (1)
set_user_case_access(448-472)source/app/iris_engine/utils/tracker.py (1)
track_activity(30-66)source/app/util.py (1)
add_obj_history_entry(49-79)
source/app/datamgmt/case/case_comments.py (1)
source/app/models/comments.py (1)
Comments(33-48)
source/app/blueprints/rest/v2/assets_routes/comments.py (6)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_asset(62-63)comments_create_for_asset(129-142)source/app/business/assets.py (1)
assets_get(70-75)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)
source/app/datamgmt/case/case_notes_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)NotesComments(106-114)source/app/models/models.py (1)
Notes(465-483)
source/app/datamgmt/case/case_rfiles_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)EvidencesComments(95-103)source/app/models/models.py (1)
CaseReceivedFile(551-573)
source/app/iris_engine/access_control/utils.py (2)
source/app/datamgmt/manage/manage_access_control_db.py (2)
check_ua_case_client(80-107)get_case_effective_access(110-121)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/datamgmt/case/case_assets_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)AssetComments(84-92)source/app/models/models.py (1)
CaseAssets(152-180)
source/app/business/evidences.py (1)
source/app/datamgmt/case/case_rfiles_db.py (1)
add_rfile(62-76)
source/app/datamgmt/case/case_events_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)EventComments(51-59)source/app/models/cases.py (1)
CasesEvent(173-208)
source/app/datamgmt/manage/manage_cases_db.py (9)
source/app/models/comments.py (3)
Comments(33-48)IocComments(73-81)AssetComments(84-92)source/app/datamgmt/case/case_rfiles_db.py (1)
delete_evidences_comments_in_case(112-122)source/app/datamgmt/case/case_notes_db.py (1)
delete_notes_comments_in_case(100-110)source/app/datamgmt/case/case_tasks_db.py (1)
delete_tasks_comments_in_case(318-328)source/app/datamgmt/case/case_events_db.py (1)
delete_events_comments_in_case(176-186)source/app/models/iocs.py (1)
Ioc(51-76)source/app/models/models.py (5)
CaseAssets(152-180)CaseReceivedFile(551-573)Notes(465-483)CaseTasks(585-608)CaseEventCategory(309-317)source/app/models/cases.py (2)
CasesEvent(173-208)Cases(50-160)source/app/datamgmt/states.py (1)
delete_case_states(80-83)
source/app/blueprints/rest/v2/evidences_routes/comments.py (9)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_evidence(66-67)comments_create_for_evidence(145-157)source/app/models/models.py (1)
CaseReceivedFile(551-573)source/app/business/evidences.py (1)
evidences_get(60-64)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/blueprints/rest/v2/assets_routes/comments.py (2)
get(51-60)create(62-73)
source/app/blueprints/rest/v2/tasks_routes/comments.py (8)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_task(78-79)comments_create_for_task(191-204)source/app/business/tasks.py (1)
tasks_get(83-87)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(318-319)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/blueprints/rest/v2/events_routes/comments.py (2)
get(52-61)create(63-74)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/datamgmt/case/case_tasks_db.py (2)
source/app/models/comments.py (2)
Comments(33-48)TaskComments(62-70)source/app/models/models.py (1)
CaseTasks(585-608)
source/app/business/alerts.py (3)
source/app/datamgmt/alerts/alerts_db.py (1)
get_alert_by_id(317-332)source/app/datamgmt/manage/manage_access_control_db.py (1)
user_has_client_access(158-176)source/app/models/alerts.py (1)
Alert(46-82)
source/app/datamgmt/comments.py (1)
source/app/models/comments.py (7)
Comments(33-48)EventComments(51-59)TaskComments(62-70)IocComments(73-81)AssetComments(84-92)EvidencesComments(95-103)NotesComments(106-114)
tests/iris.py (4)
source/app/blueprints/rest/v2/assets_routes/comments.py (1)
get(51-60)source/app/blueprints/rest/v2/case_objects/events.py (1)
get(86-101)tests/rest_api.py (1)
get(49-54)tests/docker_compose.py (1)
extract_logs(28-30)
source/app/business/comments.py (17)
source/app/business/alerts.py (2)
alerts_exists(80-83)alerts_get(73-77)source/app/datamgmt/case/case_assets_db.py (1)
add_comment_to_asset(326-332)source/app/datamgmt/case/case_rfiles_db.py (1)
add_comment_to_evidence(136-142)source/app/datamgmt/case/case_iocs_db.py (1)
add_comment_to_ioc(230-236)source/app/datamgmt/case/case_notes_db.py (1)
add_comment_to_note(335-341)source/app/datamgmt/case/case_tasks_db.py (1)
add_comment_to_task(237-243)source/app/datamgmt/case/case_events_db.py (1)
add_comment_to_event(189-195)source/app/iris_engine/module_handler/module_handler.py (1)
call_modules_hook(493-581)source/app/iris_engine/utils/tracker.py (1)
track_activity(30-66)source/app/models/comments.py (1)
Comments(33-48)source/app/models/models.py (4)
CaseAssets(152-180)CaseReceivedFile(551-573)Notes(465-483)CaseTasks(585-608)source/app/models/iocs.py (1)
Ioc(51-76)source/app/models/cases.py (1)
CasesEvent(173-208)source/app/models/pagination_parameters.py (1)
PaginationParameters(19-37)source/app/util.py (1)
add_obj_history_entry(49-79)source/app/datamgmt/comments.py (6)
get_filtered_asset_comments(38-45)get_filtered_evidence_comments(48-55)get_filtered_ioc_comments(58-65)get_filtered_note_comments(68-75)get_filtered_task_comments(78-85)get_filtered_event_comments(88-95)source/app/datamgmt/case/case_comments.py (1)
get_case_comment(23-32)
source/app/datamgmt/case/case_iocs_db.py (3)
source/app/models/cases.py (2)
Cases(50-160)CasesEvent(173-208)source/app/models/models.py (2)
Client(119-131)CaseAssets(152-180)source/app/models/comments.py (2)
Comments(33-48)IocComments(73-81)
⏰ 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). (5)
- GitHub Check: End to end tests
- GitHub Check: Database migration tests
- GitHub Check: Generate graphQL documentation
- GitHub Check: Test API
- GitHub Check: Build docker app
| - name: Stop development server | ||
| run: | | ||
| docker compose logs app > ${{ runner.temp }}/iriswebapp_app.log | ||
| docker compose down | ||
| - name: Upload artifact |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make log capture resilient and step always-run
Logs step can fail if the service isn’t named “app,” causing the stop step to error and skip teardown. Add always-run, fallback to all services, quote paths, add useful flags, and reference the same compose file used for up.
- - name: Stop development server
- run: |
- docker compose logs app > ${{ runner.temp }}/iriswebapp_app.log
- docker compose down
+ - name: Stop development server
+ if: ${{ always() }}
+ run: |
+ docker compose -f docker-compose.dev.yml logs --no-color --timestamps --tail=5000 app > "${{ runner.temp }}/iriswebapp_app.log" \
+ || docker compose -f docker-compose.dev.yml logs --no-color --timestamps --tail=5000 > "${{ runner.temp }}/iriswebapp_app.log" || true
+ docker compose -f docker-compose.dev.yml down📝 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.
| - name: Stop development server | |
| run: | | |
| docker compose logs app > ${{ runner.temp }}/iriswebapp_app.log | |
| docker compose down | |
| - name: Upload artifact | |
| - name: Stop development server | |
| if: ${{ always() }} | |
| run: | | |
| docker compose -f docker-compose.dev.yml logs --no-color --timestamps --tail=5000 app > "${{ runner.temp }}/iriswebapp_app.log" \ | |
| || docker compose -f docker-compose.dev.yml logs --no-color --timestamps --tail=5000 > "${{ runner.temp }}/iriswebapp_app.log" || true | |
| docker compose -f docker-compose.dev.yml down | |
| - name: Upload artifact |
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 186-190, the "Stop development server"
step is brittle: make it always run (use if: always()), reference the same
compose file used for "up" via -f with the path, quote the artifact path when
redirecting logs, capture logs for the named service but fall back to all
services (e.g., attempt docker compose logs app || docker compose logs --all),
add useful flags like --no-color and --timestamps to logs, and ensure docker
compose down also uses the same -f and runs even if logs fail; then keep the
subsequent "Upload artifact" step unchanged.
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: Test API iriswebapp_app logs | ||
| path: ${{ runner.temp }}/iriswebapp_app.log |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure artifact upload always executes and doesn’t fail job if file missing
Run regardless of previous failures and avoid hard-failing when the log file isn’t present; set a short retention.
- - name: Upload artifact
- uses: actions/upload-artifact@v4
- with:
- name: Test API iriswebapp_app logs
- path: ${{ runner.temp }}/iriswebapp_app.log
+ - name: Upload artifact
+ if: ${{ always() }}
+ uses: actions/upload-artifact@v4
+ with:
+ name: Test API iriswebapp_app logs
+ path: ${{ runner.temp }}/iriswebapp_app.log
+ if-no-files-found: ignore
+ retention-days: 7📝 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.
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: Test API iriswebapp_app logs | |
| path: ${{ runner.temp }}/iriswebapp_app.log | |
| if: ${{ always() }} | |
| uses: actions/upload-artifact@v4 | |
| with: | |
| name: Test API iriswebapp_app logs | |
| path: ${{ runner.temp }}/iriswebapp_app.log | |
| if-no-files-found: ignore | |
| retention-days: 7 |
🤖 Prompt for AI Agents
In .github/workflows/ci.yml around lines 191 to 194, the upload-artifact step
should always run and not fail the job if the log file is missing; add an if:
always() condition to the step, and in the with: block set if-no-files-found:
'ignore' (or 'warn') and add a short retention-days value (e.g. 1 or 3) so
missing files don’t hard-fail and artifacts are kept only briefly.
| def comments_create_for_alert(current_user, comment: Comments, alert_identifier: int): | ||
| alert = alerts_get(current_user, alert_identifier) | ||
| comment.comment_alert_id = alert_identifier | ||
| comment.comment_user_id = current_user.id | ||
| comment.comment_date = datetime.now() | ||
| comment.comment_update_date = datetime.now() | ||
|
|
||
| db.session.add(comment) | ||
|
|
||
| add_obj_history_entry(alert, 'commented') | ||
| db.session.commit() | ||
|
|
||
| hook_data = { | ||
| 'comment': comment, | ||
| 'alert': alert | ||
| } | ||
| call_modules_hook('on_postload_alert_commented', hook_data) | ||
| track_activity(f'alert "{alert.alert_id}" commented', ctx_less=True) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make comment creation atomic; avoid commit storms and orphan rows.
current state:
- _create_comment commits immediately.
- add_comment_to_* also commits.
- Each wrapper commits again.
This can persist the base comment even if join insertion fails, creating orphaned comments not returned by joins.
Minimal, localized changes:
- Replace commit in _create_comment with flush to get comment_id without committing.
- Remove the extra commits in the wrappers (the add_comment_to_* functions already commit).
@@ def comments_create_for_asset(current_user, asset: CaseAssets, comment: Comments):
- db.session.commit()
+ # commit handled by add_comment_to_asset()
@@ def comments_create_for_evidence(current_user, evidence: CaseReceivedFile, comment: Comments):
- db.session.commit()
+ # commit handled by add_comment_to_evidence()
@@ def comments_create_for_ioc(current_user, ioc: Ioc, comment: Comments):
- db.session.commit()
+ # commit handled by add_comment_to_ioc()
@@ def comments_create_for_note(current_user, note: Notes, comment: Comments):
- db.session.commit()
+ # commit handled by add_comment_to_note()
@@ def comments_create_for_task(current_user, task: CaseTasks, comment: Comments):
- db.session.commit()
+ # commit handled by add_comment_to_task()
@@ def comments_create_for_event(current_user, event: CasesEvent, comment: Comments):
- db.session.commit()
+ # commit handled by add_comment_to_event()
@@ def _create_comment(current_user, comment, case_identifier):
- db.session.add(comment)
- db.session.commit()
+ db.session.add(comment)
+ db.session.flush() # assign comment_id without committingLonger-term: make add_comment_to_* not commit and perform a single commit after both inserts in the wrappers, or wrap with session.begin() for transactional atomicity.
Also applies to: 129-143, 145-158, 160-173, 175-189, 191-205, 207-223, 225-232
🤖 Prompt for AI Agents
In source/app/business/comments.py around lines 109-127 (and similarly for
ranges 129-143,145-158,160-173,175-189,191-205,207-223,225-232), the current
implementation commits the session immediately after inserting the base comment
which can create orphan rows if subsequent join/association inserts fail;
replace db.session.commit() used to persist the new comment with
db.session.flush() so the comment id is populated without committing, and remove
redundant commits from the wrapper functions that call add_comment_to_* (since
those functions already perform commits) so that a single commit (or an explicit
transactional block later) controls persistence and prevents partial/ orphaned
inserts.
| def delete_notes_comments_in_case(case_identifier): | ||
| com_ids = NotesComments.query.with_entities( | ||
| NotesComments.comment_id | ||
| ).join(Notes).filter( | ||
| NotesComments.comment_note_id == Notes.note_id, | ||
| Notes.note_case_id == case_identifier | ||
| ).all() | ||
|
|
||
| com_ids = [c.comment_id for c in com_ids] | ||
| NotesComments.query.filter(NotesComments.comment_id.in_(com_ids)).delete() | ||
| Comments.query.filter(Comments.comment_id.in_(com_ids)).delete() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make deletion atomic and scalable (use subquery + bulk delete)
Wrap in a nested transaction and delete via a subquery to avoid loading IDs into Python; also disable session synchronization for performance.
-def delete_notes_comments_in_case(case_identifier):
- com_ids = NotesComments.query.with_entities(
- NotesComments.comment_id
- ).join(Notes).filter(
- NotesComments.comment_note_id == Notes.note_id,
- Notes.note_case_id == case_identifier
- ).all()
-
- com_ids = [c.comment_id for c in com_ids]
- NotesComments.query.filter(NotesComments.comment_id.in_(com_ids)).delete()
- Comments.query.filter(Comments.comment_id.in_(com_ids)).delete()
+def delete_notes_comments_in_case(case_identifier):
+ with db.session.begin_nested():
+ com_ids_sq = db.session.query(NotesComments.comment_id) \
+ .join(Notes) \
+ .filter(Notes.note_case_id == case_identifier) \
+ .subquery()
+
+ NotesComments.query \
+ .filter(NotesComments.comment_id.in_(com_ids_sq)) \
+ .delete(synchronize_session=False)
+ Comments.query \
+ .filter(Comments.comment_id.in_(com_ids_sq)) \
+ .delete(synchronize_session=False)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In source/app/datamgmt/case/case_notes_db.py around lines 100 to 111, the
current delete function loads comment IDs into Python and issues separate
deletes; change it to run inside a nested transaction and perform bulk deletes
using a SQL subquery so IDs are not loaded into memory: begin a
session.begin_nested() (or use the session context manager) and execute
NotesComments.query.filter(NotesComments.comment_id.in_(subquery_of_comment_ids)).delete(synchronize_session=False)
and similarly delete Comments using the same subquery, ensuring
synchronize_session=False for performance; commit/rollback via the nested
transaction.
| CaseAssetsAlias = aliased(CaseAssets) | ||
|
|
||
| # Query for CaseAssets that are not referenced in alerts and match the case_id | ||
| assets_to_delete = db.session.query(CaseAssets).filter( | ||
| and_( | ||
| CaseAssets.case_id == case_identifier, | ||
| ~db.session.query(alert_assets_association).filter( | ||
| alert_assets_association.c.asset_id == CaseAssetsAlias.asset_id | ||
| ).exists() | ||
| ) | ||
| ) | ||
| # Delete the assets | ||
| assets_to_delete.delete(synchronize_session='fetch') | ||
|
|
There was a problem hiding this comment.
Bug: uncorrelated EXISTS may delete the wrong assets
The EXISTS subquery uses CaseAssetsAlias without correlating to the outer selection, so it’s not tied to the current row. Replace with a correlated check against CaseAssets.asset_id (or use the same alias in both scopes).
- CaseAssetsAlias = aliased(CaseAssets)
-
# Query for CaseAssets that are not referenced in alerts and match the case_id
assets_to_delete = db.session.query(CaseAssets).filter(
and_(
CaseAssets.case_id == case_identifier,
- ~db.session.query(alert_assets_association).filter(
- alert_assets_association.c.asset_id == CaseAssetsAlias.asset_id
- ).exists()
+ ~db.session.query(alert_assets_association).filter(
+ alert_assets_association.c.asset_id == CaseAssets.asset_id
+ ).exists()
)
)
- # Delete the assets
- assets_to_delete.delete(synchronize_session='fetch')
+ # Delete the assets
+ assets_to_delete.delete(synchronize_session=False)📝 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.
| CaseAssetsAlias = aliased(CaseAssets) | |
| # Query for CaseAssets that are not referenced in alerts and match the case_id | |
| assets_to_delete = db.session.query(CaseAssets).filter( | |
| and_( | |
| CaseAssets.case_id == case_identifier, | |
| ~db.session.query(alert_assets_association).filter( | |
| alert_assets_association.c.asset_id == CaseAssetsAlias.asset_id | |
| ).exists() | |
| ) | |
| ) | |
| # Delete the assets | |
| assets_to_delete.delete(synchronize_session='fetch') | |
| # Query for CaseAssets that are not referenced in alerts and match the case_id | |
| assets_to_delete = db.session.query(CaseAssets).filter( | |
| and_( | |
| CaseAssets.case_id == case_identifier, | |
| ~db.session.query(alert_assets_association).filter( | |
| alert_assets_association.c.asset_id == CaseAssets.asset_id | |
| ).exists() | |
| ) | |
| ) | |
| # Delete the assets | |
| assets_to_delete.delete(synchronize_session=False) |
🤖 Prompt for AI Agents
In source/app/datamgmt/manage/manage_cases_db.py around lines 356 to 369, the
EXISTS subquery is uncorrelated and may match any row because it uses
CaseAssetsAlias without tying it to the outer CaseAssets row; change the
subquery to reference the outer CaseAssets asset_id (e.g., compare
alert_assets_association.c.asset_id == CaseAssets.asset_id) or use the same
aliased table in both outer and inner queries so the EXISTS becomes correlated,
then re-run the query and keep delete(..., synchronize_session='fetch') as
before.
There was a problem hiding this comment.
Actionable comments posted: 0
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_templates_db.py (2)
289-299: Guard None for summary/tags to avoid TypeErrorsummary and tags are nullable JSON fields; current code can raise when either is None.
- # Update summary, we want to append in order not to skip the initial case description - case.description += "\n" + case_template.summary + # Update summary, only if provided + if case_template.summary: + existing = case.description or "" + case.description = f"{existing}\n{case_template.summary}" if existing else case_template.summary @@ - # Update case tags - for tag_str in case_template.tags: - tag = Tags(tag_title=tag_str) - tag = tag.save() - case.tags.append(tag) + # Update case tags + if case_template.tags: + for tag_str in case_template.tags: + tag = Tags(tag_title=tag_str) + tag = tag.save() + case.tags.append(tag)
172-176: Handle templates with no tasksIf tasks is None, iterating will raise. Early-return keeps behavior idempotent.
def case_template_populate_tasks(case: Cases, case_template: CaseTemplate): logs = [] - # Update case tasks + # Update case tasks + if not case_template.tasks: + return logs for task_template in case_template.tasks:
♻️ Duplicate comments (1)
source/app/iris_engine/access_control/utils.py (1)
297-306: Critical: Early return bypasses deny_all check for client-based accessWhen the user gets access through the client (lines 299-306), the code returns immediately without checking if the access level is
deny_all. This creates an inconsistency where:
- Direct access (lines 297-298) goes through the deny_all check at line 308
- Client-based fallback access bypasses this critical security check
This is a duplicate of a previous review comment but remains unaddressed.
Apply this fix to ensure consistent access validation:
- if not access_level: + if access_level is None: # The user has no direct access, check if he is part of the client access_level = check_ua_case_client(user_id, cid) - if not access_level: + if access_level is None: return None ac_set_case_access_for_user(user_id, cid, access_level) - - return access_level + # Don't return early - let it fall through to deny_all check
🧹 Nitpick comments (4)
source/app/datamgmt/manage/manage_case_templates_db.py (4)
61-71: Align return docs with Optional and prefer PK lookupDocstring still says CaseTemplate; function now returns Optional. Also consider using session.get for primary-key lookup.
def get_case_template_by_id(cur_id: int) -> Optional[CaseTemplate]: - """Get a case template + """Get a case template @@ - Returns: - CaseTemplate: Case template + Returns: + Optional[CaseTemplate]: Case template or None if not found """ - return CaseTemplate.query.filter_by(id=cur_id).first() + return db.session.get(CaseTemplate, cur_id)
95-102: Remove duplicate “author length” checkDuplicate block is redundant; keep one.
# We check that author length is not above 128 chars if "author" in data and len(data["author"]) > 128: return "Author cannot be longer than 128 characters." - - # We check that author length is not above 128 chars - if "author" in data and len(data["author"]) > 128: - return "Author cannot be longer than 128 characters."
250-253: No-op assignment; remove dead codeSelf-assignment under the if does nothing.
- # Update case tasks - if case_template.note_directories: - case_template.note_directories = case_template.note_directories
266-269: Avoid committing inside the loop; flush insteadCommit per directory harms transactional integrity and performance. Flush to get PK; commit once at the end (already done in case_template_post_modifier).
- db.session.commit() + db.session.flush()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/app/datamgmt/manage/manage_case_templates_db.py(2 hunks)source/app/iris_engine/access_control/utils.py(15 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
source/app/datamgmt/manage/manage_case_templates_db.py (1)
source/app/models/models.py (1)
CaseTemplate(215-242)
source/app/iris_engine/access_control/utils.py (2)
source/app/datamgmt/manage/manage_access_control_db.py (2)
check_ua_case_client(80-107)get_case_effective_access(110-121)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
⏰ 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). (8)
- GitHub Check: Generate graphQL documentation
- GitHub Check: End to end tests
- GitHub Check: Database migration tests
- GitHub Check: Test API
- GitHub Check: Test API
- GitHub Check: End to end tests
- GitHub Check: Database migration tests
- GitHub Check: Generate graphQL documentation
🔇 Additional comments (7)
source/app/datamgmt/manage/manage_case_templates_db.py (1)
162-164: Verify: likely bug using only first character of nameIndexing name[0] suggests only the first character is kept. If name is a string, remove [0].
- case_schema.name = case_template.title_prefix + " " + case_schema.name[0] + case_schema.name = f"{case_template.title_prefix} {case_schema.name}"Please confirm the type of CaseSchema.name; if it’s not a string, adapt accordingly.
source/app/iris_engine/access_control/utils.py (6)
291-291: Good addition of type hint for expected_access_levels parameterThe explicit type hint
list[CaseAccessLevel]improves code clarity and helps with type checking.
577-577: Good addition of type hint for access_level parameterAdding the
inttype hint for theaccess_levelparameter improves type safety.
5-5: Consistent use of centralized loggerGood refactoring to use the centralized
loggerfromapp.loggerinstead of the undefinedlogvariable. This ensures proper logging configuration and consistency across the application.Also applies to: 538-538, 588-588
748-769: Good refactoring of user effective access structureThe consistent use of
ac_access_level_to_list()function to represent access levels improves code maintainability and provides a more structured representation of permissions.Also applies to: 789-800, 820-831, 851-863
603-603: Simplified commit behaviorThe removal of the optional
commitparameter and forcing all changes to commit immediately simplifies the transaction model, though it reduces flexibility for batch operations.
568-568: Ensure ac_set_case_access_for_user gets an integer (use Enum.value or pass int)
- Repo mostly passes CaseAccessLevel.<...>.value; I found calls that pass a variable instead at:
- source/app/datamgmt/manage/manage_users_db.py:472
- source/app/iris_engine/access_control/utils.py:304
- source/app/iris_engine/access_control/utils.py:571
- Action: confirm those variables are integers; if they are Enum members, pass CaseAccessLevel..value or cast to int before calling.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
source/app/blueprints/rest/v2/case_objects/notes_directories.py (1)
154-157: Fix route: missing leading slash breaks DELETE endpointThe decorator should start with '/'. Otherwise the route won’t match as expected.
Apply:
-@case_notes_directories_blueprint.delete('<int:identifier>') +@case_notes_directories_blueprint.delete('/<int:identifier>') def delete_note_directory(case_identifier, identifier): return notes_directories.delete(case_identifier, identifier)source/app/iris_engine/access_control/utils.py (1)
570-597: ac_set_case_access_for_user does nothing when no row existsMissing insert path for 0 matches; the effective access won’t be persisted.
Apply:
def ac_set_case_access_for_user(user_id, case_id, access_level: int): @@ - elif len(uac) == 1: + elif len(uac) == 1: uac = uac[0] uac.access_level = access_level + else: + uac = UserCaseEffectiveAccess() + uac.user_id = user_id + uac.case_id = case_id + uac.access_level = access_level + db.session.add(uac) db.session.commit()source/app/blueprints/rest/v2/manage_routes/groups.py (1)
67-76: Fix broken update flow: missing argument and unsafe key accessPassing no argument to groups_update and indexing group_permissions directly can error.
Apply:
def update(self, identifier): try: group = groups_get(identifier) request_data = request.get_json() request_data['group_id'] = identifier updated_group = self._load(request_data, instance=group, partial=True) - if not ac_flag_match_mask(request_data['group_permissions'], Permissions.server_administrator.value) and ac_ldp_group_update(iris_current_user.id): + new_permissions = request_data.get('group_permissions', updated_group.group_permissions) + if not ac_flag_match_mask(new_permissions, Permissions.server_administrator.value) and ac_ldp_group_update(iris_current_user.id): return response_api_error('That might not be a good idea Dave', data='Update the group permissions will lock you out') - groups_update() + groups_update(updated_group) result = self._schema.dump(updated_group) return response_api_success(result)source/app/blueprints/rest/v2/case_objects/notes.py (1)
160-160: Broken routes: missing leading slash in route decoratorsPUT/DELETE endpoints won’t register without "/". Fix both.
-@case_notes_blueprint.put('<int:identifier>') +@case_notes_blueprint.put('/<int:identifier>') @@ -@case_notes_blueprint.delete('<int:identifier>') +@case_notes_blueprint.delete('/<int:identifier>')Also applies to: 166-166
source/app/blueprints/rest/v2/case_objects/tasks.py (2)
117-119: Update requires write permission, not readAllowing CaseAccessLevel.read_only to update is a privilege escalation.
- if not ac_fast_check_current_user_has_case_access(task.task_case_id, [CaseAccessLevel.read_only, CaseAccessLevel.full_access]): + if not ac_fast_check_current_user_has_case_access(task.task_case_id, [CaseAccessLevel.full_access]): return ac_api_return_access_denied(caseid=task.task_case_id)
139-141: Wrong case id in access-denied responseShould reference the task’s case, not the task identifier.
- return ac_api_return_access_denied(caseid=identifier) + return ac_api_return_access_denied(caseid=task.task_case_id)source/app/blueprints/rest/v2/cases.py (1)
21-21: Replace top-level Werkzeug Response import with werkzeug.wrappersIn source/app/blueprints/rest/v2/cases.py (line 21) replace:
from werkzeug import Response
with:
from werkzeug.wrappers import ResponseWerkzeug 3.x removed/deprecated top-level attribute imports; update to prevent breakage on upgrade.
♻️ Duplicate comments (1)
source/app/iris_engine/access_control/utils.py (1)
288-312: Authorization bug: fallback early-return bypasses deny_all and expected-level checksReturning immediately after client-derived access can grant unintended access. Validate first; persist only if authorized.
Apply:
-def ac_fast_check_user_has_case_access(user_id, cid, expected_access_levels: list[CaseAccessLevel]): +def ac_fast_check_user_has_case_access(user_id, cid, expected_access_levels: list[CaseAccessLevel]): @@ - access_level = get_case_effective_access(user_id, cid) - - if not access_level: - # The user has no direct access, check if he is part of the client - access_level = check_ua_case_client(user_id, cid) - if not access_level: - return None - ac_set_case_access_for_user(user_id, cid, access_level) - - return access_level + derived_from_client = False + access_level = get_case_effective_access(user_id, cid) + + if access_level is None: + # No direct access; try client-based access + access_level = check_ua_case_client(user_id, cid) + if access_level is None: + return None + derived_from_client = True @@ - for acl in expected_access_levels: - if ac_flag_match_mask(access_level, acl.value): - return access_level + for acl in expected_access_levels: + if ac_flag_match_mask(access_level, acl.value): + if derived_from_client: + ac_set_case_access_for_user(user_id, cid, access_level) + return access_level
🧹 Nitpick comments (12)
source/app/blueprints/graphql/permissions.py (2)
49-63: Remove or wrap the “stricter” variant to avoid divergence.Given the TODO about possibly wrong case identifiers for GraphQL, prefer a single path. Either deprecate this method or have it extract the case id and delegate to the unified checker to keep semantics consistent.
Proposed minimal wrapper:
def permissions_check_current_user_has_some_case_access_stricter(access_levels): - redir, caseid, has_access = get_case_access_from_api(request, access_levels) - - # TODO: do we really want to keep the details of the errors, when permission is denied => more work, more complex code? - if not caseid or redir: - _deny_permission() - - if not has_access: - _deny_permission() + redir, caseid, _ = get_case_access_from_api(request, access_levels) + if not caseid or redir: + _deny_permission() + # Reuse the canonical path (keeps logic in one place) + permissions_check_current_user_has_some_case_access(caseid, access_levels)If helpful, I can scan call sites and open a follow-up to deprecate this safely.
44-46: Make the access-level shape explicit (singular vs plural)ac_fast_check_user_has_case_access(...) expects a list[CaseAccessLevel]; ac_fast_check_current_user_has_case_access(cid, access_level) simply forwards its argument and is named singular. GraphQL callers pass lists. Rename the wrapper param to access_levels or validate/coerce single values into a list in source/app/business/access_controls.py:22 (and update source/app/blueprints/graphql/permissions.py:44-46 if you prefer normalization here).
source/app/blueprints/rest/v2/tasks.py (2)
64-66: Pass the correct case id in access-denied response (delete).Currently returns the task id; should return the related case id for consistency with GET and accurate auditing.
Apply:
- if not ac_fast_check_current_user_has_case_access(task.task_case_id, [CaseAccessLevel.full_access]): - return ac_api_return_access_denied(caseid=identifier) + if not ac_fast_check_current_user_has_case_access(task.task_case_id, [CaseAccessLevel.full_access]): + return ac_api_return_access_denied(caseid=task.task_case_id)
45-55: Consider minimizing resource-existence leaks (403 vs 404).Fetching the task before the access check can let clients distinguish “exists but forbidden” (403) from “not found” (404). If your policy is to avoid enumeration, return 404 on access denial or perform a join that enforces case access in the fetch.
Would you like a patch to standardize this behavior across v2 task endpoints?
Also applies to: 61-73
source/app/blueprints/rest/v2/evidences_routes/comments.py (2)
45-51: Add precise typing to improve readability and static checks.
Consider annotating parameters with concrete types (Sequence of CaseAccessLevel) and keeping the return hint.- def _get_evidence(evidence_identifier, possible_case_access_levels) -> CaseReceivedFile: + def _get_evidence( + evidence_identifier: int, + possible_case_access_levels: Sequence[CaseAccessLevel], + ) -> CaseReceivedFile:And add:
+from typing import Sequence
72-75: Gracefully handle invalid JSON body.
request.get_json() can raise BadRequest; catch it and return a 400 with a clear message.+from werkzeug.exceptions import BadRequest @@ - except ValidationError as e: + except ValidationError as e: return response_api_error('Data error', data=e.normalized_messages()) + except BadRequest: + return response_api_error('Invalid JSON payload')source/app/models/authorization.py (1)
258-259: Type-hint and document the bitmask helperSmall polish for clarity and tooling without changing behavior.
Apply:
-def ac_flag_match_mask(flag, mask): - return (flag & mask) == mask +def ac_flag_match_mask(flag: int, mask: int) -> bool: + """Return True if all bits in `mask` are set in `flag`.""" + return (flag & mask) == masksource/app/iris_engine/access_control/utils.py (1)
142-146: Combine group permissions with OR, not ANDAND with 0 yields 0; using OR matches effective-permission semantics elsewhere.
Apply:
def ac_get_effective_permissions_from_groups(groups): @@ - final_perm = 0 - for group in groups: - final_perm &= group.group_permissions + final_perm = 0 + for group in groups: + final_perm |= group.group_permissionssource/app/blueprints/rest/v2/case_objects/iocs.py (1)
37-41: Optional: add case-existence check for parity with other v2 endpointsEvidences/Notes check cases_exists before permission checks; mirroring that here improves consistency and avoids leaking existence via 403s.
from app.business.iocs import iocs_update from app.datamgmt.case.case_iocs_db import get_filtered_iocs +from app.business.cases import cases_exists from app.business.access_controls import ac_fast_check_current_user_has_case_access from app.models.authorization import CaseAccessLevel from app.schema.marshables import IocSchemaForAPIV2 @@ try: + if not cases_exists(case_identifier): + return response_api_not_found() if not ac_fast_check_current_user_has_case_access(case_identifier, [CaseAccessLevel.read_only, CaseAccessLevel.full_access]): return ac_api_return_access_denied(caseid=case_identifier)Also applies to: 53-56
source/app/blueprints/rest/v2/case_objects/evidences.py (1)
50-54: Optional: return 404 on cross-case mismatchOther modules treat “object not in this case” as not-found. Consider raising ObjectNotFoundError for uniform API semantics.
def _get_evidence_in_case(identifier, case_identifier): evidence = evidences_get(identifier) if evidence.case_id != case_identifier: - raise BusinessProcessingError(f'Evidence {evidence.id} does not belong to case {case_identifier}') + raise ObjectNotFoundError() return evidencesource/app/blueprints/rest/v2/assets_routes/comments.py (2)
57-58: Ensure deterministic ordering in pagination.If
comments_get_filtered_by_assetdoesn’t enforce a default sort, pass an explicitdefault_order_by/default_directionso first/next pages are stable.
68-69: Optional: include Location header for the created comment.Consider returning a Location header (e.g.,
/api/v2/assets/{asset_identifier}/comments/{comment_id}) with 201 for better REST UX. Ifresponse_api_createdcan’t set headers, consider extending it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
source/app/blueprints/graphql/permissions.py(1 hunks)source/app/blueprints/pages/manage/manage_cases_routes.py(1 hunks)source/app/blueprints/rest/case/case_assets_routes.py(2 hunks)source/app/blueprints/rest/case/case_ioc_routes.py(2 hunks)source/app/blueprints/rest/manage/manage_assets_routes.py(1 hunks)source/app/blueprints/rest/manage/manage_cases_routes.py(1 hunks)source/app/blueprints/rest/manage/manage_groups.py(1 hunks)source/app/blueprints/rest/v2/assets.py(1 hunks)source/app/blueprints/rest/v2/assets_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/case_objects/assets.py(1 hunks)source/app/blueprints/rest/v2/case_objects/events.py(1 hunks)source/app/blueprints/rest/v2/case_objects/evidences.py(1 hunks)source/app/blueprints/rest/v2/case_objects/iocs.py(1 hunks)source/app/blueprints/rest/v2/case_objects/notes.py(1 hunks)source/app/blueprints/rest/v2/case_objects/notes_directories.py(1 hunks)source/app/blueprints/rest/v2/case_objects/tasks.py(1 hunks)source/app/blueprints/rest/v2/cases.py(1 hunks)source/app/blueprints/rest/v2/events_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/evidences_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/iocs.py(1 hunks)source/app/blueprints/rest/v2/iocs_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/manage_routes/groups.py(1 hunks)source/app/blueprints/rest/v2/notes_routes/comments.py(3 hunks)source/app/blueprints/rest/v2/tasks.py(1 hunks)source/app/blueprints/rest/v2/tasks_routes/comments.py(3 hunks)source/app/business/access_controls.py(1 hunks)source/app/iris_engine/access_control/utils.py(15 hunks)source/app/models/authorization.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- source/app/blueprints/rest/v2/iocs.py
- source/app/blueprints/rest/manage/manage_assets_routes.py
🚧 Files skipped from review as they are similar to previous changes (5)
- source/app/business/access_controls.py
- source/app/blueprints/rest/v2/notes_routes/comments.py
- source/app/blueprints/rest/v2/events_routes/comments.py
- source/app/blueprints/rest/case/case_assets_routes.py
- source/app/blueprints/rest/case/case_ioc_routes.py
🧰 Additional context used
🧬 Code graph analysis (20)
source/app/blueprints/rest/v2/case_objects/notes.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/case_objects/evidences.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/pages/manage/manage_cases_routes.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/evidences_routes/comments.py (6)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/business/comments.py (2)
comments_get_filtered_by_evidence(66-67)comments_create_for_evidence(145-157)source/app/models/models.py (1)
CaseReceivedFile(551-573)source/app/business/evidences.py (1)
evidences_get(60-64)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/assets.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/case_objects/iocs.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/manage/manage_cases_routes.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/tasks.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/case_objects/notes_directories.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/case_objects/events.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/case_objects/tasks.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/case_objects/assets.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/graphql/permissions.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/cases.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/manage_routes/groups.py (1)
source/app/models/authorization.py (2)
Permissions(48-65)ac_flag_match_mask(258-259)
source/app/blueprints/rest/v2/assets_routes/comments.py (8)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_asset(62-63)comments_create_for_asset(129-142)source/app/business/assets.py (1)
assets_get(70-75)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/manage/manage_groups.py (1)
source/app/models/authorization.py (2)
Permissions(48-65)ac_flag_match_mask(258-259)
source/app/blueprints/rest/v2/iocs_routes/comments.py (7)
source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_ioc(70-71)comments_create_for_ioc(160-172)source/app/business/iocs.py (1)
iocs_get(47-51)source/app/business/errors.py (1)
ObjectNotFoundError(35-38)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)
source/app/blueprints/rest/v2/tasks_routes/comments.py (8)
source/app/blueprints/access_controls.py (1)
ac_api_requires(379-383)source/app/blueprints/rest/endpoints.py (4)
response_api_paginated(40-52)response_api_not_found(72-73)response_api_created(59-60)response_api_error(63-69)source/app/blueprints/rest/parsing.py (1)
parse_pagination_parameters(46-53)source/app/business/comments.py (2)
comments_get_filtered_by_task(78-79)comments_create_for_task(191-204)source/app/business/tasks.py (1)
tasks_get(83-87)source/app/schema/marshables.py (1)
CommentSchema(393-406)source/app/business/access_controls.py (1)
ac_fast_check_current_user_has_case_access(22-23)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
source/app/iris_engine/access_control/utils.py (2)
source/app/datamgmt/manage/manage_access_control_db.py (2)
check_ua_case_client(80-107)get_case_effective_access(110-121)source/app/models/authorization.py (2)
CaseAccessLevel(38-45)ac_flag_match_mask(258-259)
🔇 Additional comments (27)
source/app/blueprints/pages/manage/manage_cases_routes.py (1)
37-37: LGTM — import move verified. Wrapper present at source/app/business/access_controls.py calling ac_fast_check_user_has_case_access(iris_current_user.id, cid, access_level); signature matches def ac_fast_check_user_has_case_access(user_id, cid, expected_access_levels) in source/app/iris_engine/access_control/utils.py. No stale imports or circular imports found.source/app/blueprints/rest/manage/manage_cases_routes.py (1)
41-41: Import relocation approved — no lingering old importsWrapper defined at source/app/business/access_controls.py (def ac_fast_check_current_user_has_case_access) and delegates to app.iris_engine.access_control.utils; search found no remaining old-path imports and call sites reference the wrapper. No circular import observed.
source/app/blueprints/graphql/permissions.py (1)
28-28: Good move: import the business-layer wrapper to reduce engine coupling.Switching to
app.business.access_controls.ac_fast_check_current_user_has_case_accesscentralizes context-aware checks and keeps GraphQL free of engine internals.source/app/blueprints/rest/v2/tasks.py (1)
33-33: Import path update LGTM — no stale imports remain.
Wrapper in app/business/access_controls.py delegates to ac_fast_check_user_has_case_access(iris_current_user.id) (no behavior change). Repository search found modules importing ac_fast_check_current_user_has_case_access from app.business.access_controls and no remaining imports from app.iris_engine.access_control.*.source/app/blueprints/rest/v2/evidences_routes/comments.py (6)
21-21: Imports look correct and are all used.Also applies to: 26-29, 31-33, 36-36
54-55: Nice refactor to centralize access checks via _get_evidence.
Keeps the GET path consistent with POST and avoids duplication.
59-60: Good switch to passing the domain object instead of the identifier.
Aligns with comments_get_filtered_by_evidence(evidence, …).
86-91: Verified — final route mounts at /api/v2/evidences/{id}/comments.
rest_v2_blueprint (url_prefix='/api/v2') → evidences_blueprint (url_prefix='/evidences') → evidences_comments_blueprint (url_prefix='/int:evidence_identifier/comments'), producing /api/v2/evidences/{id}/comments.
48-49: Verify that the access checker accepts multiple access levels (list/sequence).
ac_fast_check_current_user_has_case_access (source/app/business/access_controls.py) simply forwards to ac_fast_check_user_has_case_access from app.iris_engine.access_control.utils; that implementation isn't present in this repo — confirm it accepts iterables (e.g. [CaseAccessLevel.read_only, CaseAccessLevel.full_access]) or update the wrapper/callers to handle multiple levels explicitly.
66-66: Confirmed: full_access is required for creating comments — no lower case-access level exists.
CaseAccessLevel only defines deny_all, read_only and full_access; all v2 comment create handlers use CaseAccessLevel.full_access (alerts use Permissions.alerts_write).source/app/blueprints/rest/v2/case_objects/notes_directories.py (1)
38-38: Import relocation to business layer looks goodConsistent with the new wrapper location.
source/app/blueprints/rest/manage/manage_groups.py (1)
42-42: Import move to app.models.authorization is correctMatches the centralization of permission helpers.
source/app/blueprints/rest/v2/iocs_routes/comments.py (1)
44-50: Access check + 404 mapping is appropriateHiding authorization details behind 404 avoids object enumeration.
source/app/blueprints/rest/v2/manage_routes/groups.py (1)
34-35: Import consolidation is correctUsing ac_flag_match_mask from app.models.authorization aligns with the refactor.
source/app/blueprints/rest/v2/case_objects/assets.py (1)
41-41: Import moved to app.business.access_controls — OKConsistent with new wrapper location.
source/app/blueprints/rest/v2/assets.py (1)
31-31: Import moved to app.business.access_controls — OKNo behavioral change.
source/app/blueprints/rest/v2/case_objects/iocs.py (1)
38-38: LGTM – centralized access-control importImporting from app.business.access_controls aligns with the new wrapper and keeps v2 endpoints consistent.
source/app/blueprints/rest/v2/case_objects/notes.py (1)
25-25: LGTM – centralized access-control importConsistent with the new business-layer wrapper.
source/app/blueprints/rest/v2/case_objects/evidences.py (1)
23-23: LGTM – centralized access-control importMatches the PR-wide refactor.
source/app/blueprints/rest/v2/case_objects/tasks.py (2)
40-40: LGTM – centralized access-control importKeeps v2 tasks aligned with the new wrapper.
59-63: No change required — tasks_create returns (str, CaseTasks); current dump is correct. Rename the local variablecasetotaskfor clarity.source/app/blueprints/rest/v2/case_objects/events.py (1)
39-39: LGTM – centralized access-control importConsistent with the rest of v2 case-object modules.
source/app/blueprints/rest/v2/cases.py (1)
48-48: LGTM – centralized access-control importCases endpoints now depend on the business-layer wrapper; looks good.
source/app/blueprints/rest/v2/assets_routes/comments.py (1)
86-89: LGTM: route wiring matches the new v2 contract.Endpoint shape aligns with
/api/v2/assets/<id>/commentsand uses the shared ops class.Please confirm this blueprint is registered under the
/api/v2/assetsparent so the final path is correct.source/app/blueprints/rest/v2/tasks_routes/comments.py (3)
53-59: LGTM: centralizing access check and passing Task object improves cohesion.Using
_get_task(...)and passing the Task tocomments_get_filtered_by_taskis clearer and keeps access control in one place.
87-90: LGTM: POST route wiring is correct and consistent with GET.The handler delegates to the operation method and is guarded with
ac_api_requires().
44-50: No change required — checker accepts a list of CaseAccessLevelac_fast_check_current_user_has_case_access forwards to ac_fast_check_user_has_case_access(..., expected_access_levels: list[CaseAccessLevel]) (source/app/iris_engine/access_control/utils.py); callers passing a list are correct.
Likely an incorrect or invalid review comment.
| @staticmethod | ||
| def _get_asset(asset_identifier, possible_case_access_levels): | ||
| asset = assets_get(asset_identifier) | ||
| if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels): | ||
| raise ObjectNotFoundError() | ||
| return asset | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm access-check API accepts a list; otherwise pass a single threshold level.
The call site passes a list of levels, but ac_fast_check_current_user_has_case_access(cid, access_level) appears to take a single level. If lists aren’t supported, refactor to a single “minimum required” level and update callers.
- @staticmethod
- def _get_asset(asset_identifier, possible_case_access_levels):
+ @staticmethod
+ def _get_asset(asset_identifier, required_level):
asset = assets_get(asset_identifier)
- if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels):
+ if not ac_fast_check_current_user_has_case_access(asset.case_id, required_level):
raise ObjectNotFoundError()
return asset- asset = self._get_asset(asset_identifier, [CaseAccessLevel.read_only, CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.read_only)- asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.full_access)Also applies to: 53-53, 64-64
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== target file snippet: source/app/blueprints/rest/v2/assets_routes/comments.py ==="
sed -n '1,200p' source/app/blueprints/rest/v2/assets_routes/comments.py || true
echo
echo "=== locating definition(s) of ac_fast_check_current_user_has_case_access ==="
rg -n --hidden -S "def ac_fast_check_current_user_has_case_access\b" -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
echo "=== showing usages of ac_fast_check_current_user_has_case_access (context) ==="
rg -n --hidden -S "ac_fast_check_current_user_has_case_access\(" -C3 -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
echo "=== locating CaseAccessLevel definition/usages ==="
rg -n --hidden -S "class CaseAccessLevel\b|CaseAccessLevel\." -C3 -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
echo "=== locate assets_get definition/usages ==="
rg -n --hidden -S "def assets_get\b|assets_get\(" -C3 -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
# If a definition file for the function was found, print its surrounding function block (up to 200 lines).
for file in $(rg -n --hidden -S "def ac_fast_check_current_user_has_case_access\b" -l || true); do
echo "---- contents of $file (first 300 lines) ----"
sed -n '1,300p' "$file"
doneLength of output: 5820
Fix: pass a single CaseAccessLevel to ac_fast_check_current_user_has_case_access (don't pass lists)
ac_fast_check_current_user_has_case_access(cid, access_level) accepts a single level (it delegates to ac_fast_check_user_has_case_access in source/app/business/access_controls.py); current code passes lists — change _get_asset to take a single required level and update callers.
Location: source/app/blueprints/rest/v2/assets_routes/comments.py (lines ~44-50; call sites at ~53 and ~64)
- @staticmethod
- def _get_asset(asset_identifier, possible_case_access_levels):
+ @staticmethod
+ def _get_asset(asset_identifier, required_level):
asset = assets_get(asset_identifier)
- if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels):
+ if not ac_fast_check_current_user_has_case_access(asset.case_id, required_level):
raise ObjectNotFoundError()
return asset- asset = self._get_asset(asset_identifier, [CaseAccessLevel.read_only, CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.read_only)- asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.full_access)📝 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.
| @staticmethod | |
| def _get_asset(asset_identifier, possible_case_access_levels): | |
| asset = assets_get(asset_identifier) | |
| if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels): | |
| raise ObjectNotFoundError() | |
| return asset | |
| @staticmethod | |
| def _get_asset(asset_identifier, required_level): | |
| asset = assets_get(asset_identifier) | |
| if not ac_fast_check_current_user_has_case_access(asset.case_id, required_level): | |
| raise ObjectNotFoundError() | |
| return asset |
🤖 Prompt for AI Agents
In source/app/blueprints/rest/v2/assets_routes/comments.py around lines 44-50,
_get_asset currently accepts possible_case_access_levels and passes that list
into ac_fast_check_current_user_has_case_access which expects a single
CaseAccessLevel; change _get_asset to accept a single required_case_access_level
(or similarly named param) and pass that single level to
ac_fast_check_current_user_has_case_access, then update the two call sites near
lines ~53 and ~64 to pass a single CaseAccessLevel value instead of a list;
ensure parameter name and usages are consistent and adjust any tests or imports
if necessary.
| def create(self, asset_identifier): | ||
| try: | ||
| asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access]) | ||
| comment = self._schema.load(request.get_json()) | ||
| comments_create_for_asset(iris_current_user, asset, comment) | ||
|
|
||
| result = self._schema.dump(comment) | ||
| return response_api_created(result) | ||
| except ValidationError as e: | ||
| return response_api_error('Data error', data=e.normalized_messages()) | ||
| except ObjectNotFoundError: | ||
| return response_api_not_found() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden JSON parsing and fail on unknown fields.
- Return 400 on malformed/non-JSON payloads instead of letting a BadRequest bubble.
- Reject unknown fields up-front to avoid silent drops.
def create(self, asset_identifier):
try:
- asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
- comment = self._schema.load(request.get_json())
+ asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
+ try:
+ data = request.get_json()
+ except BadRequest:
+ return response_api_error('Invalid JSON payload')
+ if data is None:
+ return response_api_error('Expected application/json payload')
+ comment = self._schema.load(data, unknown=RAISE)
comments_create_for_asset(iris_current_user, asset, comment)
result = self._schema.dump(comment)
return response_api_created(result)
except ValidationError as e:
return response_api_error('Data error', data=e.normalized_messages())
except ObjectNotFoundError:
return response_api_not_found()Add imports (outside this hunk):
from marshmallow import RAISE
from werkzeug.exceptions import BadRequest🤖 Prompt for AI Agents
In source/app/blueprints/rest/v2/assets_routes/comments.py around lines 62 to
74, the handler currently allows malformed JSON to bubble up and silently
ignores unknown fields; update it to explicitly catch BadRequest from
request.get_json() and return a 400 response for invalid/malformed/non-JSON
payloads, and call self._schema.load(request.get_json(), unknown=RAISE) so
marshmallow raises on unknown fields; also add the required imports at top of
the file: from marshmallow import RAISE and from werkzeug.exceptions import
BadRequest.
| comment = self._schema.load(request.get_json()) | ||
| comments_create_for_asset(iris_current_user, asset, comment) |
There was a problem hiding this comment.
Block mass-assignment of ownership fields (“user”, “user_id”, “case_id”).
CommentSchema currently exposes user and includes FKs on load. A client could try to set author/case. Business logic may overwrite, but don’t rely on it here. Exclude these fields on load for this endpoint.
- comment = self._schema.load(data, unknown=RAISE)
+ # Prevent client-controlled ownership fields
+ comment = CommentSchema(
+ exclude=('user', 'user_id', 'case_id', 'comment_id')
+ ).load(data, unknown=RAISE)Follow-up (in schema): mark user as dump_only=True or move to a dedicated CommentCreateSchema that only allows the text/body field. I can open a patch if you want.
📝 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.
| comment = self._schema.load(request.get_json()) | |
| comments_create_for_asset(iris_current_user, asset, comment) | |
| # Prevent client-controlled ownership fields | |
| comment = CommentSchema( | |
| exclude=('user', 'user_id', 'case_id', 'comment_id') | |
| ).load(request.get_json(), unknown=RAISE) | |
| comments_create_for_asset(iris_current_user, asset, comment) |
🤖 Prompt for AI Agents
In source/app/blueprints/rest/v2/assets_routes/comments.py around lines 65-66,
the endpoint currently loads the entire incoming JSON which allows clients to
mass-assign ownership fields (“user”, “user_id”, “case_id”); change the load
call to explicitly exclude these fields when deserializing (e.g. call the schema
load with exclude=("user","user_id","case_id") or load only the allowed fields
from request.get_json()) so the client cannot set author/case; as a follow-up,
update CommentSchema to mark user as dump_only=True or create a dedicated
CommentCreateSchema that only exposes the body/text field.
| def create(self, evidence_identifier): | ||
| try: | ||
| evidence = self._get_evidence(evidence_identifier, [CaseAccessLevel.full_access]) | ||
| comment = self._schema.load(request.get_json()) | ||
| comments_create_for_evidence(iris_current_user, evidence, comment) | ||
|
|
||
| result = self._schema.dump(comment) | ||
| return response_api_created(result) |
There was a problem hiding this comment.
Harden create against client-controlled fields (potential privilege escalation).
CommentSchema(load_instance=True, include_fk=True) may accept user_id/case_id from the payload. Ensure these are server-controlled to prevent spoofing.
Minimal in-place hardening:
- comment = self._schema.load(request.get_json())
+ payload = request.get_json()
+ # Never trust client for ownership/linkage fields
+ comment = self._schema.load(payload, partial=('comment_id', 'user_id', 'case_id'))
+ comment.user = iris_current_user
+ comment.case_id = evidence.case_idLonger-term: introduce a dedicated CommentCreateSchema exposing only safe, writable fields (e.g., content/visibility), and use that for .load().
📝 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 create(self, evidence_identifier): | |
| try: | |
| evidence = self._get_evidence(evidence_identifier, [CaseAccessLevel.full_access]) | |
| comment = self._schema.load(request.get_json()) | |
| comments_create_for_evidence(iris_current_user, evidence, comment) | |
| result = self._schema.dump(comment) | |
| return response_api_created(result) | |
| def create(self, evidence_identifier): | |
| try: | |
| evidence = self._get_evidence(evidence_identifier, [CaseAccessLevel.full_access]) | |
| payload = request.get_json() | |
| # Never trust client for ownership/linkage fields | |
| comment = self._schema.load(payload, partial=('comment_id', 'user_id', 'case_id')) | |
| comment.user = iris_current_user | |
| comment.case_id = evidence.case_id | |
| comments_create_for_evidence(iris_current_user, evidence, comment) | |
| result = self._schema.dump(comment) | |
| return response_api_created(result) |
🤖 Prompt for AI Agents
In source/app/blueprints/rest/v2/evidences_routes/comments.py around lines 64 to
71, the CommentSchema.load currently accepts client-controlled fields (like
user_id/case_id) which can be used to spoof ownership; to fix, sanitize the
incoming payload before persisting by removing or ignoring any user_id/case_id
(and other FK/privilege fields) from request data, then set the correct
iris_current_user.id and the resolved evidence.case_id/server-controlled case id
on the comment object server-side prior to calling comments_create_for_evidence;
as a longer-term improvement replace CommentSchema.load here with a dedicated
CommentCreateSchema that only exposes safe writable fields (e.g., content,
visibility) and use that for .load().
| def create(self, ioc_identifier): | ||
| try: | ||
| ioc = self._get_ioc(ioc_identifier, [CaseAccessLevel.full_access]) | ||
|
|
||
| comment = self._schema.load(request.get_json()) | ||
| comments_create_for_ioc(iris_current_user, ioc, comment) | ||
|
|
||
| result = self._schema.dump(comment) | ||
| return response_api_created(result) | ||
| except ValidationError as e: | ||
| return response_api_error('Data error', data=e.normalized_messages()) | ||
| except ObjectNotFoundError: | ||
| return response_api_not_found() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize input to prevent user/ID spoofing on comment creation
Ensure clients cannot set comment_id/user_id. Let the server own these.
Apply:
- def create(self, ioc_identifier):
+ def create(self, ioc_identifier):
try:
ioc = self._get_ioc(ioc_identifier, [CaseAccessLevel.full_access])
- comment = self._schema.load(request.get_json())
+ payload = request.get_json() or {}
+ # Ignore client-supplied identifiers
+ for k in ('comment_id', 'user_id', 'id'):
+ payload.pop(k, None)
+ comment = self._schema.load(payload)
comments_create_for_ioc(iris_current_user, ioc, comment)
result = self._schema.dump(comment)
return response_api_created(result)📝 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 create(self, ioc_identifier): | |
| try: | |
| ioc = self._get_ioc(ioc_identifier, [CaseAccessLevel.full_access]) | |
| comment = self._schema.load(request.get_json()) | |
| comments_create_for_ioc(iris_current_user, ioc, comment) | |
| result = self._schema.dump(comment) | |
| return response_api_created(result) | |
| except ValidationError as e: | |
| return response_api_error('Data error', data=e.normalized_messages()) | |
| except ObjectNotFoundError: | |
| return response_api_not_found() | |
| def create(self, ioc_identifier): | |
| try: | |
| ioc = self._get_ioc(ioc_identifier, [CaseAccessLevel.full_access]) | |
| payload = request.get_json() or {} | |
| # Ignore client-supplied identifiers | |
| for k in ('comment_id', 'user_id', 'id'): | |
| payload.pop(k, None) | |
| comment = self._schema.load(payload) | |
| comments_create_for_ioc(iris_current_user, ioc, comment) | |
| result = self._schema.dump(comment) | |
| return response_api_created(result) | |
| except ValidationError as e: | |
| return response_api_error('Data error', data=e.normalized_messages()) | |
| except ObjectNotFoundError: | |
| return response_api_not_found() |
🤖 Prompt for AI Agents
In source/app/blueprints/rest/v2/iocs_routes/comments.py around lines 61 to 73,
the route currently trusts client-provided fields when creating a comment; strip
or ignore client-supplied comment_id and user_id before persisting and ensure
the server assigns these values (generate comment_id on create and set user_id
from iris_current_user). Concretely: after reading request.get_json(),
remove/pop any 'comment_id' and 'user_id' keys (or validate the schema to
exclude them), build the comment payload with server-controlled user info
(iris_current_user.id) and then call comments_create_for_ioc with that payload
so the persisted record is created with server-assigned IDs; return the
serialized created comment.
| comment = self._schema.load(request.get_json()) | ||
| comments_create_for_task(iris_current_user, task, comment) | ||
|
|
||
| result = self._schema.dump(comment) | ||
| return response_api_created(result) | ||
| except ValidationError as e: | ||
| return response_api_error('Data error', data=e.normalized_messages()) | ||
| except ObjectNotFoundError: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Harden create() against invalid JSON and client-controlled identity/IDs.
- Handle malformed/absent JSON to avoid 500s.
- Reject client-supplied fields like
user,user_id,id, etc.; the actor comes fromiris_current_userand IDs should be server-assigned.
+from werkzeug.exceptions import BadRequest
@@
- comment = self._schema.load(request.get_json())
+ try:
+ payload = request.get_json()
+ except BadRequest:
+ return response_api_error('Invalid JSON body')
+ if not isinstance(payload, dict):
+ return response_api_error('Invalid JSON body')
+ # Forbid client-controlled identity/ID fields
+ forbidden = ('user', 'user_id', 'comment_id', 'id', 'comment_user_id', 'created_at', 'updated_at')
+ illegal = {k: 'Field not allowed' for k in forbidden if k in payload}
+ if illegal:
+ return response_api_error('Data error', data=illegal)
+ comment = self._schema.load(payload)If CommentSchema already marks these as dump_only, feel free to ignore the forbid-list and keep just the JSON validation. Otherwise, consider a dedicated CommentCreateSchema with a minimal whitelist.
Also applies to: 21-21
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Show target route file (first 240 lines) ==="
if [ -f "source/app/blueprints/rest/v2/tasks_routes/comments.py" ]; then
nl -ba "source/app/blueprints/rest/v2/tasks_routes/comments.py" | sed -n '1,240p' || true
else
echo "MISSING: source/app/blueprints/rest/v2/tasks_routes/comments.py"
fi
echo
echo "=== 2) Search repo for CommentSchema definitions/usages and dump_only/load_only fields ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -S --type py 'class\s+CommentSchema\b' || true
rg -n --hidden -S --type py 'class\s+\w*Comment\w*Schema\b' || true
rg -n --hidden -S --type py 'CommentSchema\b' || true
rg -n --hidden -S --type py '\bdump_only\b|\bload_only\b' || true
rg -n --hidden -S --type py 'request.get_json\(' || true
else
echo "rg not found; falling back to grep"
grep -RIn --exclude-dir=venv --exclude-dir=.venv --exclude-dir=node_modules 'class CommentSchema' || true
grep -RIn --exclude-dir=venv --exclude-dir=.venv --exclude-dir=node_modules -E 'class .*Comment.*Schema' || true
grep -RIn --exclude-dir=venv --exclude-dir=.venv --exclude-dir=node_modules 'CommentSchema' || true
grep -RIn --exclude-dir=venv --exclude-dir=.venv --exclude-dir=node_modules -E 'dump_only|load_only' || true
grep -RIn --exclude-dir=venv --exclude-dir=.venv --exclude-dir=node_modules 'request.get_json' || true
fi
echo
echo "=== 3) Show any schema definitions that include user/id/created_at/updated_at fields ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden -S --type py '\b(user|user_id|comment_id|id|comment_user_id|created_at|updated_at)\b' || true
else
grep -RIn --exclude-dir=venv --exclude-dir=.venv --exclude-dir=node_modules -E 'user_id|comment_id|comment_user_id|created_at|updated_at' || true
fiLength of output: 267638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== source/app/blueprints/rest/v2/tasks_routes/comments.py (1..240) ==="
sed -n '1,240p' source/app/blueprints/rest/v2/tasks_routes/comments.py || true
echo
echo "=== source/app/schema/marshables.py (360..460) — CommentSchema vicinity ==="
sed -n '360,460p' source/app/schema/marshables.py || true
echo
echo "=== source/app/schema/marshables.py (720..820) — common dump_only fields ==="
sed -n '720,820p' source/app/schema/marshables.py || true
echo
echo "=== Search for comment_id / comment_user_id in marshables.py ==="
rg -n --hidden -S 'comment_id|comment_user_id' source/app/schema/marshables.py || trueLength of output: 12492
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show business/comments.py (1..300) ==="
sed -n '1,300p' source/app/business/comments.py || true
echo
echo "=== show business/comments.py (300..600) ==="
sed -n '300,600p' source/app/business/comments.py || true
echo
echo "=== grep for comments_create_for_task definition and usages ==="
rg -n "def\s+comments_create_for_task\b" source || true
rg -n "comments_create_for_task\(" source || trueLength of output: 9085
Harden create() against invalid JSON and client-controlled identity/IDs
- Catch malformed/absent JSON and ensure payload is a dict to avoid 500s.
- Reject client-controlled identity fields (user, user_id, comment_id, id, comment_user_id, created_at, updated_at) or use a dedicated CommentCreateSchema whitelist — CommentSchema is an SQLAlchemyAutoSchema (Meta unknown=EXCLUDE) and does not declare these fields dump_only, so load() can accept them.
File: source/app/blueprints/rest/v2/tasks_routes/comments.py (create method)
Suggested patch:
+from werkzeug.exceptions import BadRequest
@@
- comment = self._schema.load(request.get_json())
+ try:
+ payload = request.get_json()
+ except BadRequest:
+ return response_api_error('Invalid JSON body')
+ if not isinstance(payload, dict):
+ return response_api_error('Invalid JSON body')
+ # Forbid client-controlled identity/ID fields
+ forbidden = ('user', 'user_id', 'comment_id', 'id', 'comment_user_id', 'created_at', 'updated_at')
+ illegal = {k: 'Field not allowed' for k in forbidden if k in payload}
+ if illegal:
+ return response_api_error('Data error', data=illegal)
+ comment = self._schema.load(payload)Also apply same hardening to other v2 comment create handlers (notes/events/iocs/assets/alerts/evidences, etc.) where self._schema.load(request.get_json()) is used.
📝 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.
| comment = self._schema.load(request.get_json()) | |
| comments_create_for_task(iris_current_user, task, comment) | |
| result = self._schema.dump(comment) | |
| return response_api_created(result) | |
| except ValidationError as e: | |
| return response_api_error('Data error', data=e.normalized_messages()) | |
| except ObjectNotFoundError: | |
| try: | |
| payload = request.get_json() | |
| except BadRequest: | |
| return response_api_error('Invalid JSON body') | |
| if not isinstance(payload, dict): | |
| return response_api_error('Invalid JSON body') | |
| # Forbid client-controlled identity/ID fields | |
| forbidden = ('user', 'user_id', 'comment_id', 'id', 'comment_user_id', 'created_at', 'updated_at') | |
| illegal = {k: 'Field not allowed' for k in forbidden if k in payload} | |
| if illegal: | |
| return response_api_error('Data error', data=illegal) | |
| comment = self._schema.load(payload) | |
| comments_create_for_task(iris_current_user, task, comment) | |
| result = self._schema.dump(comment) | |
| return response_api_created(result) | |
| except ValidationError as e: | |
| return response_api_error('Data error', data=e.normalized_messages()) | |
| except ObjectNotFoundError: |
🤖 Prompt for AI Agents
In source/app/blueprints/rest/v2/tasks_routes/comments.py around lines 66 to 73,
the create() handler directly calls self._schema.load(request.get_json()) which
can accept malformed/absent JSON and client-controlled identity fields (user,
user_id, comment_id, id, comment_user_id, created_at, updated_at) because
CommentSchema is an SQLAlchemyAutoSchema without dump_only set; to fix, first
safely get and validate the JSON body (catch BadRequest or return an API error
if request.get_json() returns None or is not a dict), then either instantiate
and use a dedicated CommentCreateSchema (whitelisting allowed fields and marking
identity fields as dump_only/Excluded) or explicitly remove/reject the listed
identity fields from the input before calling load(), and wrap load() in a
try/except to return a proper 4xx response on invalid payload rather than
letting it raise a 500; apply the same pattern to other v2 comment create
handlers (notes/events/iocs/assets/alerts/evidences, etc.) that call
self._schema.load(request.get_json()).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/app/datamgmt/manage/manage_access_control_db.py (1)
196-205: Fix broken setter: .first() misuse and no insert pathuac = query.first() returns an entity, not a list; indexing [0] will crash, and no row is created when absent.
Apply:
-def set_user_case_effective_access(access_level, case_id, user_id): - uac = UserCaseEffectiveAccess.query.where(and_( - UserCaseEffectiveAccess.user_id == user_id, - UserCaseEffectiveAccess.case_id == case_id - )).first() - if uac: - uac = uac[0] - uac.access_level = access_level - db.session.commit() +def set_user_case_effective_access(access_level: int, case_id: int, user_id: int): + uac = ( + UserCaseEffectiveAccess.query + .filter( + UserCaseEffectiveAccess.user_id == user_id, + UserCaseEffectiveAccess.case_id == case_id + ) + .one_or_none() + ) + if uac: + uac.access_level = access_level + else: + uac = UserCaseEffectiveAccess( + user_id=user_id, + case_id=case_id, + access_level=access_level, + ) + db.session.add(uac) + db.session.commit()
♻️ Duplicate comments (1)
source/app/datamgmt/manage/manage_access_control_db.py (1)
112-124: Ensure uniqueness; avoid .first() ambiguityUse one_or_none() to avoid arbitrary row selection; this also aligns with enforcing a unique (user_id, case_id) pair.
Apply:
-def get_case_effective_access(user_identifier, case_identifier) -> Optional[int]: - row = UserCaseEffectiveAccess.query.with_entities( - UserCaseEffectiveAccess.access_level - ).filter( - UserCaseEffectiveAccess.user_id == user_identifier, - UserCaseEffectiveAccess.case_id == case_identifier - ).first() +def get_case_effective_access(user_identifier: int, case_identifier: int) -> Optional[int]: + row = ( + UserCaseEffectiveAccess.query + .with_entities(UserCaseEffectiveAccess.access_level) + .filter( + UserCaseEffectiveAccess.user_id == user_identifier, + UserCaseEffectiveAccess.case_id == case_identifier + ) + .one_or_none() + )
🧹 Nitpick comments (4)
source/app/blueprints/rest/case/case_routes.py (1)
261-263: Redundant commitset_user_case_access and track_activity both commit. The extra db.session.commit() is likely unnecessary.
Apply:
- db.session.commit() return response_success(msg=f'Case access set to {access_level} for user {user.id}')source/app/datamgmt/manage/manage_access_control_db.py (2)
82-110: Docstring and return-type are outdatedFunction now returns Optional[int], not a UserClient. Update docstring to avoid confusion.
Apply:
-def check_ua_case_client(user_id: int, case_id: int) -> Optional[int]: - """Check if the user has access to the case, through the customer of the case - (in other words, check that the customer of the case is assigned to the user) -... - Returns: - UserClient: the user relationship with the customer of the case, if it is assigned to the user - None otherwise - """ +def check_ua_case_client(user_id: int, case_id: int) -> Optional[int]: + """Check if the user has access to the case via the case's client. + + Returns: + Optional[int]: access level mask if related to the client, else None. + """
181-194: Dedup logic is fine; consider returning count removed (optional)Current implementation works. Returning the number removed could aid observability.
source/app/iris_engine/access_control/utils.py (1)
684-706: Unify access_list shape (string vs list) for consistencyDefault block uses a string name while other blocks use ac_access_level_to_list(...). Make it consistent.
Apply:
- 'access_list': CaseAccessLevel.deny_all.name, + 'access_list': ac_access_level_to_list(CaseAccessLevel.deny_all.value),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
source/app/blueprints/access_controls.py(1 hunks)source/app/blueprints/rest/case/case_routes.py(2 hunks)source/app/business/access_controls.py(1 hunks)source/app/datamgmt/manage/manage_access_control_db.py(4 hunks)source/app/datamgmt/manage/manage_users_db.py(0 hunks)source/app/iris_engine/access_control/utils.py(11 hunks)
💤 Files with no reviewable changes (1)
- source/app/datamgmt/manage/manage_users_db.py
🧰 Additional context used
🧬 Code graph analysis (5)
source/app/business/access_controls.py (7)
source/app/datamgmt/manage/manage_access_control_db.py (4)
get_case_effective_access(112-123)remove_duplicate_user_case_effective_accesses(181-193)set_user_case_effective_access(196-204)check_ua_case_client(82-109)source/app/models/authorization.py (3)
UserCaseAccess(129-140)CaseAccessLevel(38-45)ac_flag_match_mask(258-259)source/app/blueprints/rest/v2/case_objects/events.py (1)
delete(135-151)source/app/blueprints/rest/v2/case_objects/assets.py (1)
delete(139-152)source/app/blueprints/rest/v2/case_objects/evidences.py (1)
delete(122-137)source/app/blueprints/rest/v2/case_objects/notes.py (1)
delete(128-139)source/app/blueprints/rest/v2/case_objects/notes_directories.py (1)
delete(120-129)
source/app/blueprints/access_controls.py (1)
source/app/business/access_controls.py (1)
ac_fast_check_user_has_case_access(72-96)
source/app/datamgmt/manage/manage_access_control_db.py (3)
source/app/iris_engine/access_control/utils.py (1)
ac_current_user_has_permission(868-872)source/app/models/cases.py (1)
Cases(50-160)source/app/models/authorization.py (4)
Group(100-112)UserClient(184-196)UserCaseEffectiveAccess(143-154)CaseAccessLevel(38-45)
source/app/blueprints/rest/case/case_routes.py (5)
source/app/business/access_controls.py (2)
set_user_case_access(34-58)ac_fast_check_user_has_case_access(72-96)source/app/blueprints/responses.py (2)
response_error(48-54)response_success(57-63)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)source/app/iris_engine/utils/tracker.py (1)
track_activity(30-66)source/app/util.py (1)
add_obj_history_entry(49-79)
source/app/iris_engine/access_control/utils.py (2)
source/app/business/access_controls.py (1)
set_case_effective_access_for_user(61-69)source/app/models/authorization.py (1)
ac_flag_match_mask(258-259)
⏰ 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). (8)
- GitHub Check: End to end tests
- GitHub Check: Generate graphQL documentation
- GitHub Check: Database migration tests
- GitHub Check: Test API
- GitHub Check: End to end tests
- GitHub Check: Test API
- GitHub Check: Generate graphQL documentation
- GitHub Check: Database migration tests
🔇 Additional comments (8)
source/app/blueprints/access_controls.py (1)
51-51: Import relocation looks goodSwitching to app.business.access_controls for ac_fast_check_user_has_case_access aligns with the new AC module.
source/app/blueprints/rest/case/case_routes.py (2)
41-41: Correct import source for AC helpersUsing set_user_case_access and ac_fast_check_user_has_case_access from app.business.access_controls is consistent with the refactor.
245-256: Harden type checks (avoid type(...) is ...) and accept only intstype(x) is int is brittle (and accepts bools implicitly). Prefer isinstance and explicit error paths.
Apply:
- if user.id is None or type(user.id) is not int: + if user.id is None or not isinstance(user.id, int): return response_error('Invalid user id') - if case_identifier is None or type(case_identifier) is not int: + if case_identifier is None or not isinstance(case_identifier, int): return response_error('Invalid case id') - if access_level is None or type(access_level) is not int: + if access_level is None or not isinstance(access_level, int): return response_error('Invalid access level') if CaseAccessLevel.has_value(access_level) is False: return response_error('Invalid access level')Likely an incorrect or invalid review comment.
source/app/business/access_controls.py (2)
34-56: OK: duplicate cleanup and upsert for UserCaseAccessFlow handles duplicates then upserts and commits, followed by effective-access update.
89-96: Consider empty expected_access_levels semanticsIf expected_access_levels is empty, this function will return None even when the user has access. Clarify or handle empty = “any non-deny” as needed by callers.
Would you like me to adjust the function to return access_level when expected_access_levels is empty?
source/app/iris_engine/access_control/utils.py (3)
5-6: Imports aligned with new AC moduleSwitching to set_case_effective_access_for_user and importing ac_flag_match_mask from models is consistent.
Also applies to: 11-11
503-503: Good: log via central loggerUsing logger.error standardizes logging.
533-537: OK: delegate to business-layer setterac_set_case_access_for_users now uses set_case_effective_access_for_user; consistent with refactor.
| def ac_fast_check_user_has_case_access(user_id, cid, expected_access_levels: list[CaseAccessLevel]): | ||
| """ | ||
| Checks the user has access to the case with at least one of the access_level | ||
| if the user has access, returns the access level of the user to the case | ||
| Returns None otherwise | ||
| """ | ||
| access_level = get_case_effective_access(user_id, cid) | ||
|
|
||
| if not access_level: | ||
| # The user has no direct access, check if he is part of the client | ||
| access_level = check_ua_case_client(user_id, cid) | ||
| if not access_level: | ||
| return None | ||
| set_case_effective_access_for_user(user_id, cid, access_level) | ||
|
|
||
| return access_level | ||
|
|
||
| if ac_flag_match_mask(access_level, CaseAccessLevel.deny_all.value): |
There was a problem hiding this comment.
AuthZ bug: early return bypasses deny_all and expected-level checks
Returning immediately after client-fallback can authorize deny_all or insufficient levels. Also, test for None explicitly.
Apply:
- access_level = get_case_effective_access(user_id, cid)
-
- if not access_level:
+ access_level = get_case_effective_access(user_id, cid)
+ if access_level is None:
# The user has no direct access, check if he is part of the client
access_level = check_ua_case_client(user_id, cid)
- if not access_level:
+ if access_level is None:
return None
set_case_effective_access_for_user(user_id, cid, access_level)
-
- return access_level📝 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 ac_fast_check_user_has_case_access(user_id, cid, expected_access_levels: list[CaseAccessLevel]): | |
| """ | |
| Checks the user has access to the case with at least one of the access_level | |
| if the user has access, returns the access level of the user to the case | |
| Returns None otherwise | |
| """ | |
| access_level = get_case_effective_access(user_id, cid) | |
| if not access_level: | |
| # The user has no direct access, check if he is part of the client | |
| access_level = check_ua_case_client(user_id, cid) | |
| if not access_level: | |
| return None | |
| set_case_effective_access_for_user(user_id, cid, access_level) | |
| return access_level | |
| if ac_flag_match_mask(access_level, CaseAccessLevel.deny_all.value): | |
| def ac_fast_check_user_has_case_access(user_id, cid, expected_access_levels: list[CaseAccessLevel]): | |
| """ | |
| Checks the user has access to the case with at least one of the access_level | |
| if the user has access, returns the access level of the user to the case | |
| Returns None otherwise | |
| """ | |
| access_level = get_case_effective_access(user_id, cid) | |
| if access_level is None: | |
| # The user has no direct access, check if he is part of the client | |
| access_level = check_ua_case_client(user_id, cid) | |
| if access_level is None: | |
| return None | |
| set_case_effective_access_for_user(user_id, cid, access_level) | |
| if ac_flag_match_mask(access_level, CaseAccessLevel.deny_all.value): |
🤖 Prompt for AI Agents
In source/app/business/access_controls.py around lines 72 to 89, the current
early return after the client-fallback path skips the deny_all mask and
expected_access_levels checks and uses a falsy check instead of explicit None;
change the flow so that you only return None if access_level is still None (use
"is None" for the check), remove the immediate return after setting case
effective access, and continue to run the
ac_flag_match_mask(CaseAccessLevel.deny_all) check and the
expected_access_levels validation before returning the access level or None.
| from app.models.cases import Cases | ||
| from app.models.authorization import Group | ||
| from app.models.authorization import UserClient | ||
| from app.models.authorization import UserCaseEffectiveAccess |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Enforce DB uniqueness at the model level (follow-up)
Ensure UserCaseEffectiveAccess has a bound UniqueConstraint('case_id','user_id') in table_args to prevent duplicates at source.
Run:
🏁 Script executed:
#!/bin/bash
rg -nC2 "class\s+UserCaseEffectiveAccess\b" -n
rg -nC3 "__table_args__" --type=py | sed -n '1,120p'Length of output: 1148
Add UniqueConstraint(case_id, user_id) to UserCaseEffectiveAccess model
source/app/models/authorization.py — class UserCaseEffectiveAccess (around line 143) currently has no table_args; add:
table_args = (UniqueConstraint('case_id', 'user_id', name='user_case_effective_access_case_id_user_id_key'),)
and create a DB migration to enforce this on existing databases.
🤖 Prompt for AI Agents
In source/app/datamgmt/manage/manage_access_control_db.py around line 24 and in
source/app/models/authorization.py around line 143, add a UniqueConstraint on
(case_id, user_id) to the UserCaseEffectiveAccess model by setting
__table_args__ = (UniqueConstraint('case_id', 'user_id',
name='user_case_effective_access_case_id_user_id_key'),) in the model class,
then generate and include a DB migration that adds this unique constraint for
existing databases (handle any duplicate rows before applying the migration or
include a cleanup step in the migration).
…y entry on the alert
…en alert is not found
…en field has incorrect type
…ther than 403 when access to asset is forbidden (so as to not leak any info about the existence of the asset)
…be imported from the business layer)
…to business layer
c8y3
force-pushed
the
api_v2_create_object_comment
branch
from
926353e to
374dddb
Compare
Implement endpoint
POST /api/v2/{object}/{object_identifier}/commentsPOST /{object}/{object_identifier}/comments/addCommentsmodels in their own fileOptional)user_list_cases_viewincase_iocs_db.pyCommenttableiris_engine/access_controls/utils.pyinto the business layeriris_engine/access_controls/utils.pyinto the persistence layerdatamgmt/manage/manage_users_db.pyup into the API layerAssociated documentation in iris-doc-src PR#80
Summary by CodeRabbit
New Features
Improvements
Deprecations
Chores
Tests