Skip to content

auth: replace copy-paste JWT checks with real middleware#194

Open
sumnerevans wants to merge 7 commits intomasterfrom
auth-middleware
Open

auth: replace copy-paste JWT checks with real middleware#194
sumnerevans wants to merge 7 commits intomasterfrom
auth-middleware

Conversation

@sumnerevans
Copy link
Copy Markdown
Contributor

Summary

  • Adds a shared parseTokenByIssuer helper that replaces the nearly-identical isAdminByToken and isVolunteerByToken functions
  • Adds VolunteerAuthMiddleware mirroring the existing AdminAuthMiddleware
  • Consolidates all protected admin routes (pages + API) into a single subrouter wrapped with AdminAuthMiddleware via StripPrefix("/admin", ...), consistent with the existing pattern
  • Wraps volunteer scan/checkin routes with VolunteerAuthMiddleware
  • Removes the duplicated cookie+token auth checks that were scattered across GetAdminTeamsTemplate, GetAdminDietaryRestrictionsTemplate, GetVolunteerScanTemplate, and HandleVolunteerCheckIn

Closes #73

Test plan

  • GET /admin/teams without cookie → redirects to /admin/login
  • GET /admin/dietaryrestrictions without cookie → redirects to /admin/login
  • GET /admin/login without cookie → renders login page (unprotected)
  • GET /volunteer/scan without cookie → redirects to /volunteer/login
  • GET /volunteer/checkin without cookie → redirects to /volunteer/login
  • Admin login flow → admin pages render correctly
  • Volunteer login flow → volunteer scan page renders correctly

🤖 Generated with Claude Code

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors admin/volunteer authentication to use shared JWT parsing and dedicated middleware, removing duplicated cookie/token checks from individual handlers and consolidating protected route registration.

Changes:

  • Introduces parseTokenByIssuer and updates admin/volunteer login handlers to use it.
  • Adds VolunteerAuthMiddleware and applies auth middleware to volunteer scan/checkin routes.
  • Consolidates all protected admin pages + API endpoints under a single /admin/ subrouter wrapped with AdminAuthMiddleware, and simplifies template extra-data merging via maps.Copy.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
internal/volunteer.go Removes inline volunteer auth checks; uses shared token parsing for volunteer/admin token validation.
internal/middleware.go Adds shared JWT parsing helper; updates admin middleware redirect target; adds volunteer auth middleware.
internal/application.go Consolidates protected admin routing under one subrouter + middleware; wraps volunteer routes with middleware; uses maps.Copy.
internal/admin.go Removes inline admin auth checks; uses shared token parsing for admin login flow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/admin.go
@@ -179,7 +127,7 @@ func (a *Application) HandleAdminEmailLogin(w http.ResponseWriter, r *http.Reque
tok := r.URL.Query().Get("tok")
log := zerolog.Ctx(r.Context())
log.Info().Str("token", tok).Msg("got token")
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logs the raw login JWT from the query string. JWTs are bearer credentials and should not be written to logs (they can be replayed if leaked via log aggregation/support tooling). Please remove this field from logs or replace it with a safe surrogate (e.g., token hash / last few chars) and rely on request_id for correlation.

Suggested change
log.Info().Str("token", tok).Msg("got token")
log.Info().Msg("got admin login token")

Copilot uses AI. Check for mistakes.
Comment thread internal/volunteer.go

func (a *Application) HandleVolunteerEmailLogin(w http.ResponseWriter, r *http.Request) {
tok := r.URL.Query().Get("tok")
zerolog.Ctx(r.Context()).Info().Str("token", tok).Msg("got token")
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logs the raw login JWT from the query string. JWTs are bearer credentials and should not be written to logs (they can be replayed if leaked via log aggregation/support tooling). Please remove this field from logs or replace it with a safe surrogate (e.g., token hash / last few chars) and rely on request_id for correlation.

Suggested change
zerolog.Ctx(r.Context()).Info().Str("token", tok).Msg("got token")
zerolog.Ctx(r.Context()).Info().Msg("received volunteer login token")

Copilot uses AI. Check for mistakes.
@sumnerevans sumnerevans force-pushed the auth-middleware branch 2 times, most recently from f210558 to 5603ce4 Compare April 9, 2026 14:14
@sumnerevans sumnerevans marked this pull request as draft April 9, 2026 14:15
@sumnerevans sumnerevans marked this pull request as ready for review April 9, 2026 14:15
sumnerevans and others added 7 commits April 11, 2026 18:23
Add a shared parseTokenByIssuer helper and VolunteerAuthMiddleware,
consolidate all protected admin routes into a single subrouter wrapped
with AdminAuthMiddleware, and remove the duplicated cookie+token checks
that were scattered across handler and template functions.

Closes #73

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Tests for parseTokenByIssuer (empty, correct issuer, wrong issuer,
expired, wrong signing key) and both AdminAuthMiddleware and
VolunteerAuthMiddleware (no cookie, invalid token, wrong issuer,
valid token).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract BuildRouter from Start so tests can exercise the full router
without starting a real server. Add routing_test.go covering all
protected admin and volunteer routes (no cookie, wrong-issuer cookie,
valid cookie) and key unprotected routes.

Uses an in-memory SQLite DB with MaxOpenConns/MaxIdleConns=1 so
migrations run correctly on the shared connection.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
middleware_test.go now only covers parseTokenByIssuer (JWT parsing
logic in isolation). Routing tests cover the full middleware+routing
behavior: no cookie, malformed token, wrong-issuer token, and valid
token — with all protected routes checked in table-driven loops.

32 tests → 15 tests, no loss of coverage.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The renderFn loop was only redirecting to /register/teacher/login for
pages marked RedirectIfLoggedIn:true (login/confirm-email pages). Pages
with RedirectIfLoggedIn:false (schoolinfo, teams, team/edit, addmember)
would render with nil data when accessed without a session.

Now any unauthenticated GET on a /register/teacher/* protected page
redirects to /register/teacher/login with 303.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Sumner Evans <me@sumnerevans.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

use an actual middleware for auth

2 participants