-
Notifications
You must be signed in to change notification settings - Fork 29
Refactor team esnure functions (WIP) #1174
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
ff95c74
a392b1b
7a9b18f
5961548
8cfeaf5
d50e600
87f4f7e
2e3bb4b
a7fa74b
0793be5
21db4b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,13 @@ | |
| def disband_team(user: UserType, team_name: str) -> None: | ||
| teams = Team.objects.exclude(is_active=False) | ||
| team = get_object_or_404(teams, name=team_name) | ||
| team.ensure_user_can_access(user) | ||
| team.ensure_user_can_disband(user) | ||
| validators = [team.validate_user_can_access, team.validate_user_can_disband] | ||
|
|
||
| for validator in validators: | ||
| errors, is_public = validator(user) | ||
| if errors: | ||
| raise PermissionValidationError(errors, is_public=is_public) | ||
|
|
||
|
Comment on lines
+15
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainType annotation mismatch in Team.validate_user_can_disband may break type-checking This caller expects validators to return Proposed fix in def validate_user_can_disband(self, user: Optional[UserType]) -> Tuple[List[str], bool]:
# ...Run to verify annotations across all team validators: 🏁 Script executed: #!/bin/bash
# Ensure all validate_* methods on Team return Tuple[List[str], bool]
rg -nP --type=py '(def\s+validate_[a-zA-Z_]+\s*\(.*\)\s*->\s*Tuple\[[^\]]+\])' -C1 | sed -n '1,200p'Length of output: 397 Action: Correct the return‐type annotation of The current signature in def validate_user_can_disband(self, user: Optional[UserType]) -> Tuple[str, bool]:
…but callers expect - def validate_user_can_disband(self, user: Optional[UserType]) -> Tuple[str, bool]:
+ def validate_user_can_disband(self, user: Optional[UserType]) -> Tuple[List[str], bool]:
# ...Also ensure you have: from typing import Tuple, List, Optionalso that After this change, rerun your type‐checker to confirm no further mismatches. 🤖 Prompt for AI Agents💡 Verification agent 🧩 Analysis chainBug risk: create_team uses getattr(user, "service_account", None) which can raise DoesNotExist Reverse OneToOne access can raise Outside this hunk, update the check in from thunderstore.account.models import ServiceAccount
# ...
if ServiceAccount.objects.filter(user=user).exists():
raise PermissionValidationError("Service accounts cannot create teams")Alternatively, guarded access: from django.core.exceptions import ObjectDoesNotExist
try:
user.service_account
is_sa = True
except (AttributeError, ObjectDoesNotExist):
is_sa = False
if is_sa:
raise PermissionValidationError("Service accounts cannot create teams")To find other unsafe probes of 🏁 Script executed: #!/bin/bash
rg -nP --type=py -C2 '(hasattr\([^,]+,\s*[\'"]service_account[\'"]\)|getattr\([^,]+,\s*[\'"]service_account[\'"])'Length of output: 297 🏁 Script executed: #!/bin/bash
# Search for the definition and implementation of create_team
rg -nP --type=py "def create_team" -C5
echo
echo "Searching for unsafe getattr on service_account"
rg -nP --type=py -C2 "getattr\([^,]+,\s*['\"]service_account['\"]" || true
echo
echo "Searching for hasattr on service_account"
rg -nP --type=py -C2 "hasattr\([^,]+,\s*['\"]service_account['\"]" || true
echo
echo "Searching for direct attribute access .service_account"
rg -nP --type=py -C2 "\.service_account\b" || trueLength of output: 23347 Prevent ServiceAccount.DoesNotExist by avoiding getattr(user, "service_account") The current checks use Affected locations:
Replace this pattern: # current unsafe check
if getattr(user, "service_account", None) is not None:
raise PermissionValidationError("Service accounts cannot create teams")With either an existence query: from thunderstore.account.models import ServiceAccount
if ServiceAccount.objects.filter(user=user).exists():
raise PermissionValidationError("Service accounts cannot create teams")Or a guarded attribute lookup: from django.core.exceptions import ObjectDoesNotExist
try:
# will raise ServiceAccount.DoesNotExist if none exists
user.service_account
is_sa = True
except (AttributeError, ObjectDoesNotExist):
is_sa = False
if is_sa:
raise PermissionValidationError("Service accounts cannot create teams")Please update both files accordingly to eliminate the crash risk. 🤖 Prompt for AI Agents |
||
| team.delete() | ||
|
|
||
|
|
||
|
|
@@ -35,8 +40,15 @@ def create_team(user: UserType, team_name: str) -> Team: | |
|
|
||
| @transaction.atomic | ||
| def update_team(agent: UserType, team: Team, donation_link: str) -> Team: | ||
| team.ensure_user_can_access(agent) | ||
| team.ensure_user_can_edit_info(agent) | ||
| validators = [ | ||
| team.validate_user_can_access, | ||
| team.validate_user_can_edit_info, | ||
| ] | ||
|
|
||
| for validator in validators: | ||
| errors, is_public = validator(agent) | ||
| if errors: | ||
| raise PermissionValidationError(errors, is_public=is_public) | ||
|
|
||
| team.donation_link = donation_link | ||
| team.save() | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||
| from typing import Optional | ||||||||
| from typing import List, Optional, Tuple | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Import ObjectDoesNotExist to safely probe reverse OneToOne relations Accessing from typing import List, Optional, Tuple
+from django.core.exceptions import ObjectDoesNotExist📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||
|
|
||||||||
| from thunderstore.core.exceptions import PermissionValidationError | ||||||||
| from thunderstore.core.types import UserType | ||||||||
|
|
@@ -16,3 +16,20 @@ def validate_user( | |||||||
| "Service accounts are unable to perform this action" | ||||||||
| ) | ||||||||
| return user | ||||||||
|
|
||||||||
|
|
||||||||
| def check_user_permissions( | ||||||||
| user: Optional[UserType], allow_serviceaccount: bool = False | ||||||||
| ) -> Tuple[List[str], bool]: | ||||||||
| errors = [] | ||||||||
| public_error = True # Set to False if you want to hide certain errors | ||||||||
|
|
||||||||
| if not user or not user.is_authenticated: | ||||||||
| errors.append("Must be authenticated") | ||||||||
| elif not user.is_active: | ||||||||
| errors.append("User has been deactivated") | ||||||||
| public_error = False | ||||||||
| elif hasattr(user, "service_account") and not allow_serviceaccount: | ||||||||
| errors.append("Service accounts are unable to perform this action") | ||||||||
|
|
||||||||
| return errors, public_error | ||||||||
|
||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -80,10 +80,13 @@ def __init__(self, user: Optional[UserType], *args, **kwargs): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| def clean(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = super().clean() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| team = self.cleaned_data.get("team") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if team: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| team.ensure_user_can_manage_members(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValidationError("Invalid team") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not team: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValidationError("Team is required") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| errors, is_public = team.validate_can_manage_members(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if errors: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise PermissionValidationError(errors, is_public=is_public) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -97,8 +100,14 @@ def __init__(self, user: Optional[UserType], *args, **kwargs): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| def clean_membership(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| membership = self.cleaned_data["membership"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if membership.user != self.user: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| membership.team.ensure_user_can_manage_members(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| membership.team.ensure_member_can_be_removed(membership) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| errors, is_public = membership.team.validate_can_manage_members(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if errors: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise PermissionValidationError(errors, is_public=is_public) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error, is_public = membership.team.validate_member_can_be_removed(membership) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if error: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise PermissionValidationError(error, is_public=is_public) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return membership | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def save(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -120,23 +129,29 @@ def clean_role(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| team = self.instance.team | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except ObjectDoesNotExist: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| team = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if team: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| team.ensure_member_role_can_be_changed( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| member=self.instance, new_role=new_role | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not team: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValidationError("Team is missing") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error, is_public = team.validate_member_role_be_changed(self.instance, new_role) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if error: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise PermissionValidationError(error, is_public=is_public) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+133
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix method name: call validate_member_role_can_be_changed. Current call uses a non-existent name ( - error, is_public = team.validate_member_role_be_changed(self.instance, new_role)
+ error, is_public = team.validate_member_role_can_be_changed(self.instance, new_role)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new_role | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def clean(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| team = self.instance.team | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except ObjectDoesNotExist: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| team = None | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if team: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| team.ensure_user_can_manage_members(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not team: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValidationError("Team is missing") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| errors, is_public = team.validate_can_manage_members(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if errors: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise PermissionValidationError(errors, is_public=is_public) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return super().clean() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -161,12 +176,16 @@ def clean_verification(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| def clean(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not self.instance.pk: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValidationError("Missing team instance") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.instance.ensure_user_can_disband(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error, is_public = self.instance.validate_user_can_access(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if error: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise PermissionValidationError(error, is_public=is_public) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return super().clean() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @transaction.atomic | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def save(self, **kwargs): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.instance.ensure_user_can_disband(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error, is_public = self.instance.validate_user_can_access(self.user) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if error: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise PermissionValidationError(error, is_public=is_public) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.instance.delete() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+179
to
189
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: use validate_user_can_disband, not validate_user_can_access. Disband requires ownership (and no packages). Using “access” would allow any member to disband a team. Swap to the stricter validator. - error, is_public = self.instance.validate_user_can_access(self.user)
+ error, is_public = self.instance.validate_user_can_disband(self.user)
if error:
raise PermissionValidationError(error, is_public=is_public)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
+186
to
189
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apply the same disband validator in save(). Mirror the clean() logic to enforce owner/no-packages at execution time. - error, is_public = self.instance.validate_user_can_access(self.user)
+ error, is_public = self.instance.validate_user_can_disband(self.user)
if error:
raise PermissionValidationError(error, is_public=is_public)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Use PermissionValidationError to keep
is_publiccontext for create flowFor consistency with delete/edit forms and to retain
is_public, raisePermissionValidationErrorinstead of a plainforms.ValidationError.📝 Committable suggestion
🤖 Prompt for AI Agents