fix(daemons): block owner transfer while assigned

This commit is contained in:
2026-08-15 13:20:02 +09:00
parent e2102b6f2b
commit e90519ad39
9 changed files with 284 additions and 16 deletions
+3
View File
@@ -8,6 +8,7 @@ from fastapi import APIRouter, Depends, HTTPException, Response, status
from app.core.auth import AuthContext, require_admin
from app.core.config import Settings, get_settings
from app.domain.errors import DaemonInUseError
from app.repositories.factory import get_portfolio_repository_from_settings
from app.schemas.v1 import (
DaemonAdminResponse,
@@ -38,6 +39,8 @@ def _to_response(daemon) -> DaemonAdminResponse:
def _daemon_error_to_http(exc: ValueError) -> HTTPException:
detail = str(exc)
if isinstance(exc, DaemonInUseError):
return HTTPException(status_code=status.HTTP_409_CONFLICT, detail=detail)
if "찾을 수 없습니다" in detail:
return HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=detail)
if "이미 존재하는 daemon slug" in detail:
+4
View File
@@ -15,3 +15,7 @@ class DaemonNotFoundError(ValueError):
class DaemonInactiveError(ValueError):
"""Raised when a new portfolio assignment targets an inactive daemon."""
class DaemonInUseError(ValueError):
"""Raised when an owner transfer targets a daemon assigned to a portfolio."""
+12 -9
View File
@@ -13,7 +13,7 @@ from typing import Any
import psycopg
from psycopg import sql
from app.domain.errors import DaemonInactiveError, DaemonNotFoundError, StrategyInactiveError, StrategyNotFoundError
from app.domain.errors import DaemonInactiveError, DaemonInUseError, DaemonNotFoundError, StrategyInactiveError, StrategyNotFoundError
from app.domain.models import Asset, AuthToken, BrokerCredential, Daemon, LedgerEntry, Portfolio, PortfolioComment, PortfolioTag, StrategySet, Tag, TokenPrincipal, User
logger = logging.getLogger(__name__)
@@ -2843,13 +2843,10 @@ class PostgresPortfolioRepository:
transaction is rolled back, preventing the state where the owner has changed but
the previous owner's credentials remain (credential leak — dcy-011 OQ-3).
Execution order inside the transaction:
1. DELETE FROM broker_credentials WHERE daemon_id = %s (credential first)
2. UPDATE daemons SET owner_user_id = %s ... (owner second)
This order is also safe if the transaction machinery is somehow bypassed: if
only step 1 completes the owner is unchanged and credentials are gone (no leak);
if only step 2 completes the old credentials are still present but the caller
still owns them (no cross-owner leak).
The daemon row is locked FOR UPDATE before checking portfolio references.
Portfolio assignment takes the same daemon row FOR SHARE before writing, so
either an earlier assignment makes this transfer fail or an earlier transfer
makes the pending assignment fail its same-owner validation.
"""
self.ensure_schema()
conn = self._connect()
@@ -2858,11 +2855,17 @@ class PostgresPortfolioRepository:
with conn.transaction():
with conn.cursor() as cur:
cur.execute(
"SELECT id FROM daemons WHERE id = %s",
"SELECT id FROM daemons WHERE id = %s FOR UPDATE",
(daemon_id,),
)
if cur.fetchone() is None:
raise ValueError("데몬을 찾을 수 없습니다")
cur.execute(
"SELECT 1 FROM portfolios WHERE managed_by_daemon_id = %s LIMIT 1",
(daemon_id,),
)
if cur.fetchone() is not None:
raise DaemonInUseError("배정된 포트폴리오가 있는 daemon의 소유자는 변경할 수 없습니다")
# Step 1: delete credentials first (safe-order)
cur.execute(
"DELETE FROM broker_credentials WHERE daemon_id = %s",
+3 -1
View File
@@ -167,6 +167,7 @@
- `status="disabled"` (soft delete): `managed_by_daemon_id` is set to NULL; portfolio is excluded from `GET /v1/portfolios` by default. Dependent data (`portfolio_comments`, `tag_ids`) is preserved — never deleted on soft delete. READ operations on sub-resources (GET tags, GET comments) remain allowed; write mutations raise HTTP 409.
- Status restoration: PATCH with `{"status": "enabled"}` re-activates a disabled portfolio; no other fields may be set in the same request.
- Daemon assignment: a non-null `managed_by_daemon_id` on create or PATCH must reference an existing, active daemon owned by the target portfolio owner (admin `user_id` override uses the final target owner). The daemon `FOR SHARE` row lock and the portfolio INSERT/UPDATE share one transaction, so validation and write form a single atomic decision boundary; a concurrently deactivated daemon is rejected (HTTP 409) rather than committed. Deactivation after an assignment commits preserves the portfolio reference and follows the existing token-revocation contract.
- Daemon owner transfer: `update_daemon_owner()` locks the daemon row `FOR UPDATE`, rejects a referenced daemon with a dedicated conflict, and deletes broker credentials only after the no-reference check. Assignment and owner transfer therefore serialize on the same daemon row: transfer-first makes the old owner's pending assignment foreign, while assignment-first makes the transfer conflict. Neither path auto-unassigns, migrates, or reassigns portfolios.
## Entity: `User`
@@ -257,6 +258,7 @@
- A daemon must exist and be active before a service token can be bound to it at issuance.
- A daemon must exist, be active, and be owned by the target portfolio owner before a portfolio can be assigned to it (create or PATCH). Assignment validation locks the daemon row `FOR SHARE` inside the portfolio write transaction (Issue #27 / dcy-0004).
- Owner transfer locks the daemon row `FOR UPDATE` and is rejected when any portfolio references the daemon. The rejection preserves the daemon owner, all portfolio assignments, and broker credentials; an unreferenced transfer retains the existing credential invalidation policy.
- Deactivating a daemon (is_active=false) immediately revokes all bound service tokens via `revoke_tokens_by_daemon_id` (REQ-022/BAT-007) and blocks new token issuance. It does not auto-remove existing portfolio references; deactivation racing a new assignment is rejected with HTTP 409 when the deactivation commits first.
- daemon_slug is immutable after creation; only display_name and is_active can be updated.
@@ -383,4 +385,4 @@
- 2026-05-13 KST: bat-010 — REQ-009/023/028: added Entity: Daemon; added portfolios.managed_by_daemon_id; added auth_tokens.daemon_id; updated AuthToken State Rules (runtime_id vs daemon_id distinction, fail-closed policy); updated Portfolio Relationships; updated Model Summary.
- 2026-05-11 KST: bat-008 — REQ-011 AC-2: added portfolios.market column; added Entity: AuthToken (runtime_id field); added Entity: BrokerCredential; added Entity: LedgerTrade; updated Model Summary.
- 2026-05-11 KST: bat-006 — portfolios.portfolio_id changed to server-generated UUID PK; name column added with UNIQUE (owner_user_id, name) constraint; uq_portfolios_owner_name constraint documented; state rules updated.
- 2026-04-30 KST: Bootstrap baseline created from repository observation for initial StagePilot adoption.
- 2026-04-30 KST: Bootstrap baseline created from repository observation for initial StagePilot adoption.
+2 -1
View File
@@ -126,6 +126,7 @@
- Each asset (portfolio) may be assigned to exactly one daemon via `portfolios.managed_by_daemon_id`. Mismatch or unassigned asset → HTTP 403 on ledger writes.
- **Token rebind** (BAT-007/REQ-023): `pfhub service-token rebind <name> --daemon-id <id>` — daemon_id 변경, runtime_id 불변. 대상 daemon은 active이어야 하고 token 소유 user와 일치해야 한다.
- **Daemon deactivation** (BAT-007/REQ-022): `pfhub daemon update <daemon_id> --deactivate --force` — daemon is_active=false + 바인딩된 모든 service token 즉시 revoke.
- **Admin daemon owner transfer**: `PATCH /v1/admin/daemons/{daemon_id}` with a changed `owner_user_id` is allowed only for an unassigned daemon. A referenced daemon returns HTTP 409; no daemon owner, portfolio assignment, or broker credential is changed. Owner transfer locks the daemon row `FOR UPDATE`, while portfolio assignment locks it `FOR SHARE`, so a concurrent operation either transfers first and rejects the old owner's assignment as HTTP 404 or assigns first and rejects the transfer as HTTP 409.
- **NULL daemon migration CLI** (BAT-007/REQ-023): `pfhub service-token migrate-null-daemon --dry-run` — NULL daemon_id active service token inventory 출력 + user별 candidate daemon 목록. `--dry-run` 없이 실행 시 exit code 1 + 수동 안내.
### Endpoint: `GET /v1/assets`
@@ -570,4 +571,4 @@
- 2026-05-13 KST: bat-010 — REQ-006/007/008/011/023: added daemon authorization model section; updated GET /v1/assets (daemon-scoped filter, NULL 403); updated GET /v1/user_info (daemon_id field, NULL runtime_id → 200+null diagnostic); updated POST /v1/assets/{asset_id}/ledger/trades (daemon assignment authorization, fail-closed policy); added daemon CLI subcommand to CLI surface; updated v1 purpose and source discovery.
- 2026-05-11 KST: bat-008 — REQ-011 AC-1: expanded v1 Consumer API section with authentication contract, detailed request/response schemas for GET /v1/assets, GET /v1/user_info, POST /v1/assets/{asset_id}/ledger/trades; updated GET /health response schema (REQ-004); added idempotency contract.
- 2026-05-11 KST: bat-006 — Added v1 consumer API interface section (trex-basic); noted asset_id UUID change and simultaneous deployment constraint; updated CLI portfolio inputs to reflect name+user identification.
- 2026-04-30 KST: Bootstrap baseline created from repository observation for initial StagePilot adoption.
- 2026-04-30 KST: Bootstrap baseline created from repository observation for initial StagePilot adoption.
+10 -1
View File
@@ -325,6 +325,15 @@
4. `repo.revoke_tokens_by_daemon_id(daemon_id)` — 바인딩된 모든 active service token에 `revoked_at=NOW()` 설정.
5. Console에 revoked count 출력.
## Flow: `PATCH /v1/admin/daemons/{daemon_id}` — Owner Transfer
1. Admin authentication is required; an unknown daemon returns HTTP 404.
2. For an effective `owner_user_id` change, `update_daemon_owner()` starts one transaction and locks the daemon row `FOR UPDATE`.
3. It checks `portfolios.managed_by_daemon_id` while holding that lock.
- A reference exists: raise the dedicated conflict and return HTTP 409. Daemon owner, portfolio assignment, and broker credentials remain unchanged.
- No reference exists: delete credentials, update the daemon owner, and commit the existing atomic credential-invalidation flow.
4. Portfolio assignment uses `FOR SHARE` on the same daemon row before INSERT/UPDATE. Thus transfer-first makes the old owner's pending assignment fail as foreign; assignment-first makes the transfer fail HTTP 409. No automatic unassignment, portfolio owner migration, or alternate-daemon reassignment occurs.
## Flow: `Service-Token Rotate — daemon_id Invariant`
1. Operator calls `pfhub service-token rotate <name> --force`.
@@ -492,4 +501,4 @@
- 2026-05-11 KST: bat-007 — Added "Local DB Initialization and Validation" flow (Scenario A/B, repeatable run criteria); added "Alembic vs ensure_schema() Schema Ownership Boundary" flow.
- 2026-05-11 KST: bat-006 — Added CLI portfolio name+user resolution flow; updated GET /v1/assets flow to note asset_id is UUID after BAT-006 migration.
- 2026-05-01 KST: bat-001 — Added v1 API flows (service-token auth, GET /v1/assets, GET /v1/user_info, POST ledger/trades, POST /auth/service-tokens), startup validation flow.
- 2026-04-30 KST: Bootstrap baseline created from repository observation for initial StagePilot adoption.
- 2026-04-30 KST: Bootstrap baseline created from repository observation for initial StagePilot adoption.
+33 -1
View File
@@ -9,6 +9,7 @@ import pytest
from fastapi.testclient import TestClient
from app.core.config import Settings, get_settings
from app.domain.errors import DaemonInUseError
from app.domain.models import BrokerCredential, Daemon, TokenPrincipal, User
from app.main import create_app
from app.repositories.postgres import PostgresPortfolioRepository
@@ -212,6 +213,33 @@ def test_admin_patch_owner_invalidates_credentials() -> None:
mock_repo.update_daemon_owner.assert_called_once_with(DAEMON_ID, owner_user_id=OWNER_B_ID)
def test_admin_patch_referenced_daemon_owner_returns_409() -> None:
"""Issue #27: assigned daemon owner transfer fails without a partial mutation."""
settings = _make_settings()
principal = _make_admin_principal()
daemon_before = _make_daemon(owner_user_id=OWNER_A_ID)
mock_repo = MagicMock()
mock_repo.get_token_principal.return_value = principal
mock_repo.get_daemon_by_id.return_value = daemon_before
mock_repo.update_daemon_owner.side_effect = DaemonInUseError(
"배정된 포트폴리오가 있는 daemon의 소유자는 변경할 수 없습니다"
)
with patch("app.core.auth.get_portfolio_repository_from_settings", return_value=mock_repo), \
patch("app.api.v1_admin_daemons.get_portfolio_repository_from_settings", return_value=mock_repo):
client = _make_client(settings, mock_repo)
resp = client.patch(
f"/v1/admin/daemons/{DAEMON_ID}",
json={"owner_user_id": OWNER_B_ID},
headers=_auth_header(),
)
assert resp.status_code == 409
mock_repo.update_daemon_owner.assert_called_once_with(DAEMON_ID, owner_user_id=OWNER_B_ID)
mock_repo.update_daemon.assert_not_called()
# ---------------------------------------------------------------------------
# REQ-053 AC-5: owner 변경 후 이전 owner의 /v1/user_info → broker=null
# (verified via update_daemon_owner deleting credentials; AC-4 covers the mechanism)
@@ -489,7 +517,9 @@ def _make_mock_conn_for_owner_update(*, fail_on_update: bool = False):
def execute(self, sql: str, params=None) -> None:
sql_stripped = sql.strip()
executed_sqls.append(sql_stripped)
if sql_stripped.startswith("SELECT"):
if "FROM portfolios" in sql_stripped:
self._rows = []
elif sql_stripped.startswith("SELECT"):
# daemon exists
self._rows = [("daemon-001",)]
elif sql_stripped.startswith("DELETE"):
@@ -717,6 +747,8 @@ def test_update_daemon_owner_both_ops_in_single_transaction() -> None:
def fetchone(self):
sql = executed_sqls[-1] if executed_sqls else ""
if "FROM portfolios" in sql:
return None
if sql.startswith("SELECT"):
return ("daemon-001",)
if sql.startswith("UPDATE"):
+15 -1
View File
@@ -402,6 +402,20 @@ def test_patch_foreign_daemon_returns_404() -> None:
assert resp.status_code == 404
def test_patch_missing_daemon_returns_404_without_changing_portfolio() -> None:
repo = _repo_with_portfolio_and_daemons(owner=_USER_ID)
client = _make_client()
with _with_repo(repo):
resp = client.patch(
f"/v1/portfolios/{_PORTFOLIO_UUID}",
json={"managed_by_daemon_id": "ffffffff-ffff-ffff-ffff-ffffffffffff"},
headers=_auth("admin-tok"),
)
assert resp.status_code == 404
assert repo.get(_PORTFOLIO_UUID).managed_by_daemon_id is None
def test_patch_inactive_daemon_returns_409() -> None:
repo = _repo_with_portfolio_and_daemons(owner=_USER_ID, is_active=False)
client = _make_client()
@@ -412,4 +426,4 @@ def test_patch_inactive_daemon_returns_409() -> None:
json={"managed_by_daemon_id": _DAEMON_UUID},
headers=_auth("admin-tok"),
)
assert resp.status_code == 409
assert resp.status_code == 409
@@ -16,7 +16,7 @@ import pytest
from fastapi.testclient import TestClient
from app.core.config import Settings, get_settings
from app.domain.errors import DaemonInactiveError, DaemonNotFoundError
from app.domain.errors import DaemonInactiveError, DaemonInUseError, DaemonNotFoundError
from app.main import create_app
from app.repositories.postgres import PortfolioDBConfig, PostgresPortfolioRepository
@@ -79,6 +79,7 @@ def _cleanup(
asset_id: str,
owner_user_id: str,
daemon_id: str,
extra_user_ids: list[str] | None = None,
) -> None:
with repo._connect().cursor() as cur:
for portfolio_id in portfolio_ids:
@@ -89,6 +90,8 @@ def _cleanup(
cur.execute("DELETE FROM strategy_sets WHERE id = %s", (strategy_id,))
cur.execute("DELETE FROM assets WHERE asset_id = %s", (asset_id,))
cur.execute("DELETE FROM users WHERE id = %s", (owner_user_id,))
for user_id in extra_user_ids or []:
cur.execute("DELETE FROM users WHERE id = %s", (user_id,))
repo.close()
@@ -322,4 +325,201 @@ def test_patch_reassignment_persists_new_daemon() -> None:
cur.execute("DELETE FROM daemons WHERE id = %s", (daemon_b.id,))
_cleanup(repo, [], strategy_id, asset_id, owner_user_id, daemon_a)
finally:
pass
pass
def test_referenced_daemon_owner_transfer_is_atomic() -> None:
repo = _repo()
strategy_id, asset_id, owner_a, daemon_id = _prepare_daemon_fixture(repo)
owner_b = f"daemon-owner-{uuid.uuid4().hex}"
portfolio_id = str(uuid.uuid4())
try:
with repo._connect().cursor() as cur:
cur.execute(
"INSERT INTO users (id, username, permission, is_active) VALUES (%s, %s, 'trade-write', TRUE)",
(owner_b, owner_b),
)
repo.upsert_broker_credential(
owner_a,
"kis",
account_ref="owner-a-account",
encrypted_payload="ciphertext",
daemon_id=daemon_id,
)
repo.create_portfolio(
portfolio_id,
owner_user_id=owner_a,
broker="kis",
status="enabled",
name="owner-transfer-blocked",
asset_id=asset_id,
cash_balance=0,
asset_quantity=0,
asset_price=0,
profit_buffer=0,
active_strategy_id=strategy_id,
avabile_strategies=[strategy_id],
managed_by_daemon_id=daemon_id,
)
with pytest.raises(DaemonInUseError):
repo.update_daemon_owner(daemon_id, owner_user_id=owner_b)
assert repo.get_daemon_by_id(daemon_id).owner_user_id == owner_a
assert repo.get(portfolio_id).managed_by_daemon_id == daemon_id
credential = repo.get_broker_credential_by_daemon(daemon_id, "kis")
assert credential is not None
assert credential.user_id == owner_a
_cleanup(repo, [portfolio_id], strategy_id, asset_id, owner_a, daemon_id, [owner_b])
finally:
pass
def test_unreferenced_daemon_owner_transfer_invalidates_credentials() -> None:
repo = _repo()
strategy_id, asset_id, owner_a, daemon_id = _prepare_daemon_fixture(repo)
owner_b = f"daemon-owner-{uuid.uuid4().hex}"
try:
with repo._connect().cursor() as cur:
cur.execute(
"INSERT INTO users (id, username, permission, is_active) VALUES (%s, %s, 'trade-write', TRUE)",
(owner_b, owner_b),
)
repo.upsert_broker_credential(
owner_a,
"kis",
account_ref="owner-a-account",
encrypted_payload="ciphertext",
daemon_id=daemon_id,
)
moved = repo.update_daemon_owner(daemon_id, owner_user_id=owner_b)
assert moved.owner_user_id == owner_b
assert repo.get_broker_credential_by_daemon(daemon_id, "kis") is None
_cleanup(repo, [], strategy_id, asset_id, owner_a, daemon_id, [owner_b])
finally:
pass
def test_assignment_and_owner_transfer_race_preserves_same_owner_invariant() -> None:
repo = _repo()
strategy_id, asset_id, owner_a, daemon_id = _prepare_daemon_fixture(repo)
owner_b = f"daemon-owner-{uuid.uuid4().hex}"
portfolio_id = str(uuid.uuid4())
barrier = threading.Barrier(3)
outcomes: list[str] = []
errors: list[BaseException] = []
with repo._connect().cursor() as cur:
cur.execute(
"INSERT INTO users (id, username, permission, is_active) VALUES (%s, %s, 'trade-write', TRUE)",
(owner_b, owner_b),
)
def assign_daemon() -> None:
worker = _repo()
try:
barrier.wait()
worker.create_portfolio(
portfolio_id,
owner_user_id=owner_a,
broker="kis",
status="enabled",
name="owner-transfer-race",
asset_id=asset_id,
cash_balance=0,
asset_quantity=0,
asset_price=0,
profit_buffer=0,
active_strategy_id=strategy_id,
avabile_strategies=[strategy_id],
managed_by_daemon_id=daemon_id,
)
outcomes.append("assigned")
except DaemonNotFoundError:
outcomes.append("assignment-rejected")
except BaseException as exc: # pragma: no cover - populated only on failure
errors.append(exc)
finally:
worker.close()
def transfer_owner() -> None:
worker = _repo()
try:
barrier.wait()
worker.update_daemon_owner(daemon_id, owner_user_id=owner_b)
outcomes.append("transferred")
except DaemonInUseError:
outcomes.append("transfer-rejected")
except BaseException as exc: # pragma: no cover - populated only on failure
errors.append(exc)
finally:
worker.close()
threads = [threading.Thread(target=assign_daemon), threading.Thread(target=transfer_owner)]
for thread in threads:
thread.start()
barrier.wait()
for thread in threads:
thread.join(timeout=10)
assert not thread.is_alive()
assert errors == []
assert sorted(outcomes) in (
["assigned", "transfer-rejected"],
["assignment-rejected", "transferred"],
)
daemon = repo.get_daemon_by_id(daemon_id)
portfolio = repo.get(portfolio_id) if "assigned" in outcomes else None
if portfolio is not None:
assert daemon.owner_user_id == portfolio.owner_user_id == owner_a
assert portfolio.managed_by_daemon_id == daemon_id
else:
assert daemon.owner_user_id == owner_b
_cleanup(repo, [portfolio_id], strategy_id, asset_id, owner_a, daemon_id, [owner_b])
def test_deactivation_preserves_assignment_and_revokes_bound_service_token() -> None:
repo = _repo()
strategy_id, asset_id, owner_user_id, daemon_id = _prepare_daemon_fixture(repo)
portfolio_id = str(uuid.uuid4())
try:
repo.create_portfolio(
portfolio_id,
owner_user_id=owner_user_id,
broker="kis",
status="enabled",
name="deactivation-preserves-reference",
asset_id=asset_id,
cash_balance=0,
asset_quantity=0,
asset_price=0,
profit_buffer=0,
active_strategy_id=strategy_id,
avabile_strategies=[strategy_id],
managed_by_daemon_id=daemon_id,
)
token = repo.create_auth_token(
owner_user_id,
name=f"daemon-token-{uuid.uuid4().hex}",
token_hash=uuid.uuid4().hex,
token_prefix="pfh",
permission="trade-write",
token_type="service",
daemon_id=daemon_id,
)
repo.update_daemon(daemon_id, is_active=False)
assert repo.revoke_tokens_by_daemon_id(daemon_id) == 1
assert repo.get(portfolio_id).managed_by_daemon_id == daemon_id
with repo._connect().cursor() as cur:
cur.execute("SELECT revoked_at IS NOT NULL FROM auth_tokens WHERE id = %s", (token.id,))
assert cur.fetchone()[0] is True
_cleanup(repo, [portfolio_id], strategy_id, asset_id, owner_user_id, daemon_id)
finally:
pass