diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 05fb2cd3..21468c31 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -209,6 +209,21 @@ func logMissingCSRFToken(req *events.LambdaFunctionURLRequest, csrfToken string) req.RequestContext.HTTP.Method, req.RequestContext.HTTP.Path, req.RequestContext.HTTP.SourceIP) } +// requireAuth verifies the request carries a valid authentication credential +// of any kind (admin API key, user API key, or session bearer token). +// +// Used as a defence-in-depth check by Router.Route for AuthUser routes: +// validateSecurity → authenticate already runs before dispatch, but if a +// future refactor reorders middleware or a new route bypasses +// validateSecurity, this check still rejects unauthenticated requests at +// the router level. Returns nil on success, a 401 ClientError otherwise. +func (h *Handler) requireAuth(ctx context.Context, req *events.LambdaFunctionURLRequest) error { + if h.authenticate(ctx, req) { + return nil + } + return NewClientError(401, "authentication required") +} + // requireAdmin checks if the current user has admin role. // Accepts both admin API-key auth and Bearer token auth. func (h *Handler) requireAdmin(ctx context.Context, req *events.LambdaFunctionURLRequest) (*Session, error) { diff --git a/internal/api/router.go b/internal/api/router.go index 6a514531..0f9176a8 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -14,17 +14,26 @@ type RouteHandler func(ctx context.Context, req *events.LambdaFunctionURLRequest // AuthLevel controls how Router.Route() enforces authentication. // The zero value is AuthAdmin — secure by default. // Any Route without an explicit Auth field requires admin access. +// +// Router.Route enforces these levels itself as a defence-in-depth check, +// in addition to the validateSecurity → authenticate middleware that runs +// earlier in the request pipeline. If middleware ordering ever changes or +// a new route bypasses validateSecurity, the router-level enforcement +// still rejects unauthorized requests. type AuthLevel int const ( - // AuthAdmin requires admin role (API key or admin bearer token). - // Zero value — any Route without an explicit Auth field gets this. + // AuthAdmin requires admin role (admin API key or admin bearer-token + // session). Zero value — any Route without an explicit Auth field gets + // this. Enforced by Router.Route via h.requireAdmin. AuthAdmin AuthLevel = iota - // AuthUser requires any authenticated user. Use for self-service - // endpoints (logout, profile, API key management). + // AuthUser requires any authenticated user (admin API key, user API + // key, or any valid bearer-token session). Use for self-service + // endpoints (logout, profile, API key management). Enforced by + // Router.Route via h.requireAuth. AuthUser // AuthPublic requires no authentication. Must also be listed in - // isPublicEndpoint() for middleware bypass. + // isPublicEndpoint() so the middleware skips its auth/CSRF checks. AuthPublic ) @@ -214,14 +223,28 @@ func (r *Router) registerRoutes() { } // Route finds and executes the matching route handler. -// Routes with Auth == AuthAdmin (the default) require admin access before the handler is called. +// +// Authentication enforcement is defence-in-depth: validateSecurity → +// authenticate already runs in the middleware pipeline before dispatch, +// but Router.Route also enforces the per-route Auth level so routes stay +// protected even if middleware ordering changes or a new code path +// bypasses validateSecurity. Routes with Auth == AuthAdmin (the default) +// require admin access; AuthUser routes require any authenticated user; +// AuthPublic routes are unauthenticated. func (r *Router) Route(ctx context.Context, method, path string, req *events.LambdaFunctionURLRequest) (any, error) { for _, route := range r.routes { if r.matches(route, method, path) { - if route.Auth == AuthAdmin { + switch route.Auth { + case AuthAdmin: if _, err := r.h.requireAdmin(ctx, req); err != nil { return nil, err } + case AuthUser: + if err := r.h.requireAuth(ctx, req); err != nil { + return nil, err + } + case AuthPublic: + // no auth check; relied upon by middleware via isPublicEndpoint } params := r.extractParams(route, path) return route.Handler(ctx, req, params) diff --git a/internal/api/router_authuser_test.go b/internal/api/router_authuser_test.go new file mode 100644 index 00000000..eb0858fa --- /dev/null +++ b/internal/api/router_authuser_test.go @@ -0,0 +1,138 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// These tests cover the defence-in-depth AuthUser enforcement added to +// Router.Route — see issue #60. Before the fix, AuthUser routes (e.g. +// /api/auth/logout, /api/api-keys, /api/federation/iac) fell through the +// router with no auth check; only the validateSecurity middleware +// protected them. These tests pin the router-level enforcement so a +// future middleware refactor can't silently expose them. + +// TestRouterAuthUser_NoCredentials_Rejects verifies that an AuthUser +// route returns a 401 ClientError when no credential is presented. +func TestRouterAuthUser_NoCredentials_Rejects(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{}, + } + + _, err := r.Route(ctx, "POST", "/api/auth/logout", req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 401, ce.code) +} + +// TestRouterAuthUser_InvalidBearerToken_Rejects verifies that an AuthUser +// route returns 401 when the bearer token is not recognised by the auth +// service. +func TestRouterAuthUser_InvalidBearerToken_Rejects(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + mockAuth.On("ValidateUserAPIKeyAPI", ctx, ""). + Return(nil, nil, errors.New("empty key")) + mockAuth.On("ValidateSession", ctx, "bad-token"). + Return(nil, errors.New("expired")) + h := &Handler{auth: mockAuth} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer bad-token"}, + } + + _, err := r.Route(ctx, "GET", "/api/api-keys", req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected ClientError, got %T: %v", err, err) + assert.Equal(t, 401, ce.code) +} + +// TestRouterAuthUser_ValidUserSession_Accepts verifies that an AuthUser +// route dispatches to the handler when a valid non-admin user session is +// present. Before the fix, AuthUser dispatch was unconditional — this +// test ensures the new check doesn't accidentally reject legitimate +// users. +func TestRouterAuthUser_ValidUserSession_Accepts(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + userSession := &Session{UserID: "11111111-1111-1111-1111-111111111111", Role: "user"} + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) + mockAuth.On("Logout", ctx, "user-token").Return(nil) + h := &Handler{auth: mockAuth} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + } + + result, err := r.Route(ctx, "POST", "/api/auth/logout", req) + require.NoError(t, err) + assert.NotNil(t, result) +} + +// TestRouterAuthPublic_NoCredentials_Accepts verifies that AuthPublic +// routes still dispatch with no credentials — the new switch in +// Router.Route must not regress public-endpoint behaviour. +func TestRouterAuthPublic_NoCredentials_Accepts(t *testing.T) { + ctx := context.Background() + h := &Handler{} + r := NewRouter(h) + + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{}, + } + + // /api/health is AuthPublic; healthCheckHandler doesn't depend on auth. + _, err := r.Route(ctx, "GET", "/api/health", req) + require.NoError(t, err) +} + +// TestRequireAuth_AdminAPIKey verifies the new requireAuth helper accepts +// the admin API key. +func TestRequireAuth_AdminAPIKey(t *testing.T) { + h := &Handler{apiKey: "admin-secret"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"X-API-Key": "admin-secret"}, + } + require.NoError(t, h.requireAuth(context.Background(), req)) +} + +// TestRequireAuth_UserSession verifies requireAuth accepts a valid +// non-admin user session. +func TestRequireAuth_UserSession(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + userSession := &Session{UserID: "uid", Role: "user"} + mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer user-token"}, + } + require.NoError(t, h.requireAuth(ctx, req)) +} + +// TestRequireAuth_NoCredential_Rejects verifies requireAuth returns a 401 +// ClientError when no credential is presented. +func TestRequireAuth_NoCredential_Rejects(t *testing.T) { + mockAuth := new(MockAuthService) + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{}} + err := h.requireAuth(context.Background(), req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 401, ce.code) +}