Access Controls#11
Merged
Merged
Conversation
Cloudflare Access onboarding was too much friction; this replaces it with an in-app admin panel (user/band list+delete) and a DB-backed signup allow-list, gated by an env-based admin-email set that never touches git.
11 TDD tasks: admin-email config, AccessPolicy/AllowedEmail models, repo CRUD, DeleteUser cascade (factored from DeleteBand/DeleteSong), signup enforcement, RequireAdmin middleware, admin endpoints, and the frontend admin panel (users/bands/access-policy tabs).
- Add AdminEmails map[string]bool field to API struct - Implement IsAdminEmail method for case-insensitive email lookup - Add BANDWIDTH_ADMIN_EMAILS env var with empty default to viper config - Add parseAdminEmails helper to parse comma-separated admin email list - Wire admin_emails into API initialization from viper config - Add comprehensive tests for IsAdminEmail with edge cases Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Register the new models in AutoMigrate and add schema migration test coverage. Task 3's repository functions will operate on these models. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implement repository layer for access policy management (enable/disable signup gating) and allowed email list (CRUD operations). Add admin listing queries for all users and all bands with necessary metadata for admin panel display. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracts deleteBandTx and deleteSongRowsTx (pure refactor, verified against the existing test suite) so the new Repo.DeleteUser can cascade a created band and personal songs without nesting transactions. DeleteUser also clears the deleted user's sessions, backup codes, password resets, personal folders, memberships, personal-layer rows on other bands' songs, and pending invites addressed to them.
Implements the access policy check in the Signup handler. Blocks registration unless the policy is disabled, the caller is an admin email, or the email is on the allow-list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Convert userResponse from a package-level function to a method on *API so it can call a.IsAdminEmail() and add isAdmin field to every endpoint that returns a user in its JSON payload. Updates 6 call sites across account.go, auth.go, and twofa.go. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Implements the RequireAdmin middleware that rejects callers whose email is not configured as a site admin. Must run after RequireAuth in the middleware chain. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wires the Task 3-4 repository methods and Task 7 RequireAdmin middleware into 8 handler methods under /api/admin/*: list/delete users, list/delete bands, get/set the signup access policy, and add/remove allow-list emails. Self-delete is blocked (400) and missing users/bands/entries 404 before delete/remove.
Replaces the shared adminTargetID helper with adminUserID, adminBandID, and adminAllowedEmailID so malformed :id path params return the same resource-specific "not found" message as the well-formed-but-missing case, matching the songID/memberID convention elsewhere.
Implements 8 hooks for admin panel: useAdminUsers, useDeleteAdminUser, useAdminBands, useDeleteAdminBand, useAccessPolicy, useSetAccessPolicy, useAddAllowedEmail, useRemoveAllowedEmail. Follows existing hook pattern with proper query-key invalidation on mutations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Resolves the App.tsx import that has referenced a nonexistent ./pages/AdminPage module since Task 9. Full just check suite (lint, typecheck, format, Go tests, frontend tests) now passes clean end to end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
deleteBandTx deleted a band's songs, members, and invites but left its Folder/FolderEntry rows behind, permanently orphaned since folder listing is always owner-scoped. DeleteUser now reaches this path when cascading through bands the deleted user created, so fix it here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Lets the admin panel be tested locally via `just dev` by exporting the var on the host shell before starting the dev container. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Invalidate the bands query when an admin deletes a user, since DeleteUser cascades to bands that user created - Add aria-selected to the admin panel's tab buttons - Add audit logging (actor + target) to all five destructive/mutating admin handlers - Extract deleteOwnedFoldersTx, a shared batched-delete helper, to de-duplicate the band/user folder-cleanup logic in deleteBandTx and DeleteUser Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add admin panel and signup access policy
A non-descript, fixed-position link in the corner of every page (App.tsx, above the route tree, so it covers login/signup/join too) — © and a link to the GitHub profile. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add a small copyright footer link
📝 WalkthroughWalkthroughThis PR adds admin-email configuration, access-policy persistence, admin routes and UI, shared row-component updates, a footer, and small server-side and password-reset adjustments. ChangesAdmin Access Policy and Panel
Shared Row Component Consolidation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/bandwidth/server.go`:
- Around line 267-278: Add table-driven tests for parseAdminEmails to lock down
its normalization behavior. Create tests that exercise strings with commas,
extra spaces, mixed case, and empty entries, and verify the returned map from
parseAdminEmails contains only trimmed lowercase emails with duplicates/empties
removed. Use the parseAdminEmails function name in the test so it’s easy to
locate and keep the cases concise but comprehensive.
In `@docs/superpowers/plans/2026-07-03-admin-access-policy.md`:
- Around line 1038-1071: markdownlint is flagging the fenced go examples inside
the bullet list because they are missing surrounding blank lines. Update the
markdown in this section so each code fence under the account.go, auth.go, and
twofa.go bullets has an empty line before and after it, keeping the same content
but formatting it as a proper fenced block within the list.
In `@frontend/src/components/folders/SortableFolderRow.tsx`:
- Around line 10-24: `SortableFolderRow` currently gates drag-and-drop and edit
affordances with `canEdit`, but add coverage to prove the disabled state is
fully enforced. Update the tests for `SortableFolderRow` to render with
`canEdit={false}` and assert the grip/rename/delete controls are not present and
the `useSortable` interaction is disabled for that row. Use the component’s
`SortableFolderRow`/`useSortable` behavior in the test setup so the missing
affordances and disabled drag state are verified together.
In `@frontend/src/components/Footer.tsx`:
- Around line 5-10: The Footer link uses global fixed positioning with
bottom-right anchoring and a z-index, which can overlap other UI in pages that
also place controls there. Update Footer so its placement is conditional or
offset-aware, and verify it does not collide with bottom-right elements rendered
by routes like SongPage and BandPage. Use the Footer component and its fixed
positioning styles as the main place to adjust behavior.
In `@frontend/src/components/songs/SongRow.tsx`:
- Around line 9-22: Add test coverage for the new hidden-action branch in
SongRow by asserting the action button is not rendered when SongRow is passed
canEdit={false}; keep existing default behavior tests intact and reference the
SongRow component/conditional action-label rendering so the new branch is
exercised.
In `@frontend/src/pages/AdminPage.test.tsx`:
- Around line 28-126: Add test coverage for the missing delete flows in
AdminPage. The current AdminPage.test.tsx covers user deletion, band listing,
and adding allowed emails, but not the delete actions wired through
useDeleteAdminBand in AdminBandsPanel or useRemoveAllowedEmail in
AdminAccessPolicyPanel. Extend the fetch stub to handle DELETE requests for
/api/admin/bands/:id and /api/admin/access-policy/emails/:id, then add test
cases that open the Bands tab and Access Policy tab, trigger the delete/remove
actions, and assert the expected DELETE calls were made.
In `@frontend/src/pages/AdminPage.tsx`:
- Around line 24-53: The tab switcher in AdminPage is missing the required ARIA
linkage between each tab and its content. Update the tab buttons in AdminPage so
each one has a stable id and aria-controls pointing to its panel, and wrap each
rendered panel (AdminUsersPanel, AdminBandsPanel, AdminAccessPolicyPanel) with
role="tabpanel" and a matching aria-labelledby. Keep the existing tab state
logic, but ensure the tabs and panels are explicitly associated for assistive
technologies.
- Around line 57-176: `AdminUsersPanel` and `AdminBandsPanel` duplicate the same
list rendering and confirm-delete flow, so extract the shared behavior into a
reusable generic component or hook (for example, a common admin entity
list/delete helper) and keep only the entity-specific fields and copy in each
panel. Move the repeated `target` state, empty-state UI, delete button wiring,
error alert, and `ConfirmModal` setup into the shared abstraction, using the
existing `AdminUsersPanel` and `AdminBandsPanel` symbols as the call sites.
In `@internal/handlers/admin_test.go`:
- Around line 134-186: Add negative-path tests in TestAdminAccessPolicy for
malformed request bodies so the c.Bind failure path is covered for both
AdminSetAccessPolicy and AdminAddAllowedEmail. Reuse the existing admin setup
and issue PUT /api/admin/access-policy and POST /api/admin/access-policy/emails
with invalid JSON, then assert each returns HTTP 400; keep the new cases near
the current success-path checks so the coverage stays tied to
AdminSetAccessPolicy and AdminAddAllowedEmail.
In `@internal/handlers/admin.go`:
- Around line 42-48: The AdminUsers handler currently returns the full result
set from Repo.AllUsers with no limit or offset, so update AdminUsers to support
pagination and avoid loading the entire table on every request; use the existing
AdminUsers and AdminBands handlers as the touchpoints, add request-driven
limit/offset (or page/cursor) handling, and thread those values through the
repository call so only a bounded slice is fetched and serialized.
In `@internal/handlers/auth_test.go`:
- Around line 216-272: The signup-related tests are making mutating POST
requests without the required Sec-Fetch-Site header. Update the shared test
helper used by TestSignupAccessPolicy, TestSignupAccessPolicyAllowsAdminEmail,
and TestMeIncludesIsAdmin so that postJSON sends Sec-Fetch-Site: same-origin on
mutating requests, and keep the individual tests using that helper unchanged.
In `@internal/model/accesspolicy.go`:
- Around line 13-18: `AllowedEmail.Email` is only protected by a case-sensitive
unique index, so allow-list matching can break when callers pass differently
cased or padded addresses. Update the repository layer in `AddAllowedEmail` and
`EmailAllowed` to normalize emails consistently before save and lookup (for
example, trim and lowercase the input), so the behavior does not depend on every
caller in the admin and signup paths doing it correctly. Use the `AllowedEmail`
model and the repository methods as the single source of truth for email
normalization.
In `@internal/repository/users.go`:
- Around line 87-92: The user cleanup in the delete transaction only removes
`BandMember` rows and `BandInvite` rows where `invited_user_id` matches, but it
leaves invites created by the user behind. Update the delete logic in the user
repository cleanup flow to also delete `BandInvite` records where `created_by =
?` using the same `userID`, alongside the existing `invited_user_id` removal, so
all references to the deleted user are cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9b3b4daa-d39e-49e2-9845-ffe2c136685c
📒 Files selected for processing (48)
.gitignorecmd/bandwidth/main.gocmd/bandwidth/server.godocker-compose.ymldocs/superpowers/plans/2026-07-03-admin-access-policy.mddocs/superpowers/specs/2026-07-03-admin-access-policy-design.mdfrontend/src/App.tsxfrontend/src/components/Footer.test.tsxfrontend/src/components/Footer.tsxfrontend/src/components/Layout.tsxfrontend/src/components/RequireAdmin.test.tsxfrontend/src/components/RequireAdmin.tsxfrontend/src/components/bands/BandFolderRow.tsxfrontend/src/components/bands/BandFolderSidebar.tsxfrontend/src/components/bands/BandSongList.tsxfrontend/src/components/bands/BandSongRow.tsxfrontend/src/components/folders/SortableFolderRow.tsxfrontend/src/components/folders/SortableSongList.tsxfrontend/src/components/songs/SongRow.tsxfrontend/src/hooks/admin.tsfrontend/src/lib/types.tsfrontend/src/pages/AdminPage.test.tsxfrontend/src/pages/AdminPage.tsxfrontend/src/pages/HomePage.tsxinternal/handlers/account.gointernal/handlers/admin.gointernal/handlers/admin_test.gointernal/handlers/api.gointernal/handlers/api_test.gointernal/handlers/auth.gointernal/handlers/auth_test.gointernal/handlers/passwordreset.gointernal/handlers/twofa.gointernal/middleware/auth.gointernal/middleware/auth_test.gointernal/model/accesspolicy.gointernal/repository/admin.gointernal/repository/admin_test.gointernal/repository/bandinvites.gointernal/repository/bands.gointernal/repository/bands_test.gointernal/repository/folders.gointernal/repository/maintenance.gointernal/repository/repository.gointernal/repository/repository_test.gointernal/repository/songs.gointernal/repository/users.gointernal/repository/users_test.go
💤 Files with no reviewable changes (2)
- frontend/src/components/bands/BandFolderRow.tsx
- frontend/src/components/bands/BandSongRow.tsx
Comment on lines
+1038
to
+1071
| - `internal/handlers/account.go` — three identical occurrences (lines 20, 59, 102): | ||
| ```go | ||
| return c.JSON(http.StatusOK, userResponse(user)) | ||
| ``` | ||
| become: | ||
| ```go | ||
| return c.JSON(http.StatusOK, a.userResponse(user)) | ||
| ``` | ||
|
|
||
| - `internal/handlers/auth.go` — line 86: | ||
| ```go | ||
| return c.JSON(http.StatusCreated, userResponse(user)) | ||
| ``` | ||
| becomes: | ||
| ```go | ||
| return c.JSON(http.StatusCreated, a.userResponse(user)) | ||
| ``` | ||
| and line 132: | ||
| ```go | ||
| return c.JSON(http.StatusOK, userResponse(user)) | ||
| ``` | ||
| becomes: | ||
| ```go | ||
| return c.JSON(http.StatusOK, a.userResponse(user)) | ||
| ``` | ||
|
|
||
| - `internal/handlers/twofa.go` — line 100: | ||
| ```go | ||
| return c.JSON(http.StatusOK, userResponse(user)) | ||
| ``` | ||
| becomes: | ||
| ```go | ||
| return c.JSON(http.StatusOK, a.userResponse(user)) | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Fenced code blocks need surrounding blank lines (MD031).
markdownlint flags each go fence nested in this bullet list for missing blank lines before/after.
📝 Add blank lines around each fence
- `internal/handlers/account.go` — three identical occurrences (lines 20, 59, 102):
+
```go
return c.JSON(http.StatusOK, userResponse(user))- become:
-
return c.JSON(http.StatusOK, a.userResponse(user))
(apply the same pattern to the `auth.go` and `twofa.go` bullets immediately below)
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
- `internal/handlers/account.go` — three identical occurrences (lines 20, 59, 102):
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1039-1039: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1041-1041: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1043-1043: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1048-1048: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1050-1050: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1052-1052: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1054-1054: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1056-1056: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1058-1058: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1060-1060: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1065-1065: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1067-1067: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 1069-1069: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/superpowers/plans/2026-07-03-admin-access-policy.md` around lines 1038 -
1071, markdownlint is flagging the fenced go examples inside the bullet list
because they are missing surrounding blank lines. Update the markdown in this
section so each code fence under the account.go, auth.go, and twofa.go bullets
has an empty line before and after it, keeping the same content but formatting
it as a proper fenced block within the list.
Source: Linters/SAST tools
Comment on lines
+10
to
+24
| canEdit = true, | ||
| selected, | ||
| onSelect, | ||
| onRename, | ||
| onDelete, | ||
| }: { | ||
| folder: Folder; | ||
| canEdit?: boolean; | ||
| selected: boolean; | ||
| onSelect: () => void; | ||
| onRename: (name: string) => void; | ||
| onDelete: () => void; | ||
| }) { | ||
| const {attributes, listeners, setNodeRef, transform, transition} = | ||
| useSortable({id: folder.id}); | ||
| useSortable({id: folder.id, disabled: !canEdit}); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Correct gating of DnD and edit actions; add canEdit=false test coverage.
Disabling useSortable alongside hiding the grip button and rename/delete controls correctly removes both the affordance and the underlying capability. Consider adding a test asserting these controls are absent and drag is disabled when canEdit={false}.
Also applies to: 49-58, 79-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/folders/SortableFolderRow.tsx` around lines 10 - 24,
`SortableFolderRow` currently gates drag-and-drop and edit affordances with
`canEdit`, but add coverage to prove the disabled state is fully enforced.
Update the tests for `SortableFolderRow` to render with `canEdit={false}` and
assert the grip/rename/delete controls are not present and the `useSortable`
interaction is disabled for that row. Use the component’s
`SortableFolderRow`/`useSortable` behavior in the test setup so the missing
affordances and disabled drag state are verified together.
Comment on lines
+5
to
+10
| <a | ||
| href="https://github.com/jwhumphries" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="text-base-content/30 hover:text-base-content/60 fixed right-3 bottom-2 z-10 text-xs transition-colors" | ||
| > |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Verify fixed-position footer doesn't overlap other bottom-right UI.
Global fixed positioning at right-3 bottom-2 with z-10 renders on every page (App.tsx mounts it above the route tree per the App.tsx snippet). Worth double-checking it doesn't overlap toasts, floating buttons, or other bottom-right anchored controls on pages like SongPage/BandPage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/Footer.tsx` around lines 5 - 10, The Footer link uses
global fixed positioning with bottom-right anchoring and a z-index, which can
overlap other UI in pages that also place controls there. Update Footer so its
placement is conditional or offset-aware, and verify it does not collide with
bottom-right elements rendered by routes like SongPage and BandPage. Use the
Footer component and its fixed positioning styles as the main place to adjust
behavior.
Comment on lines
+9
to
+22
| linkTo, | ||
| onPracticed, | ||
| canEdit = true, | ||
| actionLabel = 'Practiced', | ||
| }: { | ||
| song: SongListItem; | ||
| linkTo: string; | ||
| onPracticed: (id: number, date: string) => void; | ||
| canEdit?: boolean; | ||
| actionLabel?: string; | ||
| }) { | ||
| return ( | ||
| <li className="group border-base-300/60 bg-base-100 hover:border-base-300 flex items-center gap-3 rounded-box border p-3 transition-all hover:shadow-md sm:gap-4 sm:p-4"> | ||
| <Link to={`/songs/${song.id}`} className="min-w-0 flex-1"> | ||
| <Link to={linkTo} className="min-w-0 flex-1"> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Solid prop extension; add coverage for canEdit=false.
Defaults preserve existing behavior for all current callers, and the conditional gating logic is correct. Since this is a new branch (hidden action button), consider adding a test asserting the button is not rendered when canEdit={false}.
Also applies to: 43-52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/songs/SongRow.tsx` around lines 9 - 22, Add test
coverage for the new hidden-action branch in SongRow by asserting the action
button is not rendered when SongRow is passed canEdit={false}; keep existing
default behavior tests intact and reference the SongRow component/conditional
action-label rendering so the new branch is exercised.
Comment on lines
+42
to
+48
| func (a *API) AdminUsers(c *echo.Context) error { | ||
| users, err := a.Repo.AllUsers() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return c.JSON(http.StatusOK, users) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Consider pagination for unbounded admin listings.
AllUsers/AllBands are returned in full with no limit/offset. Fine at current scale, but worth revisiting if the user/band count grows meaningfully, since the whole table is loaded and serialized on every admin panel visit.
Also applies to: 75-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handlers/admin.go` around lines 42 - 48, The AdminUsers handler
currently returns the full result set from Repo.AllUsers with no limit or
offset, so update AdminUsers to support pagination and avoid loading the entire
table on every request; use the existing AdminUsers and AdminBands handlers as
the touchpoints, add request-driven limit/offset (or page/cursor) handling, and
thread those values through the repository call so only a bounded slice is
fetched and serialized.
Comment on lines
+216
to
+272
| func TestSignupAccessPolicy(t *testing.T) { | ||
| e, api := newTestAPI(t) | ||
| if err := api.Repo.SetAccessPolicyEnabled(true); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| rec := postJSON(e, "/api/auth/signup", | ||
| `{"username":"outsider","email":"outsider@example.com","password":"hunter2hunter2"}`) | ||
| if rec.Code != http.StatusForbidden { | ||
| t.Fatalf("status = %d, want 403", rec.Code) | ||
| } | ||
|
|
||
| admin, _ := api.Repo.CreateUser("admin", "admin@example.com", "h") | ||
| if _, err := api.Repo.AddAllowedEmail("friend@example.com", admin.ID); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| rec = postJSON(e, "/api/auth/signup", | ||
| `{"username":"friend","email":"friend@example.com","password":"hunter2hunter2"}`) | ||
| if rec.Code != http.StatusCreated { | ||
| t.Fatalf("allow-listed signup: %d %s", rec.Code, rec.Body.String()) | ||
| } | ||
| } | ||
|
|
||
| func TestSignupAccessPolicyAllowsAdminEmail(t *testing.T) { | ||
| e, api := newTestAPI(t) | ||
| api.AdminEmails = map[string]bool{"admin@example.com": true} | ||
| if err := api.Repo.SetAccessPolicyEnabled(true); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| rec := postJSON(e, "/api/auth/signup", | ||
| `{"username":"admin","email":"admin@example.com","password":"hunter2hunter2"}`) | ||
| if rec.Code != http.StatusCreated { | ||
| t.Fatalf("admin signup: %d %s", rec.Code, rec.Body.String()) | ||
| } | ||
| } | ||
|
|
||
| func TestMeIncludesIsAdmin(t *testing.T) { | ||
| e, api := newTestAPI(t) | ||
| api.AdminEmails = map[string]bool{"alice@example.com": true} | ||
| rec := postJSON(e, "/api/auth/signup", | ||
| `{"username":"alice","email":"alice@example.com","password":"hunter2hunter2"}`) | ||
| cookie := sessionCookie(t, rec) | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/api/me", nil) | ||
| req.AddCookie(cookie) | ||
| mrec := httptest.NewRecorder() | ||
| e.ServeHTTP(mrec, req) | ||
|
|
||
| var body map[string]any | ||
| if err := json.Unmarshal(mrec.Body.Bytes(), &body); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if body["isAdmin"] != true { | ||
| t.Errorf("isAdmin = %v, want true", body["isAdmin"]) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Set Sec-Fetch-Site: same-origin on the new mutating signup requests.
TestSignupAccessPolicy, TestSignupAccessPolicyAllowsAdminEmail, and TestMeIncludesIsAdmin all POST to /api/auth/signup via postJSON, which doesn't set Sec-Fetch-Site. As per coding guidelines, "Tests must set Sec-Fetch-Site: same-origin on mutating requests."
🔧 Proposed fix (update shared helper)
func postJSON(e *echo.Echo, path, body string, cookies ...*http.Cookie) *httptest.ResponseRecorder {
req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(body))
req.Header.Set("Content-Type", "application/json")
+ req.Header.Set("Sec-Fetch-Site", "same-origin")
for _, c := range cookies {
req.AddCookie(c)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handlers/auth_test.go` around lines 216 - 272, The signup-related
tests are making mutating POST requests without the required Sec-Fetch-Site
header. Update the shared test helper used by TestSignupAccessPolicy,
TestSignupAccessPolicyAllowsAdminEmail, and TestMeIncludesIsAdmin so that
postJSON sends Sec-Fetch-Site: same-origin on mutating requests, and keep the
individual tests using that helper unchanged.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/repository/admin.go (1)
63-79: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnforce the access-policy singleton at the database level. Two concurrent first-time callers can still create duplicate rows here, since the table only has a plain primary key and no uniqueness guard for the singleton record. A fixed ID or
ON CONFLICT DO NOTHING/FirstOrCreatewould make this invariant safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/repository/admin.go` around lines 63 - 79, The accessPolicy singleton is still race-prone because Repo.accessPolicy can create duplicate rows when two first-time callers hit the ErrRecordNotFound path together. Update accessPolicy to enforce the singleton at the database level by using a fixed primary key or an atomic create path such as FirstOrCreate/ON CONFLICT DO NOTHING, so only one AccessPolicy row can ever be inserted even under concurrent access.
♻️ Duplicate comments (2)
internal/handlers/auth_test.go (1)
37-46: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
postJSONstill missingSec-Fetch-Site: same-originon mutating requests.New tests (
TestSignupAccessPolicy,TestSignupAccessPolicyAllowsAdminEmail,TestMeIncludesIsAdmin) POST to/api/auth/signupvia this helper without the header. As per coding guidelines, "Tests must setSec-Fetch-Site: same-originon mutating requests."🔧 Proposed fix
func postJSON(e *echo.Echo, path, body string, cookies ...*http.Cookie) *httptest.ResponseRecorder { req := httptest.NewRequest(http.MethodPost, path, strings.NewReader(body)) req.Header.Set("Content-Type", "application/json") + req.Header.Set("Sec-Fetch-Site", "same-origin") for _, c := range cookies { req.AddCookie(c) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/handlers/auth_test.go` around lines 37 - 46, The shared test helper postJSON in auth_test.go is missing the required Sec-Fetch-Site header for mutating requests. Update postJSON so every POST request it builds includes Sec-Fetch-Site set to same-origin before calling e.ServeHTTP, ensuring tests like TestSignupAccessPolicy, TestSignupAccessPolicyAllowsAdminEmail, and TestMeIncludesIsAdmin use the correct access policy context.Source: Coding guidelines
frontend/src/pages/AdminPage.tsx (1)
87-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate list/confirm-delete pattern still unresolved.
AdminUsersPanelandAdminBandsPanelstill repeat the sametargetstate, empty-state block, delete button wiring, error alert, andConfirmModalusage, differing only in field names/copy. This was flagged previously without an "addressed" resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/pages/AdminPage.tsx` around lines 87 - 206, The duplicate list-and-confirm-delete flow still exists in AdminUsersPanel and AdminBandsPanel, with the same target state, empty-state UI, delete button handler, error alert, and ConfirmModal wiring repeated. Refactor this shared pattern into a reusable component or helper that accepts the specific item list, labels, delete mutation hook, and display fields, and update AdminUsersPanel and AdminBandsPanel to use it while preserving their different copy and item-specific properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/repository/admin.go`:
- Around line 63-79: The accessPolicy singleton is still race-prone because
Repo.accessPolicy can create duplicate rows when two first-time callers hit the
ErrRecordNotFound path together. Update accessPolicy to enforce the singleton at
the database level by using a fixed primary key or an atomic create path such as
FirstOrCreate/ON CONFLICT DO NOTHING, so only one AccessPolicy row can ever be
inserted even under concurrent access.
---
Duplicate comments:
In `@frontend/src/pages/AdminPage.tsx`:
- Around line 87-206: The duplicate list-and-confirm-delete flow still exists in
AdminUsersPanel and AdminBandsPanel, with the same target state, empty-state UI,
delete button handler, error alert, and ConfirmModal wiring repeated. Refactor
this shared pattern into a reusable component or helper that accepts the
specific item list, labels, delete mutation hook, and display fields, and update
AdminUsersPanel and AdminBandsPanel to use it while preserving their different
copy and item-specific properties.
In `@internal/handlers/auth_test.go`:
- Around line 37-46: The shared test helper postJSON in auth_test.go is missing
the required Sec-Fetch-Site header for mutating requests. Update postJSON so
every POST request it builds includes Sec-Fetch-Site set to same-origin before
calling e.ServeHTTP, ensuring tests like TestSignupAccessPolicy,
TestSignupAccessPolicyAllowsAdminEmail, and TestMeIncludesIsAdmin use the
correct access policy context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8dd280b2-bbf2-4508-83bc-c6eb5fd53729
📒 Files selected for processing (13)
cmd/bandwidth/server_test.godocs/superpowers/plans/2026-07-03-admin-access-policy.mdfrontend/src/components/folders/SortableFolderRow.test.tsxfrontend/src/components/songs/SongRow.test.tsxfrontend/src/pages/AdminPage.test.tsxfrontend/src/pages/AdminPage.tsxfrontend/src/pages/JoinPage.test.tsxfrontend/src/pages/JoinPage.tsxinternal/handlers/admin_test.gointernal/handlers/auth.gointernal/handlers/auth_test.gointernal/repository/admin.gointernal/repository/users.go
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
1 participant
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.
Summary by CodeRabbit
New Features
/admin) for managing users, bands, and signup access policy, gated to authorized admin accounts.isAdminflag to the current-user response.Bug Fixes
Chores
.worktrees/.