fix: three independent CI failures (perm bypass, dup Role, float assert)

### 1. has_permission() loses legacy-admin bypass after auto-assign (real bug)

`User.has_permission()` calls `_auto_assign_role_from_legacy()` which
side-effects: it queries the seeded "admin" Role and appends it to
`self.roles`. After the call, `self.roles` is no longer empty — so the
backward-compat bypass `if self.role == "admin" and not self.roles`
never fires for a legacy admin user. If the seeded admin Role has no
explicit permission rows assigned (as in tests), `has_permission()`
then returns False even though the user is documented to have all
permissions.

`test_legacy_admin_user_permissions` exercises exactly this and fails
with `assert admin.has_permission("any_permission") -> False`.

Fix: move the legacy-admin bypass check BEFORE the side-effecting
auto-assign call.

### 2. test_admin_role_user duplicates the seeded "admin" Role

`tests/conftest.py:334-337` seeds baseline Role rows ("admin", "user",
"manager", "subcontractor"). `test_admin_role_user` then creates
`Role(name="admin")` and commits, tripping the unique constraint on
roles.name with `IntegrityError`.

Fix: get-or-create. Re-use the existing seeded admin Role if present.

### 3. Stale assertion: expense amount returned as float, not string

`tests/test_routes/test_api_v1_expenses_complete.py:81` asserts
`data["expense"]["amount"] == "250.75"`, but `Expense.to_dict()` at
app/models/expense.py:189 returns `float(self.amount)`. The string
assertion never matches. Same pattern as the mileage `distance_km`
issue fixed in a sibling PR.

Test plan
- pytest tests/test_permissions.py::test_legacy_admin_user_permissions
- pytest tests/test_permissions.py::test_admin_role_user
- pytest tests/test_routes/test_api_v1_expenses_complete.py::TestAPIExpensesComplete::test_create_expense_all_fields
This commit is contained in:
MacJediWizard
2026-05-14 17:19:03 -04:00
parent 8f18d3b624
commit 60449b92cb
3 changed files with 16 additions and 9 deletions
+9 -5
View File
@@ -419,15 +419,19 @@ class User(UserMixin, db.Model):
# Permission and role helpers
def has_permission(self, permission_name):
"""Check if user has a specific permission through any of their roles"""
# Legacy admin bypass: a user with role="admin" and no explicit
# roles assigned should have all permissions for backward
# compatibility. Check this BEFORE _auto_assign_role_from_legacy
# — that helper appends a Role row to self.roles as a side effect,
# and an empty seeded "admin" role would otherwise mask the
# bypass and leave the user with no effective permissions.
if self.role == "admin" and not self.roles:
return True
# Auto-assign role from legacy role field if user has no roles assigned
if not self.roles and self.role:
self._auto_assign_role_from_legacy()
# Super admin users have all permissions
if self.role == "admin" and not self.roles:
# Legacy admin users without roles have all permissions
return True
# Check if any of the user's roles have this permission
for role in self.roles:
if role.has_permission(permission_name):
+6 -3
View File
@@ -256,9 +256,12 @@ def test_admin_role_user(app):
user = User(username="testuser", role="user")
db.session.add(user)
# Create admin role
admin_role = Role(name="admin")
db.session.add(admin_role)
# The conftest fixture seeds an "admin" role; re-use it if present
# so this test doesn't trip the unique constraint on roles.name.
admin_role = Role.query.filter_by(name="admin").first()
if admin_role is None:
admin_role = Role(name="admin")
db.session.add(admin_role)
db.session.commit()
# User is not admin initially
@@ -78,7 +78,7 @@ class TestAPIExpensesComplete:
data = response.get_json()
assert "expense" in data
assert data["expense"]["title"] == "Complete Test Expense"
assert data["expense"]["amount"] == "250.75"
assert data["expense"]["amount"] == 250.75
def test_update_expense_uses_service_layer(self, app, client_with_token, expense):
"""Test that update_expense uses service layer"""