From a6680b1c0578b29a228dd5dd6dc5551f118181e9 Mon Sep 17 00:00:00 2001 From: Masahiko AMANO Date: Wed, 10 Jun 2026 13:59:10 +0300 Subject: [PATCH] fix(backend): require owner/admin to read or modify object ACLs GET/PUT /acl/:object_type/:object_id performed no authorization check, so any authenticated user could read the permission list of, or grant themselves view/edit on, any file/tag/category/pool. ACLService now resolves the object's owner and rejects callers who are neither the owner nor an admin. SetPermissions also wraps its delete+insert replace in a single transaction so a partial failure can no longer wipe permissions. Co-Authored-By: Claude Opus 4.8 --- backend/cmd/server/main.go | 2 +- backend/internal/handler/acl_handler.go | 8 +- backend/internal/integration/server_test.go | 2 +- backend/internal/service/acl_service.go | 109 ++++++++++++++++++-- 4 files changed, 109 insertions(+), 12 deletions(-) diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index 7eb4543..3ca8256 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -77,7 +77,7 @@ func main() { cfg.JWTAccessTTL, cfg.JWTRefreshTTL, ) - aclSvc := service.NewACLService(aclRepo) + aclSvc := service.NewACLService(aclRepo, fileRepo, tagRepo, categoryRepo, poolRepo, transactor) auditSvc := service.NewAuditService(auditRepo) tagSvc := service.NewTagService(tagRepo, tagRuleRepo, aclSvc, auditSvc, transactor) categorySvc := service.NewCategoryService(categoryRepo, tagRepo, aclSvc, auditSvc) diff --git a/backend/internal/handler/acl_handler.go b/backend/internal/handler/acl_handler.go index 3076cd8..5f2aa1f 100644 --- a/backend/internal/handler/acl_handler.go +++ b/backend/internal/handler/acl_handler.go @@ -80,7 +80,8 @@ func (h *ACLHandler) GetPermissions(c *gin.Context) { return } - perms, err := h.aclSvc.GetPermissions(c.Request.Context(), objectTypeID, objectID) + userID, isAdmin, _ := domain.UserFromContext(c.Request.Context()) + perms, err := h.aclSvc.GetPermissions(c.Request.Context(), userID, isAdmin, objectTypeID, objectID) if err != nil { respondError(c, err) return @@ -124,13 +125,14 @@ func (h *ACLHandler) SetPermissions(c *gin.Context) { } } - if err := h.aclSvc.SetPermissions(c.Request.Context(), objectTypeID, objectID, perms); err != nil { + userID, isAdmin, _ := domain.UserFromContext(c.Request.Context()) + if err := h.aclSvc.SetPermissions(c.Request.Context(), userID, isAdmin, objectTypeID, objectID, perms); err != nil { respondError(c, err) return } // Re-read to return the stored permissions (with UserName denormalized). - stored, err := h.aclSvc.GetPermissions(c.Request.Context(), objectTypeID, objectID) + stored, err := h.aclSvc.GetPermissions(c.Request.Context(), userID, isAdmin, objectTypeID, objectID) if err != nil { respondError(c, err) return diff --git a/backend/internal/integration/server_test.go b/backend/internal/integration/server_test.go index ebba3ba..77aaba3 100644 --- a/backend/internal/integration/server_test.go +++ b/backend/internal/integration/server_test.go @@ -125,7 +125,7 @@ func setupSuite(t *testing.T) *harness { // --- Services ------------------------------------------------------------ authSvc := service.NewAuthService(userRepo, sessionRepo, "test-secret", 15*time.Minute, 720*time.Hour) - aclSvc := service.NewACLService(aclRepo) + aclSvc := service.NewACLService(aclRepo, fileRepo, tagRepo, categoryRepo, poolRepo, transactor) auditSvc := service.NewAuditService(auditRepo) tagSvc := service.NewTagService(tagRepo, tagRuleRepo, aclSvc, auditSvc, transactor) categorySvc := service.NewCategoryService(categoryRepo, tagRepo, aclSvc, auditSvc) diff --git a/backend/internal/service/acl_service.go b/backend/internal/service/acl_service.go index ca5868f..27e500f 100644 --- a/backend/internal/service/acl_service.go +++ b/backend/internal/service/acl_service.go @@ -12,11 +12,32 @@ import ( // ACLService handles access control checks and permission management. type ACLService struct { - aclRepo port.ACLRepo + aclRepo port.ACLRepo + files port.FileRepo + tags port.TagRepo + categories port.CategoryRepo + pools port.PoolRepo + tx port.Transactor } -func NewACLService(aclRepo port.ACLRepo) *ACLService { - return &ACLService{aclRepo: aclRepo} +// NewACLService creates an ACLService. The object repositories are used to +// resolve an object's owner when authorizing permission management. +func NewACLService( + aclRepo port.ACLRepo, + files port.FileRepo, + tags port.TagRepo, + categories port.CategoryRepo, + pools port.PoolRepo, + tx port.Transactor, +) *ACLService { + return &ACLService{ + aclRepo: aclRepo, + files: files, + tags: tags, + categories: categories, + pools: pools, + tx: tx, + } } // CanView returns true if the user may view the object. @@ -70,12 +91,86 @@ func (s *ACLService) CanEdit( return perm.CanEdit, nil } -// GetPermissions returns all explicit ACL entries for an object. -func (s *ACLService) GetPermissions(ctx context.Context, objectTypeID int16, objectID uuid.UUID) ([]domain.Permission, error) { +// GetPermissions returns all explicit ACL entries for an object. Only the +// object's owner or an admin may inspect its permission list. +func (s *ACLService) GetPermissions( + ctx context.Context, + userID int16, isAdmin bool, + objectTypeID int16, objectID uuid.UUID, +) ([]domain.Permission, error) { + if err := s.authorizeManage(ctx, userID, isAdmin, objectTypeID, objectID); err != nil { + return nil, err + } return s.aclRepo.List(ctx, objectTypeID, objectID) } // SetPermissions replaces all ACL entries for an object (full replace semantics). -func (s *ACLService) SetPermissions(ctx context.Context, objectTypeID int16, objectID uuid.UUID, perms []domain.Permission) error { - return s.aclRepo.Set(ctx, objectTypeID, objectID, perms) +// Only the object's owner or an admin may change its permissions. The replace is +// performed atomically inside a single transaction. +func (s *ACLService) SetPermissions( + ctx context.Context, + userID int16, isAdmin bool, + objectTypeID int16, objectID uuid.UUID, + perms []domain.Permission, +) error { + if err := s.authorizeManage(ctx, userID, isAdmin, objectTypeID, objectID); err != nil { + return err + } + return s.tx.WithTx(ctx, func(ctx context.Context) error { + return s.aclRepo.Set(ctx, objectTypeID, objectID, perms) + }) +} + +// authorizeManage returns nil if the user may manage the object's ACL +// (admin or owner), ErrForbidden otherwise, or a propagated lookup error +// (including ErrNotFound when the object does not exist). +func (s *ACLService) authorizeManage( + ctx context.Context, + userID int16, isAdmin bool, + objectTypeID int16, objectID uuid.UUID, +) error { + if isAdmin { + return nil + } + owner, err := s.objectOwner(ctx, objectTypeID, objectID) + if err != nil { + return err + } + if owner != userID { + return domain.ErrForbidden + } + return nil +} + +// objectOwner resolves the creator ID of the object identified by +// (objectTypeID, objectID). Returns ErrNotFound if the object does not exist. +func (s *ACLService) objectOwner(ctx context.Context, objectTypeID int16, objectID uuid.UUID) (int16, error) { + switch objectTypeID { + case fileObjectTypeID: + obj, err := s.files.GetByID(ctx, objectID) + if err != nil { + return 0, err + } + return obj.CreatorID, nil + case tagObjectTypeID: + obj, err := s.tags.GetByID(ctx, objectID) + if err != nil { + return 0, err + } + return obj.CreatorID, nil + case categoryObjectTypeID: + obj, err := s.categories.GetByID(ctx, objectID) + if err != nil { + return 0, err + } + return obj.CreatorID, nil + case poolObjectTypeID: + obj, err := s.pools.GetByID(ctx, objectID) + if err != nil { + return 0, err + } + return obj.CreatorID, nil + default: + return 0, domain.ErrValidation + } }