Refactor team esnure functions (WIP) #1174
Conversation
Remove and replace ensure_can_create_service_account in the Team model with validate_can_create_service_account. The reasoning behind this change is to remove the reliance one raising errors as control flow and pass the responsibility of raising errors to the views. Update can_user_create_service_accounts and update tests accordingly to the changes made. Update locations in the code which were dependent on ensure_can_create_service_account. Refs. TS-2590
Refs. TS-2590
Refs. TS-2590
Refs. TS-2590
Refs. TS-2590
Refs. TS-2590
Refs. TS-2590
Rewrite the ensure_user_can_manage_packages function and utilize this in functions in the Package model which is dependent in the function. Refs. TS-2590
Refs. TS-2590
Refs. TS-2590
Refs. TS-2590
WalkthroughRefactors permission checks from exception-based ensure_* methods to validate_* methods returning (errors, is_public). Updates forms, services, models, and uploads to consume validators and raise PermissionValidationError with context. Adds check_user_permissions utility. Adjusts team update/disband flows and tests to the new tuple-based validation API. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User/Agent
participant F as Form/Service
participant T as Team/Owner Model
participant E as PermissionValidationError
U->>F: Trigger action (e.g., submit, API call)
F->>T: validate_* (user)
alt Errors found
T-->>F: (errors, is_public=false|true)
F->>E: Raise PermissionValidationError(errors, is_public)
E-->>U: Error surfaced (public/private signaled)
else No errors
T-->>F: ([], is_public=true)
F->>T: Perform action (save/update/disband/upload)
T-->>F: Success
F-->>U: OK
end
sequenceDiagram
autonumber
actor A as Agent
participant S as TeamService
participant T as Team
participant E as PermissionValidationError
A->>S: update_team(agent, data)
S->>T: validate_user_can_access(agent)
T-->>S: (errors1, is_public1)
S->>T: validate_user_can_edit_info(agent)
T-->>S: (errors2, is_public2)
alt Any errors
S->>E: Raise with combined errors, is_public = any(true|false)
E-->>A: Error
else Valid
S->>T: Apply changes (donation_link, etc.)
T-->>S: Saved
S-->>A: Return updated team
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1174 +/- ##
==========================================
+ Coverage 92.23% 92.25% +0.02%
==========================================
Files 331 331
Lines 10087 10179 +92
Branches 927 947 +20
==========================================
+ Hits 9304 9391 +87
- Misses 657 661 +4
- Partials 126 127 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
django/thunderstore/repository/tests/test_team.py (1)
389-438: Correct malformed assert that mistakenly compares a tuple.
assert errors, is_public == [...]evaluates as a tuple assertion and is not doing what’s intended. Remove it and rely on the subsequent per-branch assertions (which you already have), or split into two asserts as appropriate.- assert errors, is_public == ["Must be a member of team to upload package"] + # Asserts follow in the specific branches below; no combined tuple assert here.Alternatively, if you prefer asserting immediately for non-deactivated users, restructure as:
- errors, is_public = team.validate_can_upload_package(user) - # ... - if user_type == TestUserTypes.deactivated_user: + errors, is_public = team.validate_can_upload_package(user) + if user_type == TestUserTypes.deactivated_user: assert is_public is False assert errors == ["User has been deactivated"] else: assert is_public is True assert errors == ["Must be a member of team to upload package"]
🧹 Nitpick comments (7)
django/thunderstore/api/cyberstorm/services/team.py (1)
15-21: Validator pattern adoption looks good; consider unifying the loop into a helperThe switch to validator functions returning
(errors, is_public)is clear and maintains atomicity on failure.To DRY this across services, consider a small helper:
def validate_or_raise(validators, actor): for v in validators: errors, is_public = v(actor) if errors: raise PermissionValidationError(errors, is_public=is_public)Usage:
validators = [team.validate_user_can_access, team.validate_user_can_disband] validate_or_raise(validators, user)django/thunderstore/repository/package_upload.py (1)
210-213: Avoid duplicate permission checks in save; centralize or trust form cleaning
clean_teamalready validates upload permission. Re-checking insaveduplicates DB work and validator calls.Option A (preferred): centralize in a helper and call it from both places if you want belt-and-suspenders:
def _ensure_upload_permission(self, team: Team) -> None: errors, is_public = team.validate_can_upload_package(self.user) if errors: raise PermissionValidationError(errors, is_public=is_public)Then:
- errors, is_public = team.validate_can_upload_package(self.user) - if errors: - raise PermissionValidationError(errors, is_public=is_public) + self._ensure_upload_permission(team)Option B: trust the form lifecycle (i.e.,
is_valid()precedessave()) and remove thesave()-side check entirely to reduce duplicated work.django/thunderstore/repository/tests/test_team.py (1)
313-350: Rename test to match new API naming (ensure → validate).The test name still uses "ensure" while the implementation and rest of the suite have migrated to "validate". This avoids confusion when grepping and aligns with the new contract.
-def test_team_ensure_user_can_manage_members( +def test_team_validate_user_can_manage_members( team: Team, user_type: str, role: str ) -> None:django/thunderstore/repository/forms/team.py (1)
37-41: Minor: redundant lower() with __iexact.
__iexactalready normalizes case; removing.lower()avoids unnecessary transformations and keeps comparisons symmetric.- if Team.objects.filter(name__iexact=name.lower()).exists(): + if Team.objects.filter(name__iexact=name).exists(): raise ValidationError(f"A team with the provided name already exists") - if Namespace.objects.filter(name__iexact=name.lower()).exists(): + if Namespace.objects.filter(name__iexact=name).exists(): raise ValidationError("A namespace with the provided name already exists")django/thunderstore/repository/models/team.py (3)
14-14: Remove unused import.
validate_useris not used in this module; drop it to satisfy linters (F401) and keep imports tidy.-from thunderstore.permissions.utils import check_user_permissions, validate_user +from thunderstore.permissions.utils import check_user_permissions
370-380: Combine nested condition per SIM102.Slight simplification without behavior change.
- if new_role != TeamMemberRole.owner: - if self.is_last_owner(member): - return ["Cannot remove last owner from team"], public_error + if new_role != TeamMemberRole.owner and self.is_last_owner(member): + return ["Cannot remove last owner from team"], public_error
382-394: Type hint mismatch and variable naming consistency in validate_user_can_disband.The return type should be Tuple[List[str], bool] (you return lists elsewhere). Also rename local
errortoerrorsto match the rest of the file for readability.- def validate_user_can_disband(self, user: Optional[UserType]) -> Tuple[str, bool]: - error, public_error = check_user_permissions(user) - if error: - return error, public_error + def validate_user_can_disband(self, user: Optional[UserType]) -> Tuple[List[str], bool]: + errors, public_error = check_user_permissions(user) + if errors: + return errors, public_error
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
django/thunderstore/account/forms.py(4 hunks)django/thunderstore/api/cyberstorm/services/team.py(2 hunks)django/thunderstore/permissions/utils.py(2 hunks)django/thunderstore/repository/forms/team.py(4 hunks)django/thunderstore/repository/models/package.py(2 hunks)django/thunderstore/repository/models/team.py(3 hunks)django/thunderstore/repository/package_upload.py(3 hunks)django/thunderstore/repository/tests/test_team.py(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
django/thunderstore/repository/package_upload.py (2)
django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/repository/models/team.py (1)
validate_can_upload_package(321-336)
django/thunderstore/account/forms.py (2)
django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/repository/models/team.py (3)
validate_can_create_service_account(250-263)validate_can_delete_service_account(280-293)validate_can_edit_service_account(265-278)
django/thunderstore/api/cyberstorm/services/team.py (2)
django/thunderstore/repository/models/team.py (3)
validate_user_can_access(308-319)validate_user_can_disband(382-393)validate_user_can_edit_info(395-406)django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)
django/thunderstore/permissions/utils.py (2)
django/conftest.py (1)
user(115-120)django/thunderstore/core/types.py (1)
UserType(13-16)
django/thunderstore/repository/tests/test_team.py (2)
django/conftest.py (7)
team(135-136)user(115-120)TestUserTypes(550-585)get_user_by_type(564-585)fake_users(560-561)service_account(406-417)package(186-187)django/thunderstore/repository/models/team.py (15)
validate_can_manage_members(295-306)TeamMemberRole(19-21)Team(92-448)validate_user_can_access(308-319)validate_can_upload_package(321-336)validate_member_can_be_removed(351-363)validate_member_role_can_be_changed(365-380)validate_user_can_disband(382-393)validate_can_create_service_account(250-263)TeamMember(42-79)create(189-196)validate_can_edit_service_account(265-278)validate_can_delete_service_account(280-293)validate_user_can_edit_info(395-406)validate_user_can_manage_packages(338-349)
django/thunderstore/repository/models/package.py (3)
django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)django/thunderstore/repository/models/team.py (1)
validate_user_can_manage_packages(338-349)django/thunderstore/core/utils.py (1)
check_validity(55-60)
django/thunderstore/repository/forms/team.py (2)
django/thunderstore/repository/models/team.py (3)
validate_can_manage_members(295-306)validate_member_can_be_removed(351-363)validate_user_can_access(308-319)django/thunderstore/core/exceptions.py (1)
PermissionValidationError(4-7)
django/thunderstore/repository/models/team.py (1)
django/thunderstore/permissions/utils.py (2)
check_user_permissions(21-35)validate_user(7-18)
🪛 Ruff (0.12.2)
django/thunderstore/repository/tests/test_team.py
349-349: Pointless comparison. Did you mean to assign a value? Otherwise, prepend assert or remove it.
(B015)
django/thunderstore/repository/models/team.py
14-14: thunderstore.permissions.utils.validate_user imported but unused
Remove unused import: thunderstore.permissions.utils.validate_user
(F401)
376-377: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 Flake8 (7.2.0)
django/thunderstore/repository/models/team.py
[error] 14-14: 'thunderstore.permissions.utils.validate_user' imported but unused
(F401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Test pytest (6)
- GitHub Check: Test pytest (3)
- GitHub Check: Test pytest (1)
- GitHub Check: Test pytest (4)
- GitHub Check: Test pytest (5)
- GitHub Check: Test pytest (2)
- GitHub Check: Test missing migrations
- GitHub Check: Test mypy
- GitHub Check: Build docker image
- GitHub Check: Analyze (javascript)
🔇 Additional comments (30)
django/thunderstore/api/cyberstorm/services/team.py (1)
43-51: LGTM on update_team validators and explicit returnConsistent use of
(errors, is_public)and returning the updatedteamis clear and side-effect free outside the transaction block.django/thunderstore/repository/package_upload.py (2)
11-11: Import of PermissionValidationError is appropriate for permission-scope errorsAligns with the new validator-based API and preserves
is_publicpropagation.
114-116: LGTM: Form-level permission validation with is_public contextRaising
PermissionValidationError(errors, is_public=is_public)fromclean_teamaligns with the new validation contract and ensures consistent error handling in views/forms.django/thunderstore/repository/models/package.py (3)
19-19: Importing PermissionValidationError is correct for structured permission failuresKeeps
is_publicattached to raised validation errors, interoperating with callers that inspect visibility.
339-342: LGTM: Switch to validator + PermissionValidationError for deprecation managementClear delegation to
Team.validate_user_can_manage_packagesand consistent error raising semantics.
347-349: LGTM: Consistent permission validation for wiki managementSame validator flow and error structure as above; readable and maintainable.
django/thunderstore/account/forms.py (3)
5-5: Import of PermissionValidationError is fineUsed in subsequent validators to carry
is_publicthrough form errors.
48-52: LGTM: Delete permission uses validator + PermissionValidationErrorConsistent with the new pattern;
is_publicis propagated.
71-75: LGTM: Edit permission uses validator + PermissionValidationErrorMatches delete flow and encapsulates permission visibility.
django/thunderstore/repository/tests/test_team.py (17)
319-326: LGTM: unauthenticated/deactivated branches assert error payload and visibility.The assertions correctly verify both error content and the public/private visibility flag for non-authenticated and deactivated users.
337-340: LGTM: service account branch consistent with permissions utility.Error and is_public values align with check_user_permissions(allow_serviceaccount=False).
355-383: LGTM: validate_user_can_access tests are thorough.Covers fake, deactivated, member, and non-member paths with correct is_public expectations.
443-456: LGTM: happy path for member removal.Correctly ensures not-last-owner scenario passes with empty errors and public visibility.
459-467: LGTM: wrong-team member removal returns public error.Matches model’s validator contract.
470-477: LGTM: no-member removal yields public “Invalid member”.Covers the None path explicitly.
480-492: LGTM: last-owner removal blocked with public error.Asserts align with validator semantics.
496-504: LGTM: role change on wrong-team member returns public error.Consistent with validator behavior.
508-515: LGTM: role change with no member returns public error.Correct edge-case coverage.
519-527: LGTM: invalid role inputs correctly rejected.Validates both None and invalid string.
530-539: LGTM: last owner demotion is rejected.Matches business rule.
544-557: LGTM: role change matrix covers last-owner guard.Good comprehensive coverage with dynamic setup.
562-596: LGTM: disband validator scenarios are well covered.Asserts for fake, deactivated, service account, non-owner, and owner pass cases are correct.
599-607: LGTM: disband blocked when team has packages.Correct message and visibility.
651-685: LGTM: create service account validator.Error mapping per user type and role is accurate; success path validated.
690-716: LGTM: edit service account validator.Table-driven expected_errors keeps the tests concise and robust.
721-753: LGTM: delete service account validator + can_user mirror assertions.Nice addition asserting the can_user_* method coherence with validator output.
django/thunderstore/repository/forms/team.py (2)
83-89: LGTM: enforce team presence before permission checks.Early explicit “Team is required” improves UX and avoids Nones downstream.
103-110: LGTM: permission + domain validation composition is correct.You first ensure actor can manage members, then validate member removability; error raising preserves is_public.
django/thunderstore/repository/models/team.py (2)
250-362: LGTM: new validator methods replace ensure_ with clear (errors, is_public) contract.*Consistent use of check_user_permissions + membership/role checks reads cleanly and is easy to propagate to forms/services.
408-449: LGTM: can_user_ wrappers properly mirror validator outcomes.*Each wrapper defers to its validator and returns True iff errors are empty.
| errors, _ = team.validate_can_create_service_account(self.user) | ||
| if errors: | ||
| raise forms.ValidationError(errors) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use PermissionValidationError to keep is_public context for create flow
For consistency with delete/edit forms and to retain is_public, raise PermissionValidationError instead of a plain forms.ValidationError.
- errors, _ = team.validate_can_create_service_account(self.user)
- if errors:
- raise forms.ValidationError(errors)
+ errors, is_public = team.validate_can_create_service_account(self.user)
+ if errors:
+ raise PermissionValidationError(errors, is_public=is_public)📝 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.
| errors, _ = team.validate_can_create_service_account(self.user) | |
| if errors: | |
| raise forms.ValidationError(errors) | |
| errors, is_public = team.validate_can_create_service_account(self.user) | |
| if errors: | |
| raise PermissionValidationError(errors, is_public=is_public) |
🤖 Prompt for AI Agents
In django/thunderstore/account/forms.py around lines 22-24, replace raising
forms.ValidationError with raising PermissionValidationError so the create flow
preserves is_public context; update the code to raise
PermissionValidationError(errors, is_public=self.is_public) (or the equivalent
used elsewhere in delete/edit forms) and add the corresponding import for
PermissionValidationError from the module used by the other account forms.
| 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) | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type annotation mismatch in Team.validate_user_can_disband may break type-checking
This caller expects validators to return Tuple[List[str], bool]. In repository/models/team.py, validate_user_can_disband is annotated as Tuple[str, bool] in the provided snippet while returning lists. Align annotation to Tuple[List[str], bool].
Proposed fix in django/thunderstore/repository/models/team.py:
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 validate_user_can_disband
The current signature in django/thunderstore/repository/models/team.py (line 382) is:
def validate_user_can_disband(self, user: Optional[UserType]) -> Tuple[str, bool]:
…but callers expect Tuple[List[str], bool]. Update it to:
- 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 List is recognized.
After this change, rerun your type‐checker to confirm no further mismatches.
🤖 Prompt for AI Agents
In django/thunderstore/repository/models/team.py around line 382 the function
validate_user_can_disband currently declares its return type as Tuple[str, bool]
but callers expect Tuple[List[str], bool]; update the signature to return
Tuple[List[str], bool] and adjust any internal return sites to return a list of
error strings (even if single element) instead of a bare string; also ensure the
module imports Tuple, List, Optional from typing (add "from typing import Tuple,
List, Optional" if missing) and then rerun the type-checker to verify there are
no remaining mismatches.
💡 Verification agent
🧩 Analysis chain
Bug risk: create_team uses getattr(user, "service_account", None) which can raise DoesNotExist
Reverse OneToOne access can raise ServiceAccount.DoesNotExist, even when a default is provided to getattr. Use an existence query or a guarded try/except.
Outside this hunk, update the check in create_team(...):
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 service_account:
🏁 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 getattr(user, "service_account", None), but Django’s reverse OneToOne lookup raises ServiceAccount.DoesNotExist (a subclass of ObjectDoesNotExist) when no related instance exists—this isn’t caught by getattr’s default, so it will error instead of returning None. We need to update both occurrences to safely detect service accounts.
Affected locations:
- django/thunderstore/api/cyberstorm/services/team.py (lines 29–30)
- django/thunderstore/repository/forms/team.py (lines 46–47)
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
In django/thunderstore/api/cyberstorm/services/team.py around lines 15 to 21
(and also update django/thunderstore/api/cyberstorm/services/team.py lines 29–30
as referenced) replace the unsafe getattr(user, "service_account", None) check
which can raise ServiceAccount.DoesNotExist with a safe existence check: either
use ServiceAccount.objects.filter(user=user).exists() to detect service
accounts, or perform a guarded lookup that catches ObjectDoesNotExist (and
AttributeError) around user.service_account; then raise
PermissionValidationError when a service account exists. Apply the same change
to django/thunderstore/repository/forms/team.py around lines 46–47 so both
locations use the same safe pattern.
| @@ -1,4 +1,4 @@ | |||
| from typing import Optional | |||
| from typing import List, Optional, Tuple | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import ObjectDoesNotExist to safely probe reverse OneToOne relations
Accessing user.service_account can raise ServiceAccount.DoesNotExist, which is a subclass of ObjectDoesNotExist. We need this import for a safe try/except around the service account check below.
from typing import List, Optional, Tuple
+from django.core.exceptions import ObjectDoesNotExist📝 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.
| from typing import List, Optional, Tuple | |
| from typing import List, Optional, Tuple | |
| from django.core.exceptions import ObjectDoesNotExist |
🤖 Prompt for AI Agents
In django/thunderstore/permissions/utils.py around line 1, the module currently
imports typing only and does not import ObjectDoesNotExist; accessing
user.service_account can raise ServiceAccount.DoesNotExist (a subclass of
ObjectDoesNotExist), so add "from django.core.exceptions import
ObjectDoesNotExist" to the imports and update the service account check to wrap
the access in a try/except that catches ObjectDoesNotExist (instead of or in
addition to ServiceAccount.DoesNotExist) to safely handle missing reverse
OneToOne relations and avoid unhandled exceptions.
There was a problem hiding this comment.
Fix: hasattr/getattr on reverse OneToOne may raise DoesNotExist (crashes permission checks)
hasattr(user, "service_account") on a User without a related ServiceAccount raises ServiceAccount.DoesNotExist (not AttributeError), causing this function to error instead of returning a validation error. Replace with a safe try/except check.
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")
+ else:
+ # Reverse OneToOne access may raise DoesNotExist when the relation is missing
+ try:
+ getattr(user, "service_account")
+ is_service_account = True
+ except (AttributeError, ObjectDoesNotExist):
+ is_service_account = False
+ if is_service_account and not allow_serviceaccount:
+ errors.append("Service accounts are unable to perform this action")
return errors, public_errorAs a follow-up, mirror the same safe pattern in validate_user(...) to prevent identical crashes there:
# In validate_user(...)
try:
getattr(user, "service_account")
is_service_account = True
except (AttributeError, ObjectDoesNotExist):
is_service_account = False
if is_service_account and not allow_serviceaccount:
raise PermissionValidationError("Service accounts are unable to perform this action")I can send a separate PR patch touching both places if helpful.
🤖 Prompt for AI Agents
In django/thunderstore/permissions/utils.py around lines 21 to 35, using
hasattr(user, "service_account") can raise ServiceAccount.DoesNotExist (an
ObjectDoesNotExist subclass) and crash permission checks; replace the hasattr
call with a safe getattr/try-except that catches AttributeError and
django.core.exceptions.ObjectDoesNotExist, set a boolean like is_service_account
accordingly, and append the service account error only when is_service_account
is True and allow_serviceaccount is False; also apply the same safe
getattr/try-except pattern in validate_user(...) to avoid the identical crash
there.
| 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) | ||
|
|
There was a problem hiding this comment.
Fix method name: call validate_member_role_can_be_changed.
Current call uses a non-existent name (validate_member_role_be_changed), which will raise AttributeError at runtime.
- 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
‼️ 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.
| 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) | |
| if not team: | |
| raise ValidationError("Team is missing") | |
| error, is_public = team.validate_member_role_can_be_changed(self.instance, new_role) | |
| if error: | |
| raise PermissionValidationError(error, is_public=is_public) |
🤖 Prompt for AI Agents
django/thunderstore/repository/forms/team.py around lines 133 to 139: the code
calls a non-existent method validate_member_role_be_changed which causes
AttributeError at runtime; change the call to
validate_member_role_can_be_changed (preserving the same args and handling of
returned error and is_public), and run tests/lint to ensure no other references
to the old name remain.
| 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() |
There was a problem hiding this comment.
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
‼️ 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.
| 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() | |
| @transaction.atomic | |
| def save(self, **kwargs): | |
| error, is_public = self.instance.validate_user_can_disband(self.user) | |
| if error: | |
| raise PermissionValidationError(error, is_public=is_public) | |
| self.instance.delete() |
🤖 Prompt for AI Agents
In django/thunderstore/repository/forms/team.py around lines 179 to 189, the
code calls validate_user_can_access for both cleaning and saving (deletion),
which is too permissive for disbanding a team; replace those calls with
validate_user_can_disband so the stricter ownership-and-no-packages check is
enforced, and keep raising PermissionValidationError(error, is_public=is_public)
when the disband validator returns an error.
| error, is_public = self.instance.validate_user_can_access(self.user) | ||
| if error: | ||
| raise PermissionValidationError(error, is_public=is_public) | ||
| self.instance.delete() |
There was a problem hiding this comment.
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
‼️ 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.
| error, is_public = self.instance.validate_user_can_access(self.user) | |
| if error: | |
| raise PermissionValidationError(error, is_public=is_public) | |
| self.instance.delete() | |
| error, is_public = self.instance.validate_user_can_disband(self.user) | |
| if error: | |
| raise PermissionValidationError(error, is_public=is_public) | |
| self.instance.delete() |
🤖 Prompt for AI Agents
In django/thunderstore/repository/forms/team.py around lines 186 to 189, the
form's save() currently deletes the team without re-checking the same disband
validator used in clean(); update save() to call
self.instance.validate_user_can_access(self.user) before performing the delete,
raise PermissionValidationError(error, is_public=is_public) if it returns an
error, and only then proceed to self.instance.delete() so the owner/no-packages
check is enforced at execution time.
| assert errors == [] | ||
| else: | ||
| assert team.can_user_manage_members(user) is False | ||
| with pytest.raises(ValidationError) as e: | ||
| team.ensure_user_can_manage_members(user) | ||
| assert "Must be an owner to manage team members" in str(e.value) | ||
| errors, is_public = team.validate_can_manage_members(user) | ||
| errors = ["Must be an owner to manage team members"] | ||
| is_public is True |
There was a problem hiding this comment.
Fix stray assignment and ineffective comparison; assert the expectations.
The current code reassigns errors and uses a pointless comparison on is_public without asserting, causing the test to pass/fail incorrectly. Also flagged by static analysis (B015).
- errors, is_public = team.validate_can_manage_members(user)
- errors = ["Must be an owner to manage team members"]
- is_public is True
+ errors, is_public = team.validate_can_manage_members(user)
+ assert errors == ["Must be an owner to manage team members"]
+ assert is_public is True📝 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.
| assert errors == [] | |
| else: | |
| assert team.can_user_manage_members(user) is False | |
| with pytest.raises(ValidationError) as e: | |
| team.ensure_user_can_manage_members(user) | |
| assert "Must be an owner to manage team members" in str(e.value) | |
| errors, is_public = team.validate_can_manage_members(user) | |
| errors = ["Must be an owner to manage team members"] | |
| is_public is True | |
| assert errors == [] | |
| else: | |
| assert team.can_user_manage_members(user) is False | |
| errors, is_public = team.validate_can_manage_members(user) | |
| assert errors == ["Must be an owner to manage team members"] | |
| assert is_public is True |
🧰 Tools
🪛 Ruff (0.12.2)
349-349: Pointless comparison. Did you mean to assign a value? Otherwise, prepend assert or remove it.
(B015)
🤖 Prompt for AI Agents
In django/thunderstore/repository/tests/test_team.py around lines 344 to 349,
the test mistakenly reassigns the errors variable and uses a bare boolean
expression for is_public instead of assertions; replace the stray assignment
with an equality assertion comparing the returned errors to ["Must be an owner
to manage team members"] (do not overwrite the errors variable), and change the
bare "is_public is True" expression to an actual assertion (e.g., assert
is_public is True) so both expectations are properly asserted.
1 participant
WIP - refactor ensure_x
functionsin theTeammodelTODO:
Summary by CodeRabbit
Bug Fixes
Refactor
Tests