Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
156 changes: 154 additions & 2 deletions forms_pro/api/team.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
import frappe
from frappe.core.api.user_invitation import invite_by_email
from frappe.core.doctype.docshare.docshare import DocShare
from frappe.core.doctype.user_invitation.user_invitation import UserInvitation
from frappe.share import get_share_name

from forms_pro.forms_pro.doctype.fp_team.fp_team import FPTeam, GetTeamMembersResponse
from forms_pro.utils.teams import GetTeamFormsResponseSchema, set_current_team
from forms_pro.utils.teams import get_team_forms as get_team_forms_utils
from forms_pro.utils.teams import (
GetTeamFormsResponseSchema,
set_current_team,
)
from forms_pro.utils.teams import (
get_team_forms as get_team_forms_utils,
)


@frappe.whitelist()
Expand Down Expand Up @@ -36,6 +45,9 @@ def get_team_members(team_id: str) -> list[GetTeamMembersResponse]:
throw=True,
)

# Clear cache so we read fresh DocShare data (e.g. after toggle_can_edit_team)
frappe.clear_document_cache("FP Team", team_id)

team: FPTeam = frappe.get_doc("FP Team", team_id)
members = team.team_members

Expand Down Expand Up @@ -77,3 +89,143 @@ def switch_team(team_id: str) -> None:
raise frappe.PermissionError("You do not have permission to switch to this team")

set_current_team(team_id, frappe.session.user)


@frappe.whitelist(methods=["POST"])
def invite_team_members(team_id: str, emails: list[str]) -> None:
"""
Invite team members to a team
"""

if not frappe.has_permission(
doctype="FP Team",
ptype="write",
doc=team_id,
user=frappe.session.user,
):
raise frappe.PermissionError(
"You do not have write permission on this team; write access is required to invite members"
)

emails_str = ", ".join(emails)

invite_by_email(
emails=emails_str,
roles=["Forms Pro User"],
redirect_to_path=f"/api/v2/method/forms_pro.api.team.add_member_to_team_via_invitation?team_id={team_id}",
app_name="forms_pro",
)


@frappe.whitelist()
def add_member_to_team_via_invitation(team_id: str, invite_id: str | None = None) -> None:
"""
Add a member to a team when an invitation is accepted.
Accepts invite_id from query param (URL may send it as 'id').
"""
invite_id = invite_id or frappe.form_dict.get("id")
if not invite_id:
raise frappe.PermissionError("Invitation id is required")

invite: UserInvitation = frappe.get_doc("User Invitation", invite_id)

if invite.status != "Accepted":
raise frappe.PermissionError("Invitation not accepted")

if not frappe.has_permission(
doctype="FP Team",
ptype="read",
doc=team_id,
user=invite.invited_by,
):
raise frappe.PermissionError("You do not have permission to add a member to this team")

if not frappe.db.exists("User", invite.email):
raise frappe.PermissionError("User not found")

team: FPTeam = frappe.get_doc("FP Team", team_id)

if team.is_team_member(invite.email):
raise frappe.DuplicateEntryError("User is already a member of the team")

team.add_to_team(invite.email)
team.save(ignore_permissions=True)
Comment on lines +126 to +152
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Bind invitation completion to the invited team.

This endpoint trusts the caller-supplied team_id and never verifies that it matches the team encoded in the accepted invitation. If the inviter can read multiple teams, the same accepted invite_id can be replayed against a different team_id and add the user to the wrong team. Derive the team from invitation metadata, or reject mismatches before team.add_to_team(...).

🧰 Tools
🪛 Ruff (0.15.4)

[warning] 126-126: Avoid specifying long messages outside the exception class

(TRY003)


[warning] 131-131: Avoid specifying long messages outside the exception class

(TRY003)


[warning] 139-139: Avoid specifying long messages outside the exception class

(TRY003)


[warning] 142-142: Avoid specifying long messages outside the exception class

(TRY003)


[warning] 147-147: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@forms_pro/api/team.py` around lines 124 - 150, The endpoint currently trusts
the caller-supplied team_id and never verifies it against the invitation; fetch
the team id from the accepted invitation (use the invitation field that encodes
the target team, e.g., invite.team or invite.reference_docname) and either
reject if the caller-supplied team_id mismatches or ignore the caller value and
use invite's team id for all subsequent operations (permission check using
invite.invited_by, team lookup via FPTeam using the invite-derived id, and final
team.add_to_team(invite.email)); if the invitation has no team field, add and
validate one on the User Invitation model and enforce the match before calling
team.add_to_team.

set_current_team(team_id, invite.email)

frappe.local.response["type"] = "redirect"
frappe.local.response["location"] = "/forms"
Comment thread
coderabbitai[bot] marked this conversation as resolved.


@frappe.whitelist(methods=["POST"])
def toggle_can_edit_team(team_id: str, member_email: str) -> None:
"""
Toggle the can_edit_team permission for a team member
"""

if not frappe.has_permission(
doctype="FP Team",
ptype="write",
doc=team_id,
user=frappe.session.user,
):
raise frappe.PermissionError(
"You do not have permission to toggle the can_edit_team permission for this team member"
)

team: FPTeam = frappe.get_doc("FP Team", team_id)
if team.owner == member_email:
raise frappe.PermissionError(
"The team owner always retains full permissions and cannot have edit access toggled"
)

share_name = get_share_name(doctype="FP Team", name=team_id, user=member_email, everyone=0)
if not share_name:
raise frappe.PermissionError(
"You do not have permission to toggle the can_edit_team permission for this team member"
)

share: DocShare = frappe.get_doc("DocShare", share_name)
share.write = not share.write
share.share = not share.share
share.save(ignore_permissions=True)
Comment thread
coderabbitai[bot] marked this conversation as resolved.


@frappe.whitelist(methods=["POST"])
def save(team_id: str, fields: dict) -> None:
"""
Update team fields. Only fields present in the dict are updated.
"""
frappe.has_permission(
doctype="FP Team",
ptype="write",
doc=team_id,
user=frappe.session.user,
throw=True,
)

ALLOWED_SAVE_FIELDS = ["team_name", "logo"]

team: FPTeam = frappe.get_doc("FP Team", team_id)
for key, value in fields.items():
if key not in ALLOWED_SAVE_FIELDS:
frappe.throw(f"Field '{key}' is not allowed")
setattr(team, key, value)
team.save()


@frappe.whitelist(methods=["POST"])
def remove_member_from_team(team_id: str, member_email: str) -> None:
"""
Remove a member from a team
"""

if not frappe.has_permission(
doctype="FP Team",
ptype="write",
doc=team_id,
user=frappe.session.user,
):
raise frappe.PermissionError("You do not have permission to remove a member from this team")

team: FPTeam = frappe.get_doc("FP Team", team_id)
team.remove_from_team(member_email)
Comment on lines +216 to +231
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

No guard prevents removing the team owner.

The remove_from_team method (per fp_team.py:105-120) has no owner check. A member with write permission could remove the team owner, leaving the team in an inconsistent state where is_owner checks would fail for all remaining members.

Add a guard to prevent owner removal:

🛡️ Proposed fix
 `@frappe.whitelist`(methods=["POST"])
 def remove_member_from_team(team_id: str, member_email: str) -> None:
     """
     Remove a member from a team
     """

     if not frappe.has_permission(
         doctype="FP Team",
         ptype="write",
         doc=team_id,
         user=frappe.session.user,
     ):
         raise frappe.PermissionError("You do not have permission to remove a member from this team")

     team: FPTeam = frappe.get_doc("FP Team", team_id)
+    if team.owner == member_email:
+        raise frappe.PermissionError("Cannot remove the team owner")
     team.remove_from_team(member_email)
🧰 Tools
🪛 Ruff (0.15.4)

[warning] 216-216: Avoid specifying long messages outside the exception class

(TRY003)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@forms_pro/api/team.py` around lines 204 - 219, Prevent accidental removal of
a team owner by adding an ownership guard: in the API function
remove_member_from_team and/or the FPTeam.remove_from_team method, detect if the
target member_email corresponds to a member with is_owner (or equivalent owner
flag) on the FPTeam and if so raise a permission/validation error instead of
removing them; update remove_member_from_team to check team.get("members") or
call a new FPTeam.is_owner(member_email) helper before invoking
team.remove_from_team, and add the same owner-check inside
FPTeam.remove_from_team to enforce safety at the model level.

33 changes: 31 additions & 2 deletions forms_pro/forms_pro/doctype/fp_team/fp_team.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@

import frappe
from frappe.model.document import Document
from frappe.share import add_docshare
from frappe.share import add_docshare, get_share_name
from frappe.share import remove as remove_docshare
from pydantic import BaseModel, EmailStr

from forms_pro.api.user import get_user
Expand All @@ -14,6 +15,8 @@ class GetTeamMembersResponse(BaseModel):
full_name: str
user_image: str | None
email: EmailStr
can_edit_team: bool
is_owner: bool


class FPTeam(Document):
Expand Down Expand Up @@ -48,6 +51,9 @@ def team_members(self) -> list[GetTeamMembersResponse]:
for member in self.users:
_user = get_user(member.user)
_user["email"] = member.user
share_name = get_share_name(doctype="FP Team", name=self.name, user=member.user, everyone=0)
_user["can_edit_team"] = frappe.db.get_value("DocShare", share_name, "write")
_user["is_owner"] = self.owner == member.user
members.append(GetTeamMembersResponse.model_validate(_user).model_dump())

return members
Expand Down Expand Up @@ -76,7 +82,7 @@ def add_to_team(self, user: str) -> None:
Args:
user: The user email address
"""
if user == "Administrator":
if user == "Administrator" or user == "Guest":
return

if self.is_team_member(user):
Expand All @@ -95,3 +101,26 @@ def add_to_team(self, user: str) -> None:
share=1,
flags={"ignore_share_permission": True},
)

def remove_from_team(self, user: str) -> None:
"""
Remove a user from the team

Args:
user: The user email address
"""

if user == self.owner:
frappe.throw(
frappe._("Cannot remove the owner from the team"),
frappe.ValidationError,
)

self.users = [member for member in self.users if member.user != user]
self.save()
remove_docshare(
doctype="FP Team",
name=self.name,
user=user,
flags={"ignore_permissions": True},
)
Comment on lines +105 to +126
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Missing guard to prevent owner removal.

The remove_from_team method doesn't prevent removing the team owner. This could orphan the team with no valid owner, breaking ownership-based permission checks. The API endpoint remove_member_from_team (at forms_pro/api/team.py:204-219) also lacks this validation.

🛡️ Proposed fix
     def remove_from_team(self, user: str) -> None:
         """
         Remove a user from the team

         Args:
             user: The user email address
         """
+        if user == self.owner:
+            frappe.throw(
+                frappe._("Cannot remove the team owner"),
+                frappe.ValidationError,
+            )

         self.users = [member for member in self.users if member.user != user]
         self.save()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def remove_from_team(self, user: str) -> None:
"""
Remove a user from the team
Args:
user: The user email address
"""
self.users = [member for member in self.users if member.user != user]
self.save()
remove_docshare(
doctype="FP Team",
name=self.name,
user=user,
flags={"ignore_permissions": True},
)
def remove_from_team(self, user: str) -> None:
"""
Remove a user from the team
Args:
user: The user email address
"""
if user == self.owner:
frappe.throw(
frappe._("Cannot remove the team owner"),
frappe.ValidationError,
)
self.users = [member for member in self.users if member.user != user]
self.save()
remove_docshare(
doctype="FP Team",
name=self.name,
user=user,
flags={"ignore_permissions": True},
)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@forms_pro/forms_pro/doctype/fp_team/fp_team.py` around lines 105 - 120, Add a
guard to prevent removing the team owner: in the doctype method remove_from_team
check whether the passed user equals the team document owner (self.owner or the
owner field on the FP Team doc) and raise a validation/permission error instead
of removing them; mirror the same check in the API function
remove_member_from_team (validate the user against the team.owner before calling
remove_from_team) and return an appropriate HTTP/validation error (e.g., raise a
ValidationError or return 400) so owner removal is blocked at both the model and
API layers.

11 changes: 10 additions & 1 deletion forms_pro/hooks.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@
doc_events = {
"User": {
"on_update": "forms_pro.overrides.roles.handle_forms_pro_role_change",
}
},
"User Invitation": {
"after_insert": "forms_pro.overrides.invitations.after_insert",
},
}

# Scheduled Tasks
Expand Down Expand Up @@ -239,6 +242,12 @@
# "Logging DocType Name": 30 # days to retain logs
# }

user_invitation = {
"allowed_roles": {
"Forms Pro User": ["Forms Pro User"],
},
"after_accept": ["forms_pro.overrides.invitations.after_accept"],
}

website_route_rules = [
{"from_route": "/forms/<path:app_path>", "to_route": "forms"},
Expand Down
56 changes: 56 additions & 0 deletions forms_pro/overrides/invitations.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
from urllib.parse import parse_qs, urlparse

import frappe
from frappe.model.document import Document

from forms_pro.utils.teams import set_current_team


def after_accept(invitation: Document, user: Document, user_inserted: bool) -> None:
"""
Called by Frappe after a User Invitation is accepted.
Adds the invited user to the team they were invited to and updates the
in-memory redirect path so the browser lands on /forms (not the API endpoint).
"""
parsed = urlparse(invitation.redirect_to_path)
qs = parse_qs(parsed.query)
team_id = qs.get("team_id", [None])[0]

if not team_id or not frappe.db.exists("FP Team", team_id):
return

from forms_pro.forms_pro.doctype.fp_team.fp_team import FPTeam

team: FPTeam = frappe.get_doc("FP Team", team_id)
if not team.is_team_member(user.name):
team.add_to_team(user.name)
team.save(ignore_permissions=True)

set_current_team(team_id, user.name)

# Update the in-memory path so _accept_invitation redirects the browser to
# /forms instead of the API endpoint URL (which breaks when URL-embedded as
# a redirect_to query param during the password-reset flow).
invitation.redirect_to_path = "/forms"


def after_insert(doc: Document, method: str) -> None:
"""
After an invitation is inserted, add the user to the team
"""
if doc.app_name != "forms_pro":
return

role_names = [r.role for r in doc.roles] if doc.roles else []
if role_names != ["Forms Pro User"]:
return

parsed = urlparse(doc.redirect_to_path)
qs = parse_qs(parsed.query)
team_id = qs.get("team_id", [None])[0]
if not team_id:
return

# Set the redirect path to add the member to the team (invite_id so API receives it)
doc.redirect_to_path = f"/api/v2/method/forms_pro.api.team.add_member_to_team_via_invitation?team_id={team_id}&invite_id={doc.name}"
doc.save(ignore_permissions=True)
7 changes: 7 additions & 0 deletions forms_pro/overrides/roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ def handle_forms_pro_role_change(doc, method) -> None:
if not has_forms_pro_role_before_save and has_forms_pro_role_after_save:
if len(get_user_teams(user.name)) > 0:
return
# If the user was invited via Forms Pro, the invitation redirect will
# add them to the correct team — don't create a spurious default team.
if frappe.db.exists(
"User Invitation",
{"email": user.name, "app_name": "forms_pro", "status": ["in", ["Pending", "Accepted"]]},
):
return
create_default_team_for_user(user)


Expand Down
Loading