From 72b72a81fa8085af3d56c3827ebef70e459fc85c Mon Sep 17 00:00:00 2001 From: Diptanil Saha Date: Wed, 15 Jul 2026 12:20:27 +0530 Subject: [PATCH] fix(project): improved access control for project users (#56675) * fix: permission check for `get_task_html` and `get_timesheet_html` * fix(project): enabled project access control for users without `Projects User` Role * fix(portal): validate user permissions for project portal * fix: patch to add docshare for the project users * fix(patch): selecting correct column on the query * fix(project): grant access to all the current users for new project * fix(portal): fixed condition to display timesheets on project * test(portal): add access control tests for project user * fix(project): using `frappe.has_permission` instead of `self.has_permission` to validate user permissions * fix(project): granting docshare access for every ProjectUser Roles for an User can be removed any time or an User Permission can be added which might restrict the access to the Project. * fix(patch): create docshare documents for non-cancelled projects and users who have no docshare documents * test(project): removed `test_control_access_does_not_touch_users_with_real_permission` --- erpnext/patches.txt | 1 + .../v16_0/access_control_for_project_users.py | 34 +++++++++ erpnext/projects/doctype/project/project.json | 18 ++++- erpnext/projects/doctype/project/project.py | 30 ++++++++ .../projects/doctype/project/test_project.py | 55 ++++++++++++++ erpnext/templates/pages/projects.py | 22 +++--- erpnext/templates/pages/test_projects.py | 72 +++++++++++++++++++ 7 files changed, 220 insertions(+), 12 deletions(-) create mode 100644 erpnext/patches/v16_0/access_control_for_project_users.py create mode 100644 erpnext/templates/pages/test_projects.py diff --git a/erpnext/patches.txt b/erpnext/patches.txt index 6a9632dc51f..e748cab0008 100644 --- a/erpnext/patches.txt +++ b/erpnext/patches.txt @@ -498,3 +498,4 @@ erpnext.patches.v16_0.create_shop_floor_roles erpnext.patches.v15_0.backfill_sla_link_filters_on_custom_field erpnext.patches.v15_0.backfill_sla_link_filters_on_docfield erpnext.patches.v16_0.crm_settings_handle_allowed_users_for_frappe_crm +erpnext.patches.v16_0.access_control_for_project_users diff --git a/erpnext/patches/v16_0/access_control_for_project_users.py b/erpnext/patches/v16_0/access_control_for_project_users.py new file mode 100644 index 00000000000..7202e6c71ea --- /dev/null +++ b/erpnext/patches/v16_0/access_control_for_project_users.py @@ -0,0 +1,34 @@ +import frappe + + +def execute(): + Project = frappe.qb.DocType("Project") + ProjectUser = frappe.qb.DocType("Project User") + + query = ( + frappe.qb.from_(Project) + .join(ProjectUser) + .on(Project.name == ProjectUser.parent) + .select(Project.name, ProjectUser.user) + .where(Project.status != "Cancelled") # Not considering cancelled Projects. + ) + + proj_users = query.run(as_dict=1) + + project_mapped_users = get_project_mapped_users(proj_users) + + for d in proj_users: + if d.user in project_mapped_users[d.name]: + continue + + frappe.share.add_docshare("Project", d.name, user=d.user) + + +def get_project_mapped_users(proj_users): + projects = set([d.name for d in proj_users]) + project_mapped_users = {} + + for d in projects: + project_mapped_users[d] = [d.user for d in frappe.share.get_users("Project", d)] + + return project_mapped_users diff --git a/erpnext/projects/doctype/project/project.json b/erpnext/projects/doctype/project/project.json index d40bb75595b..b55cec332bd 100644 --- a/erpnext/projects/doctype/project/project.json +++ b/erpnext/projects/doctype/project/project.json @@ -210,13 +210,15 @@ "fieldname": "users", "fieldtype": "Table", "label": "Users", - "options": "Project User" + "options": "Project User", + "permlevel": 1 }, { "fieldname": "copied_from", "fieldtype": "Data", "hidden": 1, "label": "Copied From", + "permlevel": 1, "read_only": 1 }, { @@ -482,13 +484,25 @@ "index_web_pages_for_search": 1, "links": [], "max_attachments": 4, - "modified": "2026-07-14 14:20:50.418911", + "modified": "2026-07-14 14:32:11.328347", "modified_by": "Administrator", "module": "Projects", "name": "Project", "naming_rule": "By \"Naming Series\" field", "owner": "Administrator", "permissions": [ + { + "delete": 1, + "email": 1, + "export": 1, + "permlevel": 1, + "print": 1, + "read": 1, + "report": 1, + "role": "Projects Manager", + "share": 1, + "write": 1 + }, { "create": 1, "delete": 1, diff --git a/erpnext/projects/doctype/project/project.py b/erpnext/projects/doctype/project/project.py index 63c75d61f4c..fc85099bf6c 100644 --- a/erpnext/projects/doctype/project/project.py +++ b/erpnext/projects/doctype/project/project.py @@ -90,6 +90,7 @@ class Project(Document): def validate(self): if not self.is_new(): self.copy_from_template() + self.control_access_for_project_users() self.send_welcome_email() self.update_costing() self.update_percent_complete() @@ -239,6 +240,7 @@ class Project(Document): def after_insert(self): self.copy_from_template("after_insert") self.link_with_sales_order() + self.control_access_for_project_users() def link_with_sales_order(self) -> None: """Back-link the source Sales Order to this project. @@ -434,6 +436,34 @@ class Project(Document): ) user.welcome_email_sent = 1 + def control_access_for_project_users(self): + def revoke_access_for_project_users(removed_users): + users = set([d.user for d in frappe.share.get_users(self.doctype, self.name)]) + for user in removed_users: + if user not in users: + continue + + frappe.share.remove(self.doctype, self.name, user) + + def grant_access_for_project_users(new_users): + for user in new_users: + frappe.share.add_docshare(self.doctype, self.name, user=user) + + current_users = set([d.user for d in self.users]) + old_doc = self.get_doc_before_save() + + if not old_doc: + grant_access_for_project_users(current_users) + return + + previous_users = set([d.user for d in old_doc.users]) + + new_users = current_users - previous_users + removed_users = previous_users - current_users + + revoke_access_for_project_users(removed_users) + grant_access_for_project_users(new_users) + def get_timeline_data(doctype: str, name: str) -> dict[int, int]: """Return timeline for attendance""" diff --git a/erpnext/projects/doctype/project/test_project.py b/erpnext/projects/doctype/project/test_project.py index 90e8d78f60e..d8d11f3ffa0 100644 --- a/erpnext/projects/doctype/project/test_project.py +++ b/erpnext/projects/doctype/project/test_project.py @@ -436,6 +436,61 @@ class TestProject(ERPNextTestSuite): self.assertEqual(project.total_consumed_material_cost, sum(row.amount for row in issue.items)) self.assertGreater(project.total_consumed_material_cost, 0) + def _create_portal_user(self, email): + """A user with no Project-related role, so read access can only come from + control_access_for_project_users() sharing the doc with them.""" + if not frappe.db.exists("User", email): + frappe.get_doc( + { + "doctype": "User", + "email": email, + "first_name": "Portal", + "send_welcome_email": 0, + } + ).insert(ignore_permissions=True) + return email + + def test_new_project_grants_access_to_its_users(self): + member = self._create_portal_user(f"new_proj_member_{frappe.generate_hash(length=6)}@example.com") + + project = frappe.get_doc( + doctype="Project", + project_name=f"_Test New Project Access {frappe.generate_hash(length=6)}", + status="Open", + company="_Test Company", + ) + project.append("users", {"user": member, "welcome_email_sent": 1}) + project.insert() # must not raise + + self.assertTrue(project.has_permission(user=member)) + shared_with = [d.user for d in frappe.share.get_users("Project", project.name)] + self.assertIn(member, shared_with) + + def test_adding_and_removing_project_user_updates_access(self): + stays = self._create_portal_user(f"stays_{frappe.generate_hash(length=6)}@example.com") + leaves = self._create_portal_user(f"leaves_{frappe.generate_hash(length=6)}@example.com") + + project = frappe.get_doc( + doctype="Project", + project_name=f"_Test Project User Membership {frappe.generate_hash(length=6)}", + status="Open", + company="_Test Company", + ) + project.append("users", {"user": stays, "welcome_email_sent": 1}) + project.insert() + self.assertTrue(project.has_permission(user=stays)) + + # adding a user on update (not insert) must also grant them access + project.append("users", {"user": leaves, "welcome_email_sent": 1}) + project.save() + self.assertTrue(project.has_permission(user=leaves)) + + # removing a user must revoke the share that was granted for membership + project.users = [d for d in project.users if d.user != leaves] + project.save() + self.assertFalse(project.has_permission(user=leaves)) + self.assertTrue(project.has_permission(user=stays)) + def get_project(name, template): project = frappe.get_doc( diff --git a/erpnext/templates/pages/projects.py b/erpnext/templates/pages/projects.py index 46ad25ed6ed..646e2085ace 100644 --- a/erpnext/templates/pages/projects.py +++ b/erpnext/templates/pages/projects.py @@ -6,21 +6,12 @@ import frappe def get_context(context): - project_user = frappe.db.get_value( - "Project User", - {"parent": frappe.form_dict.project, "user": frappe.session.user}, - ["user", "view_attachments", "hide_timesheets"], - as_dict=True, - ) - if frappe.session.user != "Administrator" and (not project_user or frappe.session.user == "Guest"): - raise frappe.PermissionError + project_user = validate_and_get_project_user(project=frappe.form_dict.project) context.no_cache = 1 context.show_sidebar = True project = frappe.get_doc("Project", frappe.form_dict.project) - project.has_permission("read") - project.tasks = get_tasks( project.name, start=0, item_status="open", search=frappe.form_dict.get("search") ) @@ -66,6 +57,7 @@ def get_tasks(project, start=0, search=None, item_status=None): @frappe.whitelist() def get_task_html(project: str, start: int = 0, item_status: str | None = None): + validate_and_get_project_user(project=project) return frappe.render_template( "erpnext/templates/includes/projects/project_tasks.html", { @@ -106,6 +98,7 @@ def get_timesheets(project, start=0, search=None): @frappe.whitelist() def get_timesheet_html(project: str, start: int = 0): + validate_and_get_project_user(project=project) return frappe.render_template( "erpnext/templates/includes/projects/project_timesheets.html", {"doc": {"timesheets": get_timesheets(project, start)}}, @@ -119,3 +112,12 @@ def get_attachments(project): filters={"attached_to_name": project, "attached_to_doctype": "Project", "is_private": 0}, fields=["file_name", "file_url", "file_size"], ) + + +def validate_and_get_project_user(project: str): + project_doc = frappe.get_doc("Project", project) + project_doc.check_permission() + + project_user = next((d for d in project_doc.users if d.user == frappe.session.user), None) + + return project_user diff --git a/erpnext/templates/pages/test_projects.py b/erpnext/templates/pages/test_projects.py new file mode 100644 index 00000000000..8d66ce95bc5 --- /dev/null +++ b/erpnext/templates/pages/test_projects.py @@ -0,0 +1,72 @@ +# Copyright (c) 2026, Frappe Technologies Pvt. Ltd. and Contributors +# License: GNU General Public License v3. See license.txt + +import frappe + +from erpnext.projects.doctype.project.test_project import make_project +from erpnext.templates.pages.projects import validate_and_get_project_user +from erpnext.tests.utils import ERPNextTestSuite + + +class TestProjectsPage(ERPNextTestSuite): + """validate_and_get_project_user() gates the /projects portal page. It must raise + frappe.PermissionError for a user who can't read the Project, and otherwise return + that user's Project User row (or None if they're permitted but not listed as one -- + e.g. an internal Projects Manager browsing the portal).""" + + def _create_user(self, email): + if not frappe.db.exists("User", email): + frappe.get_doc( + { + "doctype": "User", + "email": email, + "first_name": "Portal", + "send_welcome_email": 0, + } + ).insert(ignore_permissions=True) + return email + + def test_raises_permission_error_for_user_without_access(self): + project = make_project({"project_name": f"_Test Portal Access {frappe.generate_hash(length=6)}"}) + outsider = self._create_user(f"outsider_{frappe.generate_hash(length=6)}@example.com") + + with self.set_user(outsider): + self.assertRaises(frappe.PermissionError, validate_and_get_project_user, project.name) + + def test_allows_user_listed_as_project_user_and_returns_their_row(self): + # Being a Project User shares the Project with that user (see + # Project.control_access_for_project_users), which is what lets them past + # check_permission() here. + member = self._create_user(f"member_{frappe.generate_hash(length=6)}@example.com") + + project = frappe.get_doc( + doctype="Project", + project_name=f"_Test Portal Access {frappe.generate_hash(length=6)}", + status="Open", + company="_Test Company", + ) + project.append( + "users", {"user": member, "view_attachments": 1, "hide_timesheets": 1, "welcome_email_sent": 1} + ) + project.insert() + + with self.set_user(member): + project_user = validate_and_get_project_user(project.name) + + self.assertIsNotNone(project_user) + self.assertEqual(project_user.user, member) + self.assertEqual(project_user.view_attachments, 1) + self.assertEqual(project_user.hide_timesheets, 1) + + def test_allows_internally_permitted_user_not_listed_as_project_user(self): + # The permission gate must be the real permission system (check_permission()), + # not "is this user in the Project's users child table" -- a Projects Manager + # can open any project's portal page without ever being added as its user. + project = make_project({"project_name": f"_Test Portal Access {frappe.generate_hash(length=6)}"}) + manager = self._create_user(f"manager_{frappe.generate_hash(length=6)}@example.com") + frappe.get_doc("User", manager).add_roles("Projects Manager") + + with self.set_user(manager): + project_user = validate_and_get_project_user(project.name) + + self.assertIsNone(project_user)