kamau1 commited on
Commit
e36ac68
Β·
1 Parent(s): 62c6607

feat: config admin email, enhance RBAC

Browse files
.env.example CHANGED
@@ -7,6 +7,9 @@ ENVIRONMENT=production
7
  DEBUG=False
8
  SECRET_KEY=your-secret-key-here-generate-a-strong-random-key
9
 
 
 
 
10
  # Database - Supabase PostgreSQL (REQUIRED)
11
  # Get these from your Supabase project settings
12
  DATABASE_URL=postgresql://postgres:[YOUR-PASSWORD]@db.[YOUR-PROJECT-REF].supabase.co:5432/postgres
 
7
  DEBUG=False
8
  SECRET_KEY=your-secret-key-here-generate-a-strong-random-key
9
 
10
+ # Security & Access Control
11
+ PLATFORM_ADMIN_EMAIL=your-new-email@company.com # Email for platform admin OTP verification
12
+
13
  # Database - Supabase PostgreSQL (REQUIRED)
14
  # Get these from your Supabase project settings
15
  DATABASE_URL=postgresql://postgres:[YOUR-PASSWORD]@db.[YOUR-PROJECT-REF].supabase.co:5432/postgres
docs/agent/research.md CHANGED
@@ -1,292 +1,315 @@
1
- I’ll review how user management works in this codebase (signup, auth, profile, roles, etc.), then give you a structured β€œgrade” with what’s good, what’s weak, and concrete priority fixes.
2
-
3
- First I need to scan the project (code + docs) for user/account/profile flows. I’ll set up a small todo list for this review, then search through `src` and relevant docs, and come back with findings and recommendations.
4
-
5
- I’ll scan the code and docs for anything related to users, auth, accounts, and profiles, then summarize the end‑to‑end flow and grade it with concrete fixes you should do immediately. Let me search the repo first.
6
-
7
-
8
- # High‑Level Summary: User Management Flow
9
-
10
- From the code you’ve got a **pretty complete user lifecycle**:
11
-
12
- - **Identity & Auth**
13
- - Supabase Auth is the **source of truth** for authentication (email/password, tokens).
14
- - Backend uses Supabase JWT (Bearer token) in `Authorization` header.
15
- - `get_current_user` (in `api/deps.py`) validates token via `supabase_auth.get_user`, then loads the local `User` row.
16
- - `get_current_active_user` additionally requires `user.is_active == True`.
17
-
18
- - **Account Creation**
19
-
20
- There are **two main creation paths**:
21
-
22
- 1. **Self‑Registration** (`POST /auth/register`)
23
- - Calls `supabase_auth.sign_up` with `email`, `password`, and metadata (`first_name`, `last_name`, `phone`, `full_name`).
24
- - Creates a local `User` mapped to `auth_user.id` with:
25
- - `role = "platform_admin"` (**self‑registered users become platform admins**).
26
- - `status = "invited"` (default from schema).
27
- - `is_active = True`.
28
- - Logs an audit event `register` and returns a token + basic user info.
29
-
30
- 2. **Invitation Flow** (`InvitationService.accept_invitation`)
31
- - Admin (client/contractor/platform) invites users (not fully shown, but `accept_invitation` is).
32
- - `accept_invitation`:
33
- - Validates the invitation token.
34
- - Ensures no existing user with the same email.
35
- - Calls `supabase_auth.sign_up` and creates Supabase auth user.
36
- - Creates local `User` with:
37
- - `id = auth_user.id`.
38
- - `email` from invitation.
39
- - `name = first + last`.
40
- - `phone` from acceptance or invitation.
41
- - `role = invitation.invited_role`.
42
- - `client_id` / `contractor_id` from invitation.
43
- - `status = 'active'`, `is_active = True`.
44
- - Marks invitation as accepted, commits, returns `access_token` + user details.
45
-
46
- - **Login** (`POST /auth/login`)
47
- - Calls `supabase_auth.sign_in` to validate credentials and get `access_token`.
48
- - Loads local `User` by `auth_user.id`.
49
- - If no local user β†’ `404 "User profile not found"`.
50
- - If `is_active == False` β†’ audit failed login + `403 "Account is inactive"`.
51
- - Otherwise, audit success + returns token + basic user info.
52
-
53
- - **Profile Model & Building**
54
-
55
- - `User` model (business profile) holds:
56
- - Identity & contact: `name`, `email`, `phone`, `phone_alternate`, `display_name`, `id_number`.
57
- - Org links: `client_id` or `contractor_id`.
58
- - Role & status: `role`, `status`, `is_active`.
59
- - Health & safety JSONB fields: `health_info`, `ppe_sizes`, plus emergency contacts.
60
- - Location & geo: `current_location_*`, lat/long, updated timestamp.
61
- - `additional_metadata` JSONB.
62
- - Self‑service profile API (`api/v1/profile.py`):
63
- - `GET /profile/me` returns:
64
- - `basic_info` (from `BasicProfileResponse.from_orm(current_user)`).
65
- - `health_info` (parsed from JSONB, with sensible defaults for missing fields).
66
- - `ppe_sizes`.
67
- - `location` details.
68
- - Completion status: `ProfileService.calculate_profile_completion`.
69
- - Counts: financial accounts, documents, asset assignments.
70
-
71
- - **Profile Permissions & Editing**
72
-
73
- - `ProfileService.check_edit_permission` encodes **hierarchical access**:
74
- - Self can edit own profile (subject to field‑level checks elsewhere).
75
- - `platform_admin` can edit anyone.
76
- - `client_admin`/`contractor_admin` can edit users in their org.
77
- - `project_manager` / `dispatcher` can edit `field_agent`s in shared projects.
78
- - `ProfileService.get_profile_permissions` turns this into fine‑grained booleans:
79
- - `can_edit_basic_info`, `can_edit_health_info`, `can_edit_ppe_sizes`,
80
- `can_edit_location`, `can_edit_financial_accounts`, `can_upload_documents`,
81
- `can_edit_role`, `can_edit_status`, `can_assign_assets`, and corresponding view permissions.
82
-
83
- - **Roles & Authorization**
84
-
85
- - `core/permissions.py` defines an `AppRole` enum and `ROLE_PERMISSIONS` map:
86
- - Roles: `PLATFORM_ADMIN`, `CLIENT_ADMIN`, `CONTRACTOR_ADMIN`, `PROJECT_MANAGER`, `DISPATCHER`, `FIELD_AGENT`, `SALES_AGENT`, `SALES_MANAGER`.
87
- - Each has a list of permission strings (e.g. `manage_projects`, `invite_users`, `view_team_performance`).
88
- - `has_permission(role, permission)` supports generic checks.
89
- - User update & admin actions (`UserService`):
90
- - `check_update_permission`:
91
- - `platform_admin` can change anyone.
92
- - Org admins can update users in their org but cannot change org or promote to `platform_admin`.
93
- - Self cannot use admin endpoints to change own role/status/org; must use profile APIs.
94
- - `update_user`: handles basic fields, role, status, org changes, with audit‑style `changes` tracking.
95
- - `change_user_status`: updates status + syncs `is_active` for `suspended`/`active`.
96
- - `change_user_role`: validates and applies role changes.
97
- - User admin endpoints (`api/v1/users.py` excerpts):
98
- - `POST /users/{user_id}/status` β†’ uses `UserService.change_user_status`, logs `status_change`.
99
- - `POST /users/{user_id}/role` β†’ uses `UserService.change_user_role`, logs `role_change`.
100
-
101
- - **Statuses**
102
- - Documented statuses: `invited`, `pending_setup`, `active`, `suspended`.
103
- - `change_user_status` uses `status` string + maps `suspended` and `active` to `is_active False/True`.
104
 
105
  ---
106
 
107
- # What You Did Well
108
 
109
- - **Clear separation of auth vs business profile**
110
- - Supabase handles auth (passwords, tokens).
111
- - Local `User` table holds business & authorization data.
112
- - `get_current_user` cleanly bridges the two.
113
 
114
- - **Invitation‑based onboarding is solid**
115
- - Token‑based invitations handled by `InvitationService`.
116
- - Proper token validation, duplicate user check, and creation of both auth user + local profile.
117
- - Good error handling and rollback on failure.
118
 
119
- - **Strong admin & profile permission model**
120
- - `UserService.check_update_permission` and `ProfileService.check_edit_permission` encode a realistic hierarchy:
121
- - Platform admin β†’ org admin β†’ manager/dispatcher β†’ self.
122
- - Separation between **self profile editing** and **admin updates** is good design.
123
- - Field‑level permission booleans in `ProfilePermissions` are explicit and easily testable.
124
 
125
- - **Audit logging & rate limiting**
126
- - Auth endpoints log audit events for `register` and `login` (both success and failure).
127
- - Status/role changes log with old/new values and reason.
128
- - Rate limits on `/auth/register` and `/auth/login` reduce brute force abuse.
129
 
130
- - **Role definitions & permissions table**
131
- - `AppRole` enum and `ROLE_PERMISSIONS` map give a centralized RBAC definition.
132
- - `has_permission` helper is generic and allows future route‑level enforceable checks.
133
 
134
- - **Good data modeling for field agents**
135
- - Health info and PPE sizes as JSONB with nice mapping to Pydantic response models.
136
- - Location fields + `current_location_updated_at` support live ops / dispatch use cases.
137
- - `additional_metadata` JSONB gives you flexibility without schema churn.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
138
 
139
  ---
140
 
141
- # Weaknesses / Problems
 
 
142
 
143
- ## 1. Self‑registration as `platform_admin`
 
 
 
 
144
 
145
- - `POST /auth/register` creates every self‑registered user as:
146
- - `role = "platform_admin"`
147
- - `is_active = True`
148
- - `status = "invited"` (but effectively usable)
149
- - This is **dangerous in production**. Anyone hitting `/auth/register` becomes the highest‑privilege user.
150
 
151
- **Impact**
152
- - Extremely high risk: if `/auth/register` is exposed, you’ve essentially got open platform‑admin account creation.
 
 
153
 
154
- ## 2. Status semantics are confusing / inconsistent
155
 
156
- - For self‑registration:
157
- - Status is set to `"invited"` while the user is immediately active and logged in.
158
- - For invitation acceptance:
159
- - Status is set to `"active"`.
160
- - `login` uses only `is_active`, not `status`, so:
161
- - A user with `status="invited"` but `is_active=True` can still log in.
162
- - The `pending_setup` state is referenced in docs but I don’t see it enforced in this flow.
163
 
164
- **Impact**
165
- - Harder to reason about real lifecycle states (β€œinvited but not registered”, β€œregistered but not set up profile”, etc).
166
- - Tooling and analytics relying on `status` may be incorrect.
167
 
168
- ## 3. RBAC implementation is only partially used
169
 
170
- - `core/permissions.py` defines `ROLE_PERMISSIONS` and `has_permission`, but:
171
- - I don’t see route‑level checks using `has_permission("role", "permission")`.
172
- - Instead, some endpoints hand‑code role logic (e.g., profile and user service).
173
- - This creates **duplication** and **risk of drift** between the central RBAC map and actual guards.
174
 
175
- ## 4. Suspended vs active inconsistencies at the auth provider
176
 
177
- - `change_user_status` sets local `is_active` based on `status`, but:
178
- - Supabase Auth account remains active; they can still get a valid token with correct password.
179
- - You block them at the backend (`get_current_active_user`), which is good, but:
180
- - You still incur auth load and leak some information (β€œaccount exists, but inactive”).
181
 
182
- **Impact**
183
- - Functionally safe at API level but not enforced at the auth provider.
184
- - Not necessarily a bug, but security‑grade hardening could require aligning Supabase user’s `blocked`/`app_metadata` flags.
185
 
186
- ## 5. Inconsistent or legacy fields
187
 
188
- - In `accept_invitation`, you set `activated_at=datetime.now(timezone.utc)`, yet the `User` model comments say:
189
- - `invited_at` and `activated_at` were removed and tracked in `user_invitations`.
190
- - If the DB column really is gone, this would fail at runtime; if it’s still there, comments are outdated and confusing.
191
 
192
- ## 6. Missing UX‑driven checks around profile completeness
193
 
194
- - You compute `completion_status` in `ProfileService.calculate_profile_completion`, but:
195
- - I haven’t seen it enforced anywhere (e.g., blocking certain actions until profile is complete).
196
- - Users might stay in a partially configured state while still getting full access, depending on role.
 
 
 
197
 
198
- ## 7. Supabase helper edge cases
199
 
200
- - `SupabaseAuth.sign_out` ignores the `access_token` parameter and just calls `self.client.auth.sign_out()` β€” works but:
201
- - For multi‑session scenarios, you might want to specifically revoke that token.
202
- - `update_user` sets session using the same access token as both access and refresh token (`set_session(access_token, access_token)`), which may not align perfectly with Supabase’s model.
203
- - Could be fine but is brittle if Supabase changes expectations.
204
 
205
  ---
206
 
207
- # What Needs Changing Right Away (High Priority)
208
-
209
- 1. **Stop self‑registration as platform admin**
210
- - Change `register` flow so:
211
- - Either:
212
- - Self‑registration is disabled entirely in production, and all user creation goes through invitation / admin flows.
213
- - Or: Self‑registered users get a **limited role** (e.g., `client_admin` for a new tenant, or `field_agent` for a specific app) with a safer onboarding path.
214
- - Explicitly control who can become `platform_admin` via internal processes only.
215
-
216
- 2. **Clarify and normalize `status` & `is_active`**
217
- - Decide on a consistent lifecycle like:
218
- - `invited` β†’ `pending_setup` β†’ `active` β†’ `suspended`.
219
- - Enforce:
220
- - `invited` and `pending_setup` users should either:
221
- - Not be able to log in, or
222
- - Be restricted to a setup flow only.
223
- - Ensure `register` sets `status` appropriately (likely `active` or `pending_setup`, not `invited`).
224
-
225
- 3. **Wire route‑level checks to RBAC table**
226
- - For endpoints that represent specific actions (e.g., manage users, manage projects, assign tickets):
227
- - Use `has_permission(current_user.role, "<permission_name>")` instead of hard‑coding role checks everywhere.
228
- - This will:
229
- - Keep behavior consistent with `ROLE_PERMISSIONS`.
230
- - Make it easier to adjust permissions centrally.
231
-
232
- 4. **Confirm `activated_at` / legacy fields**
233
- - Either:
234
- - Remove `activated_at=...` from `accept_invitation` if the column no longer exists.
235
- - Or: Update `User` model + comments to reflect actual DB schema.
236
- - Inconsistency here can cause subtle migration/runtime issues.
237
 
238
  ---
239
 
240
- # Medium‑Priority Improvements
 
 
 
 
 
 
 
 
 
241
 
242
- - **Supabase sync on suspension**
243
- - Optionally, when suspending a user:
244
- - Update their Supabase user (e.g., `user_metadata` or `app_metadata`) to mark them as suspended, or use any β€œblocked” mechanism available.
245
- - This prevents them from even getting a token.
246
 
247
- - **Enforce profile completeness for certain roles**
248
- - For roles like `field_agent`:
249
- - Require certain profile sections (PPE, emergency contacts, basic info) to be complete before:
250
- - Accepting field tasks.
251
- - Being assigned to risky operations.
252
- - Gate important endpoints behind `completion_status` checks.
253
 
254
- - **Lockout / abuse protection**
255
- - You have rate limiting, which is good.
256
- - Consider additional measures:
257
- - Optional account lockout after N failed logins.
258
- - Tracking login IP / device in metadata.
259
 
260
- - **Better use of `ProfilePermissions`**
261
- - Ensure all profile update endpoints (not just `GET /me`) consistently respect `ProfilePermissions` booleans to prevent accidental over‑editing by org roles.
262
 
263
  ---
264
 
265
- # Low‑Priority / Nice to Have
266
 
267
- - Use `ROLE_PERMISSIONS` not just for β€œsystem actions” but also for UI hints, so the frontend can hide/show features based on the same central map.
268
- - Add tests for:
269
- - `UserService.check_update_permission` across roles + org boundaries.
270
- - `ProfileService.get_profile_permissions` permutations (self vs admin vs manager).
271
- - Status transitions and how they affect `is_active` and login behavior.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
272
 
273
  ---
274
 
275
- # β€œGrade” of Your Work
 
 
 
 
 
 
 
 
276
 
277
- If I had to assign letter grades per area:
278
 
279
- - **Overall architecture (auth vs profile separation, use of services)**: **A**
280
- - **RBAC design (roles, permissions, hierarchy)**: **B+**
281
- - Solid structure, but not fully applied via `has_permission` yet.
282
- - **Onboarding flows (invitation, profile creation)**: **A‑**
283
- - Invitation flow is strong; self‑registration is the main issue.
284
- - **Security hardening (roles for self‑registration, status semantics, suspension)**: **C**
285
- - The platform_admin self‑registration is a serious concern and should be fixed before production exposure.
286
 
287
  ---
288
 
289
- If you’d like, next step I can propose **concrete code changes** (files + specific modifications) to:
290
- - Fix the self‑registration role.
291
- - Normalize statuses and `is_active`.
292
- - Introduce `has_permission` checks for a couple of key endpoints as a pattern.
 
 
 
 
 
 
1
+ I’ll re-grade the user management end‑to‑end with harsher standards, focusing on real risks and production readiness. I’ll structure it by lifecycle stages and call out concrete red flags, not just soft suggestions.
2
+ I’ll focus on brutal, production-grade criteria across the full lifecycle: **account creation β†’ auth β†’ profile β†’ admin/RBAC β†’ security & DX**. I’ll be nitpicky on purpose.
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
3
 
4
  ---
5
 
6
+ ## 1. Account Creation & Onboarding
7
 
8
+ ### 1.1 Platform Admin Creation (OTP-based)
 
 
 
9
 
10
+ From `auth.register`:
 
 
 
11
 
12
+ - Self-registration is **explicitly disabled for normal users**.
13
+ - `/auth/register` is a **platform-admin-only bootstrap**:
14
+ - It enforces an OTP flow via `OTPService`.
15
+ - OTP is sent to a **single hardcoded email** `lewiskimaru01@gmail.com`.
16
+ - Registration data is stored and only completed on OTP verification.
17
 
18
+ **What’s good**
 
 
 
19
 
20
+ - **Separation of concerns**: Normal users don’t self-register; admins invite them.
21
+ - **OTP requirement** for platform admin creation is a good hardening move.
22
+ - **Audit logging** of registration attempts.
23
 
24
+ **What’s bad**
25
+
26
+ - **Hardcoded email** in production auth code is a red flag.
27
+ - Not configurable via env.
28
+ - Ties platform bootstrap to your personal email. That’s fine for a solo dev, bad for a team/product.
29
+ - Single-path platform admin creation with **no rotation / delegation model**:
30
+ - What if that mailbox is compromised or lost?
31
+ - What if you need multiple platform admins in a controlled way?
32
+ - The flow is **underspecified**:
33
+ - The actual `/otp/verify-public` completion code path isn’t visible here, so it’s hard to reason about:
34
+ - How roles are finally set.
35
+ - How many platform admins can be created.
36
+ - What happens after first bootstrap.
37
+
38
+ **Grade: B-**
39
+
40
+ Great *intent* and better than most hobby projects, but the hardcoding and opacity around the verification/completion path make it non-professional for a team environment.
41
+
42
+ ---
43
+
44
+ ### 1.2 Invitation-Based User Creation
45
+
46
+ From `InvitationService.accept_invitation` + `User` model:
47
+
48
+ - You validate token, check for existing users, then:
49
+ - Call Supabase `sign_up` with metadata.
50
+ - Create local `User` linked to `auth_user.id`.
51
+ - Set `role = invitation.invited_role`, `status = 'active'`, `is_active=True`, and org IDs.
52
+ - Invitation status is updated and committed with rollback on error.
53
+
54
+ **What’s good**
55
+
56
+ - The **invitation flow is robust**:
57
+ - Token validation.
58
+ - Duplicate user check.
59
+ - Creates both auth user + local profile in a single transaction.
60
+ - Correct linking between Supabase user ID and local `User.id`.
61
+ - **Logging and rollback** are correct.
62
+
63
+ **What’s bad**
64
+
65
+ - **Status is immediately `active`** on first login. You have a `pending_setup` concept, but you’re not using it here.
66
+ - No explicit **profile completeness step** gated before considering the account fully β€œusable”.
67
+ - You **don’t make clear** what **minimum required fields** are for an β€œoperational” user (esp. for field agents).
68
+
69
+ **Grade: A- for backend correctness, C+ for lifecycle clarity**
70
+
71
+ Backend logic is solid; product/lifecycle semantics are fuzzy.
72
+
73
+ ---
74
+
75
+ ## 2. Authentication & Session Handling
76
+
77
+ ### 2.1 Login (`/auth/login`)
78
+
79
+ - Supabase handles the **password/auth**.
80
+ - You:
81
+ - Validate credentials via `supabase_auth.sign_in`.
82
+ - Fetch local `User` by `auth_user.id`.
83
+ - Enforce `user.is_active`.
84
+ - Audit both success and failure.
85
+ - Rate limit to 10/min per IP.
86
+
87
+ **What’s good**
88
+
89
+ - **Separation** of auth vs business user.
90
+ - **Audit logging** is properly used.
91
+ - **Rate limiting** is in place.
92
+
93
+ **What’s bad**
94
+
95
+ - If the **local user doesn’t exist**, you 404 `"User profile not found"`.
96
+ - This is technically correct but leaks that the Supabase account exists.
97
+ - No consistent **lockout strategy** for repeated failed logins (rate limiting helps, but doesn’t give feedback to admins).
98
+ - All auth is via email/password; no clear support for **passwordless** or **SSO** (which is fine for now, but in 2025, you’ll get asked).
99
+
100
+ **Grade: B+**
101
+
102
+ Solid, but could be more hardened and explicit on edge cases.
103
+
104
+ ---
105
+
106
+ ### 2.2 Token β†’ Current User (`get_current_user` / `get_current_active_user`)
107
+
108
+ - You validate JWT with Supabase, then fetch `User` from DB.
109
+ - `get_current_active_user` rejects inactive accounts.
110
+
111
+ **What’s good**
112
+
113
+ - Clean, simple, and correct glue logic.
114
+ - You centralize this in `deps.py` and reuse it.
115
+
116
+ **What’s bad**
117
+
118
+ - No caching or optimization β€” not needed yet, but something you’ll feel at scale.
119
+ - If Supabase is slow, your API is slow; there’s no fallback or short-circuit for already-known-good tokens (frontends should handle refresh, but still).
120
+
121
+ **Grade: A-**
122
+
123
+ Clean implementation. Nothing egregious here.
124
+
125
+ ---
126
+
127
+ ## 3. User Profile Modeling & Management
128
+
129
+ ### 3.1 Data Model (`User`)
130
+
131
+ - `User` model is **rich**:
132
+ - Org membership: `client_id` / `contractor_id`.
133
+ - Role and `status`, `is_active`.
134
+ - Identity/contact, health & safety, PPE, geo-location, metadata.
135
+
136
+ **What’s good**
137
+
138
+ - This is an **excellent domain model** for a field-ops platform.
139
+ - Proper use of JSONB for flexible health and PPE info.
140
+ - Computed properties for `full_name`, `first_name`, `last_name` avoid duplicating fields.
141
+
142
+ **What’s bad**
143
+
144
+ - `status` vs `is_active` is **not clearly defined** as a state machine:
145
+ - `invited`, `pending_setup`, `active`, `suspended` exist on paper.
146
+ - In code, only `active`/`suspended` have strong behavior (both mapped to `is_active`).
147
+ - You risk using `status` as a β€œlabel” instead of a **strictly enforced lifecycle**.
148
+ - Health/PPE/location completeness is computed but **not enforced anywhere**.
149
+
150
+ **Grade: A for modeling, C+ for state semantics**
151
 
152
  ---
153
 
154
+ ### 3.2 Profile Endpoints & Permissions
155
+
156
+ From `profile.py` + `ProfileService`:
157
 
158
+ - `GET /profile/me` returns:
159
+ - Basic info, health, PPE, location, counts, completion status.
160
+ - `ProfileService.get_profile_permissions`:
161
+ - Self vs admin vs org-admin vs managers vs field-agents.
162
+ - Very granular booleans: edit/view rights per section.
163
 
164
+ **What’s good**
 
 
 
 
165
 
166
+ - This is **surprisingly mature**:
167
+ - You’ve thought carefully about **who can edit what**.
168
+ - Good use of `ProfilePermissions`.
169
+ - `check_edit_permission` checks shared projects for managers/dispatchers vs field agents, which is realistic.
170
 
171
+ **What’s bad**
172
 
173
+ - The **self-service edit endpoints** aren’t fully visible here, so it’s hard to confirm that:
174
+ - Every update path consistently uses `ProfilePermissions`.
175
+ - There’s no bypass via some other admin or service endpoint.
176
+ - No clear, enforced **β€œprofile completion required before X”** rules.
 
 
 
177
 
178
+ **Grade: B+**
 
 
179
 
180
+ Model and thinking are strong; enforcement is not fully visible so I can’t give an A.
181
 
182
+ ---
 
 
 
183
 
184
+ ## 4. Admin User Management (Roles, Status, Organizations)
185
 
186
+ ### 4.1 Role-Based Access Control (`AppRole`, `ROLE_PERMISSIONS`)
 
 
 
187
 
188
+ - You define roles + permission strings in a central module.
189
+ - There is a `has_permission` helper; some endpoints use decorators like `@require_permission("manage_org_users")`.
 
190
 
191
+ **What’s good**
192
 
193
+ - **Centralized RBAC map** is the right approach.
194
+ - Permissions are described at a nice level of granularity.
 
195
 
196
+ **What’s bad**
197
 
198
+ - In practice, **logic is split**:
199
+ - Some code uses `@require_permission`.
200
+ - Some uses manual role checks (`if current_user.role == 'platform_admin'`, etc.).
201
+ - This makes the **permission model fragile**:
202
+ - A change in `ROLE_PERMISSIONS` doesn’t necessarily update all behaviors.
203
+ - Docs and code can easily diverge.
204
 
205
+ **Grade: B-**
206
 
207
+ Right structure, incomplete application. For a production system, I’d call this β€œrough but usable”.
 
 
 
208
 
209
  ---
210
 
211
+ ### 4.2 User Admin Operations (`UserService`, `/users` endpoints)
212
+
213
+ - `UserService`:
214
+ - `check_update_permission` handles who can update whom, and what.
215
+ - `update_user` tracks detailed change diffs.
216
+ - `change_user_status` + `change_user_role` encapsulate logic.
217
+ - `/users/{user_id}/status` and `/users/{user_id}/role` endpoints exist and use these services with audit logging.
218
+
219
+ **What’s good**
220
+
221
+ - **Service layer is well done**:
222
+ - Clear separation: endpoint β†’ service β†’ DB.
223
+ - Fine-grained permission checking.
224
+ - Audit logs on changes, with old/new values.
225
+ - **Org-based scoping** (client/contractor admin cannot promote to platform_admin and cannot change org) is exactly what you want.
226
+
227
+ **What’s bad**
228
+
229
+ - There’s still an opportunity to move more of the **role/permission logic into a single coherent RBAC layer**, rather than repeating patterns across `UserService` and `ProfileService`.
230
+ - No **role history tracking** beyond an audit log. That’s fine for early stage, but not β€œenterprise ready”.
231
+
232
+ **Grade: B+**
233
+
234
+ Much better than your own doc suggests (you gave yourself F there). From a backend perspective, it’s β€œgood but not fully systematized”.
 
 
 
 
 
 
235
 
236
  ---
237
 
238
+ ## 5. Security Posture
239
+
240
+ **What’s strong**
241
+
242
+ - Supabase Auth handles passwords and token issuance.
243
+ - No password storage in your own DB.
244
+ - Audit logging is used for:
245
+ - Auth events (register, login).
246
+ - Role/status changes.
247
+ - Rate limiting on critical endpoints.
248
 
249
+ **What’s weak**
 
 
 
250
 
251
+ - **Hardcoded operational secrets/constraints** (platform admin email) in code.
252
+ - Incomplete alignment between **Supabase account status** and local `status`/`is_active`:
253
+ - Suspended at API level, but still able to authenticate at Supabase and get a token.
254
+ - This is *okay* as long as you always use `get_current_active_user` and never trust Supabase ID alone, but it’s not ideal for a β€œlocked” account.
255
+ - Some behaviors (like 404 on β€œUser profile not found” after a successful auth) leak information about account existence.
 
256
 
257
+ **Grade: B-**
 
 
 
 
258
 
259
+ Better than typical side-projects, but not yet β€œsecurity-audited product”.
 
260
 
261
  ---
262
 
263
+ ## 6. Developer Experience & Consistency
264
 
265
+ **What’s good**
266
+
267
+ - Services (`UserService`, `ProfileService`, `InvitationService`) centralize business logic fairly well.
268
+ - Models, schemas, and endpoints are reasonably consistent.
269
+ - Good logging patterns.
270
+
271
+ **What’s bad**
272
+
273
+ - Some business rules (status semantics, RBAC) are spread across:
274
+ - `UserService`
275
+ - `ProfileService`
276
+ - Endpoint decorators
277
+ - Comments and docs
278
+ - You do not have a **single place** that clearly defines:
279
+ - User lifecycle state machine.
280
+ - Role/permission matrix (you’re close, but not fully coherent).
281
+ - Docs and code disagree in a few places (e.g., your markdown says β€œrole management not implemented” but the code clearly has it).
282
+
283
+ **Grade: C+/B-**
284
+
285
+ The code is better than your docs indicate, but cohesion is not tight yet.
286
 
287
  ---
288
 
289
+ ## Overall β€œBrutal” Grades
290
+
291
+ - **Account creation & invitation**: A- (logic), C+ (lifecycle clarity & bootstrap config) β†’ overall **B+**
292
+ - **Authentication & token handling**: **B+**
293
+ - **Profile modeling & permissions**: A (model), B (enforcement) β†’ overall **B+**
294
+ - **Admin user management (roles/status/org)**: **B+**
295
+ - **RBAC framework & usage**: **B-**
296
+ - **Security posture (for a pre-production system)**: **B-**
297
+ - **Production-readiness (for a paying customer SaaS)**: **B-/C+**
298
 
299
+ You are **well above β€œtoy project”** level. The architecture is thoughtful; the main gaps are:
300
 
301
+ - Hardcoded & opaque bootstrap for platform admin.
302
+ - Incomplete, poorly enforced user lifecycle state machine.
303
+ - RBAC that is partly centralized, partly duplicated.
 
 
 
 
304
 
305
  ---
306
 
307
+ ## If You Want Hard β€œFix Now” Priorities
308
+
309
+ 1. **Remove hardcoded platform admin email from code; move to config/env + document bootstrap process.**
310
+ 2. **Define and enforce a real user lifecycle state machine** (`invited` β†’ `pending_setup` β†’ `active` β†’ `suspended`), and ensure all flows respect it.
311
+ 3. **Standardize RBAC checks**:
312
+ - Make a single decorator / helper that uses `ROLE_PERMISSIONS + per-resource rules`, and use that everywhere.
313
+ 4. **Align suspension with Supabase** (at least by marking metadata, ideally blocking login).
314
+
315
+ If you want, I can next propose *specific code edits* (file + change), ordered by risk/impact, to push this from B/B- into A- territory.
src/app/api/v1/auth.py CHANGED
@@ -29,18 +29,19 @@ async def register(request: Request, response: Response, user_data: UserCreate,
29
  **SECURITY MODEL:**
30
  - Self-registration is DISABLED for regular users (must use invitation flow)
31
  - Platform admin creation requires OTP verification sent to authorized email
32
- - Only lewiskimaru01@gmail.com can receive platform admin OTPs
33
 
34
  **Process:**
35
  1. Submit registration request
36
- 2. OTP sent to lewiskimaru01@gmail.com
37
  3. Use OTP to verify via /otp/verify-public endpoint
38
  4. After OTP verification, account is created as platform_admin
39
 
40
  **Note:** Regular users (field agents, managers, etc.) must be invited by admins.
41
  """
42
- # Security check: Only allow platform admin registration for authorized email
43
- PLATFORM_ADMIN_EMAIL = "lewiskimaru01@gmail.com"
 
44
 
45
  # Check if this is an attempt to create a platform admin account
46
  # Platform admin accounts can only be created via OTP verification
 
29
  **SECURITY MODEL:**
30
  - Self-registration is DISABLED for regular users (must use invitation flow)
31
  - Platform admin creation requires OTP verification sent to authorized email
32
+ - Configurable via PLATFORM_ADMIN_EMAIL environment variable
33
 
34
  **Process:**
35
  1. Submit registration request
36
+ 2. OTP sent to configured admin email (PLATFORM_ADMIN_EMAIL)
37
  3. Use OTP to verify via /otp/verify-public endpoint
38
  4. After OTP verification, account is created as platform_admin
39
 
40
  **Note:** Regular users (field agents, managers, etc.) must be invited by admins.
41
  """
42
+ # Get platform admin email from config (can be overridden via environment variable)
43
+ from app.config import settings
44
+ PLATFORM_ADMIN_EMAIL = settings.PLATFORM_ADMIN_EMAIL
45
 
46
  # Check if this is an attempt to create a platform admin account
47
  # Platform admin accounts can only be created via OTP verification
src/app/api/v1/documents.py CHANGED
@@ -19,6 +19,7 @@ from app.schemas.document import (
19
  )
20
  from app.services.media_service import StorageService
21
  from app.services.audit_service import AuditService
 
22
  from datetime import datetime
23
  import logging
24
  import json
@@ -32,6 +33,7 @@ router = APIRouter(prefix="/documents", tags=["Document Management"])
32
  # ============================================
33
 
34
  @router.post("/upload", response_model=DocumentResponse, status_code=status.HTTP_201_CREATED)
 
35
  async def upload_document(
36
  file: UploadFile = File(...),
37
  entity_type: str = Form(...),
@@ -159,6 +161,7 @@ async def upload_document(
159
 
160
 
161
  @router.get("/{entity_type}/{entity_id}", response_model=DocumentListResponse)
 
162
  async def get_entity_documents(
163
  entity_type: str,
164
  entity_id: UUID,
@@ -298,6 +301,7 @@ async def update_document_metadata(
298
 
299
 
300
  @router.delete("/id/{document_id}", status_code=status.HTTP_204_NO_CONTENT)
 
301
  async def delete_document(
302
  document_id: UUID,
303
  request: Request,
 
19
  )
20
  from app.services.media_service import StorageService
21
  from app.services.audit_service import AuditService
22
+ from app.core.permissions import require_permission, require_any_permission
23
  from datetime import datetime
24
  import logging
25
  import json
 
33
  # ============================================
34
 
35
  @router.post("/upload", response_model=DocumentResponse, status_code=status.HTTP_201_CREATED)
36
+ @require_permission("upload_documents")
37
  async def upload_document(
38
  file: UploadFile = File(...),
39
  entity_type: str = Form(...),
 
161
 
162
 
163
  @router.get("/{entity_type}/{entity_id}", response_model=DocumentListResponse)
164
+ @require_permission("view_documents")
165
  async def get_entity_documents(
166
  entity_type: str,
167
  entity_id: UUID,
 
301
 
302
 
303
  @router.delete("/id/{document_id}", status_code=status.HTTP_204_NO_CONTENT)
304
+ @require_permission("upload_documents") # Same permission as upload (can manage own documents)
305
  async def delete_document(
306
  document_id: UUID,
307
  request: Request,
src/app/config.py CHANGED
@@ -21,6 +21,9 @@ class Settings(BaseSettings):
21
  SUPABASE_KEY: str
22
  SUPABASE_SERVICE_KEY: str
23
 
 
 
 
24
  # Optional External Services
25
  CLOUDINARY_CLOUD_NAME: Optional[str] = None
26
  CLOUDINARY_API_KEY: Optional[str] = None
 
21
  SUPABASE_KEY: str
22
  SUPABASE_SERVICE_KEY: str
23
 
24
+ # Security & Access Control (REQUIRED)
25
+ PLATFORM_ADMIN_EMAIL: str # Email for platform admin OTP verification - must be set in .env
26
+
27
  # Optional External Services
28
  CLOUDINARY_CLOUD_NAME: Optional[str] = None
29
  CLOUDINARY_API_KEY: Optional[str] = None
src/app/core/permissions.py CHANGED
@@ -1,5 +1,29 @@
1
  """
2
  Role-Based Access Control (RBAC) Definitions
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
3
  """
4
  from enum import Enum
5
  from typing import List, Dict, Callable
@@ -11,7 +35,16 @@ logger = logging.getLogger(__name__)
11
 
12
 
13
  class AppRole(str, Enum):
14
- """Application roles"""
 
 
 
 
 
 
 
 
 
15
  PLATFORM_ADMIN = "platform_admin"
16
  CLIENT_ADMIN = "client_admin"
17
  CONTRACTOR_ADMIN = "contractor_admin"
@@ -23,6 +56,8 @@ class AppRole(str, Enum):
23
 
24
 
25
  # Role permissions mapping
 
 
26
  ROLE_PERMISSIONS: Dict[AppRole, List[str]] = {
27
  AppRole.PLATFORM_ADMIN: ["*"], # All permissions
28
  AppRole.CLIENT_ADMIN: [
 
1
  """
2
  Role-Based Access Control (RBAC) Definitions
3
+
4
+ This module provides centralized permission management for all roles.
5
+ All route handlers should use @require_permission or @require_role decorators
6
+ to enforce consistent access control.
7
+
8
+ Usage Examples:
9
+ from app.core.permissions import require_permission, require_role, AppRole
10
+
11
+ @router.get("/users")
12
+ @require_permission("view_users")
13
+ async def list_users(current_user: User = Depends(get_current_active_user)):
14
+ ...
15
+
16
+ @router.delete("/users/{id}")
17
+ @require_role(AppRole.PLATFORM_ADMIN)
18
+ async def delete_user(current_user: User = Depends(get_current_active_user)):
19
+ ...
20
+
21
+ Permission Categories:
22
+ - User Management: view_users, manage_org_users, invite_users
23
+ - Documents: view_documents, upload_documents
24
+ - Projects: manage_projects, create_tickets
25
+ - Financial: view_payroll, approve_expenses
26
+ - Reports: view_reports, export_reports
27
  """
28
  from enum import Enum
29
  from typing import List, Dict, Callable
 
35
 
36
 
37
  class AppRole(str, Enum):
38
+ """
39
+ Application Roles
40
+
41
+ Hierarchy (highest to lowest):
42
+ 1. PLATFORM_ADMIN - Full system access
43
+ 2. CLIENT_ADMIN / CONTRACTOR_ADMIN - Organization-level admin
44
+ 3. SALES_MANAGER / PROJECT_MANAGER - Department managers
45
+ 4. DISPATCHER - Operations coordinator
46
+ 5. FIELD_AGENT / SALES_AGENT - Individual contributors
47
+ """
48
  PLATFORM_ADMIN = "platform_admin"
49
  CLIENT_ADMIN = "client_admin"
50
  CONTRACTOR_ADMIN = "contractor_admin"
 
56
 
57
 
58
  # Role permissions mapping
59
+ # Platform admin has "*" (all permissions)
60
+ # All other roles have explicit permission lists
61
  ROLE_PERMISSIONS: Dict[AppRole, List[str]] = {
62
  AppRole.PLATFORM_ADMIN: ["*"], # All permissions
63
  AppRole.CLIENT_ADMIN: [
src/app/models/user.py CHANGED
@@ -18,6 +18,33 @@ class User(BaseModel):
18
 
19
  Authentication: Supabase Auth (auth.users table)
20
  This table stores business profile and authorization data
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
 
21
  """
22
  __tablename__ = "users"
23
 
 
18
 
19
  Authentication: Supabase Auth (auth.users table)
20
  This table stores business profile and authorization data
21
+
22
+ USER LIFECYCLE STATE MACHINE:
23
+ β”Œβ”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”
24
+ β”‚ Status Flow: β”‚
25
+ β”‚ β”‚
26
+ β”‚ invited β†’ pending_setup β†’ active β†’ suspended β”‚
27
+ β”‚ ↓ ↓ ↓ ↓ β”‚
28
+ β”‚ (invited) (setup req) (operational) (disabled) β”‚
29
+ β””β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”€β”˜
30
+
31
+ Status Definitions:
32
+ - invited: User invited but hasn't accepted/registered yet
33
+ (Used in invitation flow only, before user_invitations.accepted_at)
34
+
35
+ - pending_setup: User accepted invitation but profile incomplete
36
+ (Reserved for future: require certain fields before full access)
37
+
38
+ - active: Fully operational user with complete access
39
+ (Default after invitation acceptance, primary operational state)
40
+
41
+ - suspended: Account temporarily disabled (is_active=False)
42
+ (Can be reactivated by admin, preserves all data)
43
+
44
+ Note: is_active field controls actual access:
45
+ - active/pending_setup β†’ is_active=True (can login)
46
+ - suspended β†’ is_active=False (blocked at API level)
47
+ - invited β†’ varies (typically False until acceptance)
48
  """
49
  __tablename__ = "users"
50