fix: restrict API for pwa to fetch current logged-in employee only#4207
fix: restrict API for pwa to fetch current logged-in employee only#4207iamkhanraheel merged 4 commits intofrappe:developfrom
Conversation
WalkthroughFrontend code removed explicit employee context and employee request parameters from AttendanceCalendar.vue, Dashboard.vue, and data modules (advances.js, claims.js, leaves.js), so API calls no longer include an employee filter. Backend added a new get_current_employee() helper and updated API functions (get_attendance_calendar_events, get_shifts, get_leave_balance_map, get_expense_claim_summary, get_employee_advance_balance) to resolve the employee internally, removing employee parameters from their signatures and updating call sites. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hrms/api/__init__.py`:
- Around line 81-85: get_current_employee() calls
get_current_employee_info().get("name") directly which raises AttributeError
when get_current_employee_info() returns None; change it to first assign the
result to a variable (e.g., info = get_current_employee_info()), check if info
is falsy and call frappe.throw(_("Employee not found"), frappe.PermissionError)
before accessing info.get("name"), then proceed to extract and validate the
"name" and throw the same PermissionError if missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19736e76-3c56-46fd-8044-4643a0793b67
📒 Files selected for processing (6)
frontend/src/components/AttendanceCalendar.vuefrontend/src/data/advances.jsfrontend/src/data/claims.jsfrontend/src/data/leaves.jsfrontend/src/views/attendance/Dashboard.vuehrms/api/__init__.py
💤 Files with no reviewable changes (5)
- frontend/src/data/leaves.js
- frontend/src/data/advances.js
- frontend/src/data/claims.js
- frontend/src/views/attendance/Dashboard.vue
- frontend/src/components/AttendanceCalendar.vue
|
rebase and merge |
|
@Mergifyio rebase |
…_employee_advance_balance, get_attendance_calendar_events
iamkhanraheel
force-pushed
the
fix/pwa_perm_check
branch
from
15c3169 to
915abae
Compare
✅ Branch has been successfully rebased |
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/__init__.py (1)
63-78:⚠️ Potential issue | 🟠 Major
get_all_employees()is now half-way between a directory API and a self API.Line 64 switches this to permission-scoped
frappe.get_list()with no explicit filter for the session user. This means users with broadEmployeeread access can still get multiple rows, whilefrontend/src/data/employees.js:1-35consumes this endpoint as a directory and already hasemployeeResourcefor self data. The PR objective is to restrict PWA APIs to the current logged-in employee. Please make the contract explicit: either addfilters={"user": frappe.session.user}here, or keep this as the unrestricted directory API and useget_current_employee_info()for self-only flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/api/__init__.py` around lines 63 - 78, The get_all_employees() function currently uses permission-scoped frappe.get_list() but isn't explicit about restricting results to the current session user; either make it a directory endpoint or restrict it to the logged-in employee—add filters={"user": frappe.session.user} to the frappe.get_list call in get_all_employees() to return only the current user's employee row, or alternatively leave get_all_employees() as an unrestricted directory API and update callers to use get_current_employee_info() (or similar self-only function) for self-only flows; pick one approach and make the contract explicit in get_all_employees().
♻️ Duplicate comments (1)
hrms/api/__init__.py (1)
81-85:⚠️ Potential issue | 🔴 CriticalGuard the missing-employee path before calling
.get().Line 82 dereferences
.get("name")on the result ofget_current_employee_info(), butfrappe.db.get_value(..., as_dict=True)returnsNonewhen no activeEmployeematches. That turns the expectedPermissionErrorinto a 500 for every endpoint that now depends on this helper.🛠️ Proposed fix
def get_current_employee() -> str: - employee = get_current_employee_info().get("name") + employee_info = get_current_employee_info() + employee = employee_info.get("name") if employee_info else None if not employee: frappe.throw(_("Employee not found"), frappe.PermissionError) return employeeIn current Frappe, what does frappe.db.get_value(..., as_dict=True) return when no document matches the filters?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hrms/api/__init__.py` around lines 81 - 85, The helper get_current_employee dereferences get_current_employee_info().get("name") without checking for None; since frappe.db.get_value(..., as_dict=True) returns None when no Employee matches, modify get_current_employee to first assign info = get_current_employee_info(), check if info is truthy (or is None) and call frappe.throw(_("Employee not found"), frappe.PermissionError) before accessing info.get("name"), then return the name; reference get_current_employee and get_current_employee_info to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@hrms/api/__init__.py`:
- Around line 63-78: The get_all_employees() function currently uses
permission-scoped frappe.get_list() but isn't explicit about restricting results
to the current session user; either make it a directory endpoint or restrict it
to the logged-in employee—add filters={"user": frappe.session.user} to the
frappe.get_list call in get_all_employees() to return only the current user's
employee row, or alternatively leave get_all_employees() as an unrestricted
directory API and update callers to use get_current_employee_info() (or similar
self-only function) for self-only flows; pick one approach and make the contract
explicit in get_all_employees().
---
Duplicate comments:
In `@hrms/api/__init__.py`:
- Around line 81-85: The helper get_current_employee dereferences
get_current_employee_info().get("name") without checking for None; since
frappe.db.get_value(..., as_dict=True) returns None when no Employee matches,
modify get_current_employee to first assign info = get_current_employee_info(),
check if info is truthy (or is None) and call frappe.throw(_("Employee not
found"), frappe.PermissionError) before accessing info.get("name"), then return
the name; reference get_current_employee and get_current_employee_info to locate
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e160e14-1c7c-4ad6-b031-11b769aeed29
📒 Files selected for processing (6)
frontend/src/components/AttendanceCalendar.vuefrontend/src/data/advances.jsfrontend/src/data/claims.jsfrontend/src/data/leaves.jsfrontend/src/views/attendance/Dashboard.vuehrms/api/__init__.py
💤 Files with no reviewable changes (5)
- frontend/src/data/advances.js
- frontend/src/data/claims.js
- frontend/src/components/AttendanceCalendar.vue
- frontend/src/views/attendance/Dashboard.vue
- frontend/src/data/leaves.js
…4207 fix: restrict API for pwa to fetch current logged-in employee only (backport #4207)
…4207 fix: restrict API for pwa to fetch current logged-in employee only (backport #4207)
2 participants
Summary by CodeRabbit