commit
This commit is contained in:
446
docs/reviews/phase-3-permission-authorization-audit.md
Normal file
446
docs/reviews/phase-3-permission-authorization-audit.md
Normal file
@@ -0,0 +1,446 @@
|
||||
# Phase 3 - Permission & Authorization Audit
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The authorization model is partially migrated to a permission-first design, but the live implementation is still mixed across `effectivePermissions`, `membership.role`, `businessRole`, and `systemRole`. That split creates real consistency risk: several sensitive pages and API routes still authorize by role-derived compatibility helpers even though the project now has permission templates, per-user overrides, and `effectivePermissions`.
|
||||
|
||||
The most important gap is that permission templates are not yet the single source of truth for access decisions. In practice, revoking a capability from an HRD/admin-style user through templates or overrides does not guarantee the capability is removed everywhere, because multiple paths still trust role-based helpers such as `requireHRD()`, `requireIT()`, `canManageTrainingMatrix(membership.role)`, and `canViewAllReports(role)`.
|
||||
|
||||
## Authorization Architecture Overview
|
||||
|
||||
Current authorization layers:
|
||||
|
||||
1. Authentication boundary
|
||||
- `src/proxy.ts`
|
||||
- Protects `/dashboard/**` and a subset of `/api/**` routes from unauthenticated access.
|
||||
2. Session and organization access
|
||||
- `src/lib/auth/session.ts`
|
||||
- `requireSession()` -> `requireOrganizationAccess()` -> specialized guards.
|
||||
3. Compatibility role helpers
|
||||
- `src/lib/auth/roles.ts`
|
||||
- Maps `super_admin`, membership roles, and legacy business roles into `IT`, `HRD`, and `EMPLOYEE`.
|
||||
4. Permission helpers
|
||||
- `src/lib/auth/authorization.ts`
|
||||
- `hasPermission`, `hasAnyPermission`, `hasAllPermissions`.
|
||||
5. Effective permission resolution
|
||||
- `src/lib/auth/authorization.server.ts`
|
||||
- Merges template assignments and user overrides into `effectivePermissions`.
|
||||
6. Navigation filtering
|
||||
- `src/config/nav-config.ts`
|
||||
- `src/hooks/use-nav.ts`
|
||||
- Mostly UX-only visibility filtering.
|
||||
|
||||
Observed architecture state:
|
||||
|
||||
- Permission-first exists and is used correctly in some modules such as `training-policy`.
|
||||
- Compatibility-mode role checks still control many sensitive routes and data scopes.
|
||||
- Navigation, page guards, API guards, and data-scope rules are not fully aligned yet.
|
||||
|
||||
## Permission Inventory
|
||||
|
||||
Defined permission families from `src/lib/auth/permissions.ts`:
|
||||
|
||||
- Organization: `organization:manage`, `organization:switch`
|
||||
- Permission templates: `permission_template:*`
|
||||
- User permission overrides: `user_permission:*`
|
||||
- Users: `users:*`, `users:manage`
|
||||
- Employee directory: `employee_directory:*`, `employee_import:run`, `employee_master_review:approve`
|
||||
- Courses and matrix: `courses:*`, `training_policy:*`, `training_matrix:*`
|
||||
- Training records: self/all read-write plus review workflow permissions
|
||||
- Announcements: full publish workflow permissions
|
||||
- Online lessons: full publish workflow permissions
|
||||
- Reports: `reports:self_read`, `reports:organization_read`, `reports:export`, `reports:export_excel`, `reports:export_pdf`
|
||||
- Audit logs: `audit_logs:read`, `audit_logs:export`
|
||||
- Notifications: `notifications:read`, `notifications:manage`
|
||||
|
||||
Inventory assessment:
|
||||
|
||||
- Coverage is broad enough for a permission-first model.
|
||||
- The problem is not missing permission keys; the problem is inconsistent enforcement of those keys.
|
||||
|
||||
## Route Permission Matrix
|
||||
|
||||
Representative page-level checks:
|
||||
|
||||
| Route | Nav Rule | Page Guard | Effective Result |
|
||||
|---|---|---|---|
|
||||
| `/dashboard/training-policy` | permission-based in nav | `requirePermissionDashboardAccess('training_policy:read')` | Consistent permission-first |
|
||||
| `/dashboard/users` | `businessRoles: ['IT']` | `requireUserManagementDashboardAccess()` | Role-based compatibility check |
|
||||
| `/dashboard/employee-directory` | `businessRoles: ['HRD', 'IT']` | `requireHRDDashboardAccess()` | Page is narrower than nav and ignores permission keys |
|
||||
| `/dashboard/training-matrix` | `businessRoles: ['HRD', 'IT']` | no page guard | Direct URL can render page shell without authorization gate |
|
||||
| `/dashboard/reports` | `businessRole: 'HRD'` | `requireEmployeeDashboardAccess()` | Page allows broader access than nav; server content later scopes by role |
|
||||
| `/dashboard/audit-logs` | `systemRole: 'super_admin'` | `requireITDashboardAccess()` | Nav and page criteria differ |
|
||||
| `/dashboard/organizers` | `systemRole: 'super_admin'` | inline `isHRD(...)` check | Page allows HRD-compatible access even though nav is super-admin only |
|
||||
| `/dashboard/permissions/*` | `systemRole: 'super_admin'` | `requireSuperAdminDashboardAccess()` | Consistent, super-admin only |
|
||||
|
||||
## API Permission Matrix
|
||||
|
||||
Representative API-level checks:
|
||||
|
||||
| API Route | Guard Style | Assessment |
|
||||
|---|---|---|
|
||||
| `/api/training-policies` | `requirePermission('training_policy:read/write')` | Good permission-first enforcement |
|
||||
| `/api/permission-templates` | `requireSuperAdminOrganizationAccess()` | Strong but role-based, not template-based |
|
||||
| `/api/users` | `requireUserManagementAccess()` | Compatibility-mode `IT` check, not `users:*` permissions |
|
||||
| `/api/training-matrix` | `requireOrganizationAccess()` + `canManageTrainingMatrix(membership.role)` | Role-based, bypasses `training_matrix:*` permissions |
|
||||
| `/api/reports/export` | `getReportsAccess()` + `canViewAllReports(role)` | Scope based on membership role, not report permissions |
|
||||
| `/api/training-records/[id]/review` | `requireOrganizationAccess()` + `canReviewTrainingRecords(membership.role)` | Role-based workflow approval |
|
||||
| `/api/master-review` | `requireHRD()` | Role-based, bypasses employee master-review permission keys |
|
||||
| `/api/employees` | `requireOrganizationAccess()` only | Any org member can list/create employees by direct API call |
|
||||
| `/api/products` | `requireOrganizationAccess()` only | Any org member can list/create products by direct API call |
|
||||
| `/api/notifications` | `requireOrganizationAccess()` + self-scope | Acceptable because data is always self-scoped |
|
||||
|
||||
## Action-Level Permission Matrix
|
||||
|
||||
Representative action enforcement:
|
||||
|
||||
| Action | UI Check | API Check | Assessment |
|
||||
|---|---|---|---|
|
||||
| View training policy | permission-based | permission-based | Consistent |
|
||||
| Create/update training policy | permission-based | permission-based | Consistent |
|
||||
| Review training record | mixed role/permission UI | role-based API | Template revokes may not be honored everywhere |
|
||||
| Manage training matrix | nav role-based | role-based API | `training_matrix:write` is not the real gate |
|
||||
| Export reports | page accessible broadly | export scope constrained by membership role | `reports:export*` keys do not drive the actual outcome |
|
||||
| Manage employee directory | page role-based | employees API open to all org members | UI/API mismatch |
|
||||
| Manage users | page/API `IT` compatibility | no `users:*` permission-first gate | Fine-grained permission model not fully active |
|
||||
| Read audit logs | nav super-admin, page/API IT | inconsistent source of truth |
|
||||
|
||||
## Role / Template Mapping
|
||||
|
||||
Observed effective role mapping from `src/lib/auth/roles.ts`:
|
||||
|
||||
- `super_admin` -> treated as `IT`
|
||||
- membership `owner`/`admin` -> treated as `HRD`
|
||||
- membership `member` -> treated as `EMPLOYEE`
|
||||
|
||||
Template behavior from `src/lib/auth/authorization.server.ts`:
|
||||
|
||||
- If user has assigned permission templates, template permissions replace fallback role defaults.
|
||||
- User overrides then allow or deny individual permissions.
|
||||
|
||||
Audit conclusion:
|
||||
|
||||
- The permission engine is capable of precise access control.
|
||||
- The application layer still frequently ignores that engine and falls back to role compatibility rules.
|
||||
- Role names and template names therefore do not reliably predict real access without tracing the exact route.
|
||||
|
||||
## Data Scope Assessment
|
||||
|
||||
Positive observations:
|
||||
|
||||
- Notifications are self-scoped by `organizationId + userId`.
|
||||
- Training records use organization scoping and record-level lookup helpers.
|
||||
- Reports distinguish self vs organization views in data-loading logic.
|
||||
|
||||
Scope weaknesses:
|
||||
|
||||
- Reports determine organization-wide visibility with `canViewAllReports(role)` in `src/features/reports/server/report-data.ts`, not with `reports:organization_read`.
|
||||
- Department summary, annual summary, and training matrix reports are therefore governed by membership role instead of permission keys.
|
||||
- Training matrix management also trusts `membership.role`, not `training_matrix:read/write`.
|
||||
- Employees and products APIs are scoped to organization, but not to action-level authority inside that organization.
|
||||
|
||||
## Direct URL Assessment
|
||||
|
||||
Direct URL behavior is mixed:
|
||||
|
||||
- `/dashboard/**` is authentication-protected by `src/proxy.ts`, but authorization still depends on page-level guards.
|
||||
- `/dashboard/training-matrix` has no page guard, so an authenticated user can load the page shell directly even though its data API later blocks non-admin membership roles.
|
||||
- Several APIs are missing proxy-level authentication coverage entirely, including:
|
||||
- `/api/online-lessons/**`
|
||||
- `/api/permission-templates/**`
|
||||
- `/api/user-permissions/**`
|
||||
- `/api/permissions/catalog`
|
||||
- `/api/permission-audit-logs`
|
||||
- Those routes appear to rely on route-handler guards instead of proxy enforcement. That is acceptable for security if every handler is guarded, but it is inconsistent with the rest of the API surface and increases review burden.
|
||||
|
||||
## UI vs API Authorization Consistency
|
||||
|
||||
Main inconsistencies:
|
||||
|
||||
- Employee directory UI is HRD-only at page level, but `/api/employees` only requires organization membership.
|
||||
- Training matrix nav suggests HRD/IT management access, but the page has no server guard and the API uses membership-role checks only.
|
||||
- Reports nav is HRD-only, but the page allows all employee-area users and delegates scope reduction to server data logic.
|
||||
- Audit logs nav is super-admin only, while page and API enforce `IT` access.
|
||||
- Organizers nav is super-admin only, while the page uses `isHRD(...)`.
|
||||
|
||||
Conclusion:
|
||||
|
||||
- Navigation is not a reliable indicator of backend authorization.
|
||||
- Several pages are stricter or looser than their APIs.
|
||||
- Permission template rollout is incomplete because UI and API do not consistently consume the same access model.
|
||||
|
||||
## Compatibility Mode Findings
|
||||
|
||||
Compatibility-mode authorization is still widespread:
|
||||
|
||||
- `requireHRD()`
|
||||
- `requireIT()`
|
||||
- `requireUserManagementAccess()`
|
||||
- `canManageTrainingMatrix(membership.role)`
|
||||
- `canViewAllReports(role)`
|
||||
- `canReviewTrainingRecords(membership.role)`
|
||||
- inline `isHRD(...)` page checks
|
||||
|
||||
Risk:
|
||||
|
||||
- Any path that authorizes from compatibility roles can bypass explicit permission denies from templates or user overrides.
|
||||
- This is especially important for review, reporting, user management, training matrix, and master-review flows.
|
||||
|
||||
## Missing Permission Descriptions
|
||||
|
||||
Permission metadata review in `src/features/permission-management/server/permission-management-catalog.ts`:
|
||||
|
||||
- Most permission keys have catalog metadata and Thai descriptions.
|
||||
- `descriptionEn` is not populated for most entries.
|
||||
- The file content shows text encoding corruption in the stored description strings, which will degrade readability in the permission-management UI and audit output.
|
||||
|
||||
## Findings by Severity
|
||||
|
||||
### [High] Permission templates and user overrides are not the single source of truth for authorization
|
||||
|
||||
**Module:** Cross-cutting authorization
|
||||
|
||||
**Location Found:**
|
||||
|
||||
- `src/app/api/training-records/[id]/review/route.ts`
|
||||
- `src/app/api/training-matrix/route.ts`
|
||||
- `src/app/api/master-review/route.ts`
|
||||
- `src/features/reports/server/report-data.ts`
|
||||
- `src/app/dashboard/users/page.tsx`
|
||||
- `src/app/dashboard/employee-directory/page.tsx`
|
||||
|
||||
**Details:**
|
||||
|
||||
The codebase already resolves `effectivePermissions`, but many sensitive routes still authorize from membership or business-role compatibility logic. That means a permission template deny may not remove access from all paths for the same user.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- Fine-grained RBAC is unreliable.
|
||||
- Permission template rollout can produce false confidence.
|
||||
- Privileged workflows may remain available after explicit deny overrides.
|
||||
|
||||
**Evidence from Code:**
|
||||
|
||||
- Training matrix uses `canManageTrainingMatrix(membership.role)`.
|
||||
- Reports use `canViewAllReports(role)`.
|
||||
- Master review uses `requireHRD()`.
|
||||
- User management uses `requireUserManagementAccess()` which maps to `IT`.
|
||||
|
||||
**Example Scenario:**
|
||||
|
||||
An admin-style member receives a template that removes report-wide access or training-matrix management. The user can still pass role-based guards because the route trusts membership role instead of `effectivePermissions`.
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
Standardize all sensitive server-side authorization on `requirePermission`, `requireAnyPermission`, or `requireAllPermissions`, and reserve compatibility helpers only for temporary transitional wrappers.
|
||||
|
||||
**Must Fix Before Production:** Yes
|
||||
|
||||
### [High] Employee and product management APIs allow any organization member to perform management actions by direct API access
|
||||
|
||||
**Module:** Employee Management, Products
|
||||
|
||||
**Location Found:**
|
||||
|
||||
- `src/app/api/employees/route.ts`
|
||||
- `src/app/api/products/route.ts`
|
||||
|
||||
**Details:**
|
||||
|
||||
Both APIs only require `requireOrganizationAccess()`. There is no action-level permission check for list or create operations.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- Any authenticated member in the same organization can query or create records through direct API calls even if the dashboard UI hides those actions.
|
||||
- This is a true UI-vs-API authorization gap.
|
||||
|
||||
**Evidence from Code:**
|
||||
|
||||
- `GET /api/employees` and `POST /api/employees` have no `employee_directory:*` or import/review permission gate.
|
||||
- `GET /api/products` and `POST /api/products` have no product-specific privilege gate.
|
||||
|
||||
**Example Scenario:**
|
||||
|
||||
A standard employee cannot reach the employee directory through nav, but can still call `/api/employees` directly once authenticated in the same organization.
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
Add server-side action gates for every management API and align them with the intended permission model.
|
||||
|
||||
**Must Fix Before Production:** Yes
|
||||
|
||||
### [High] Reports authorization and scope are driven by membership role instead of report permissions
|
||||
|
||||
**Module:** Reports
|
||||
|
||||
**Location Found:**
|
||||
|
||||
- `src/features/reports/server/report-data.ts`
|
||||
- `src/app/api/reports/export/route.ts`
|
||||
- `src/app/dashboard/reports/page.tsx`
|
||||
|
||||
**Details:**
|
||||
|
||||
`canViewAllReports(role)` grants organization-wide reporting only to membership `owner` or `admin`. The actual permission keys `reports:self_read`, `reports:organization_read`, and `reports:export*` do not control the real scope boundary.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- Report permissions are partially decorative.
|
||||
- Templates cannot precisely grant or revoke report scope independent of membership role.
|
||||
- Export rules depend on role-derived access rather than permission-derived access.
|
||||
|
||||
**Evidence from Code:**
|
||||
|
||||
- `getReportsAccess()` derives `canViewAll` from `canViewAllReports(scope.role)`.
|
||||
- Export route blocks non-organization-wide users from all reports except `employeeTranscript` using `canViewAll`, not `reports:organization_read`.
|
||||
|
||||
**Example Scenario:**
|
||||
|
||||
A user granted `reports:organization_read` through a template but holding membership `member` still cannot access org-wide reports, while an `admin` membership can.
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
Replace report scope decisions with explicit report permissions and keep role defaults only as template seed values.
|
||||
|
||||
**Must Fix Before Production:** Yes
|
||||
|
||||
### [Medium] Navigation, page guards, and API guards are inconsistent across key modules
|
||||
|
||||
**Module:** Navigation and dashboard access
|
||||
|
||||
**Location Found:**
|
||||
|
||||
- `src/config/nav-config.ts`
|
||||
- `src/hooks/use-nav.ts`
|
||||
- `src/app/dashboard/audit-logs/page.tsx`
|
||||
- `src/app/dashboard/organizers/page.tsx`
|
||||
- `src/app/dashboard/reports/page.tsx`
|
||||
- `src/app/dashboard/training-matrix/page.tsx`
|
||||
|
||||
**Details:**
|
||||
|
||||
The same feature is often gated differently in nav, page render, and API access.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- Users get confusing UX.
|
||||
- Direct URL behavior is harder to reason about.
|
||||
- Authorization reviews become more error-prone.
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
Define one canonical access contract per module and apply it consistently to nav visibility, page guard, and API handlers.
|
||||
|
||||
**Must Fix Before Production:** Consider
|
||||
|
||||
### [Medium] Super-admin organization access mutates membership data as a side effect of authorization
|
||||
|
||||
**Module:** Session and organization access
|
||||
|
||||
**Location Found:**
|
||||
|
||||
- `src/lib/auth/session.ts`
|
||||
|
||||
**Details:**
|
||||
|
||||
`requireOrganizationAccess()` automatically inserts an `admin` membership when a `super_admin` selects an organization without an existing membership.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- Authorization has write-side effects.
|
||||
- Auditability and least-privilege reasoning become harder.
|
||||
- Accidental access expansion is possible through normal navigation.
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
Separate "temporary super-admin impersonation/access" from persistent membership creation, or make membership creation an explicit administrative action.
|
||||
|
||||
**Must Fix Before Production:** Consider
|
||||
|
||||
### [Low] Permission catalog descriptions have metadata quality issues
|
||||
|
||||
**Module:** Permission Management
|
||||
|
||||
**Location Found:**
|
||||
|
||||
- `src/features/permission-management/server/permission-management-catalog.ts`
|
||||
|
||||
**Details:**
|
||||
|
||||
Descriptions exist, but `descriptionEn` is mostly absent and Thai strings appear encoding-corrupted in the file.
|
||||
|
||||
**Impact:**
|
||||
|
||||
- Permission management UX and audit readability are reduced.
|
||||
- Not a direct security failure.
|
||||
|
||||
**Recommendation:**
|
||||
|
||||
Normalize encoding and complete optional description metadata where needed.
|
||||
|
||||
**Must Fix Before Production:** No
|
||||
|
||||
## Privilege Escalation Risks
|
||||
|
||||
Primary privilege-escalation paths observed:
|
||||
|
||||
1. Direct API access to `/api/employees` and `/api/products` by ordinary org members.
|
||||
2. Role-based fallback that ignores explicit permission denies in templates/overrides.
|
||||
3. Super-admin membership auto-creation inside `requireOrganizationAccess()`.
|
||||
|
||||
## Recommended Remediation Order
|
||||
|
||||
1. Lock down management APIs that currently require only organization membership.
|
||||
2. Convert sensitive server routes from compatibility-role checks to permission-first checks.
|
||||
3. Redefine reports scope and export authorization around `reports:*` permissions.
|
||||
4. Add missing page guards, especially for `training-matrix`, and align page/API/nav contracts.
|
||||
5. Remove authorization side effects from super-admin organization access.
|
||||
6. Clean up permission catalog metadata and encoding.
|
||||
|
||||
## Files Reviewed
|
||||
|
||||
- `plans/tms-system-full-audit-prompts.md`
|
||||
- `src/config/nav-config.ts`
|
||||
- `src/hooks/use-nav.ts`
|
||||
- `src/components/layout/app-sidebar.tsx`
|
||||
- `src/proxy.ts`
|
||||
- `src/lib/auth/session.ts`
|
||||
- `src/lib/auth/page-guards.ts`
|
||||
- `src/lib/auth/roles.ts`
|
||||
- `src/lib/auth/authorization.ts`
|
||||
- `src/lib/auth/authorization.server.ts`
|
||||
- `src/lib/auth/permissions.ts`
|
||||
- `src/app/dashboard/users/page.tsx`
|
||||
- `src/app/dashboard/employee-directory/page.tsx`
|
||||
- `src/app/dashboard/training-matrix/page.tsx`
|
||||
- `src/app/dashboard/reports/page.tsx`
|
||||
- `src/app/dashboard/audit-logs/page.tsx`
|
||||
- `src/app/dashboard/organizers/page.tsx`
|
||||
- `src/app/dashboard/training-policy/page.tsx`
|
||||
- `src/app/dashboard/permissions/templates/page.tsx`
|
||||
- `src/app/dashboard/permissions/users/page.tsx`
|
||||
- `src/app/dashboard/permissions/audit-logs/page.tsx`
|
||||
- `src/app/api/users/route.ts`
|
||||
- `src/app/api/employees/route.ts`
|
||||
- `src/app/api/products/route.ts`
|
||||
- `src/app/api/training-matrix/route.ts`
|
||||
- `src/app/api/training-records/[id]/review/route.ts`
|
||||
- `src/app/api/reports/export/route.ts`
|
||||
- `src/app/api/audit-logs/route.ts`
|
||||
- `src/app/api/audit-logs/export/route.ts`
|
||||
- `src/app/api/training-policies/route.ts`
|
||||
- `src/app/api/master-review/route.ts`
|
||||
- `src/app/api/notifications/route.ts`
|
||||
- `src/app/api/permission-templates/route.ts`
|
||||
- `src/app/api/permissions/catalog/route.ts`
|
||||
- `src/features/reports/server/report-data.ts`
|
||||
- `src/features/training-matrix/server/training-matrix-data.ts`
|
||||
- `src/features/permission-management/server/permission-management-catalog.ts`
|
||||
|
||||
## Areas Not Verified
|
||||
|
||||
- Runtime browser behavior for redirects, hidden actions, and toast/error handling
|
||||
- Full API execution against seeded users for every role/template combination
|
||||
- Exact UI rendering impact of permission catalog encoding issues
|
||||
- Online lessons, announcements, and user-permission routes at full action-by-action depth
|
||||
- Whether all non-proxy-covered API routes always apply equivalent route-handler guards in every nested handler
|
||||
Reference in New Issue
Block a user