feat: add filter validation and permission checks in roster API#4569
Conversation
WalkthroughThis PR hardens the roster API by introducing centralized input validation for filter parameters and adding explicit permission enforcement across mutation endpoints. It defines allowlists for 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hrms/api/roster.py (1)
221-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing write permission checks for adjacent shift modifications.
The permission checks at lines 221-222 verify Employee read and Shift Assignment create, but lines 236-247 modify/delete
prev_shiftandnext_shiftdocuments usingfrappe.db.set_value(which bypasses permission checks entirely).If a user can create shifts for an employee but has restricted write access to existing shift assignments (e.g., role-based restrictions on certain shift types), they could inadvertently modify adjacent shifts without proper authorization.
🔒 Proposed fix to add write permission checks
frappe.has_permission("Employee", "read", employee, throw=True) frappe.has_permission("Shift Assignment", "create", throw=True) filters = { "doctype": "Shift Assignment", "employee": employee, "company": company, "shift_type": shift_type, "status": status, "shift_location": shift_location, } prev_shift = frappe.db.exists(dict({"end_date": add_days(start_date, -1)}, **filters)) next_shift = ( frappe.db.exists(dict({"start_date": add_days(end_date, 1)}, **filters)) if end_date else None ) if prev_shift: + frappe.get_doc("Shift Assignment", prev_shift).check_permission("write") if next_shift: + next_shift_doc = frappe.get_doc("Shift Assignment", next_shift) + next_shift_doc.check_permission("delete") end_date = frappe.db.get_value("Shift Assignment", next_shift, "end_date") frappe.db.set_value("Shift Assignment", next_shift, "docstatus", 2) - frappe.delete_doc("Shift Assignment", next_shift) + frappe.delete_doc("Shift Assignment", next_shift, ignore_permissions=True) frappe.db.set_value("Shift Assignment", prev_shift, "end_date", end_date or None) elif next_shift: + frappe.get_doc("Shift Assignment", next_shift).check_permission("write") frappe.db.set_value("Shift Assignment", next_shift, "start_date", start_date)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hrms/api/roster.py` around lines 221 - 247, The code currently modifies/deletes adjacent Shift Assignment records (prev_shift, next_shift) using frappe.db.set_value and frappe.delete_doc without verifying write permission; before any call that mutates or deletes these records (the branches that call frappe.db.set_value on prev_shift/next_shift and frappe.delete_doc on next_shift), add explicit permission checks such as frappe.has_permission("Shift Assignment", "write", <docname>, throw=True) or load the document via frappe.get_doc("Shift Assignment", <docname>) and call doc.check_permission("write") to enforce authorization; ensure you check write permission for both prev_shift and next_shift prior to set_value/delete operations and only proceed to set_value/delete if the permission check passes, while preserving the existing create_shift_assignment call for the new case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@hrms/api/roster.py`:
- Around line 221-247: The code currently modifies/deletes adjacent Shift
Assignment records (prev_shift, next_shift) using frappe.db.set_value and
frappe.delete_doc without verifying write permission; before any call that
mutates or deletes these records (the branches that call frappe.db.set_value on
prev_shift/next_shift and frappe.delete_doc on next_shift), add explicit
permission checks such as frappe.has_permission("Shift Assignment", "write",
<docname>, throw=True) or load the document via frappe.get_doc("Shift
Assignment", <docname>) and call doc.check_permission("write") to enforce
authorization; ensure you check write permission for both prev_shift and
next_shift prior to set_value/delete operations and only proceed to
set_value/delete if the permission check passes, while preserving the existing
create_shift_assignment call for the new case.
1 participant
Changes:
no-docsSummary by CodeRabbit