Api v2 delete event #776
Api v2 delete event #776
Conversation
… event returns 404
…4 when case does not exist
…4 when event does not exist
…3 when user has no access to case
…idence when user has no permission to access case should not delete evidence
… when event case identifier and case_identifier do not match
…ELETE /api/v2/cases/{case_identifier}/events/{identifier}
… on_preload hooks which will be deprecated
…PI layer, for events
…reate the EventSchema
WalkthroughThis pull request updates several components of the application. The linter configuration in Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant API as REST Endpoint
participant E as Events Class
participant B as Business Logic
participant DB as Database
U->>API: Request deletion of event
API->>E: Call delete(event, case_identifier)
E->>B: Validate event, permissions, and case match
B->>DB: Execute deletion query
DB-->>B: Confirm deletion
B-->>E: Return success status
E-->>API: Return 204 No Content response
API-->>U: Send response (204)
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
source/app/schema/utils.py (1)
34-49: Consider combining nestedifchecks.
You can simplify the nested checks formax_len,max_val, andmin_valinto a single condition per block. For example:- if max_len: - if len(input_var) > max_len: + if max_len and len(input_var) > max_len: raise ValidationError('Invalid data - max length exceeded', field_name=field_name if field_name else 'type')🧰 Tools
🪛 Ruff (0.8.2)
35-36: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
40-41: Use a single
ifstatement instead of nestedifstatements(SIM102)
45-46: Use a single
ifstatement instead of nestedifstatements(SIM102)
source/app/business/events.py (3)
53-53: Use direct assignment instead of setattr.- setattr(event, 'event_category_id', event_category_id) + event.event_category_id = event_category_idThis avoids the overhead of
setattrfor a single known field.🧰 Tools
🪛 Ruff (0.8.2)
53-53: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
65-65: Use direct assignment instead of setattr.- setattr(event, 'event_category_id', event_category_id) + event.event_category_id = event_category_id🧰 Tools
🪛 Ruff (0.8.2)
65-65: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
88-88: Direct assignment is more conventional than setattr.- setattr(event, 'event_category_id', event_category_id) + event.event_category_id = event_category_id🧰 Tools
🪛 Ruff (0.8.2)
88-88: Do not call
setattrwith a constant attribute value. It is not any safer than normal property access.Replace
setattrwith assignment(B010)
source/app/blueprints/rest/v2/case_objects/events.py (1)
55-59: Consider moving date validation inside the schema.
Although callingvalidate_dateexternally works, placing date validation within the Marshmallow schema can further encapsulate data validation processes and reduce coupling.def _load(self, request_data, **kwargs): - event = self._schema.load(request_data, **kwargs) - event.event_date, event.event_date_wtz = self._schema.validate_date(request_data.get(u'event_date'), - request_data.get(u'event_tz')) + event = self._schema.load(request_data, **kwargs) # The schema handles all date validation return event
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
pyproject.toml(1 hunks)source/app/blueprints/pages/manage/manage_srv_settings_routes.py(0 hunks)source/app/blueprints/pages/manage/templates/manage_make_update.html(0 hunks)source/app/blueprints/pages/manage/templates/modal_server_updates.html(0 hunks)source/app/blueprints/rest/case/case_timeline_routes.py(12 hunks)source/app/blueprints/rest/v2/case_objects/events.py(2 hunks)source/app/blueprints/rest/v2/case_objects/evidences.py(1 hunks)source/app/business/events.py(3 hunks)source/app/datamgmt/case/case_events_db.py(2 hunks)source/app/iris_engine/module_handler/module_handler.py(19 hunks)source/app/schema/marshables.py(1 hunks)source/app/schema/utils.py(1 hunks)source/app/util.py(0 hunks)tests/tests_rest_events.py(1 hunks)tests/tests_rest_evidences.py(1 hunks)ui/src/pages/updates.handler.js(0 hunks)
💤 Files with no reviewable changes (5)
- source/app/blueprints/pages/manage/templates/manage_make_update.html
- source/app/blueprints/pages/manage/templates/modal_server_updates.html
- source/app/util.py
- source/app/blueprints/pages/manage/manage_srv_settings_routes.py
- ui/src/pages/updates.handler.js
🧰 Additional context used
🧬 Code Graph Analysis (7)
source/app/blueprints/rest/v2/case_objects/evidences.py (1)
source/app/business/evidences.py (1)
evidences_delete(86-90)
source/app/datamgmt/case/case_events_db.py (3)
source/app/blueprints/rest/v2/case_objects/events.py (2)
delete_event(179-180)delete(136-152)source/app/models/models.py (2)
CaseEventsAssets(280-290)CaseEventsIoc(293-303)source/app/datamgmt/states.py (1)
update_timeline_state(86-87)
source/app/iris_engine/module_handler/module_handler.py (1)
source/app/models/models.py (2)
IrisModuleHook(877-891)IrisHook(869-874)
tests/tests_rest_evidences.py (2)
tests/iris.py (2)
create_dummy_case(72-80)create_dummy_user(69-70)source/app/blueprints/rest/v2/case_objects/events.py (3)
create(61-84)delete(136-152)get(86-102)
source/app/business/events.py (3)
source/app/datamgmt/case/case_events_db.py (1)
delete_event(354-384)source/app/iris_engine/utils/tracker.py (1)
track_activity(30-66)source/app/iris_engine/utils/collab.py (1)
collab_notify(6-15)
source/app/blueprints/rest/case/case_timeline_routes.py (5)
source/app/business/events.py (1)
events_delete(106-111)source/app/iris_engine/module_handler/module_handler.py (1)
call_deprecated_on_preload_modules_hook(584-587)source/app/blueprints/rest/endpoints.py (1)
endpoint_deprecated(76-86)source/app/blueprints/responses.py (1)
response_error(48-54)source/app/schema/marshables.py (2)
EventSchema(1132-1284)validate_date(1159-1185)
source/app/blueprints/rest/v2/case_objects/events.py (5)
source/app/business/events.py (1)
events_delete(106-111)source/app/schema/marshables.py (2)
EventSchema(1132-1284)validate_date(1159-1185)source/app/business/cases.py (1)
cases_exists(71-72)source/app/iris_engine/access_control/utils.py (1)
ac_fast_check_current_user_has_case_access(320-321)source/app/models/authorization.py (1)
CaseAccessLevel(38-45)
🪛 Ruff (0.8.2)
source/app/iris_engine/module_handler/module_handler.py
468-468: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
source/app/schema/utils.py
35-36: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
40-41: Use a single if statement instead of nested if statements
(SIM102)
45-46: Use a single if statement instead of nested if statements
(SIM102)
source/app/business/events.py
53-53: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
65-65: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
88-88: Do not call setattr with a constant attribute value. It is not any safer than normal property access.
Replace setattr with assignment
(B010)
🔇 Additional comments (51)
source/app/blueprints/rest/v2/case_objects/evidences.py (1)
141-142: Proper security check implementation.The evidences_delete call is now correctly positioned after the access control checks, which ensures that evidence will only be deleted when the user has appropriate access to the case. This prevents unauthorized deletion of evidence and aligns with the principle of complete authorization before performing sensitive operations.
tests/tests_rest_evidences.py (1)
303-313: Good test case for unauthorized evidence deletion.This test appropriately verifies that evidence remains intact when a user without permissions attempts to delete it. It complements the fix in the
delete_evidencefunction, ensuring that unauthorized deletion attempts are properly blocked while still returning an appropriate response code.source/app/datamgmt/case/case_events_db.py (4)
354-356: Good refactoring of parameter passing.The function now extracts the case ID from the event object rather than requiring it as a separate parameter. This makes the interface more consistent and less prone to errors where mismatched case IDs might be provided.
358-361: Updated query to use case_identifier from event.The filtering condition now correctly uses the case_identifier variable extracted from the event object, maintaining consistency with the function signature change.
363-366: Updated query to use case_identifier from event.Similar to the previous segment, this filtering condition is now correctly using the case_identifier variable from the event.
382-382: Updated parameter passing to match refactored function signature.The call to update_timeline_state now correctly passes the case_identifier extracted from the event, completing the refactoring of the function parameter signature.
source/app/iris_engine/module_handler/module_handler.py (2)
31-31: Standardized logger import.Replacing the local logger variable with a direct import from app.logger improves code consistency and maintainability.
584-588: Added helper function for deprecated hooks.This new function properly handles deprecated hooks by adding a warning log while maintaining backward compatibility. This is a good approach for phasing out deprecated functionality.
source/app/schema/marshables.py (1)
96-99: Reorganized imports look good.
These imports fromapp.schema.utilsare consistent with the new modular structure. No concerns here.source/app/blueprints/rest/case/case_timeline_routes.py (15)
24-24: Improved explicit import for ValidationError.
This direct import frommarshmallow.exceptionsis clear and standard.
77-78: Added imports for new event-deletion logic.
Bringing inevents_deleteandcall_deprecated_on_preload_modules_hookaligns with the refactoring.
160-160: Proper exception handling.
CatchingValidationErrorhere is a clean approach for reporting data issues.
629-629: Great use of deprecation decorator.
Using@endpoint_deprecatedproperly guides clients to the new DELETE API route.
639-639: Refined event deletion call.
The switch toevents_deletecentralizes the deletion logic in the business layer.
693-694: Preload hook invoked before update.
No issues found. The call to a deprecated hook is explicitly signaled.
695-701: Extracted event fields and invoked _load.
Assigningevent_idand then calling_loadkeeps the logic consistent. Looks good.
702-702: Updated event via the new events_update function.
Centralizing update logic in the business layer fosters maintainability.
714-715: Better error feedback for Marshmallow ValidationError.
Usinge.get_message()ande.get_data()clarifies the issue for the client.
726-727: Preload hook invoked before create.
Matches the pattern used in update. No concerns.
728-728: _load used to parse the event data.
Good approach for consistent data loading.
730-734: Pulling relevant fields from request_data.
Straightforward retrieval of event parameters.
735-735: Creating event via events_create in the business layer.
This is consistent with the new architecture.
809-809: Catching ValidationError for duplication scenario.
Still consistent with the other flows; no issues.
1009-1015: Helper_loadfunction fosters code reuse.
This centralized approach to schema validation ensures consistency across create and update flows.source/app/business/events.py (11)
33-34: Importing the new delete function.
Centralizing deletion logic throughdelete_eventis a good dependency injection.
39-39: Enhanced events_create signature.
This explicit approach clarifies which parameters are handled, improving readability.
51-51: Saving event category upon creation.
Makes sense to keep category logic at the business layer.
67-67: Post-creation module hook.
No issues spotted. The event is passed to external hooks for further processing.
80-80: Refined signature for events_update.
Similarly clarifies what parameters are used for updating.
81-84: Committed changes quickly after partial updates.
No problem, though consider batching if performance is a concern.
86-86: Category saving for an update.
Ensures category is maintained or changed properly.
90-99: Updating event assets & IOCs.
Catching and raisingBusinessProcessingErrorfor failures is clear.
100-100: Post-update module hook.
Consistent approach to external integration.
102-103: Track changes with an activity entry.
Fine approach for logging updated events.
106-112: New events_delete function centralizes removal logic.
Covers deletion steps, calls module hook, notifies collaborators, and tracks activity.source/app/blueprints/rest/v2/case_objects/events.py (9)
21-21: New imports look good and consistent.
No issues found with the introduced imports forValidationError,response_api_deleted,events_delete, andcall_deprecated_on_preload_modules_hook.Also applies to: 26-26, 33-33, 42-42
45-49: Encapsulating event logic inEventsclass is a solid design choice.
The introduction of this class centralizes event logic, improving maintainability and readability.
51-53: Error handling for mismatched case identifiers is correct.
RaisingBusinessProcessingErrorhere is consistent with the rest of the codebase, and the 400 error is handled properly in higher-level exception handling.
61-86: Overallcreatemethod implementation is robust.
- Properly checks case existence and access.
- Uses hooks for data pre-processing.
- Raises
ValidationErrorwith a clear message.No further concerns here.
87-103:getmethod gracefully handles missing or restricted events.
The checks for case existence, user access, and event mismatch are consistent and thorough.
104-135:updatemethod logic is well-structured.
- Good use of
tryblocks for business logic.- The updates follow the same pattern as creation, ensuring consistency.
136-153:deletemethod is properly secured and straightforward.
- Checks existence and access before calling
events_delete.- Returns a 204-equivalent response (
response_api_deleted()).
155-156: Instantiation and blueprint creation are appropriate.
No issues with the blueprint registration or theevents = Events()assignment.
161-162: Route definitions align with the new class-based logic.
The mapping of HTTP verbs to class methods is cleanly handled.Also applies to: 168-169, 172-175, 177-179, 180-181
tests/tests_rest_events.py (7)
362-362: Good addition of new test cases for deletion scenarios.
This improves overall coverage and ensures correct API behavior for event deletion.
363-372:test_delete_event_should_return_204
Correctly validates that a successful deletion returns a 204 status code.
373-383:test_get_event_should_return_404_after_it_has_been_deleted
Adequately confirms that a deleted event cannot be retrieved.
384-393:test_delete_event_should_return_404_when_case_does_not_exist
Ensures proper handling of invalid case identifiers.
394-398:test_delete_event_should_return_404_when_the_event_does_not_exist
Verifies that deleting a non-existent event responds with 404, as expected.
399-410:test_delete_event_should_return_403_when_user_has_no_permission_to_access_case
Checks that the API enforces access control, which is crucial for data security.
411-420:test_delete_event_should_return_400_when_case_identifier_does_not_match_event_case
Confirms correct handling when the case identifier is mismatched, aligning with_check_event_and_case_identifier_match().
Implement endpoint
DELETE /api/v2/cases/{case_identifier}/events/{identifier}to delete an evidence.POST /case/timeline/events/delete/{event_id}DELETE /api/v2/cases/{case_identifier}/evidences/{identifier}should not delete evidence, when user has no permission to access casemarshmallowand the schema (moved up in the API)Eventsso as to avoid to duplicate and recreate theEventSchemaseveral times in the code (and at runtime)GET /manage/server/check-updates/modalGET /manage/server/make-updateapp.utilcloser to their use in namespaceapp.schema.utilsNote: this pull request goes hand in hand with the documentation PR#56 in iris-doc-src.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests