From 05face4b2a01ad876e73f70ac8388eeb73292bb7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 5 Apr 2026 21:31:35 +0000 Subject: [PATCH] migrate upload_handler and profile_handler to service layer - upload_handler.go: uses PermissionService.HasChannelPerm instead of the deleted hasChannelPermREST helper - profile_handler.go: delegates to UserService for profile updates, password changes, session listing, and session revocation - Remove hasChannelPermREST from channel_handler.go (no longer needed) - UserService.UpdateProfile now returns ErrConflict on duplicate username https://claude.ai/code/session_01CBFF3r84ywkJRWwuqw8zD8 --- Server/api/channel_handler.go | 19 ----- Server/api/profile_handler.go | 154 ++++++++++------------------------ Server/api/router.go | 4 +- Server/api/upload_handler.go | 9 +- Server/service/user.go | 5 +- 5 files changed, 53 insertions(+), 138 deletions(-) diff --git a/Server/api/channel_handler.go b/Server/api/channel_handler.go index 38751d5c..5b556d7a 100644 --- a/Server/api/channel_handler.go +++ b/Server/api/channel_handler.go @@ -11,7 +11,6 @@ import ( "github.com/go-chi/chi/v5" "github.com/owncord/server/auth" "github.com/owncord/server/db" - "github.com/owncord/server/permissions" "github.com/owncord/server/service" ) @@ -66,24 +65,6 @@ func MountChannelRoutes(r chi.Router, database *db.DB, svc *service.Services, li ).Get("/api/v1/search", handleSearch(svc)) } -// hasChannelPermREST checks whether the role has the given permission on the channel, -// accounting for Administrator bypass and channel overrides. -// Used by non-migrated handlers (e.g., upload_handler.go). -func hasChannelPermREST(database *db.DB, role *db.Role, channelID, perm int64) bool { - if role == nil { - return false - } - if permissions.HasAdmin(role.Permissions) { - return true - } - allow, deny, err := database.GetChannelPermissions(channelID, role.ID) - if err != nil { - return false - } - effective := permissions.EffectivePerms(role.Permissions, allow, deny) - return effective&perm == perm -} - // handleListChannels returns all channels the authenticated user can see. func handleListChannels(svc *service.Services) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { diff --git a/Server/api/profile_handler.go b/Server/api/profile_handler.go index 13d0f260..a4553207 100644 --- a/Server/api/profile_handler.go +++ b/Server/api/profile_handler.go @@ -2,7 +2,6 @@ package api import ( "encoding/json" - "errors" "fmt" "log/slog" "net/http" @@ -13,6 +12,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/owncord/server/auth" "github.com/owncord/server/db" + "github.com/owncord/server/service" ) // ─── Request / Response types ──────────────────────────────────────────────── @@ -54,18 +54,18 @@ type ProfileBroadcaster interface { // MountProfileRoutes registers user profile management endpoints. // All routes require authentication. trustedProxies is used for rate limiting. -func MountProfileRoutes(r chi.Router, database *db.DB, limiter *auth.RateLimiter, trustedProxies []string, broadcaster ProfileBroadcaster) { +func MountProfileRoutes(r chi.Router, database *db.DB, svc *service.Services, limiter *auth.RateLimiter, trustedProxies []string, broadcaster ProfileBroadcaster) { r.Route("/api/v1/users/me", func(r chi.Router) { r.Use(AuthMiddleware(database)) r.With(RateLimitMiddleware(limiter, profileUpdateRateLimitPerMinute, time.Minute, trustedProxies)). - Patch("/", handleUpdateProfile(database, broadcaster)) + Patch("/", handleUpdateProfile(svc, broadcaster)) r.With(RateLimitMiddleware(limiter, profilePasswordRateLimitPerMinute, time.Minute, trustedProxies)). - Put("/password", handleChangePassword(database, limiter)) + Put("/password", handleChangePassword(svc, limiter)) - r.Get("/sessions", handleListSessions(database)) - r.Delete("/sessions/{id}", handleRevokeSession(database)) + r.Get("/sessions", handleListSessions(svc)) + r.Delete("/sessions/{id}", handleRevokeSession(svc)) }) } @@ -90,13 +90,12 @@ func validateAvatarURL(avatar string) error { // ─── Handlers ──────────────────────────────────────────────────────────────── // handleUpdateProfile processes PATCH /api/v1/users/me. -func handleUpdateProfile(database *db.DB, broadcaster ProfileBroadcaster) http.HandlerFunc { +func handleUpdateProfile(svc *service.Services, broadcaster ProfileBroadcaster) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { user, ok := r.Context().Value(UserKey).(*db.User) if !ok || user == nil { writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", + Error: "UNAUTHORIZED", Message: "not authenticated", }) return } @@ -104,26 +103,21 @@ func handleUpdateProfile(database *db.DB, broadcaster ProfileBroadcaster) http.H var req updateProfileRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: "malformed request body", + Error: "INVALID_INPUT", Message: "malformed request body", }) return } req.Username = strings.TrimSpace(sanitizer.Sanitize(req.Username)) - if req.Username == "" { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: "username is required", + Error: "INVALID_INPUT", Message: "username is required", }) return } - if err := auth.ValidateUsername(req.Username); err != nil { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: err.Error(), + Error: "INVALID_INPUT", Message: err.Error(), }) return } @@ -133,44 +127,19 @@ func handleUpdateProfile(database *db.DB, broadcaster ProfileBroadcaster) http.H trimmed := strings.TrimSpace(sanitizer.Sanitize(*req.Avatar)) if err := validateAvatarURL(trimmed); err != nil { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: err.Error(), + Error: "INVALID_INPUT", Message: err.Error(), }) return } req.Avatar = &trimmed } - if err := database.UpdateUserProfile(user.ID, req.Username, req.Avatar); err != nil { - if db.IsUniqueConstraintError(err) { - writeJSON(w, http.StatusConflict, errorResponse{ - Error: "CONFLICT", - Message: "username is already taken", - }) - return - } - slog.Error("UpdateUserProfile failed", "err", err, "user_id", user.ID) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to update profile", - }) + updated, err := svc.Users.UpdateProfile(user.ID, req.Username, req.Avatar) + if err != nil { + writeServiceError(w, err) return } - // Re-fetch user for the response. - updated, err := database.GetUserByID(user.ID) - if err != nil || updated == nil { - slog.Error("failed to fetch user after profile update", "user_id", user.ID, "error", err) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "profile updated but fetch failed", - }) - return - } - - slog.Info("profile updated", "user_id", user.ID, "new_username", req.Username) - _ = database.LogAudit(user.ID, "profile_update", "user", user.ID, "profile updated") - // Broadcast profile change to all connected WebSocket clients. if broadcaster != nil { broadcaster.BroadcastUserUpdate(updated.ID, updated.Username, updated.Avatar) @@ -181,13 +150,12 @@ func handleUpdateProfile(database *db.DB, broadcaster ProfileBroadcaster) http.H } // handleChangePassword processes PUT /api/v1/users/me/password. -func handleChangePassword(database *db.DB, limiter *auth.RateLimiter) http.HandlerFunc { +func handleChangePassword(svc *service.Services, limiter *auth.RateLimiter) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { user, ok := r.Context().Value(UserKey).(*db.User) if !ok || user == nil { writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", + Error: "UNAUTHORIZED", Message: "not authenticated", }) return } @@ -196,8 +164,7 @@ func handleChangePassword(database *db.DB, limiter *auth.RateLimiter) http.Handl lockKey := fmt.Sprintf("pw_confirm_lock:%d", user.ID) if limiter.IsLockedOut(lockKey) { writeJSON(w, http.StatusTooManyRequests, errorResponse{ - Error: "RATE_LIMITED", - Message: "too many failed attempts, try again later", + Error: "RATE_LIMITED", Message: "too many failed attempts, try again later", }) return } @@ -205,16 +172,14 @@ func handleChangePassword(database *db.DB, limiter *auth.RateLimiter) http.Handl var req changePasswordRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: "malformed request body", + Error: "INVALID_INPUT", Message: "malformed request body", }) return } if req.OldPassword == "" || req.NewPassword == "" { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: "old_password and new_password are required", + Error: "INVALID_INPUT", Message: "old_password and new_password are required", }) return } @@ -226,8 +191,7 @@ func handleChangePassword(database *db.DB, limiter *auth.RateLimiter) http.Handl limiter.Lockout(lockKey, pwConfirmLockoutDuration) } writeJSON(w, http.StatusForbidden, errorResponse{ - Error: "FORBIDDEN", - Message: "incorrect password", + Error: "FORBIDDEN", Message: "incorrect password", }) return } @@ -236,8 +200,7 @@ func handleChangePassword(database *db.DB, limiter *auth.RateLimiter) http.Handl // Reject same old/new password. if req.OldPassword == req.NewPassword { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: "new password must be different from old password", + Error: "INVALID_INPUT", Message: "new password must be different from old password", }) return } @@ -245,8 +208,7 @@ func handleChangePassword(database *db.DB, limiter *auth.RateLimiter) http.Handl // Validate new password strength. if err := auth.ValidatePasswordStrength(req.NewPassword); err != nil { writeJSON(w, http.StatusBadRequest, errorResponse{ - Error: "INVALID_INPUT", - Message: err.Error(), + Error: "INVALID_INPUT", Message: err.Error(), }) return } @@ -255,46 +217,34 @@ func handleChangePassword(database *db.DB, limiter *auth.RateLimiter) http.Handl hash, err := auth.HashPassword(req.NewPassword) if err != nil { writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to process password change", + Error: "INTERNAL_ERROR", Message: "failed to process password change", }) return } - if err := database.UpdateUserPassword(user.ID, hash); err != nil { - slog.Error("UpdateUserPassword failed", "err", err, "user_id", user.ID) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to update password", - }) + // Delegate to service for password update + session revocation. + sess, _ := r.Context().Value(SessionKey).(*db.Session) + keepSessionID := int64(0) + if sess != nil { + keepSessionID = sess.ID + } + + if _, err := svc.Users.ChangePassword(user.ID, hash, keepSessionID); err != nil { + writeServiceError(w, err) return } - // BUG-108: Revoke all other sessions after password change. - if sess, ok := r.Context().Value(SessionKey).(*db.Session); ok && sess != nil { - n, err := database.DeleteOtherSessions(user.ID, sess.ID) - if err != nil { - slog.Error("DeleteOtherSessions after password change", "err", err, "user_id", user.ID) - } else if n > 0 { - slog.Info("revoked other sessions after password change", "user_id", user.ID, "revoked", n) - } - } - - slog.Info("password changed", "user_id", user.ID) - _ = database.LogAudit(user.ID, "password_change", "user", user.ID, "password changed") - w.WriteHeader(http.StatusNoContent) } } // handleListSessions processes GET /api/v1/users/me/sessions. -func handleListSessions(database *db.DB) http.HandlerFunc { +func handleListSessions(svc *service.Services) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { user, ok := r.Context().Value(UserKey).(*db.User) if !ok || user == nil { writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", + Error: "UNAUTHORIZED", Message: "not authenticated", }) return } @@ -302,19 +252,14 @@ func handleListSessions(database *db.DB) http.HandlerFunc { sess, ok := r.Context().Value(SessionKey).(*db.Session) if !ok || sess == nil { writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", + Error: "UNAUTHORIZED", Message: "not authenticated", }) return } - sessions, err := database.ListUserSessions(user.ID) + sessions, err := svc.Users.ListSessions(user.ID) if err != nil { - slog.Error("ListUserSessions failed", "err", err, "user_id", user.ID) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to list sessions", - }) + writeServiceError(w, err) return } @@ -337,13 +282,12 @@ func handleListSessions(database *db.DB) http.HandlerFunc { } // handleRevokeSession processes DELETE /api/v1/users/me/sessions/{id}. -func handleRevokeSession(database *db.DB) http.HandlerFunc { +func handleRevokeSession(svc *service.Services) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { user, ok := r.Context().Value(UserKey).(*db.User) if !ok || user == nil { writeJSON(w, http.StatusUnauthorized, errorResponse{ - Error: "UNAUTHORIZED", - Message: "not authenticated", + Error: "UNAUTHORIZED", Message: "not authenticated", }) return } @@ -353,25 +297,11 @@ func handleRevokeSession(database *db.DB) http.HandlerFunc { return } - if err := database.DeleteSessionByID(sessionID, user.ID); err != nil { - if errors.Is(err, db.ErrNotFound) { - writeJSON(w, http.StatusNotFound, errorResponse{ - Error: "NOT_FOUND", - Message: "session not found", - }) - return - } - slog.Error("DeleteSessionByID failed", "err", err, "session_id", sessionID, "user_id", user.ID) - writeJSON(w, http.StatusInternalServerError, errorResponse{ - Error: "INTERNAL_ERROR", - Message: "failed to revoke session", - }) + if err := svc.Users.RevokeSession(user.ID, sessionID); err != nil { + writeServiceError(w, err) return } - slog.Info("session revoked", "user_id", user.ID, "session_id", sessionID) - _ = database.LogAudit(user.ID, "session_revoke", "session", sessionID, "session revoked") - w.WriteHeader(http.StatusNoContent) } } diff --git a/Server/api/router.go b/Server/api/router.go index 5f7750ba..b4f5174d 100644 --- a/Server/api/router.go +++ b/Server/api/router.go @@ -114,7 +114,7 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri if storeErr != nil { slog.Error("failed to create file storage", "error", storeErr) } else { - MountUploadRoutes(r, database, store, limiter, cfg.Server.AllowedOrigins) + MountUploadRoutes(r, database, store, limiter, cfg.Server.AllowedOrigins, svc.Permissions) } // WebSocket hub — WS does its own in-band auth, so no AuthMiddleware here. @@ -177,7 +177,7 @@ func NewRouter(cfg *config.Config, database *db.DB, ver string, logBuf *admin.Ri // Profile routes: update profile, change password, session management. // Mounted after hub creation so the hub can broadcast user_update events. - MountProfileRoutes(r, database, limiter, cfg.Server.TrustedProxies, hub) + MountProfileRoutes(r, database, svc, limiter, cfg.Server.TrustedProxies, hub) // DM (direct message) REST routes — mounted after hub creation so the // hub can send real-time dm_channel_close events to WebSocket clients. diff --git a/Server/api/upload_handler.go b/Server/api/upload_handler.go index a6ffad27..5d997998 100644 --- a/Server/api/upload_handler.go +++ b/Server/api/upload_handler.go @@ -20,6 +20,7 @@ import ( "github.com/owncord/server/auth" "github.com/owncord/server/db" "github.com/owncord/server/permissions" + "github.com/owncord/server/service" "github.com/owncord/server/storage" ) @@ -75,14 +76,14 @@ func isUnsafeInlineMIME(mimeType string) bool { // MountUploadRoutes registers upload and file-serving endpoints. // allowedOrigins controls the Access-Control-Allow-Origin header on served files. -func MountUploadRoutes(r chi.Router, database *db.DB, store *storage.Storage, limiter *auth.RateLimiter, allowedOrigins []string) { +func MountUploadRoutes(r chi.Router, database *db.DB, store *storage.Storage, limiter *auth.RateLimiter, allowedOrigins []string, permSvc *service.PermissionService) { // Upload requires authentication and a higher body size limit (100 MB). r.With( AuthMiddleware(database), MaxBodySize(uploadMaxBodySize), ).Post("/api/v1/uploads", handleUpload(database, store, limiter)) // File serving requires authentication for channel-level access control. - r.With(AuthMiddleware(database)).Get("/api/v1/files/{id}", handleServeFile(database, store, allowedOrigins)) + r.With(AuthMiddleware(database)).Get("/api/v1/files/{id}", handleServeFile(database, store, allowedOrigins, permSvc)) } func handleUpload(database *db.DB, store *storage.Storage, limiter *auth.RateLimiter) http.HandlerFunc { @@ -202,7 +203,7 @@ func handleUpload(database *db.DB, store *storage.Storage, limiter *auth.RateLim } } -func handleServeFile(database *db.DB, store *storage.Storage, allowedOrigins []string) http.HandlerFunc { +func handleServeFile(database *db.DB, store *storage.Storage, allowedOrigins []string, permSvc *service.PermissionService) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { fileID := chi.URLParam(r, "id") if fileID == "" { @@ -268,7 +269,7 @@ func handleServeFile(database *db.DB, store *storage.Storage, allowedOrigins []s }) return } - } else if !hasChannelPermREST(database, role, *aa.ChannelID, permissions.ReadMessages) { + } else if user == nil || !permSvc.HasChannelPerm(user.ID, *aa.ChannelID, permissions.ReadMessages) { writeJSON(w, http.StatusForbidden, errorResponse{ Error: "FORBIDDEN", Message: "you do not have access to this file", diff --git a/Server/service/user.go b/Server/service/user.go index e5d80a16..b141dbb4 100644 --- a/Server/service/user.go +++ b/Server/service/user.go @@ -22,7 +22,10 @@ func NewUserService(st store.Store) *UserService { // Returns the updated user for response building. func (s *UserService) UpdateProfile(userID int64, username string, avatar *string) (*db.User, error) { if err := s.st.UpdateUserProfile(userID, username, avatar); err != nil { - return nil, fmt.Errorf("%w: %v", ErrInternal, err) + if db.IsUniqueConstraintError(err) { + return nil, fmt.Errorf("%w: username is already taken", ErrConflict) + } + return nil, fmt.Errorf("%w: failed to update profile", ErrInternal) } user, err := s.st.GetUserByID(userID) if err != nil {