From 2fb2c0e8a5061d2138ccee057e70709e561af24d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20S=C3=A1nchez?= Date: Wed, 19 Aug 2026 23:21:36 -0600 Subject: [PATCH 1/2] fix[frontend](branding): wired dark icon change --- frontend/src/features/auth/pages/LoginPage.tsx | 6 +++++- frontend/src/shared/layouts/DashboardLayout/Topbar.tsx | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/frontend/src/features/auth/pages/LoginPage.tsx b/frontend/src/features/auth/pages/LoginPage.tsx index 7eaf817b6..e3be37db1 100644 --- a/frontend/src/features/auth/pages/LoginPage.tsx +++ b/frontend/src/features/auth/pages/LoginPage.tsx @@ -1,6 +1,7 @@ import { useEffect, useState } from 'react' import { useNavigate, useLocation, useSearchParams } from 'react-router-dom' import { useBranding } from '@/features/branding' +import { useThemeContext } from '@/app/providers' import { ssoHttpService, type PublicIdentityProvider } from '../services/sso-http.service' import { useTranslation } from 'react-i18next' import { ArrowLeft, Building2, KeyRound, Loader2, Lock, Mail, ShieldCheck, User as UserIcon } from 'lucide-react' @@ -35,9 +36,12 @@ export function LoginPage() { .then(setProviders) .catch(() => setProviders([])) }, []) + const { theme } = useThemeContext() const brandActive = !!branding?.enabled const brandName = (brandActive && branding?.productName) || 'UTMStack' - const brandLogo = (brandActive && branding?.logoUrl) || '/logo.svg' + const brandLogo = + (brandActive && (theme === 'dark' ? branding?.logoDarkUrl || branding?.logoUrl : branding?.logoUrl)) || + '/logo.svg' const { t } = useTranslation() const { login, verifyTfaCode, requestPasswordReset, finishPasswordReset, adoptSession } = useAuth() diff --git a/frontend/src/shared/layouts/DashboardLayout/Topbar.tsx b/frontend/src/shared/layouts/DashboardLayout/Topbar.tsx index 628e4b9a2..a1d42458e 100644 --- a/frontend/src/shared/layouts/DashboardLayout/Topbar.tsx +++ b/frontend/src/shared/layouts/DashboardLayout/Topbar.tsx @@ -132,7 +132,9 @@ export function Topbar() { const { branding } = useBranding() const brandActive = !!branding?.enabled const brandName = (brandActive && branding?.productName) || 'UTMStack' - const brandLogo = (brandActive && branding?.logoUrl) || '/logo.svg' + const brandLogo = + (brandActive && (theme === 'dark' ? branding?.logoDarkUrl || branding?.logoUrl : branding?.logoUrl)) || + '/logo.svg' const userName = user?.name || user?.email || 'User' const userInitials = userName From 14ef8521fc5f70cb4c288d37410213ea81937a86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20S=C3=A1nchez?= Date: Thu, 20 Aug 2026 07:44:49 -0600 Subject: [PATCH 2/2] fix[backend](branding): stop admin upload from deleting tenant icons --- .../appconfig/connectors/repository.go | 4 + .../modules/appconfig/connectors/usecase.go | 6 +- .../appconfig/handler/branding_assets.go | 31 ++++--- .../appconfig/handler/bulk_branding.go | 9 +- .../modules/appconfig/repository/config.go | 15 ++++ backend/modules/appconfig/usecase/branding.go | 35 +++++--- .../appconfig/usecase/branding_test.go | 89 +++++++++++++++++++ 7 files changed, 164 insertions(+), 25 deletions(-) create mode 100644 backend/modules/appconfig/usecase/branding_test.go diff --git a/backend/modules/appconfig/connectors/repository.go b/backend/modules/appconfig/connectors/repository.go index e1604ae47..159e746db 100644 --- a/backend/modules/appconfig/connectors/repository.go +++ b/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) } diff --git a/backend/modules/appconfig/connectors/usecase.go b/backend/modules/appconfig/connectors/usecase.go index d8e6bab38..f5dddb334 100644 --- a/backend/modules/appconfig/connectors/usecase.go +++ b/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 } diff --git a/backend/modules/appconfig/handler/branding_assets.go b/backend/modules/appconfig/handler/branding_assets.go index 8dcffc22d..3198eddd1 100644 --- a/backend/modules/appconfig/handler/branding_assets.go +++ b/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) } diff --git a/backend/modules/appconfig/handler/bulk_branding.go b/backend/modules/appconfig/handler/bulk_branding.go index 90bed681e..d66a5e671 100644 --- a/backend/modules/appconfig/handler/bulk_branding.go +++ b/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) } diff --git a/backend/modules/appconfig/repository/config.go b/backend/modules/appconfig/repository/config.go index ec4c70c81..1342f3dca 100644 --- a/backend/modules/appconfig/repository/config.go +++ b/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) diff --git a/backend/modules/appconfig/usecase/branding.go b/backend/modules/appconfig/usecase/branding.go index b710023f5..429acb701 100644 --- a/backend/modules/appconfig/usecase/branding.go +++ b/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. diff --git a/backend/modules/appconfig/usecase/branding_test.go b/backend/modules/appconfig/usecase/branding_test.go new file mode 100644 index 000000000..ae0b17527 --- /dev/null +++ b/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) + } +}