Open
Conversation
…n-major-updates chore(deps): update dependency postcss to ^8.5.7 (feature/beta-release)
Wikid82
added
critical
Contributor
✅ Supply Chain Verification Results✅ PASSED 📦 SBOM Summary
🔍 Vulnerability Scan
📎 Artifacts
Generated by Supply Chain Verification workflow • View Details |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…n-major-updates chore(deps): update dependency postcss to ^8.5.8 (feature/beta-release)
…to address false DOWN states
- Changed user role representation from string to UserRole type in User model. - Updated role assignments in various services and handlers to use the new UserRole constants. - Modified middleware to handle UserRole type for role checks. - Refactored tests to align with the new UserRole type. - Added migration function to convert legacy "viewer" roles to "passthrough". - Ensured all role checks and assignments are consistent across the application.
- Deleted the Account page and its associated logic. - Introduced a new PassthroughLanding page for users without management access. - Updated Settings page to conditionally display the Users link for admin users. - Enhanced UsersPage to support passthrough user role, including invite functionality and user detail modal. - Updated tests to reflect changes in user roles and navigation.
…ap hook - Wrapped the Settings component in RequireRole to enforce access control for admin and user roles. - Introduced a new custom hook `useFocusTrap` to manage focus within modal dialogs, enhancing accessibility. - Applied the focus trap in InviteModal, PermissionsModal, and UserDetailModal to prevent focus from leaving the dialog. - Updated PassthroughLanding to focus on the heading when the component mounts.
…rity rebuild
The scheduled weekly rebuild was failing because GitHub Actions froze
github.sha at job-queue time. When the Sunday cron queued a job on
March 1 with Feb 23 code (CADDY_VERSION=2.11.0-beta.2), that job ran
two days later on March 3 still using the old code, missing the caddy
version fix that had since landed on main.
Additionally, caddy-security was unpinned, so xcaddy auto-resolved it
to v1.1.36 which requires caddy/v2@v2.11.1 — conflicting with xcaddy's
internally bundled v2.11.0-beta.2 reference.
- Add ref: github.ref_name to checkout step so the rebuild always
fetches current branch HEAD at run time, not the SHA frozen at queue
time
- Add CADDY_SECURITY_VERSION=1.1.36 ARG to pin the caddy-security
plugin to a known-compatible version; pass it via --with so xcaddy
picks up the pinned release
- Add --with github.com/caddyserver/caddy/v2@v${CADDY_TARGET_VERSION}
to force xcaddy to use the declared Caddy version, overriding its own
internal go.sum pin for caddy
- Add Renovate custom manager for CADDY_SECURITY_VERSION so future
caddy-security releases trigger an automated PR instead of silently
breaking the build
Fixes weekly security rebuild CI failures introduced ~Feb 22 when
caddy-security v1.1.36 was published.
The scheduled CodeQL analysis explicitly passed ref: github.sha, which is frozen when a cron job is queued, not when it runs. Under load or during a long queue, the analysis could scan code that is days old, missing vulnerabilities introduced since the last scheduling window. Replace with ref: github.ref_name so all trigger types — scheduled, push, and pull_request — consistently scan the current HEAD of the branch being processed.
- Implemented middleware to restrict access for passthrough users in management routes. - Added unit tests for management access requirements based on user roles. - Updated user model tests to include passthrough role validation. - Enhanced frontend user management to support passthrough role in invite modal. - Created end-to-end tests for passthrough user access restrictions and navigation visibility. - Verified self-service profile management for admins and regular users.
… for improved functionality
…sx test sections The Account.tsx page was removed in PR-2b and replaced by UsersPage.tsx with a UserDetailModal. Several E2E test sections still referenced UI elements that only existed in the deleted page, causing CI failures across shards. - admin-onboarding: update header profile link locator from /settings/account to /settings/users to match the new navigation target in Layout.tsx - account-settings: skip five legacy test sections (Profile Management, Certificate Email, Password Change, API Key Management, Accessibility) that reference deleted Account.tsx elements (#profile-name, #profile-email, #useUserEmail, #cert-email) or assume these fields are directly on the page rather than inside the UserDetailModal - Each skipped section includes an explanatory comment pointing to the PR-3 'Self-Service Profile via Users Page (F10)' suite as the equivalent coverage Verified: admin-onboarding 8/8 pass; account-settings 8 pass / 20 skipped
Three tests broke when the Admin/User/Passthrough privilege model replaced
the old admin/user/guest hierarchy in PR-3.
- user-management: tighten heading locator to name='User Management' to avoid
strict mode violation; the settings layout now renders a second h1
('Settings') alongside the page content heading
- user-lifecycle: update audit trail assertion from 2 to 1; users are now
created with a role in a single API call so the backend does not emit a
user_update audit entry when STEP 2 sends the same role value as creation
- auth-fixtures: replace invalid role='guest' with role='passthrough' in the
guestUser fixture; the 'guest' role was removed in PR-3 and 'passthrough' is
the equivalent lowest-privilege role in the new model
Verified: all three previously-failing tests now pass locally.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates user/account management into the /settings/users area, introduces a new lowest-privilege passthrough role, and updates frontend/backend routing, authorization, and tests to reflect the new structure.
Changes:
- Add
passthroughrole end-to-end (backend model + middleware checks, frontend routing + landing page, and updated fixtures/tests). - Consolidate/redirect legacy account and user-management routes to
/settings/users, and gate settings/users pages by role. - Refactor role handling across backend services/tests to use typed role constants and add migration coverage for legacy
viewer.
Reviewed changes
Copilot reviewed 60 out of 64 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/settings/user-management.spec.ts | Adds/adjusts E2E coverage for consolidated users UI and modal behaviors |
| tests/settings/user-lifecycle.spec.ts | Updates lifecycle workflow assertions; adds passthrough/nav restriction E2E suites |
| tests/settings/account-settings.spec.ts | Adds redirect/self-service coverage; skips suites that referenced removed Account page |
| tests/fixtures/auth-fixtures.ts | Updates “guest” fixture to use passthrough role |
| tests/core/admin-onboarding.spec.ts | Updates header link expectation to /settings/users |
| package-lock.json | Bumps postcss (root lockfile) |
| frontend/src/pages/tests/Settings.test.tsx | Updates Settings nav test to use Users; mocks useAuth |
| frontend/src/pages/Settings.tsx | Shows Users settings item only for admins |
| frontend/src/pages/PassthroughLanding.tsx | Adds passthrough landing page with logout-only UX |
| frontend/src/hooks/useFocusTrap.ts | Adds focus-trap hook used by dialogs for keyboard accessibility |
| frontend/src/hooks/tests/useAuth.test.tsx | Tightens role typing in hook test |
| frontend/src/context/AuthContextValue.ts | Narrows User.role to `'admin' |
| frontend/src/components/RequireRole.tsx | Adds role-gating component with passthrough redirect |
| frontend/src/components/Layout.tsx | Consolidates settings nav; hides navigation for passthrough users; updates header profile link |
| frontend/src/api/users.ts | Updates role union; merges self-service profile API surface into users API module |
| frontend/src/api/user.ts | Removes old self-service API module (moved into users.ts) |
| frontend/src/api/tests/user.test.ts | Updates API tests to import from users module |
| frontend/src/App.tsx | Removes Account route; adds passthrough route; adds role gating and legacy redirects |
| frontend/package.json | Bumps postcss version |
| frontend/package-lock.json | Updates frontend lockfile deps (postcss, floating-ui, caniuse-lite, etc.) |
| frontend/src/locales/en/translation.json | Adds strings for passthrough + self-service modal text; removes legacy account labels |
| frontend/src/locales/de/translation.json | Same as above (German) |
| frontend/src/locales/fr/translation.json | Same as above (French) |
| frontend/src/locales/es/translation.json | Same as above (Spanish) |
| frontend/src/locales/zh/translation.json | Same as above (Chinese) |
| backend/internal/services/backup_service_rehydrate_test.go | Updates role usage to typed constants |
| backend/internal/services/auth_service_test.go | Updates role expectations and JWT claim role typing |
| backend/internal/services/auth_service.go | Switches registration roles to typed constants; ensures JWT role claim is string |
| backend/internal/models/user_test.go | Updates role usage and adds role constant/validation tests |
| backend/internal/models/user.go | Introduces UserRole type + constants + validation; updates User.Role type |
| backend/internal/cerberus/cerberus.go | Updates admin-role check to use typed constant |
| backend/internal/api/tests/user_smtp_audit_test.go | Updates handler wiring/signatures; updates roles; improves admin-only endpoint tests |
| backend/internal/api/routes/routes_test.go | Adds coverage for viewer→passthrough migration |
| backend/internal/api/routes/routes_import_test.go | Updates non-admin role creation to typed constant |
| backend/internal/api/middleware/optional_auth_test.go | Updates seeded user role to typed constant |
| backend/internal/api/middleware/optional_auth.go | Stores role in context as string for downstream checks |
| backend/internal/api/middleware/auth_test.go | Adds tests for RequireManagementAccess behavior by role |
| backend/internal/api/middleware/auth.go | Stores role in context as string; updates RequireRole typing; adds RequireManagementAccess |
| backend/internal/api/handlers/user_integration_test.go | Updates UserHandler constructor signature usage |
| backend/internal/api/handlers/user_handler_coverage_test.go | Updates UserHandler constructor signature and role constants in tests |
| backend/internal/api/handlers/user_handler.go | Adds passthrough restrictions to profile/api-key; refactors admin checks; adds session invalidation + last-admin protections |
| backend/internal/api/handlers/security_handler.go | Uses shared requireAdmin helper |
| backend/internal/api/handlers/security_event_intake_test.go | Updates role constant usage |
| backend/internal/api/handlers/permission_helpers.go | Updates isAdmin to use c.GetString("role") + typed constant |
| backend/internal/api/handlers/emergency_handler.go | Uses shared requireAdmin helper |
| backend/internal/api/handlers/auth_handler_test.go | Updates seeded user roles to typed constants |
| backend/internal/api/handlers/auth_handler.go | Updates forwarded role header to string(role) |
| backend/go.mod | Bumps otelhttp instrumentation version |
| backend/go.sum | Updates sums for OpenTelemetry dependency bump |
| backend/cmd/seed/seed_smoke_test.go | Updates legacy viewer expectations to passthrough |
| backend/cmd/api/main_test.go | Updates seeded role to typed constants |
| Dockerfile | Pins caddy-security plugin version and wires it into the xcaddy build |
| .github/workflows/security-weekly-rebuild.yml | Ensures scheduled rebuild checks out latest HEAD of the branch ref |
| .github/workflows/codeql.yml | Fixes checkout ref to use github.ref for correct PR behavior |
| .github/renovate.json | Adds custom manager to track Dockerfile caddy-security version arg |
| .github/instructions/ARCHITECTURE.instructions.md | Updates documented Caddy version |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
Comment on lines
+758
to
+759
| if req.Role != "" || req.Enabled != nil { | ||
| c.JSON(http.StatusForbidden, gin.H{"error": "Cannot modify role or enabled status"}) |
There was a problem hiding this comment.
UpdateUser allows non-admin callers to update their own record, but the non-admin guard only blocks role and enabled. Because req.Email is still accepted and updated without any current-password verification, a regular authenticated user can change their email via PUT /api/v1/users/:id, bypassing the stricter checks in UpdateProfile.
Tighten this path by either (a) making /users/:id truly admin-only (and handle self-service exclusively via /user/profile + /auth/change-password), or (b) explicitly forbid req.Email for non-admins (and/or require current_password verification for email changes).
Suggested change
| if req.Role != "" || req.Enabled != nil { | |
| c.JSON(http.StatusForbidden, gin.H{"error": "Cannot modify role or enabled status"}) | |
| if req.Role != "" || req.Enabled != nil || req.Email != "" { | |
| c.JSON(http.StatusForbidden, gin.H{"error": "Cannot modify role, enabled status, or email"}) |
Comment on lines
440
to
+446
| const createEntry = findLifecycleEntry(createEntries, testUser.email, 'user_create'); | ||
| const updateEntry = findLifecycleEntry(updateEntries, testUser.email, 'user_update'); | ||
| return Number(Boolean(createEntry)) + Number(Boolean(updateEntry)); | ||
| }, { | ||
| timeout: 30000, | ||
| message: `Expected user lifecycle audit entries for ${testUser.email}`, | ||
| }).toBe(2); | ||
| }).toBe(1); |
There was a problem hiding this comment.
This audit assertion counts both a user_create and a user_update entry (via Boolean(createEntry) + Boolean(updateEntry)), but now expects the total to be 1. That makes the intent ambiguous and will fail if the system legitimately logs both lifecycle events.
Either change the expectation back to 2 (if both events should exist), or simplify the poll to assert only the specific event(s) that are expected in this workflow (e.g., remove the user_update check entirely if no update happens).
Comment on lines
+219
to
+253
| export interface UserProfile { | ||
| id: number | ||
| email: string | ||
| name: string | ||
| role: string | ||
| has_api_key: boolean | ||
| api_key_masked: string | ||
| } | ||
|
|
||
| /** Response from API key regeneration. */ | ||
| export interface RegenerateApiKeyResponse { | ||
| message: string | ||
| has_api_key: boolean | ||
| api_key_masked: string | ||
| api_key_updated: string | ||
| } | ||
|
|
||
| /** | ||
| * Fetches the current user's profile. | ||
| * @returns Promise resolving to UserProfile | ||
| */ | ||
| export const getProfile = async (): Promise<UserProfile> => { | ||
| const response = await client.get('/user/profile') | ||
| return response.data | ||
| } | ||
|
|
||
| /** | ||
| * Updates the current user's profile. | ||
| * @param data - Object with name, email, and optional current_password for verification | ||
| * @returns Promise resolving to success message | ||
| */ | ||
| export const updateProfile = async (data: { name: string; email: string; current_password?: string }): Promise<{ message: string }> => { | ||
| const response = await client.post('/user/profile', data) | ||
| return response.data | ||
| } |
There was a problem hiding this comment.
The newly added self-service profile types/functions are losing type-safety:
UserProfile.roleis typed asstring, even though the app now has a constrained role model ('admin' | 'user' | 'passthrough').getProfile/updateProfilecallclient.get/client.postwithout Axios generics, soresponse.datais effectivelyanyand won’t be checked againstUserProfile/{ message: string }.
Consider narrowing role to the same union used elsewhere, and apply the Axios generics on these calls so the compiler can enforce the contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is to restructure the user management settings into one location to prepare for future feature that would involve the ability to add users.