Fix tenant cache leak, dead DB pool/mock fallback, timezone month bounds, decimal money

.gitignore

@@ -0,0 +1,4 @@
+__pycache__/
+*.pyc
+.pytest_cache/
+.venv/

FINDINGS.md

@@ -0,0 +1,47 @@
+# Findings: Property Revenue Dashboard
+
+Scope: the dashboard has exactly one real code path —
+`Dashboard.tsx → RevenueSummary.tsx → GET /api/v1/dashboard/summary → services/cache.py → services/reservations.py → core/database_pool.py`.
+Everything else in the repo is unrelated scaffolding and was deliberately left untouched (this is a debugging task, not a rebuild).
+
+## Bugs and fixes
+
+| # | Symptom reported | Root cause | Fix |
+|---|---|---|---|
+| 1 | **Ocean Rentals** intermittently sees another company's numbers on refresh | `services/cache.py` cached by `revenue:{property_id}` only. Property IDs are unique *per tenant* (PK is `(id, tenant_id)`; both tenants own `prop-001`), so whoever warmed the key served their numbers to the other tenant for the 300 s TTL. | Key is now `revenue:v2:{tenant_id}:{property_id}:{period}`. Cached payload is also checked against the requesting tenant before being served (defense in depth). |
+| 2 | Numbers "don't match internal records" (both clients) | The dashboard **never read the database**. `core/database_pool.py` built its DSN from `settings.supabase_db_*` fields that don't exist → `AttributeError`; `get_session` was `async def` but used with `async with`; a new pool was built per request. `calculate_total_revenue` swallowed every exception and returned a **hard-coded mock table** keyed by property only — so `prop-001` showed `1000.00 / 3` to *both* tenants (a second cross-tenant leak, and it hid the outage). | Pool uses `settings.database_url` (rewritten to `postgresql+asyncpg://`), single global pool with lazy, lock-guarded init, `get_session` returns the session. The mock fallback is deleted: DB errors now surface as **503**. Financial data must fail loudly, never fabricate. |
+| 3 | **Sunset Properties** March total differs from their books | `calculate_monthly_revenue` used naive `datetime(year, month, 1)` bounds against `TIMESTAMPTZ`, i.e. UTC months. Seed row `res-tz-1` checks in at `2024-02-29 23:30 UTC` = **1 March 00:30 in Europe/Paris**, so 1 250.00 fell into February. The function was also a stub returning `0`, lacked its `tenant_id` param, and was never called. | Month boundaries are evaluated in the property's own timezone: `check_in_date AT TIME ZONE properties.timezone` compared to half-open local bounds. `month`/`year` query params were wired into `/dashboard/summary` and a period picker added to the UI. |
+| 4 | Finance sees totals "off by a few cents" | `dashboard.py` did `float(total)` on a `NUMERIC(10,3)` sum, and the UI re-rounded with `Math.round(x*100)/100`. Sub-cent rows (`333.333 + 333.333 + 333.334`) round to `999.99` if rounded per row or through float, vs the correct `1000.00`. | Sum in SQL (exact), round **once** on the total with `Decimal.quantize(0.01, ROUND_HALF_UP)`, serialise as a string. UI formats the string; no float arithmetic anywhere. |
+| 5 | Ocean sees Sunset's property names | `Dashboard.tsx` hard-coded all five properties for every tenant. `RevenueSummary.tsx` also rendered a hard-coded "▲ 12%" trend badge (commented in code as fake) next to the revenue figure. | New tenant-scoped `GET /api/v1/dashboard/properties`; the selector is populated from it. The fabricated trend badge is removed — a finance dashboard must not show made-up numbers. |
+| 6 | (security hardening found while investigating) | `dashboard.py` fell back to `"default_tenant"` when the user had no tenant; `TenantResolver` defaulted *unknown* users to `tenant-a`; the UI sent an `X-Simulated-Tenant: candidate` header; the summary endpoint didn't check the property belongs to the caller. | Missing tenant → **403**. Resolver reads the signed JWT claim (`app_metadata.tenant_id`), then a known-account map, else `None`. Header removed. Unknown property for this tenant → **404** (no cross-tenant probing). |
+
+## Expected values after the fix (seed data)
+
+| Login | Property | All time | March 2024 (property tz) |
+|---|---|---|---|
+| Sunset (tenant-a) | prop-001 Beach House Alpha (Paris) | **2250.00 / 4** | **2250.00 / 4** (UTC logic would say 1000.00 / 3) |
+| Sunset | prop-002 / prop-003 | 4975.50 / 4, 6100.50 / 2 | same |
+| Ocean (tenant-b) | prop-001 Mountain Lodge Beta (New York) | **0.00 / 0** (was showing Sunset's 1000.00 / 3) | 0.00 / 0 |
+| Ocean | prop-004 / prop-005 | 1776.50 / 4, 3256.00 / 3 | same |
+
+## How to verify
+
+```bash
+docker compose up --build
+# UI: http://localhost:3000  (log in as each client, compare prop-001)
+# API: http://localhost:8000/docs
+
+# Unit tests (pure functions, no DB needed)
+cd backend && python -m pytest -q
+
+# Cache keys are now tenant-scoped
+docker compose exec redis redis-cli KEYS 'revenue:*'
+```
+
+## Left as follow-ups (out of scope for a debugging pass)
+
+- `schema.sql` enables RLS on `properties`/`reservations` but defines no policies; the app connects as superuser so RLS is a no-op. Real isolation should add policies keyed on a `current_tenant_id()` setting and connect as a non-superuser role.
+- `SECRET_KEY` and DB credentials live in `docker-compose.yml`; move to env/secrets.
+- The in-process auth cache keys on a 16-hex token hash for 30 min; fine for now, but token revocation needs the Redis pub/sub path.
+- The frontend `SecureAPI` request cache looks for `tenant_id` in `user_metadata` and requires a UUID, while our JWT carries it in `app_metadata` as `tenant-a`; it therefore never matches and client-side caching is silently disabled (safe, but worth aligning).
+- A tenant-aware cache helper that *requires* `tenant_id` (as `revenue_cache_key` now does) should be the only way to touch Redis, so bug #1 cannot be reintroduced.

backend/app/api/v1/dashboard.py

@@ -1,25 +1,74 @@
-from fastapi import APIRouter, Depends, HTTPException
-from typing import Dict, Any
-from app.services.cache import get_revenue_summary
+import logging
+from typing import Dict, Any, Optional, List
+
+from fastapi import APIRouter, Depends, HTTPException, Query, status
+
 from app.core.auth import authenticate_request as get_current_user
+from app.models.auth import AuthenticatedUser
+from app.services.cache import get_revenue_summary
+from app.services.reservations import list_properties
 
+logger = logging.getLogger(__name__)
 router = APIRouter()
 
+
+def _require_tenant(current_user: AuthenticatedUser) -> str:
+    """
+    BUG FIX: the endpoint used to fall back to "default_tenant" when the user had no
+    tenant. A missing tenant is an authorization failure, never a shared bucket.
+    """
+    tenant_id = current_user.tenant_id
+    if not tenant_id:
+        logger.warning(f"Dashboard access denied - no tenant for user {current_user.email}")
+        raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="No tenant associated with this account")
+    return tenant_id
+
+
+@router.get("/dashboard/properties")
+async def get_dashboard_properties(
+    current_user: AuthenticatedUser = Depends(get_current_user),
+) -> List[Dict[str, Any]]:
+    """Properties belonging to the caller's tenant (drives the dashboard selector)."""
+    tenant_id = _require_tenant(current_user)
+    try:
+        return await list_properties(tenant_id)
+    except Exception as e:
+        logger.exception(f"Failed to list properties for tenant {tenant_id}: {e}")
+        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Property data temporarily unavailable")
+
+
 @router.get("/dashboard/summary")
 async def get_dashboard_summary(
     property_id: str,
-    current_user: dict = Depends(get_current_user)
+    month: Optional[int] = Query(None, ge=1, le=12, description="Calendar month (in the property's timezone)"),
+    year: Optional[int] = Query(None, ge=2000, le=2100),
+    current_user: AuthenticatedUser = Depends(get_current_user),
 ) -> Dict[str, Any]:
-
-    tenant_id = getattr(current_user, "tenant_id", "default_tenant") or "default_tenant"
-
-    revenue_data = await get_revenue_summary(property_id, tenant_id)
-
-    total_revenue_float = float(revenue_data['total'])
-
+    tenant_id = _require_tenant(current_user)
+
+    if (month is None) != (year is None):
+        raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="month and year must be provided together")
+
+    try:
+        revenue_data = await get_revenue_summary(property_id, tenant_id, month=month, year=year)
+    except Exception as e:
+        # BUG FIX: the service used to swallow DB errors and return hard-coded mock
+        # numbers (shared across tenants). Financial data must fail loudly instead.
+        logger.exception(f"Revenue lookup failed for tenant={tenant_id} property={property_id}: {e}")
+        raise HTTPException(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, detail="Revenue data temporarily unavailable")
+
+    if revenue_data is None:
+        # Property does not exist *for this tenant* - do not reveal whether it exists elsewhere.
+        raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Property not found")
+
+    # BUG FIX: total was converted with float() here, which loses cents on NUMERIC values.
+    # The service already rounded once (ROUND_HALF_UP); pass it through as a string.
     return {
-        "property_id": revenue_data['property_id'],
-        "total_revenue": total_revenue_float,
-        "currency": revenue_data['currency'],
-        "reservations_count": revenue_data['count']
+        "property_id": revenue_data["property_id"],
+        "property_name": revenue_data.get("property_name"),
+        "timezone": revenue_data.get("timezone"),
+        "total_revenue": revenue_data["total"],
+        "currency": revenue_data["currency"],
+        "reservations_count": revenue_data["count"],
+        "period": {"month": month, "year": year} if month is not None else None,
     }

backend/app/core/database_pool.py

@@ -1,60 +1,91 @@
 import asyncio
 from sqlalchemy.ext.asyncio import create_async_engine, AsyncSession, async_sessionmaker
-from sqlalchemy.pool import QueuePool
 import logging
 from ..config import settings
 
 logger = logging.getLogger(__name__)
 
+
+def _async_database_url(url: str) -> str:
+    """Normalise a DATABASE_URL into the SQLAlchemy asyncpg dialect form."""
+    if url.startswith("postgresql+asyncpg://"):
+        return url
+    for prefix in ("postgresql://", "postgres://"):
+        if url.startswith(prefix):
+            return "postgresql+asyncpg://" + url[len(prefix):]
+    return url
+
+
 class DatabasePool:
     def __init__(self):
         self.engine = None
         self.session_factory = None
-
+        self._init_lock = asyncio.Lock()
+
     async def initialize(self):
-        """Initialize database connection pool"""
-        try:
-            # Create async engine with connection pooling
-            database_url = f"postgresql+asyncpg://{settings.supabase_db_user}:{settings.supabase_db_password}@{settings.supabase_db_host}:{settings.supabase_db_port}/{settings.supabase_db_name}"
-
-            self.engine = create_async_engine(
-                database_url,
-                poolclass=QueuePool,
-                pool_size=20,  # Number of connections to maintain
-                max_overflow=30,  # Additional connections when needed
-                pool_pre_ping=True,  # Validate connections
-                pool_recycle=3600,  # Recycle connections every hour
-                echo=False  # Set to True for SQL debugging
-            )
-
-            self.session_factory = async_sessionmaker(
-                bind=self.engine,
-                class_=AsyncSession,
-                expire_on_commit=False
-            )
-
-            logger.info("✅ Database connection pool initialized")
-
-        except Exception as e:
-            logger.error(f"❌ Database pool initialization failed: {e}")
-            self.engine = None
-            self.session_factory = None
-
+        """Initialize database connection pool (idempotent, safe under concurrency)."""
+        if self.session_factory:
+            return
+        async with self._init_lock:
+            if self.session_factory:
+                return
+            try:
+                # BUG FIX: previously built the DSN from settings.supabase_db_* fields that
+                # do not exist on Settings -> AttributeError on every request -> the revenue
+                # service silently fell back to hard-coded mock data. Use settings.database_url.
+                database_url = _async_database_url(settings.database_url)
+
+                # BUG FIX: QueuePool is not valid for async engines; use SQLAlchemy's default
+                # AsyncAdaptedQueuePool by not passing poolclass.
+                self.engine = create_async_engine(
+                    database_url,
+                    pool_size=settings.database_pool_size,
+                    max_overflow=settings.database_max_overflow,
+                    pool_timeout=settings.database_pool_timeout,
+                    pool_pre_ping=True,
+                    pool_recycle=settings.database_pool_recycle,
+                    echo=False,
+                )
+
+                self.session_factory = async_sessionmaker(
+                    bind=self.engine,
+                    class_=AsyncSession,
+                    expire_on_commit=False,
+                )
+
+                logger.info("✅ Database connection pool initialized")
+
+            except Exception as e:
+                logger.error(f"❌ Database pool initialization failed: {e}")
+                self.engine = None
+                self.session_factory = None
+                raise
+
     async def close(self):
         """Close database connections"""
         if self.engine:
             await self.engine.dispose()
-
-    async def get_session(self) -> AsyncSession:
-        """Get database session from pool"""
+            self.engine = None
+            self.session_factory = None
+
+    def get_session(self) -> AsyncSession:
+        """Get database session from pool.
+
+        BUG FIX: this was `async def`, so callers doing `async with db_pool.get_session()`
+        received a coroutine (no __aenter__) and raised TypeError. It must be a plain
+        method returning the AsyncSession, which is itself an async context manager.
+        """
         if not self.session_factory:
-            raise Exception("Database pool not initialized")
+            raise RuntimeError("Database pool not initialized")
         return self.session_factory()
 
-# Global database pool instance
+
+# Global database pool instance - reuse this, never construct a new pool per request.
 db_pool = DatabasePool()
 
+
 async def get_db_session() -> AsyncSession:
     """Dependency to get database session"""
+    await db_pool.initialize()
     async with db_pool.get_session() as session:
         yield session

backend/app/core/tenant_resolver.py

@@ -4,6 +4,10 @@
 from typing import Optional
 import logging
 
+from jose import jwt, JWTError
+
+from ..config import settings
+
 logger = logging.getLogger(__name__)
 
 
@@ -21,17 +25,17 @@ def resolve_tenant_from_token(token_payload: dict) -> Optional[str]:
         Returns:
             Tenant ID if found, None otherwise
         """
-        # Try user_metadata first (most common location)
-        if 'user_metadata' in token_payload:
-            tenant_id = token_payload['user_metadata'].get('tenant_id')
-            if tenant_id:
-                return tenant_id
+        # Try app_metadata first (server-controlled claims; this is where login.py puts it)
+        app_metadata = token_payload.get('app_metadata') or {}
+        tenant_id = app_metadata.get('tenant_id')
+        if tenant_id:
+            return tenant_id
 
-        # Try app_metadata as fallback
-        if 'app_metadata' in token_payload:
-            tenant_id = token_payload['app_metadata'].get('tenant_id')
-            if tenant_id:
-                return tenant_id
+        # Try user_metadata as fallback
+        user_metadata = token_payload.get('user_metadata') or {}
+        tenant_id = user_metadata.get('tenant_id')
+        if tenant_id:
+            return tenant_id
 
         # Try root level
         tenant_id = token_payload.get('tenant_id')
@@ -68,34 +72,46 @@ def resolve_tenant_from_user(user_data: dict) -> Optional[str]:
 
         return None
 
+    # Known challenge accounts. Used only as a fallback when the token carries no claim.
+    _EMAIL_TENANT_MAP = {
+        "sunset@propertyflow.com": "tenant-a",
+        "ocean@propertyflow.com": "tenant-b",
+        "candidate@propertyflow.com": "tenant-a",
+    }
+
     @staticmethod
-    async def resolve_tenant_id(user_id: str, user_email: str, token: Optional[str] = None) -> str:
+    async def resolve_tenant_id(user_id: str, user_email: str, token: Optional[str] = None) -> Optional[str]:
         """
         Resolve tenant ID for a user.
-
-        Args:
-            user_id: User ID
-            user_email: User email
-
-        Returns:
-            Tenant ID
+
+        Order: signed JWT claim -> known account mapping -> None.
+
+        BUG FIX: this used to return "tenant-a" for *any* unknown user, so any
+        authenticated account that lacked a tenant silently became Sunset Properties.
+        Unknown tenant must resolve to None and be rejected downstream (403).
         """
-        # Fallback mapping by known user email.
-        if user_email == "sunset@propertyflow.com":
-            return "tenant-a"
-        if user_email == "ocean@propertyflow.com":
-            return "tenant-b"
-        if user_email == "candidate@propertyflow.com":
-            return "tenant-a"
-
-        # Default fallback
-        return "tenant-a"
+        if token:
+            try:
+                payload = jwt.decode(token, settings.secret_key, algorithms=["HS256"], audience="authenticated")
+                tenant_id = TenantResolver.resolve_tenant_from_token(payload)
+                if tenant_id:
+                    return tenant_id
+            except JWTError:
+                # Not one of our JWTs (e.g. a Supabase token) - fall through to the mapping.
+                pass
+
+        tenant_id = TenantResolver._EMAIL_TENANT_MAP.get((user_email or "").lower())
+        if tenant_id:
+            return tenant_id
+
+        logger.warning(f"Could not resolve tenant for user {user_email} ({user_id})")
+        return None
 
     @staticmethod
     async def update_user_tenant_metadata(user_id: str, tenant_id: str) -> None:
         """
         Update user metadata with tenant_id.
-        
+
         Args:
             user_id: User ID
             tenant_id: Tenant ID