fix(auth): exact-match authorization; guard reviews; centralize name validation
Auth + input-validation cluster (CODE-REVIEW-2026-06 #5, #6, #16, #17 + libra-#36, libra-#51, libra-#52): - can_access_user_data compares full user ids only. The 8-char prefix comparison was a 32-bit space: any prefix collision (or a crafted short target id) let one user read another's data. - can_access_account matches the User-{short} SEGMENT exactly; the substring test also matched accounts merely containing it (Expenses:Misc-User-deadbeef). - Manual-payment approve/reject are status-guarded (UPDATE ... WHERE status='pending' + rowcount): concurrent admins can't double-book. The approve endpoint claims the request BEFORE writing the ledger entry and reverts the claim if the write fails, so at most one journal entry can exist per request. - Account-name validation centralized into account_utils.validate_account_name (libra-#51) — called from crud.create_account (the choke point for every creation path, virtual parents allowed a bare root), the admin add-account endpoint, and fava_client.add_account at the writer boundary (libra-#52). - crud.create_account translates backend unique-violations into AccountExistsError instead of leaking sqlalchemy internals (libra-#36); POST /accounts returns 409 on duplicates and 400 on malformed names. get_or_create_user_account catches the domain error instead of string-matching the SQLite message (which never matched on Postgres). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
4d63e08a69
commit
c0d371036b
6 changed files with 422 additions and 103 deletions
142
crud.py
142
crud.py
|
|
@ -66,7 +66,21 @@ PERMISSION_CACHE_TTL = 60 # 1 minute
|
|||
# ===== ACCOUNT OPERATIONS =====
|
||||
|
||||
|
||||
class AccountExistsError(Exception):
|
||||
"""Raised when creating an account whose name is already taken."""
|
||||
|
||||
def __init__(self, name: str):
|
||||
super().__init__(f"Account already exists: {name}")
|
||||
self.name = name
|
||||
|
||||
|
||||
async def create_account(data: CreateAccount) -> Account:
|
||||
# Single validation choke point for every account-creation path
|
||||
# (libra-#51). Virtual parents may be a bare root ("Expenses").
|
||||
from .account_utils import validate_account_name
|
||||
|
||||
validate_account_name(data.name, allow_root_only=data.is_virtual)
|
||||
|
||||
account_id = urlsafe_short_hash()
|
||||
account = Account(
|
||||
id=account_id,
|
||||
|
|
@ -77,7 +91,17 @@ async def create_account(data: CreateAccount) -> Account:
|
|||
is_virtual=data.is_virtual,
|
||||
created_at=datetime.now(),
|
||||
)
|
||||
await db.insert("accounts", account)
|
||||
try:
|
||||
await db.insert("accounts", account)
|
||||
except Exception as e:
|
||||
# Translate backend-specific unique-violation errors (SQLite:
|
||||
# "UNIQUE constraint failed", Postgres: "duplicate key value")
|
||||
# into a domain error instead of leaking sqlalchemy internals
|
||||
# (libra-#36).
|
||||
msg = str(e).lower()
|
||||
if "unique" in msg or "duplicate" in msg:
|
||||
raise AccountExistsError(data.name) from e
|
||||
raise
|
||||
|
||||
# Invalidate cache for this account (Cache class doesn't have delete method, use pop)
|
||||
account_cache._values.pop(f"account:id:{account_id}", None)
|
||||
|
|
@ -305,44 +329,39 @@ async def get_or_create_user_account(
|
|||
user_id=user_id,
|
||||
)
|
||||
)
|
||||
except Exception as e:
|
||||
# Handle UNIQUE constraint error - account already exists
|
||||
if "UNIQUE constraint failed" in str(e) and "accounts.name" in str(e):
|
||||
logger.warning(f"[LIBRA DB] Account already exists (UNIQUE constraint), fetching by name: {account_name}")
|
||||
# Fetch existing account by name only (ignore user_id in query)
|
||||
account = await db.fetchone(
|
||||
"""
|
||||
SELECT * FROM accounts
|
||||
WHERE name = :name
|
||||
""",
|
||||
{"name": account_name},
|
||||
Account,
|
||||
)
|
||||
if account:
|
||||
logger.info(f"[LIBRA DB] Found existing account: {account_name} (user_id: {account.user_id})")
|
||||
# Update user_id if it's NULL or different
|
||||
if account.user_id != user_id:
|
||||
logger.info(f"[LIBRA DB] Updating account user_id from {account.user_id} to {user_id}")
|
||||
await db.execute(
|
||||
"""
|
||||
UPDATE accounts
|
||||
SET user_id = :user_id
|
||||
WHERE name = :name
|
||||
""",
|
||||
{"user_id": user_id, "name": account_name}
|
||||
)
|
||||
# Refresh account from DB
|
||||
account = await db.fetchone(
|
||||
"""
|
||||
SELECT * FROM accounts
|
||||
WHERE name = :name
|
||||
""",
|
||||
{"name": account_name},
|
||||
Account,
|
||||
)
|
||||
else:
|
||||
# Re-raise if it's a different error
|
||||
raise
|
||||
except AccountExistsError:
|
||||
logger.warning(f"[LIBRA DB] Account already exists, fetching by name: {account_name}")
|
||||
# Fetch existing account by name only (ignore user_id in query)
|
||||
account = await db.fetchone(
|
||||
"""
|
||||
SELECT * FROM accounts
|
||||
WHERE name = :name
|
||||
""",
|
||||
{"name": account_name},
|
||||
Account,
|
||||
)
|
||||
if account:
|
||||
logger.info(f"[LIBRA DB] Found existing account: {account_name} (user_id: {account.user_id})")
|
||||
# Update user_id if it's NULL or different
|
||||
if account.user_id != user_id:
|
||||
logger.info(f"[LIBRA DB] Updating account user_id from {account.user_id} to {user_id}")
|
||||
await db.execute(
|
||||
"""
|
||||
UPDATE accounts
|
||||
SET user_id = :user_id
|
||||
WHERE name = :name
|
||||
""",
|
||||
{"user_id": user_id, "name": account_name}
|
||||
)
|
||||
# Refresh account from DB
|
||||
account = await db.fetchone(
|
||||
"""
|
||||
SELECT * FROM accounts
|
||||
WHERE name = :name
|
||||
""",
|
||||
{"name": account_name},
|
||||
Account,
|
||||
)
|
||||
else:
|
||||
logger.info(f"[LIBRA DB] Account already exists in Libra DB: {account_name}")
|
||||
|
||||
|
|
@ -563,14 +582,20 @@ async def get_all_manual_payment_requests(
|
|||
async def approve_manual_payment_request(
|
||||
request_id: str, reviewed_by: str, journal_entry_id: str
|
||||
) -> Optional["ManualPaymentRequest"]:
|
||||
"""Approve a manual payment request"""
|
||||
from .models import ManualPaymentRequest
|
||||
"""Approve a manual payment request.
|
||||
|
||||
await db.execute(
|
||||
Status-guarded: only a 'pending' request can be approved, so two
|
||||
concurrent admins can't both win (the loser gets None and must not
|
||||
create a second journal entry).
|
||||
|
||||
Returns:
|
||||
The approved request, or None if it wasn't pending anymore.
|
||||
"""
|
||||
result = await db.execute(
|
||||
"""
|
||||
UPDATE manual_payment_requests
|
||||
SET status = 'approved', reviewed_at = :reviewed_at, reviewed_by = :reviewed_by, journal_entry_id = :journal_entry_id
|
||||
WHERE id = :id
|
||||
WHERE id = :id AND status = 'pending'
|
||||
""",
|
||||
{
|
||||
"id": request_id,
|
||||
|
|
@ -579,21 +604,42 @@ async def approve_manual_payment_request(
|
|||
"journal_entry_id": journal_entry_id,
|
||||
},
|
||||
)
|
||||
if result.rowcount == 0:
|
||||
return None
|
||||
|
||||
return await get_manual_payment_request(request_id)
|
||||
|
||||
|
||||
async def revert_manual_payment_request(request_id: str) -> None:
|
||||
"""Roll an approved request back to pending.
|
||||
|
||||
Compensation for the approve flow: the status is claimed BEFORE the
|
||||
journal entry is written (so concurrent admins can't double-book);
|
||||
if the ledger write then fails, the claim must be released.
|
||||
"""
|
||||
await db.execute(
|
||||
"""
|
||||
UPDATE manual_payment_requests
|
||||
SET status = 'pending', reviewed_at = NULL, reviewed_by = NULL, journal_entry_id = NULL
|
||||
WHERE id = :id AND status = 'approved'
|
||||
""",
|
||||
{"id": request_id},
|
||||
)
|
||||
|
||||
|
||||
async def reject_manual_payment_request(
|
||||
request_id: str, reviewed_by: str
|
||||
) -> Optional["ManualPaymentRequest"]:
|
||||
"""Reject a manual payment request"""
|
||||
from .models import ManualPaymentRequest
|
||||
"""Reject a manual payment request.
|
||||
|
||||
await db.execute(
|
||||
Status-guarded like approve_manual_payment_request; returns None when
|
||||
the request wasn't pending anymore.
|
||||
"""
|
||||
result = await db.execute(
|
||||
"""
|
||||
UPDATE manual_payment_requests
|
||||
SET status = 'rejected', reviewed_at = :reviewed_at, reviewed_by = :reviewed_by
|
||||
WHERE id = :id
|
||||
WHERE id = :id AND status = 'pending'
|
||||
""",
|
||||
{
|
||||
"id": request_id,
|
||||
|
|
@ -601,6 +647,8 @@ async def reject_manual_payment_request(
|
|||
"reviewed_by": reviewed_by,
|
||||
},
|
||||
)
|
||||
if result.rowcount == 0:
|
||||
return None
|
||||
|
||||
return await get_manual_payment_request(request_id)
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue