Skip to content

Commit ca64cf1

Browse files
Aurelioloclaude
Aureliolo
and
claude
committed
fix: address 25 PR review findings from local agents and external reviewers
Guards: add empty-roles validation, set __name__ on closures, fix require_read_access docstring to clarify SYSTEM exclusion. UserController: add asyncio.Lock for CEO uniqueness TOCTOU, move password validation before DB reads, extract validation helpers (functions now under 50 lines), add logger.warning on all error paths, check delete() return value, remove PII from logs, reorder delete checks so self-deletion is testable, use tuple instead of list return. Frontend: remove board_member from WRITE_ROLES (matches backend), add board_member exclusion assertion in constants test. Tests: parametrize role creation tests, tighten list assertion, assert first POST in duplicate test, add system user update test, add backup controller HTTP-level guard tests. Docs: add /api/v1/users to API Surface table, update Board Member and CEO role descriptions, fix escalation chain role value, update CLAUDE.md package structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 4e4cb31 commit ca64cf1

8 files changed

Lines changed: 225 additions & 125 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ curl http://localhost:3000/api/v1/health # backend (via web proxy)
103103

104104
```text
105105
src/synthorg/
106-
api/ # Litestar REST + WebSocket API, RFC 9457 errors, setup wizard, auth/, auto-wiring, lifecycle
106+
api/ # Litestar REST + WebSocket API, RFC 9457 errors, setup wizard, auth/, guards (role-based access control), user management, auto-wiring, lifecycle
107107
backup/ # Backup/restore orchestrator, scheduler, retention, handlers/
108108
budget/ # Cost tracking, budget enforcement, quota degradation, CFO optimization, trend analysis, budget forecasting
109109
cli/ # Python CLI module (superseded by top-level cli/ Go binary)

‎docs/design/operations.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -995,7 +995,7 @@ feedback arrives.
995995
timeout_minutes: 120
996996
- role: "department_head"
997997
timeout_minutes: 240
998-
- role: "ceo_or_board"
998+
- role: "ceo"
999999
timeout_minutes: 480
10001000
on_chain_exhausted: "deny" # deny if entire chain times out
10011001
```
@@ -1098,6 +1098,7 @@ future CLI tool are thin clients that call the API -- they contain no business l
10981098
| `/api/v1/settings` | Runtime-editable configuration (9 namespaces), schema discovery |
10991099
| `GET /api/v1/providers`, `POST /api/v1/providers`, `PUT /api/v1/providers/{name}`, `DELETE /api/v1/providers/{name}`, `POST /api/v1/providers/{name}/test`, `GET /api/v1/providers/presets`, `POST /api/v1/providers/from-preset`, `POST /api/v1/providers/{name}/discover-models`, `POST /api/v1/providers/probe-preset`, `GET /api/v1/providers/discovery-policy`, `POST /api/v1/providers/discovery-policy/entries`, `POST /api/v1/providers/discovery-policy/remove-entry` | Provider CRUD, connection testing, presets, preset auto-probe, model discovery, discovery SSRF allowlist management, 4 auth types (api_key, oauth, custom_header, none) |
11001100
| `GET /api/v1/setup/status`, `GET /api/v1/setup/templates`, `POST /api/v1/setup/company`, `POST /api/v1/setup/agent`, `GET /api/v1/setup/agents`, `PUT /api/v1/setup/agents/{index}/model` (`{index}` = zero-based position in the list returned by `GET /api/v1/setup/agents`; not a stable ID -- re-fetch to resolve; out-of-range returns 404), `GET /api/v1/setup/name-locales/available`, `GET /api/v1/setup/name-locales`, `PUT /api/v1/setup/name-locales`, `POST /api/v1/setup/complete` | First-run setup wizard: status check (public, reports `has_company`/`has_agents`/`has_providers`/`has_name_locales` for step resume), template listing, company creation (auto-creates template agents with model matching), agent listing + model reassignment, manual agent creation (blank path), name locale management (list available Faker locales, get/set selected locales for agent name generation), completion gate (requires company + agents + providers) |
1101+
| `/api/v1/users` | CEO-only user CRUD: create, list, get, update role, delete human user accounts |
11011102
| `/api/v1/admin/backups` | Manual backup, list, detail, delete |
11021103
| `/api/v1/ws` | WebSocket for real-time updates (ticket auth via `?ticket=`) |
11031104
| `POST /api/v1/auth/ws-ticket` | Exchange JWT for one-time WebSocket connection ticket |
@@ -1182,8 +1183,8 @@ For the full page list, navigation hierarchy, URL routing map, and WebSocket cha
11821183

11831184
| Role | Access | Description |
11841185
|------|--------|-------------|
1185-
| **Board Member** | Observe + major approvals only | Minimal involvement, strategic oversight |
1186-
| **CEO** | Full authority, replaces CEO agent | Human IS the CEO, agents are the team |
1186+
| **Board Member** | Read-only + approve/reject | Strategic oversight; can view all resources and decide on pending approvals, but cannot create or modify resources |
1187+
| **CEO** | Full authority, user management | Human IS the CEO, agents are the team. Sole authority to create, modify, and delete user accounts |
11871188
| **Manager** | Department-level authority | Manages one team/department directly |
11881189
| **Observer** | Read-only | Watch the company operate, no intervention |
11891190
| **Pair Programmer** | Direct collaboration with one agent | Work alongside a specific agent in real-time |

‎src/synthorg/api/controllers/users.py‎

Lines changed: 139 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
"""User management controller -- CEO-only CRUD for human users."""
22

3+
import asyncio
34
import uuid
45
from datetime import UTC, datetime
56
from typing import Any
@@ -19,10 +20,13 @@
1920
from synthorg.core.types import NotBlankStr # noqa: TC001
2021
from synthorg.observability import get_logger
2122
from synthorg.observability.events.api import (
23+
API_RESOURCE_CONFLICT,
24+
API_RESOURCE_NOT_FOUND,
2225
API_USER_CREATED,
2326
API_USER_DELETED,
2427
API_USER_LISTED,
2528
API_USER_UPDATED,
29+
API_VALIDATION_FAILED,
2630
)
2731

2832
logger = get_logger(__name__)
@@ -33,6 +37,9 @@
3337
# Roles that cannot be assigned via the user management API.
3438
_FORBIDDEN_ROLES: frozenset[HumanRole] = frozenset({HumanRole.SYSTEM})
3539

40+
# Serializes CEO uniqueness check-then-act sequences.
41+
_CEO_LOCK = asyncio.Lock()
42+
3643

3744
# -- Request / Response DTOs -------------------------------------------
3845

@@ -69,6 +76,7 @@ class UserResponse(BaseModel):
6976

7077

7178
def _to_response(user: User) -> UserResponse:
79+
"""Map a ``User`` domain model to the public ``UserResponse`` DTO."""
7280
return UserResponse(
7381
id=user.id,
7482
username=user.username,
@@ -79,6 +87,80 @@ def _to_response(user: User) -> UserResponse:
7987
)
8088

8189

90+
# -- Validation helpers ------------------------------------------------
91+
92+
93+
def _validate_assignable_role(role: HumanRole) -> None:
94+
"""Reject roles that cannot be assigned via the API."""
95+
if role in _FORBIDDEN_ROLES:
96+
msg = f"Cannot assign role: {role.value}"
97+
logger.warning(API_VALIDATION_FAILED, reason=msg)
98+
raise ApiValidationError(msg)
99+
100+
101+
async def _get_user_or_404(
102+
app_state: AppState,
103+
user_id: str,
104+
) -> User:
105+
"""Fetch a user by ID, raising NotFoundError if missing."""
106+
user = await app_state.persistence.users.get(user_id)
107+
if user is None:
108+
msg = f"User not found: {user_id}"
109+
logger.warning(API_RESOURCE_NOT_FOUND, reason=msg)
110+
raise NotFoundError(msg)
111+
return user
112+
113+
114+
async def _validate_ceo_create(app_state: AppState) -> None:
115+
"""Reject creation of a second CEO."""
116+
ceo_count = await app_state.persistence.users.count_by_role(
117+
HumanRole.CEO,
118+
)
119+
if ceo_count > 0:
120+
msg = "A CEO user already exists"
121+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
122+
raise ConflictError(msg)
123+
124+
125+
async def _validate_username_unique(
126+
app_state: AppState,
127+
username: str,
128+
) -> None:
129+
"""Reject duplicate usernames."""
130+
existing = await app_state.persistence.users.get_by_username(username)
131+
if existing is not None:
132+
msg = f"Username already taken: {username}"
133+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
134+
raise ConflictError(msg)
135+
136+
137+
async def _validate_role_change(
138+
app_state: AppState,
139+
user: User,
140+
new_role: HumanRole,
141+
) -> None:
142+
"""Validate CEO-related constraints on role changes."""
143+
# Prevent removing the only CEO
144+
if user.role == HumanRole.CEO and new_role != HumanRole.CEO:
145+
ceo_count = await app_state.persistence.users.count_by_role(
146+
HumanRole.CEO,
147+
)
148+
if ceo_count <= 1:
149+
msg = "Cannot change the only CEO's role"
150+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
151+
raise ConflictError(msg)
152+
153+
# Prevent creating a second CEO
154+
if new_role == HumanRole.CEO and user.role != HumanRole.CEO:
155+
ceo_count = await app_state.persistence.users.count_by_role(
156+
HumanRole.CEO,
157+
)
158+
if ceo_count > 0:
159+
msg = "A CEO user already exists"
160+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
161+
raise ConflictError(msg)
162+
163+
82164
# -- Controller --------------------------------------------------------
83165

84166

@@ -115,48 +197,37 @@ async def create_user(
115197
"""
116198
app_state: AppState = state.app_state
117199

118-
if data.role in _FORBIDDEN_ROLES:
119-
msg = f"Cannot assign role: {data.role.value}"
120-
raise ApiValidationError(msg)
121-
122-
if data.role == HumanRole.CEO:
123-
ceo_count = await app_state.persistence.users.count_by_role(
124-
HumanRole.CEO,
125-
)
126-
if ceo_count > 0:
127-
msg = "A CEO user already exists"
128-
raise ConflictError(msg)
129-
130-
existing = await app_state.persistence.users.get_by_username(
131-
data.username,
132-
)
133-
if existing is not None:
134-
msg = f"Username already taken: {data.username}"
135-
raise ConflictError(msg)
200+
_validate_assignable_role(data.role)
136201

137202
if len(data.password) < _MIN_PASSWORD_LENGTH:
138203
msg = f"Password must be at least {_MIN_PASSWORD_LENGTH} characters"
204+
logger.warning(API_VALIDATION_FAILED, reason=msg)
139205
raise ApiValidationError(msg)
140206

141-
now = datetime.now(UTC)
142-
password_hash = await app_state.auth_service.hash_password_async(
143-
data.password,
144-
)
145-
user = User(
146-
id=str(uuid.uuid4()),
147-
username=data.username,
148-
password_hash=password_hash,
149-
role=data.role,
150-
must_change_password=True,
151-
created_at=now,
152-
updated_at=now,
153-
)
154-
await app_state.persistence.users.save(user)
207+
async with _CEO_LOCK:
208+
if data.role == HumanRole.CEO:
209+
await _validate_ceo_create(app_state)
210+
211+
await _validate_username_unique(app_state, data.username)
212+
213+
now = datetime.now(UTC)
214+
password_hash = await app_state.auth_service.hash_password_async(
215+
data.password,
216+
)
217+
user = User(
218+
id=str(uuid.uuid4()),
219+
username=data.username,
220+
password_hash=password_hash,
221+
role=data.role,
222+
must_change_password=True,
223+
created_at=now,
224+
updated_at=now,
225+
)
226+
await app_state.persistence.users.save(user)
155227

156228
logger.info(
157229
API_USER_CREATED,
158230
user_id=user.id,
159-
username=user.username,
160231
role=user.role.value,
161232
)
162233
return ApiResponse(data=_to_response(user))
@@ -165,19 +236,21 @@ async def create_user(
165236
async def list_users(
166237
self,
167238
state: State,
168-
) -> ApiResponse[list[UserResponse]]:
239+
) -> ApiResponse[tuple[UserResponse, ...]]:
169240
"""List all human users (excludes system user).
170241
171242
Args:
172243
state: Application state.
173244
174245
Returns:
175-
List of user responses.
246+
Tuple of user responses.
176247
"""
177248
app_state: AppState = state.app_state
178249
users = await app_state.persistence.users.list_users()
179250
logger.debug(API_USER_LISTED, count=len(users))
180-
return ApiResponse(data=[_to_response(u) for u in users])
251+
return ApiResponse(
252+
data=tuple(_to_response(u) for u in users),
253+
)
181254

182255
@get("/{user_id:str}")
183256
async def get_user(
@@ -198,10 +271,7 @@ async def get_user(
198271
NotFoundError: If the user is not found.
199272
"""
200273
app_state: AppState = state.app_state
201-
user = await app_state.persistence.users.get(user_id)
202-
if user is None:
203-
msg = f"User not found: {user_id}"
204-
raise NotFoundError(msg)
274+
user = await _get_user_or_404(app_state, user_id)
205275
return ApiResponse(data=_to_response(user))
206276

207277
@patch("/{user_id:str}")
@@ -224,47 +294,28 @@ async def update_user_role(
224294
Raises:
225295
NotFoundError: If the user is not found.
226296
ApiValidationError: If the target role is SYSTEM.
227-
ConflictError: If changing the only CEO's role or
228-
assigning a second CEO.
297+
ConflictError: If the target user is the system user,
298+
changing the only CEO's role, or assigning a
299+
second CEO.
229300
"""
230301
app_state: AppState = state.app_state
231302

232-
if data.role in _FORBIDDEN_ROLES:
233-
msg = f"Cannot assign role: {data.role.value}"
234-
raise ApiValidationError(msg)
235-
236-
user = await app_state.persistence.users.get(user_id)
237-
if user is None:
238-
msg = f"User not found: {user_id}"
239-
raise NotFoundError(msg)
303+
_validate_assignable_role(data.role)
304+
user = await _get_user_or_404(app_state, user_id)
240305

241306
if user.role == HumanRole.SYSTEM:
242307
msg = "Cannot modify the system user"
308+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
243309
raise ConflictError(msg)
244310

245-
# Prevent removing the only CEO
246-
if user.role == HumanRole.CEO and data.role != HumanRole.CEO:
247-
ceo_count = await app_state.persistence.users.count_by_role(
248-
HumanRole.CEO,
249-
)
250-
if ceo_count <= 1:
251-
msg = "Cannot change the only CEO's role"
252-
raise ConflictError(msg)
253-
254-
# Prevent creating a second CEO
255-
if data.role == HumanRole.CEO and user.role != HumanRole.CEO:
256-
ceo_count = await app_state.persistence.users.count_by_role(
257-
HumanRole.CEO,
258-
)
259-
if ceo_count > 0:
260-
msg = "A CEO user already exists"
261-
raise ConflictError(msg)
311+
async with _CEO_LOCK:
312+
await _validate_role_change(app_state, user, data.role)
262313

263-
now = datetime.now(UTC)
264-
updated = user.model_copy(
265-
update={"role": data.role, "updated_at": now},
266-
)
267-
await app_state.persistence.users.save(updated)
314+
now = datetime.now(UTC)
315+
updated = user.model_copy(
316+
update={"role": data.role, "updated_at": now},
317+
)
318+
await app_state.persistence.users.save(updated)
268319

269320
logger.info(
270321
API_USER_UPDATED,
@@ -290,34 +341,37 @@ async def delete_user(
290341
291342
Raises:
292343
NotFoundError: If the user is not found.
293-
ConflictError: If attempting to delete the CEO or
294-
the system user.
344+
ConflictError: If attempting to delete your own account,
345+
the system user, or the CEO.
295346
"""
296347
app_state: AppState = state.app_state
297348
auth_user: AuthenticatedUser = request.scope["user"]
298349

299-
user = await app_state.persistence.users.get(user_id)
300-
if user is None:
301-
msg = f"User not found: {user_id}"
302-
raise NotFoundError(msg)
350+
user = await _get_user_or_404(app_state, user_id)
351+
352+
if user.id == auth_user.user_id:
353+
msg = "Cannot delete your own account"
354+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
355+
raise ConflictError(msg)
303356

304357
if user.role == HumanRole.SYSTEM:
305358
msg = "Cannot delete the system user"
359+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
306360
raise ConflictError(msg)
307361

308362
if user.role == HumanRole.CEO:
309363
msg = "Cannot delete the CEO user"
364+
logger.warning(API_RESOURCE_CONFLICT, reason=msg)
310365
raise ConflictError(msg)
311366

312-
if user.id == auth_user.user_id:
313-
msg = "Cannot delete your own account"
314-
raise ConflictError(msg)
315-
316-
await app_state.persistence.users.delete(user_id)
367+
deleted = await app_state.persistence.users.delete(user_id)
368+
if not deleted:
369+
msg = f"User not found: {user_id}"
370+
logger.warning(API_RESOURCE_NOT_FOUND, reason=msg)
371+
raise NotFoundError(msg)
317372

318373
logger.info(
319374
API_USER_DELETED,
320375
user_id=user.id,
321-
username=user.username,
322-
deleted_by=auth_user.username,
376+
deleted_by_user_id=auth_user.user_id,
323377
)

0 commit comments

Comments
 (0)