fix(setup): check user access when unlinking an employee and sync status only on change

erpnext/setup/doctype/employee/employee.py

@@ -53,6 +53,7 @@ class Employee(NestedSet):
 		else:
 			existing_user_id = frappe.db.get_value("Employee", self.name, "user_id")
 			if existing_user_id:
+				self.validate_user_access(existing_user_id)
 				user = frappe.get_doc("User", existing_user_id)
 				validate_employee_role(user, ignore_emp_check=True)
 				user.save(ignore_permissions=True)
@@ -75,16 +76,17 @@ class Employee(NestedSet):
 		if any(self.has_value_changed(field) for field in ("user_id", "status", "create_user_permission")):
 			self.validate_user_access()
 
-	def validate_user_access(self):
+	def validate_user_access(self, user: str | None = None):
 		"""Only a System Manager may link or update a User holding roles the editor lacks."""
 		editor_roles = frappe.get_roles()
 		if "System Manager" in editor_roles:
 			return
 
+		user = user or self.user_id
 		ignored_roles = {GUEST_ROLE, ALL_USER_ROLE, SYSTEM_USER_ROLE, "Employee"}
-		if set(frappe.get_roles(self.user_id)) - set(editor_roles) - ignored_roles:
+		if set(frappe.get_roles(user)) - set(editor_roles) - ignored_roles:
 			frappe.throw(
-				_("You are not permitted to link or update User {0}").format(frappe.bold(self.user_id)),
+				_("You are not permitted to link or update User {0}").format(frappe.bold(user)),
 				frappe.PermissionError,
 			)
 
@@ -207,6 +209,9 @@ class Employee(NestedSet):
 		if not self.user_id:
 			return
 
+		if not self.has_value_changed("status") and not self.has_value_changed("user_id"):
+			return
+
 		user = frappe.get_doc("User", self.user_id)
 		enabled = user.enabled
 		if self.status != "Active" and enabled or self.status == "Active" and enabled == 0:

erpnext/setup/doctype/employee/test_employee.py

@@ -36,6 +36,29 @@ class TestEmployee(unittest.TestCase):
 		employee_doc.save()
 		self.assertTrue("Employee" not in frappe.get_roles(user))
 
+	def test_hr_user_cannot_unlink_or_reenable_users_with_extra_roles(self):
+		from erpnext.tests.permission_test_utils import as_user, make_fenced_user
+
+		hr_user = make_fenced_user("employee-hr@example.com", ["HR User"])
+		manager = make_fenced_user("employee-linked-manager@example.com", ["System Manager", "Employee"])
+		frappe.db.set_value("User", manager, "enabled", 1)
+		frappe.db.delete("Employee", {"user_id": manager})
+		employee = make_employee(manager, company="_Test Company")
+
+		with as_user(hr_user):
+			linked = frappe.get_doc("Employee", employee)
+			linked.user_id = ""
+			self.assertRaises(frappe.PermissionError, linked.save)
+		self.assertEqual(frappe.db.get_value("Employee", employee, "user_id"), manager)
+		self.assertIn("Employee", frappe.get_roles(manager))
+
+		frappe.db.set_value("User", manager, "enabled", 0)
+		with as_user(hr_user):
+			linked = frappe.get_doc("Employee", employee)
+			linked.cell_number = "9999999999"
+			linked.save()
+		self.assertEqual(frappe.db.get_value("User", manager, "enabled"), 0)
+
 	def test_hr_user_links_only_users_without_extra_roles(self):
 		from erpnext.tests.permission_test_utils import as_user, make_fenced_user