Backlog/v12 brading icons

backend/modules/appconfig/connectors/repository.go

@@ -11,4 +11,8 @@ type Repository interface {
 	GetByKey(ctx context.Context, key string) (*domain.Config, error)
 	GetOwn(ctx context.Context, key string) (*domain.Config, error)
 	Save(ctx context.Context, c *domain.Config) error
+	// CountValueContains returns how many rows for `key` (across all tenants)
+	// have `needle` as a substring of their JSON value. Used to check whether
+	// a branding asset URL is still referenced before deleting its file.
+	CountValueContains(ctx context.Context, key, needle string) (int, error)
 }

backend/modules/appconfig/connectors/usecase.go

@@ -19,7 +19,11 @@ type BrandingUsecase interface {
 	Get(ctx context.Context) (*dto.BrandingResponse, error)
 	Update(ctx context.Context, actor string, req dto.BrandingRequest) (*dto.BrandingResponse, error)
 	Seed(ctx context.Context, req dto.BrandingRequest) (*dto.BrandingResponse, error)
-	SetAsset(ctx context.Context, actor, slot, url string) (*dto.BrandingResponse, error)
+	// SetAsset returns the updated branding plus the URL previously stored in
+	// `slot` (empty if none). Callers use the previous URL to garbage-collect
+	// the now-unreferenced file.
+	SetAsset(ctx context.Context, actor, slot, url string) (resp *dto.BrandingResponse, previousURL string, err error)
+	IsBrandingAssetReferenced(ctx context.Context, url string) (bool, error)
 	GetPublic(ctx context.Context) (*dto.BrandingPublic, error)
 	BrandNameProvider
 }

backend/modules/appconfig/handler/branding_assets.go

@@ -2,6 +2,7 @@ package handler
 
 import (
 	"bytes"
+	"context"
 	"crypto/rand"
 	"encoding/hex"
 	"errors"
@@ -99,24 +100,25 @@ func (h *BrandingHandler) storeBrandingFile(slot string, fh *multipart.FileHeade
 		return "", err
 	}
 
-	// Best-effort: drop older files for this slot so they don't accumulate.
-	deleteBrandingFilesExcept(dir, slot+"-", filename)
 	return brandingURLPrefix + "/" + filename, nil
 }
 
-func deleteBrandingFilesExcept(dir, prefix, keep string) {
-	entries, err := os.ReadDir(dir)
-	if err != nil {
+// removeIfUnreferenced deletes the file backing `url` when no tenant's branding
+// row still references it. `url` must be a stored branding URL (returned by
+// storeBrandingFile) — external URLs are ignored.
+func (h *BrandingHandler) removeIfUnreferenced(ctx context.Context, url string) {
+	if !strings.HasPrefix(url, brandingURLPrefix+"/") {
 		return
 	}
-	for _, e := range entries {
-		if e.IsDir() {
-			continue
-		}
-		if name := e.Name(); strings.HasPrefix(name, prefix) && name != keep {
-			_ = os.Remove(filepath.Join(dir, name))
-		}
+	referenced, err := h.usecase.IsBrandingAssetReferenced(ctx, url)
+	if err != nil || referenced {
+		return
+	}
+	name := strings.TrimPrefix(url, brandingURLPrefix+"/")
+	if name == "" || strings.ContainsAny(name, "/\\") {
+		return
 	}
+	_ = os.Remove(filepath.Join(h.uploadDir, brandingSubdir, name))
 }
 
 // UploadAsset godoc
@@ -155,7 +157,7 @@ func (h *BrandingHandler) UploadAsset(c *gin.Context) {
 		c.JSON(http.StatusUnsupportedMediaType, gin.H{"error": err.Error()})
 		return
 	}
-	resp, err := h.usecase.SetAsset(c.Request.Context(), c.GetString("user_email"), slot, url)
+	resp, previous, err := h.usecase.SetAsset(c.Request.Context(), c.GetString("user_email"), slot, url)
 	audit.Record(c, audit_connectors.Event{Action: "branding.asset.uploaded", ResourceType: "branding", ResourceID: slot},
 		audit_domain.CONFIG_CHANGED, audit_domain.CONFIG_CHANGED, err)
 	if errors.Is(err, usecase.ErrUnknownAssetSlot) {
@@ -167,6 +169,9 @@ func (h *BrandingHandler) UploadAsset(c *gin.Context) {
 		c.JSON(http.StatusInternalServerError, gin.H{"error": "could not save asset"})
 		return
 	}
+	if previous != "" && previous != url {
+		h.removeIfUnreferenced(c.Request.Context(), previous)
+	}
 	c.JSON(http.StatusOK, resp)
 }
 

backend/modules/appconfig/handler/bulk_branding.go

@@ -141,14 +141,21 @@ func (h *BulkBrandingHandler) UploadAsset(c *gin.Context) {
 	}
 	actorEmail := c.GetString("user_email")
 	var result common_models.BulkResult
+	replaced := make(map[string]struct{})
 	for _, tid := range tenantIDs {
 		// ponytail: skip default (platform-plane) tenant — bulk calls must not silently overwrite operator branding
 		if tid == authz.DefaultTenantID {
 			continue
 		}
 		ctx := authz.WithTenantID(c.Request.Context(), tid)
-		_, err := h.brand.SetAsset(ctx, actorEmail, slot, url)
+		_, previous, err := h.brand.SetAsset(ctx, actorEmail, slot, url)
 		result.Append(tid, err)
+		if err == nil && previous != "" && previous != url {
+			replaced[previous] = struct{}{}
+		}
+	}
+	for previous := range replaced {
+		bh.removeIfUnreferenced(c.Request.Context(), previous)
 	}
 	c.JSON(http.StatusOK, result)
 }

backend/modules/appconfig/repository/config.go

@@ -99,6 +99,21 @@ func (r *pgRepo) GetOwn(ctx context.Context, key string) (*domain.Config, error)
 	return &c, nil
 }
 
+// CountValueContains counts rows for `key` across every tenant whose JSON value
+// contains `needle`. Branding URLs carry a 16-hex-char nonce, so a substring
+// match is unambiguous — no other row's value can collide.
+func (r *pgRepo) CountValueContains(ctx context.Context, key, needle string) (int, error) {
+	var n int64
+	err := r.db.WithContext(tenancy.WithAllTenantsRead(ctx)).
+		Model(&domain.Config{}).
+		Where("key = ? AND value LIKE ?", key, "%"+needle+"%").
+		Count(&n).Error
+	if err != nil {
+		return 0, err
+	}
+	return int(n), nil
+}
+
 func (r *pgRepo) Save(ctx context.Context, c *domain.Config) error {
 	tenant := actingTenant(ctx)
 

backend/modules/appconfig/usecase/branding.go

@@ -134,29 +134,44 @@ func (s *brandingService) Seed(ctx context.Context, req dto.BrandingRequest) (*d
 	return &resp, nil
 }
 
-func (s *brandingService) SetAsset(ctx context.Context, actor, slot, url string) (*dto.BrandingResponse, error) {
+func (s *brandingService) SetAsset(ctx context.Context, actor, slot, url string) (*dto.BrandingResponse, string, error) {
 	cur, err := s.read(ctx)
 	if err != nil {
-		return nil, err
+		return nil, "", err
 	}
+	var previous string
 	switch slot {
 	case AssetLogo:
-		cur.LogoURL = url
+		previous, cur.LogoURL = cur.LogoURL, url
 	case AssetLogoDark:
-		cur.LogoDarkURL = url
+		previous, cur.LogoDarkURL = cur.LogoDarkURL, url
 	case AssetFavicon:
-		cur.FaviconURL = url
+		previous, cur.FaviconURL = cur.FaviconURL, url
 	case AssetReportLogo:
-		cur.ReportLogoURL = url
+		previous, cur.ReportLogoURL = cur.ReportLogoURL, url
 	case AssetReportCover:
-		cur.ReportCoverURL = url
+		previous, cur.ReportCoverURL = cur.ReportCoverURL, url
 	default:
-		return nil, ErrUnknownAssetSlot
+		return nil, "", ErrUnknownAssetSlot
 	}
 	if err := s.save(ctx, actor, &cur); err != nil {
-		return nil, err
+		return nil, "", err
+	}
+	return &cur, previous, nil
+}
+
+// IsBrandingAssetReferenced reports whether any tenant's branding row still
+// mentions `url`. Callers use this after replacing an asset to decide whether
+// the file on disk can be removed.
+func (s *brandingService) IsBrandingAssetReferenced(ctx context.Context, url string) (bool, error) {
+	if strings.TrimSpace(url) == "" {
+		return false, nil
+	}
+	n, err := s.repo.CountValueContains(ctx, brandingConfigKey, url)
+	if err != nil {
+		return false, err
 	}
-	return &cur, nil
+	return n > 0, nil
 }
 
 // GetPublic returns the effective branding for the (unauthenticated) login page.

backend/modules/appconfig/usecase/branding_test.go

@@ -0,0 +1,89 @@
+package usecase
+
+import (
+	"context"
+	"testing"
+
+	"github.com/utmstack/utmstack/backend/modules/appconfig/domain"
+)
+
+// A tenant that never customized branding must NOT see the master's brand.
+// Bug: master edits branding → tenants without their own row inherited it via
+// GetByKey's default-tenant fallback. Fix: read via GetOwn.
+type fakeRepo struct{ own *domain.Config }
+
+func (f *fakeRepo) List(context.Context) ([]domain.Config, error)  { return nil, nil }
+func (f *fakeRepo) Save(context.Context, *domain.Config) error     { return nil }
+func (f *fakeRepo) GetByKey(context.Context, string) (*domain.Config, error) {
+	return &domain.Config{Key: brandingConfigKey, Value: `{"enabled":true,"productName":"MasterCo"}`}, nil
+}
+func (f *fakeRepo) GetOwn(context.Context, string) (*domain.Config, error) { return f.own, nil }
+func (f *fakeRepo) CountValueContains(context.Context, string, string) (int, error) {
+	return 0, nil
+}
+
+func TestBrandingDoesNotInheritMaster(t *testing.T) {
+	s := NewBranding(&fakeRepo{own: nil})
+	got, err := s.Get(context.Background())
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	if got.ProductName != defaultProductName {
+		t.Errorf("productName = %q, want defaults (%q) — leaked master brand", got.ProductName, defaultProductName)
+	}
+	if got.Enabled {
+		t.Error("enabled = true, want false — leaked master brand")
+	}
+}
+
+// SetAsset must return the URL that used to occupy the slot so the caller can
+// decide whether to garbage-collect the old file. Without this, an admin
+// re-upload silently orphans the previous file and its cleanup nukes files
+// other tenants still point to.
+func TestSetAssetReturnsPreviousURL(t *testing.T) {
+	repo := &fakeRepo{own: &domain.Config{Key: brandingConfigKey, Value: `{"logoURL":"/uploads/branding/logo-old.png"}`}}
+	s := NewBranding(repo)
+	_, previous, err := s.SetAsset(context.Background(), "alex", AssetLogo, "/uploads/branding/logo-new.png")
+	if err != nil {
+		t.Fatalf("SetAsset: %v", err)
+	}
+	if previous != "/uploads/branding/logo-old.png" {
+		t.Errorf("previous = %q, want the URL that was in the slot before", previous)
+	}
+}
+
+func TestSetAssetPreviousIsEmptyWhenSlotWasUnset(t *testing.T) {
+	s := NewBranding(&fakeRepo{own: nil})
+	_, previous, err := s.SetAsset(context.Background(), "alex", AssetLogo, "/uploads/branding/logo-first.png")
+	if err != nil {
+		t.Fatalf("SetAsset: %v", err)
+	}
+	if previous != "" {
+		t.Errorf("previous = %q, want empty — nothing to GC on first upload", previous)
+	}
+}
+
+// IsBrandingAssetReferenced routes through the repo count so callers only delete
+// files no tenant still points to.
+type countingRepo struct {
+	fakeRepo
+	count int
+}
+
+func (c *countingRepo) CountValueContains(context.Context, string, string) (int, error) {
+	return c.count, nil
+}
+
+func TestIsReferencedTracksCount(t *testing.T) {
+	s := NewBranding(&countingRepo{count: 2})
+	ref, err := s.IsBrandingAssetReferenced(context.Background(), "/uploads/branding/logo-x.png")
+	if err != nil || !ref {
+		t.Fatalf("want referenced=true, got %v err=%v", ref, err)
+	}
+
+	s = NewBranding(&countingRepo{count: 0})
+	ref, err = s.IsBrandingAssetReferenced(context.Background(), "/uploads/branding/logo-x.png")
+	if err != nil || ref {
+		t.Fatalf("want referenced=false, got %v err=%v", ref, err)
+	}
+}