From 05c41f57d585f8d352a8c9c90020e86fceac8920 Mon Sep 17 00:00:00 2001 From: osama1998H Date: Sat, 28 Mar 2026 20:40:28 +0300 Subject: [PATCH 1/2] Optimize auth middleware and add performance benchmarks --- .gitignore | 4 + Makefile | 7 +- internal/api/middleware/benchmark_test.go | 64 ++++++++ internal/api/router.go | 5 +- internal/api/router_authorization_test.go | 10 +- internal/api/router_benchmark_test.go | 139 ++++++++++++++++++ internal/api/router_rate_limit_test.go | 84 +++++++++++ internal/repository/postgres/roles.go | 31 ++++ .../repository/postgres/tenant_scope_test.go | 82 +++++++++++ internal/service/apikey.go | 29 ++-- internal/service/async.go | 48 ++++++ internal/service/async_test.go | 78 ++++++++++ internal/service/audit.go | 23 ++- internal/service/rbac.go | 18 +-- internal/service/rbac_scope_test.go | 7 + internal/service/webhook.go | 45 ++++-- internal/testutil/testcache.go | 47 ++++++ 17 files changed, 667 insertions(+), 54 deletions(-) create mode 100644 internal/api/middleware/benchmark_test.go create mode 100644 internal/api/router_benchmark_test.go create mode 100644 internal/api/router_rate_limit_test.go create mode 100644 internal/service/async.go create mode 100644 internal/service/async_test.go create mode 100644 internal/testutil/testcache.go diff --git a/.gitignore b/.gitignore index 5d99ad7..ff6e48f 100644 --- a/.gitignore +++ b/.gitignore @@ -27,3 +27,7 @@ vendor/ # Logs *.log + +# Local benchmark captures +before.txt +after.txt diff --git a/Makefile b/Makefile index 3b068f5..5a20468 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,9 @@ -.PHONY: all build run test lint fmt clean migrate generate help +.PHONY: all build run test lint fmt clean migrate generate help bench-perf BINARY=uniauth CMD=./cmd/server +BENCH_PERF_PATTERN='^(BenchmarkRouter_(Health|ListUsers_(Permitted|Superuser))|BenchmarkJWTAuth_BlacklistLookup|BenchmarkRateLimit_APIRequest)$$' +BENCHCOUNT?=1 # ─── Build ─────────────────────────────────────────────────────────────────── @@ -25,6 +27,9 @@ test: test-short: go test ./... -short -timeout 30s +bench-perf: + go test ./internal/api ./internal/api/middleware -run '^$$' -bench $(BENCH_PERF_PATTERN) -benchmem -count $(BENCHCOUNT) + # ─── Code Quality ──────────────────────────────────────────────────────────── lint: diff --git a/internal/api/middleware/benchmark_test.go b/internal/api/middleware/benchmark_test.go new file mode 100644 index 0000000..a59f1ad --- /dev/null +++ b/internal/api/middleware/benchmark_test.go @@ -0,0 +1,64 @@ +package middleware + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/osama1998h/uniauth/internal/testutil" + "github.com/osama1998h/uniauth/pkg/token" +) + +func BenchmarkJWTAuth_BlacklistLookup(b *testing.B) { + testutil.RequireEnv(b, "REDIS_URL") + maker := token.NewMaker(testJWTSecret, 15*time.Minute, 7*24*time.Hour) + redisCache := testutil.RequireTestCache(b) + userID := uuid.New() + orgID := uuid.New() + + tokenStr, _, err := maker.CreateAccessToken(userID, orgID) + if err != nil { + b.Fatalf("create access token: %v", err) + } + + handler := JWTAuth(maker, redisCache, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + req := httptest.NewRequest(http.MethodGet, "/api/v1/users", nil) + req.Header.Set("Authorization", "Bearer "+tokenStr) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + b.Fatalf("JWTAuth status = %d, want %d", rec.Code, http.StatusOK) + } + } +} + +func BenchmarkRateLimit_APIRequest(b *testing.B) { + testutil.RequireEnv(b, "REDIS_URL") + redisCache := testutil.RequireTestCache(b) + handler := RateLimit(redisCache, 1_000_000_000, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + req := httptest.NewRequest(http.MethodGet, "/api/v1/users", nil) + req.RemoteAddr = "203.0.113.13:4000" + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + b.Fatalf("RateLimit status = %d, want %d", rec.Code, http.StatusOK) + } + } +} diff --git a/internal/api/router.go b/internal/api/router.go index b53ac88..17e1ea4 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -36,7 +36,7 @@ func NewRouter( userSvc := service.NewUserService(store, auditSvc, webhookSvc) orgSvc := service.NewOrgService(store) rbacSvc := service.NewRBACService(store, auditSvc) - apiKeySvc := service.NewAPIKeyService(store, auditSvc) + apiKeySvc := service.NewAPIKeyService(store, auditSvc, logger) // --- Handlers --- healthH := handlers.NewHealthHandler(store, redisCache) @@ -64,7 +64,6 @@ func NewRouter( AllowCredentials: true, MaxAge: 300, })) - r.Use(middleware.RateLimit(redisCache, cfg.Auth.RateLimitPerMinute, logger)) // Health r.Get("/health", healthH.Live) @@ -77,6 +76,8 @@ func NewRouter( // API v1 r.Route("/api/v1", func(r chi.Router) { + r.Use(middleware.RateLimit(redisCache, cfg.Auth.RateLimitPerMinute, logger)) + // Auth — public r.Route("/auth", func(r chi.Router) { r.Post("/register", authH.Register) diff --git a/internal/api/router_authorization_test.go b/internal/api/router_authorization_test.go index fd0563c..8cd48e4 100644 --- a/internal/api/router_authorization_test.go +++ b/internal/api/router_authorization_test.go @@ -287,10 +287,10 @@ func TestRouterAllowsSuperuserBypass(t *testing.T) { wantStatus: http.StatusOK, }, { - name: "create user", - method: http.MethodPost, - path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/users" }, - body: `{"email":"superuser-create-` + uuid.NewString() + `@example.com","password":"S3cur3P@ss!"}`, + name: "create user", + method: http.MethodPost, + path: func(_ *testing.T, _ context.Context, _ *db.Store, _ uuid.UUID) string { return "/api/v1/users" }, + body: `{"email":"superuser-create-` + uuid.NewString() + `@example.com","password":"S3cur3P@ss!"}`, wantStatus: http.StatusCreated, }, { @@ -398,7 +398,7 @@ func createSuperuser(t *testing.T, ctx context.Context, store *db.Store, orgID u return user } -func grantPermissionToUser(t *testing.T, ctx context.Context, store *db.Store, orgID, userID uuid.UUID, permission string) { +func grantPermissionToUser(t testing.TB, ctx context.Context, store *db.Store, orgID, userID uuid.UUID, permission string) { t.Helper() role := testutil.CreateRole(t, store, orgID, "router-permission-role") diff --git a/internal/api/router_benchmark_test.go b/internal/api/router_benchmark_test.go new file mode 100644 index 0000000..27f976a --- /dev/null +++ b/internal/api/router_benchmark_test.go @@ -0,0 +1,139 @@ +package api_test + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/osama1998h/uniauth/internal/api" + "github.com/osama1998h/uniauth/internal/config" + "github.com/osama1998h/uniauth/internal/domain" + "github.com/osama1998h/uniauth/internal/testutil" + "github.com/osama1998h/uniauth/pkg/token" +) + +const benchmarkJWTSecret = "benchmark-secret-at-least-32-chars!" + +func BenchmarkRouter_Health(b *testing.B) { + handler := newBenchmarkRouter(b) + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + req := httptest.NewRequest(http.MethodGet, "/health", nil) + req.RemoteAddr = "203.0.113.10:4000" + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + b.Fatalf("GET /health status = %d, want %d", rec.Code, http.StatusOK) + } + } +} + +func BenchmarkRouter_ListUsers_Permitted(b *testing.B) { + handler, accessToken := newBenchmarkListUsersRouter(b, false) + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + req := httptest.NewRequest(http.MethodGet, "/api/v1/users", nil) + req.RemoteAddr = "203.0.113.11:4000" + req.Header.Set("Authorization", "Bearer "+accessToken) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + b.Fatalf("GET /api/v1/users status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } + } +} + +func BenchmarkRouter_ListUsers_Superuser(b *testing.B) { + handler, accessToken := newBenchmarkListUsersRouter(b, true) + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + req := httptest.NewRequest(http.MethodGet, "/api/v1/users", nil) + req.RemoteAddr = "203.0.113.12:4000" + req.Header.Set("Authorization", "Bearer "+accessToken) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + b.Fatalf("GET /api/v1/users status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } + } +} + +func newBenchmarkRouter(b *testing.B) http.Handler { + b.Helper() + + testutil.RequireEnv(b, "DATABASE_URL", "REDIS_URL", "JWT_SECRET") + store := testutil.RequireTestStore(b) + redisCache := testutil.RequireTestCache(b) + + return api.NewRouter(newBenchmarkConfig(), store, redisCache, testutil.DiscardLogger()) +} + +func newBenchmarkListUsersRouter(b *testing.B, superuser bool) (http.Handler, string) { + b.Helper() + + testutil.RequireEnv(b, "DATABASE_URL", "REDIS_URL", "JWT_SECRET") + store := testutil.RequireTestStore(b) + redisCache := testutil.RequireTestCache(b) + handler := api.NewRouter(newBenchmarkConfig(), store, redisCache, testutil.DiscardLogger()) + + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + org := testutil.CreateOrganization(b, store, "bench-users-org") + for i := 0; i < 20; i++ { + testutil.CreateUser(b, store, org.ID, "bench-users-seed") + } + + var userID uuid.UUID + if superuser { + admin, err := store.CreateUser(ctx, org.ID, "bench-superuser@example.com", "hashed-password", nil, true) + if err != nil { + b.Fatalf("CreateUser(superuser) error = %v", err) + } + userID = admin.ID + } else { + user := testutil.CreateUser(b, store, org.ID, "bench-users-reader") + grantPermissionToUser(b, ctx, store, org.ID, user.ID, domain.PermissionUsersRead) + userID = user.ID + } + + return handler, issueBenchmarkAccessToken(b, userID, org.ID) +} + +func newBenchmarkConfig() *config.Config { + return &config.Config{ + Server: config.ServerConfig{ + Environment: "development", + }, + Auth: config.AuthConfig{ + JWTSecret: benchmarkJWTSecret, + AccessTokenDuration: 15 * time.Minute, + RefreshTokenDuration: 7 * 24 * time.Hour, + RateLimitPerMinute: 1_000_000_000, + }, + Email: config.EmailConfig{ + BaseURL: "http://localhost:8080", + }, + } +} + +func issueBenchmarkAccessToken(b *testing.B, userID, orgID uuid.UUID) string { + b.Helper() + + maker := token.NewMaker(benchmarkJWTSecret, 15*time.Minute, 7*24*time.Hour) + tokenStr, _, err := maker.CreateAccessToken(userID, orgID) + if err != nil { + b.Fatalf("CreateAccessToken() error = %v", err) + } + return tokenStr +} diff --git a/internal/api/router_rate_limit_test.go b/internal/api/router_rate_limit_test.go new file mode 100644 index 0000000..4bed357 --- /dev/null +++ b/internal/api/router_rate_limit_test.go @@ -0,0 +1,84 @@ +package api_test + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/osama1998h/uniauth/internal/api" + "github.com/osama1998h/uniauth/internal/config" + "github.com/osama1998h/uniauth/internal/testutil" +) + +func TestRouterDoesNotRateLimitHealthOrReady(t *testing.T) { + testutil.RequireEnv(t, "DATABASE_URL", "REDIS_URL") + + store := testutil.RequireTestStore(t) + redisCache := testutil.RequireTestCache(t) + handler := api.NewRouter(newLowRateLimitConfig(), store, redisCache, testutil.DiscardLogger()) + + tests := []struct { + name string + path string + ip string + }{ + {name: "health", path: "/health", ip: "203.0.113.31:4000"}, + {name: "ready", path: "/ready", ip: "203.0.113.32:4000"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + for i := 0; i < 2; i++ { + req := httptest.NewRequest(http.MethodGet, tc.path, nil) + req.RemoteAddr = tc.ip + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("%s request %d status = %d, want %d, body=%s", tc.path, i+1, rec.Code, http.StatusOK, rec.Body.String()) + } + } + }) + } +} + +func TestRouterStillRateLimitsAPIRoutes(t *testing.T) { + testutil.RequireEnv(t, "DATABASE_URL", "REDIS_URL") + + store := testutil.RequireTestStore(t) + redisCache := testutil.RequireTestCache(t) + handler := api.NewRouter(newLowRateLimitConfig(), store, redisCache, testutil.DiscardLogger()) + + req1 := httptest.NewRequest(http.MethodGet, "/api/v1/users/me", nil) + req1.RemoteAddr = "203.0.113.33:4000" + rec1 := httptest.NewRecorder() + handler.ServeHTTP(rec1, req1) + if rec1.Code != http.StatusUnauthorized { + t.Fatalf("first API request status = %d, want %d", rec1.Code, http.StatusUnauthorized) + } + + req2 := httptest.NewRequest(http.MethodGet, "/api/v1/users/me", nil) + req2.RemoteAddr = "203.0.113.33:4000" + rec2 := httptest.NewRecorder() + handler.ServeHTTP(rec2, req2) + if rec2.Code != http.StatusTooManyRequests { + t.Fatalf("second API request status = %d, want %d, body=%s", rec2.Code, http.StatusTooManyRequests, rec2.Body.String()) + } +} + +func newLowRateLimitConfig() *config.Config { + return &config.Config{ + Server: config.ServerConfig{ + Environment: "development", + }, + Auth: config.AuthConfig{ + JWTSecret: routerTestJWTSecret, + AccessTokenDuration: 15 * time.Minute, + RefreshTokenDuration: 7 * 24 * time.Hour, + RateLimitPerMinute: 1, + }, + Email: config.EmailConfig{ + BaseURL: "http://localhost:8080", + }, + } +} diff --git a/internal/repository/postgres/roles.go b/internal/repository/postgres/roles.go index c031d1e..df72612 100644 --- a/internal/repository/postgres/roles.go +++ b/internal/repository/postgres/roles.go @@ -192,6 +192,37 @@ func (s *Store) UserHasPermission(ctx context.Context, orgID, userID uuid.UUID, return allowed, nil } +func (s *Store) AuthorizeUser(ctx context.Context, orgID, userID uuid.UUID, permission string) (bool, bool, error) { + row := s.pool.QueryRow(ctx, + `WITH actor AS ( + SELECT is_superuser + FROM users + WHERE org_id = $1 AND id = $2 + ) + SELECT + EXISTS (SELECT 1 FROM actor) AS user_exists, + CASE + WHEN COALESCE((SELECT is_superuser FROM actor LIMIT 1), false) THEN true + ELSE EXISTS ( + SELECT 1 + FROM user_roles ur + JOIN role_permissions rp ON rp.role_id = ur.role_id + JOIN permissions p ON p.id = rp.permission_id + WHERE ur.org_id = $1 AND ur.user_id = $2 AND p.name = $3 + ) + END AS authorized`, + orgID, userID, permission, + ) + + var userExists bool + var authorized bool + if err := row.Scan(&userExists, &authorized); err != nil { + return false, false, fmt.Errorf("authorize user: %w", err) + } + + return userExists, authorized, nil +} + func (s *Store) GetPermissionByName(ctx context.Context, name string) (*domain.Permission, error) { row := s.pool.QueryRow(ctx, `SELECT id, name, description FROM permissions WHERE name = $1`, name) p := &domain.Permission{} diff --git a/internal/repository/postgres/tenant_scope_test.go b/internal/repository/postgres/tenant_scope_test.go index e287719..0c5cf86 100644 --- a/internal/repository/postgres/tenant_scope_test.go +++ b/internal/repository/postgres/tenant_scope_test.go @@ -277,3 +277,85 @@ func TestStoreUserHasPermissionHonorsOrgScope(t *testing.T) { t.Fatal("expected missing permission to be denied") } } + +func TestStoreAuthorizeUser(t *testing.T) { + store := testutil.RequireTestStore(t) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + org := testutil.CreateOrganization(t, store, "authorize-user-org") + user := testutil.CreateUser(t, store, org.ID, "authorize-user") + superuser, err := store.CreateUser(ctx, org.ID, "authorize-superuser@example.com", "hashed-password", nil, true) + if err != nil { + t.Fatalf("CreateUser(superuser) error = %v", err) + } + + role := testutil.CreateRole(t, store, org.ID, "authorize-role") + perm, err := store.GetPermissionByName(ctx, domain.PermissionUsersRead) + if err != nil { + t.Fatalf("GetPermissionByName() error = %v", err) + } + if err := store.AssignPermissionToRole(ctx, role.ID, perm.ID); err != nil { + t.Fatalf("AssignPermissionToRole() error = %v", err) + } + if err := store.AssignRoleToUser(ctx, org.ID, user.ID, role.ID); err != nil { + t.Fatalf("AssignRoleToUser() error = %v", err) + } + + tests := []struct { + name string + orgID uuid.UUID + userID uuid.UUID + permission string + wantUserExists bool + wantAuthorized bool + }{ + { + name: "allows granted permission", + orgID: org.ID, + userID: user.ID, + permission: domain.PermissionUsersRead, + wantUserExists: true, + wantAuthorized: true, + }, + { + name: "denies missing permission", + orgID: org.ID, + userID: user.ID, + permission: domain.PermissionUsersDelete, + wantUserExists: true, + wantAuthorized: false, + }, + { + name: "allows superuser", + orgID: org.ID, + userID: superuser.ID, + permission: domain.PermissionUsersDelete, + wantUserExists: true, + wantAuthorized: true, + }, + { + name: "missing user is unauthorized", + orgID: org.ID, + userID: uuid.New(), + permission: domain.PermissionUsersRead, + wantUserExists: false, + wantAuthorized: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + userExists, authorized, err := store.AuthorizeUser(ctx, tc.orgID, tc.userID, tc.permission) + if err != nil { + t.Fatalf("AuthorizeUser() error = %v", err) + } + if userExists != tc.wantUserExists { + t.Fatalf("userExists = %v, want %v", userExists, tc.wantUserExists) + } + if authorized != tc.wantAuthorized { + t.Fatalf("authorized = %v, want %v", authorized, tc.wantAuthorized) + } + }) + } +} diff --git a/internal/service/apikey.go b/internal/service/apikey.go index c75c161..1a3c517 100644 --- a/internal/service/apikey.go +++ b/internal/service/apikey.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "log/slog" "time" "github.com/google/uuid" @@ -14,13 +15,24 @@ import ( // APIKeyService manages API keys. type APIKeyService struct { - store *db.Store - auditSvc *AuditService + store *db.Store + auditSvc *AuditService + logger *slog.Logger + lastUsedQueue *asyncDispatcher[uuid.UUID] } // NewAPIKeyService creates an APIKeyService. -func NewAPIKeyService(store *db.Store, auditSvc *AuditService) *APIKeyService { - return &APIKeyService{store: store, auditSvc: auditSvc} +func NewAPIKeyService(store *db.Store, auditSvc *AuditService, logger *slog.Logger) *APIKeyService { + svc := &APIKeyService{store: store, auditSvc: auditSvc, logger: logger} + svc.lastUsedQueue = newAsyncDispatcher("api_key_last_used", logger, 1024, 1, func(id uuid.UUID) { + if svc.store == nil { + return + } + if err := svc.store.UpdateAPIKeyLastUsed(context.Background(), id); err != nil && svc.logger != nil { + svc.logger.Error("failed to update api key last used timestamp", "error", err, "api_key_id", id) + } + }) + return svc } // CreateAPIKeyInput holds parameters for creating an API key. @@ -53,7 +65,7 @@ func (s *APIKeyService) Create(ctx context.Context, in CreateAPIKeyInput, actorI s.auditSvc.Log(&domain.AuditLog{ OrgID: &in.OrgID, UserID: &actorID, - Action: domain.AuditActionAPIKeyCreated, + Action: domain.AuditActionAPIKeyCreated, ResourceType: strPtr("api_key"), ResourceID: strPtr(key.ID.String()), }) @@ -72,7 +84,7 @@ func (s *APIKeyService) Revoke(ctx context.Context, keyID, orgID, actorID uuid.U } s.auditSvc.Log(&domain.AuditLog{ OrgID: &orgID, UserID: &actorID, - Action: domain.AuditActionAPIKeyRevoked, + Action: domain.AuditActionAPIKeyRevoked, ResourceType: strPtr("api_key"), ResourceID: strPtr(keyID.String()), }) return nil @@ -92,10 +104,7 @@ func (s *APIKeyService) ValidateAPIKey(ctx context.Context, plaintext string) (* return nil, domain.ErrAPIKeyExpired } - // Update last_used_at async - go func() { - _ = s.store.UpdateAPIKeyLastUsed(context.Background(), key.ID) - }() + s.lastUsedQueue.Enqueue(key.ID) return key, nil } diff --git a/internal/service/async.go b/internal/service/async.go new file mode 100644 index 0000000..9e9abc6 --- /dev/null +++ b/internal/service/async.go @@ -0,0 +1,48 @@ +package service + +import "log/slog" + +type asyncDispatcher[T any] struct { + name string + logger *slog.Logger + tasks chan T +} + +func newAsyncDispatcher[T any](name string, logger *slog.Logger, buffer, workers int, handler func(T)) *asyncDispatcher[T] { + if buffer <= 0 { + buffer = 1 + } + if workers <= 0 { + workers = 1 + } + + dispatcher := &asyncDispatcher[T]{ + name: name, + logger: logger, + tasks: make(chan T, buffer), + } + + for i := 0; i < workers; i++ { + go func() { + for task := range dispatcher.tasks { + handler(task) + } + }() + } + + return dispatcher +} + +func (d *asyncDispatcher[T]) Enqueue(task T) { + if d == nil { + return + } + + select { + case d.tasks <- task: + default: + if d.logger != nil { + d.logger.Warn("async dispatcher queue full", "dispatcher", d.name, "capacity", cap(d.tasks)) + } + } +} diff --git a/internal/service/async_test.go b/internal/service/async_test.go new file mode 100644 index 0000000..d182c53 --- /dev/null +++ b/internal/service/async_test.go @@ -0,0 +1,78 @@ +package service + +import ( + "sync/atomic" + "testing" + "time" + + "github.com/osama1998h/uniauth/internal/testutil" +) + +func TestAsyncDispatcherCapsConcurrency(t *testing.T) { + const workers = 4 + + started := make(chan struct{}, workers) + release := make(chan struct{}) + var current atomic.Int32 + var maxConcurrent atomic.Int32 + + dispatcher := newAsyncDispatcher("test", testutil.DiscardLogger(), 32, workers, func(_ int) { + concurrency := current.Add(1) + for { + maxSeen := maxConcurrent.Load() + if concurrency <= maxSeen || maxConcurrent.CompareAndSwap(maxSeen, concurrency) { + break + } + } + started <- struct{}{} + <-release + current.Add(-1) + }) + + for i := 0; i < 16; i++ { + dispatcher.Enqueue(i) + } + + for i := 0; i < workers; i++ { + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for workers to start") + } + } + + if got := maxConcurrent.Load(); got > workers { + t.Fatalf("max concurrency = %d, want <= %d", got, workers) + } + + close(release) +} + +func TestAsyncDispatcherDropsWithoutBlockingWhenQueueIsFull(t *testing.T) { + started := make(chan struct{}, 1) + release := make(chan struct{}) + + dispatcher := newAsyncDispatcher("test", testutil.DiscardLogger(), 1, 1, func(_ int) { + started <- struct{}{} + <-release + }) + + dispatcher.Enqueue(1) + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for worker to start") + } + + dispatcher.Enqueue(2) + + start := time.Now() + for i := 0; i < 1000; i++ { + dispatcher.Enqueue(i + 3) + } + if elapsed := time.Since(start); elapsed > 50*time.Millisecond { + t.Fatalf("enqueue took %s with full queue, want < 50ms", elapsed) + } + + close(release) +} diff --git a/internal/service/audit.go b/internal/service/audit.go index 5694663..e2c244f 100644 --- a/internal/service/audit.go +++ b/internal/service/audit.go @@ -14,20 +14,29 @@ import ( type AuditService struct { store *db.Store logger *slog.Logger + queue *asyncDispatcher[*domain.AuditLog] } // NewAuditService creates an AuditService. func NewAuditService(store *db.Store, logger *slog.Logger) *AuditService { - return &AuditService{store: store, logger: logger} + svc := &AuditService{store: store, logger: logger} + svc.queue = newAsyncDispatcher("audit_logs", logger, 1024, 2, func(log *domain.AuditLog) { + if svc.store == nil { + return + } + if err := svc.store.CreateAuditLog(context.Background(), log); err != nil && svc.logger != nil { + svc.logger.Error("failed to write audit log", "error", err, "action", log.Action) + } + }) + return svc } -// Log writes an audit entry in a fire-and-forget goroutine so it never blocks the request. +// Log writes an audit entry through a bounded background worker so it never blocks the request. func (a *AuditService) Log(log *domain.AuditLog) { - go func() { - if err := a.store.CreateAuditLog(context.Background(), log); err != nil { - a.logger.Error("failed to write audit log", "error", err, "action", log.Action) - } - }() + if log == nil { + return + } + a.queue.Enqueue(log) } // List returns paginated audit logs for an organization. diff --git a/internal/service/rbac.go b/internal/service/rbac.go index e216cdf..777f9de 100644 --- a/internal/service/rbac.go +++ b/internal/service/rbac.go @@ -2,7 +2,6 @@ package service import ( "context" - "errors" "fmt" "github.com/google/uuid" @@ -117,21 +116,12 @@ func (s *RBACService) HasPermission(ctx context.Context, orgID, userID uuid.UUID // Authorize verifies that a user is allowed to perform an action in an organization. func (s *RBACService) Authorize(ctx context.Context, orgID, userID uuid.UUID, permission string) error { - user, err := s.store.GetUserByID(ctx, orgID, userID) + userExists, allowed, err := s.store.AuthorizeUser(ctx, orgID, userID, permission) if err != nil { - if errors.Is(err, domain.ErrNotFound) { - return domain.ErrUnauthorized - } - return fmt.Errorf("get actor: %w", err) - } - - if user.IsSuperuser { - return nil + return fmt.Errorf("authorize actor: %w", err) } - - allowed, err := s.store.UserHasPermission(ctx, orgID, userID, permission) - if err != nil { - return fmt.Errorf("check permission: %w", err) + if !userExists { + return domain.ErrUnauthorized } if !allowed { return domain.ErrForbidden diff --git a/internal/service/rbac_scope_test.go b/internal/service/rbac_scope_test.go index dc19814..d5d10c6 100644 --- a/internal/service/rbac_scope_test.go +++ b/internal/service/rbac_scope_test.go @@ -100,4 +100,11 @@ func TestRBACServiceAuthorize(t *testing.T) { t.Fatalf("Authorize(superuser) error = %v", err) } }) + + t.Run("returns unauthorized for missing user", func(t *testing.T) { + err := svc.Authorize(ctx, org.ID, uuid.New(), domain.PermissionUsersRead) + if !errors.Is(err, domain.ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got %v", err) + } + }) } diff --git a/internal/service/webhook.go b/internal/service/webhook.go index f2d8464..ee9dcaa 100644 --- a/internal/service/webhook.go +++ b/internal/service/webhook.go @@ -33,6 +33,12 @@ type webhookResolver interface { type webhookDialContext func(ctx context.Context, network, address string) (net.Conn, error) +type webhookDispatchJob struct { + orgID uuid.UUID + event string + payload any +} + // WebhookService dispatches webhook events to registered endpoints. type WebhookService struct { store *db.Store @@ -40,6 +46,7 @@ type WebhookService struct { resolver webhookResolver dialContext webhookDialContext client *http.Client + queue *asyncDispatcher[webhookDispatchJob] } // NewWebhookService creates a WebhookService. @@ -81,6 +88,28 @@ func newWebhookServiceWithNetworking( }, } + svc.queue = newAsyncDispatcher("webhook_dispatch", logger, 256, 4, func(job webhookDispatchJob) { + if svc.store == nil { + return + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + hooks, err := svc.store.ListActiveWebhooksForEvent(ctx, job.orgID, job.event) + if err != nil { + if svc.logger != nil { + svc.logger.Error("webhook: list hooks", "error", err) + } + return + } + for _, hook := range hooks { + if err := svc.deliver(ctx, hook.URL, hook.Secret, job.event, job.payload); err != nil && svc.logger != nil { + svc.logger.Warn("webhook: delivery skipped", "url", hook.URL, "error", err) + } + } + }) + return svc } @@ -148,21 +177,7 @@ func (w *WebhookService) Delete(ctx context.Context, id, orgID uuid.UUID) error // Dispatch fires webhook events for a given org+event asynchronously. func (w *WebhookService) Dispatch(orgID uuid.UUID, event string, payload any) { - go func() { - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - hooks, err := w.store.ListActiveWebhooksForEvent(ctx, orgID, event) - if err != nil { - w.logger.Error("webhook: list hooks", "error", err) - return - } - for _, hook := range hooks { - if err := w.deliver(ctx, hook.URL, hook.Secret, event, payload); err != nil { - w.logger.Warn("webhook: delivery skipped", "url", hook.URL, "error", err) - } - } - }() + w.queue.Enqueue(webhookDispatchJob{orgID: orgID, event: event, payload: payload}) } func (w *WebhookService) deliver(ctx context.Context, targetURL, secret, event string, payload any) error { diff --git a/internal/testutil/testcache.go b/internal/testutil/testcache.go new file mode 100644 index 0000000..0b53458 --- /dev/null +++ b/internal/testutil/testcache.go @@ -0,0 +1,47 @@ +package testutil + +import ( + "context" + "fmt" + "os" + "testing" + "time" + + "github.com/osama1998h/uniauth/internal/repository/cache" +) + +// RequireTestCache returns a Redis-backed cache using REDIS_URL. +func RequireTestCache(t testing.TB) *cache.Cache { + t.Helper() + + redisURL := os.Getenv("REDIS_URL") + if redisURL == "" { + t.Skip("REDIS_URL is not set") + } + + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + + redisCache, err := cache.New(ctx, redisURL) + if err != nil { + t.Fatalf("connect test redis: %v", err) + } + t.Cleanup(func() { + if closeErr := redisCache.Close(); closeErr != nil { + t.Fatalf("close test redis: %v", closeErr) + } + }) + + return redisCache +} + +// RequireEnv ensures the named environment variables are present. +func RequireEnv(t testing.TB, names ...string) { + t.Helper() + + for _, name := range names { + if os.Getenv(name) == "" { + t.Skip(fmt.Sprintf("%s is not set", name)) + } + } +} From 0abbc66159c36c7658c5ce96a083fed857b683f4 Mon Sep 17 00:00:00 2001 From: osama1998H Date: Sat, 28 Mar 2026 20:43:04 +0300 Subject: [PATCH 2/2] Use t.Skipf in test cache helper --- internal/testutil/testcache.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/testutil/testcache.go b/internal/testutil/testcache.go index 0b53458..7014f4f 100644 --- a/internal/testutil/testcache.go +++ b/internal/testutil/testcache.go @@ -2,7 +2,6 @@ package testutil import ( "context" - "fmt" "os" "testing" "time" @@ -41,7 +40,7 @@ func RequireEnv(t testing.TB, names ...string) { for _, name := range names { if os.Getenv(name) == "" { - t.Skip(fmt.Sprintf("%s is not set", name)) + t.Skipf("%s is not set", name) } } }