Refactor/extract utilities #11
Conversation
- Create internal/api/middleware/ package with auth, permissions, logging, security components - Add PermissionMiddleware with SSAR-based Kubernetes permission checking - Wire pods listing endpoint to use new RequirePermission middleware - Maintain identical HTTP responses and status codes (no breaking changes) - Add comprehensive unit tests for auth and permission middleware - Fix chi middleware import conflicts with proper aliasing Acceptance: - GET /api/pods now uses PermissionMiddleware for 'list pods' SSAR check - Auth mode 'none' bypasses permission checks (development mode) - All existing tests pass, new middleware tests at 100% coverage - Foundation ready for broader middleware adoption in future PRs
There was a problem hiding this comment.
Pull Request Overview
This PR extracts and centralizes HTTP middleware components from the API server into a dedicated internal/api/middleware package to reduce code duplication and improve maintainability.
- Extracts reusable middleware for authentication, permissions, impersonation, logging, and security
- Implements one route group (pods listing) using the new permission middleware as a proof of concept
- Adds comprehensive test coverage for the new middleware components
Reviewed Changes
Copilot reviewed 10 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/api/server.go | Updates imports to use new middleware package and integrates permission middleware for pods endpoint |
| internal/api/middleware/security.go | Centralizes security headers and CORS handling middleware |
| internal/api/middleware/permissions_test.go | Comprehensive unit tests for permission middleware functionality |
| internal/api/middleware/permissions.go | Implements RBAC permission checking middleware using Kubernetes SSAR |
| internal/api/middleware/logging.go | Enhanced request logging with security context and audit capabilities |
| internal/api/middleware/impersonation.go | Extracts Kubernetes client impersonation logic into reusable middleware |
| internal/api/middleware/doc.go | Package documentation |
| internal/api/middleware/auth_test.go | Unit tests for authentication extraction utilities |
| internal/api/middleware/auth.go | Centralized authentication extraction utilities |
| internal/api/middleware/README.md | Comprehensive documentation for the middleware package |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Permissions Policy (restrict dangerous browser features) | ||
| permissionsPolicy := "camera=(), microphone=(), geolocation=(), payment=(), " + | ||
| "usb=(), magnetometer=(), gyroscope=(), accelerometer=(), " + | ||
| "ambient-light-sensor=(), autoplay=self, encrypted-media=*" |
There was a problem hiding this comment.
The permissions policy string construction is hard to maintain. Consider using a slice of policies and joining them for better readability and maintainability.
Copilot uses AI. Check for mistakes.
| // slicesEqual checks if two string slices are equal | ||
| func slicesEqual(a, b []string) bool { | ||
| if len(a) != len(b) { | ||
| return false | ||
| } | ||
| for i, v := range a { | ||
| if v != b[i] { | ||
| return false | ||
| } | ||
| } | ||
| return true | ||
| } |
There was a problem hiding this comment.
Consider using slices.Equal from the Go standard library (available since Go 1.21) instead of implementing a custom slicesEqual function, or move this utility to a shared package if it's used elsewhere.
Copilot uses AI. Check for mistakes.
| timeSeriesWSManager *TimeSeriesWSManager | ||
| capabilityService *authz.CapabilityService | ||
|
|
||
| // New middleware components (PR 2) |
There was a problem hiding this comment.
The comment '(PR 2)' is not meaningful in the final codebase. Consider removing this temporary comment or replacing it with a more descriptive comment about the purpose of these middleware components.
| // New middleware components (PR 2) | |
| // Middleware for permission and impersonation handling |
Copilot uses AI. Check for mistakes.
| // Set authentication middleware on WebSocket hub | ||
| s.wsHub.SetAuthMiddleware(s.authMiddleware) | ||
|
|
||
| // Initialize new middleware components (PR 2) |
There was a problem hiding this comment.
The comment '(PR 2)' should be removed or replaced with a more descriptive comment explaining what these middleware components do.
| // Initialize new middleware components (PR 2) | |
| // Initialize permission and impersonation middleware for access control and user impersonation features |
Copilot uses AI. Check for mistakes.
| // Pods endpoints with new permission middleware (PR 2 test) | ||
| r.Group(func(r chi.Router) { | ||
| // Use new permission middleware for pods listing |
There was a problem hiding this comment.
The comment '(PR 2 test)' is temporary and should be removed or replaced with a more descriptive comment about the permission middleware usage.
| // Pods endpoints with new permission middleware (PR 2 test) | |
| r.Group(func(r chi.Router) { | |
| // Use new permission middleware for pods listing | |
| // Pods endpoints protected by permission middleware: only users with "list pods" permission can access the pods listing | |
| r.Group(func(r chi.Router) { | |
| // Use permission middleware to enforce "list pods" permission |
Copilot uses AI. Check for mistakes.
1 participant
No description provided.