Skip to content

fix: enforce student and doctype permissions in education API #449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tuwafula wants to merge 1 commit into from
Closed

fix: enforce student and doctype permissions in education API #449

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 50 additions & 8 deletions education/education/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,11 @@ def check_attendance_records_exist(course_schedule=None, student_group=None, dat

@frappe.whitelist()
def mark_attendance(
students_present, students_absent, course_schedule=None, student_group=None, date=None
students_present,
students_absent,
course_schedule=None,
student_group=None,
date=None,
):
"""Creates Multiple Attendance Records.

Expand Down Expand Up @@ -117,12 +121,22 @@ def mark_attendance(

for d in present:
make_attendance_records(
d["student"], d["student_name"], "Present", course_schedule, student_group, date
d["student"],
d["student_name"],
"Present",
course_schedule,
student_group,
date,
)

for d in absent:
make_attendance_records(
d["student"], d["student_name"], "Absent", course_schedule, student_group, date
d["student"],
d["student_name"],
"Absent",
course_schedule,
student_group,
date,
)

frappe.db.commit()
Expand Down Expand Up @@ -160,7 +174,6 @@ def make_attendance_records(
student_attendance.submit()


@frappe.whitelist()
def get_student_guardians(student):
"""Returns List of Guardians of a Student.

Expand All @@ -178,6 +191,9 @@ def get_student_group_students(student_group, include_inactive=0):

:param student_group: Student Group.
"""
if not frappe.has_permission("Student Group", "read", student_group):
raise frappe.PermissionError("You are not authorized to access this student group")

if include_inactive:
students = frappe.get_all(
"Student Group Student",
Expand Down Expand Up @@ -298,6 +314,9 @@ def get_assessment_criteria(course):

@frappe.whitelist()
def get_assessment_students(assessment_plan, student_group):
if not frappe.has_permission("Assessment Result Tool", "read"):
raise frappe.PermissionError("Not Authorized")
Comment on lines +317 to +318

@greptile-apps greptile-apps Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Assessment Result Tool may not have a standard doctype permission entry

frappe.has_permission("Assessment Result Tool", "read") relies on this DocType having a permissions table configured in the system. Assessment Result Tool is a page/tool form in Frappe Education — if it has no DocType record with role permissions defined, frappe.has_permission will either raise or return False for everyone, effectively blocking all access to get_assessment_students. Consider checking against a data doctype that is always configured (e.g., "Assessment Result") or explicitly guarding with frappe.db.exists("DocType", "Assessment Result Tool") before relying on this check.

Context Used: This is a Frappe Framework application (Python bac... (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: education/education/api.py
Line: 317-318

Comment:
**`Assessment Result Tool` may not have a standard doctype permission entry**

`frappe.has_permission("Assessment Result Tool", "read")` relies on this DocType having a permissions table configured in the system. `Assessment Result Tool` is a page/tool form in Frappe Education — if it has no `DocType` record with role permissions defined, `frappe.has_permission` will either raise or return `False` for everyone, effectively blocking all access to `get_assessment_students`. Consider checking against a data doctype that is always configured (e.g., `"Assessment Result"`) or explicitly guarding with `frappe.db.exists("DocType", "Assessment Result Tool")` before relying on this check.

**Context Used:** This is a Frappe Framework application (Python bac... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.


student_list = get_student_group_students(student_group)
for i, student in enumerate(student_list):
result = get_result(student.student, assessment_plan)
Expand All @@ -306,7 +325,10 @@ def get_assessment_students(assessment_plan, student_group):
for d in result.details:
student_result.update({d.assessment_criteria: [cstr(d.score), d.grade]})
student_result.update(
{"total_score": [cstr(result.total_score), result.grade], "comment": result.comment}
{
"total_score": [cstr(result.total_score), result.grade],
"comment": result.comment,
}
)
student.update(
{
Expand Down Expand Up @@ -334,7 +356,6 @@ def get_assessment_details(assessment_plan):
)


@frappe.whitelist()
def get_result(student, assessment_plan):
"""Returns Submitted Result of given student for specified Assessment Plan

Expand Down Expand Up @@ -541,7 +562,8 @@ def get_student_info():

@frappe.whitelist()
def get_student_programs(student):
# student = 'EDU-STU-2023-00043'
check_permission(student, "programs")

programs = frappe.db.get_list(
"Program Enrollment",
fields=["program", "name"],
Expand Down Expand Up @@ -664,6 +686,8 @@ def apply_leave_based_on_student_group(leave_data, program_name):

@frappe.whitelist()
def get_student_invoices(student):
check_permission(student, "invoices")

student_sales_invoices = []

sales_invoice_list = frappe.db.get_list(
Expand Down Expand Up @@ -760,8 +784,26 @@ def get_school_abbr_logo():

@frappe.whitelist()
def get_student_attendance(student, student_group):
check_permission(student, "attendance")

return frappe.db.get_list(
"Student Attendance",
filters={"student": student, "student_group": student_group, "docstatus": 1},
filters={
"student": student,
"student_group": student_group,
"docstatus": 1,
},
fields=["date", "status", "name"],
)


def check_permission(student, resource_type):
user = frappe.session.user
if user == "Administrator":
return

student_user = frappe.db.get_value("Student", student, "user")
if student_user != user:
raise frappe.PermissionError(
f"You are not authorized to access this student's {resource_type}"
Comment on lines +807 to +808

@greptile-apps greptile-apps Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 User-visible error strings are not wrapped with _()

The f-string f"You are not authorized to access this student's {resource_type}" cannot be wrapped with _() as-is and will not be translated. The same applies to the bare string literals in get_student_group_students (line 195) and get_assessment_students (line 318). Use frappe.throw(_("..."), frappe.PermissionError) with a static translatable string, or at minimum use _("Not Authorized") for the simple cases.

Context Used: This is a Frappe Framework application (Python bac... (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: education/education/api.py
Line: 807-808

Comment:
**User-visible error strings are not wrapped with `_()`**

The f-string `f"You are not authorized to access this student's {resource_type}"` cannot be wrapped with `_()` as-is and will not be translated. The same applies to the bare string literals in `get_student_group_students` (line 195) and `get_assessment_students` (line 318). Use `frappe.throw(_("..."), frappe.PermissionError)` with a static translatable string, or at minimum use `_("Not Authorized")` for the simple cases.

**Context Used:** This is a Frappe Framework application (Python bac... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

)
Comment on lines +800 to +809

@greptile-apps greptile-apps Bot Jun 24, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Ownership-only guard blocks all non-Administrator staff and fails silently when Student.user is unset

check_permission only exempts the literal "Administrator" username; any other privileged user (System Manager, Instructor, Education Manager) calling get_student_programs, get_student_invoices, or get_student_attendance on behalf of a student will get a PermissionError. More critically, frappe.db.get_value("Student", student, "user") returns None if the Student record has no linked portal user — None != user is always True, so every non-Administrator call for such students is denied, silently locking out legitimate access. A role-based fallback (e.g., frappe.has_permission("Student", "read", student)) should be checked before rejecting the request.

Context Used: This is a Frappe Framework application (Python bac... (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: education/education/api.py
Line: 800-809

Comment:
**Ownership-only guard blocks all non-Administrator staff and fails silently when `Student.user` is unset**

`check_permission` only exempts the literal `"Administrator"` username; any other privileged user (System Manager, Instructor, Education Manager) calling `get_student_programs`, `get_student_invoices`, or `get_student_attendance` on behalf of a student will get a `PermissionError`. More critically, `frappe.db.get_value("Student", student, "user")` returns `None` if the Student record has no linked portal user — `None != user` is always `True`, so every non-Administrator call for such students is denied, silently locking out legitimate access. A role-based fallback (e.g., `frappe.has_permission("Student", "read", student)`) should be checked before rejecting the request.

**Context Used:** This is a Frappe Framework application (Python bac... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.

Loading