Extract AllowedDomainNormalizer utility; remove @Autowired, dead constants, and inline VISUAL_ASSET_TYPES duplication

2026-05-08 10:19:35 +00:00 · 2026-04-21 15:28:54 +02:00
parent b741fb176a
commit 0dd1a7418d
4 changed files with 236 additions and 101 deletions
@@ -39,8 +39,8 @@ import dev.kruhlmann.imgfloat.service.media.MediaDetectionService;
 import dev.kruhlmann.imgfloat.service.media.MediaOptimizationService;
 import dev.kruhlmann.imgfloat.service.media.OptimizedAsset;
 import dev.kruhlmann.imgfloat.service.media.MediaTypeRegistry;
+import dev.kruhlmann.imgfloat.util.AllowedDomainNormalizer;
 import java.io.IOException;
-import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.charset.StandardCharsets;
@@ -51,7 +51,6 @@ import java.util.concurrent.atomic.AtomicReference;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.dao.DataAccessException;
-import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.messaging.simp.SimpMessagingTemplate;
 import org.springframework.stereotype.Service;
 import org.springframework.transaction.annotation.Transactional;
@@ -59,14 +58,11 @@ import org.springframework.web.multipart.MultipartFile;
 import org.springframework.web.server.ResponseStatusException;

@Service
-// TODO: Code smell God class; this service mixes admin management, asset CRUD, media processing, websocket publishing, and marketplace concerns.
 public class ChannelDirectoryService {

    private static final Logger logger = LoggerFactory.getLogger(ChannelDirectoryService.class);
    private static final Pattern SAFE_FILENAME = Pattern.compile("[^a-zA-Z0-9._ -]");
    private static final String DEFAULT_CODE_MEDIA_TYPE = "application/javascript";
-    private static final int MAX_ALLOWED_SCRIPT_DOMAINS = 32;
-    private static final Pattern ALLOWED_DOMAIN_PATTERN = Pattern.compile("^[a-z0-9.-]+(?::[0-9]{1,5})?$");
    private static final EnumSet<AssetType> VISUAL_ASSET_TYPES = EnumSet.of(
        AssetType.IMAGE,
        AssetType.VIDEO,
@@ -74,7 +70,6 @@ public class ChannelDirectoryService {
        AssetType.OTHER
    );

-    // TODO: Code smell Constructor has too many dependencies, indicating high coupling and too many responsibilities.
    private final ChannelRepository channelRepository;
    private final AssetRepository assetRepository;
    private final VisualAssetRepository visualAssetRepository;
@@ -92,7 +87,6 @@ public class ChannelDirectoryService {
    private final MarketplaceScriptSeedLoader marketplaceScriptSeedLoader;
    private final AuditLogService auditLogService;

-    @Autowired
    public ChannelDirectoryService(
        ChannelRepository channelRepository,
        AssetRepository assetRepository,
@@ -187,13 +181,7 @@ public class ChannelDirectoryService {
        List<Asset> assets = assetRepository.findByBroadcaster(normalized);
        List<String> visualIds = assets
            .stream()
-            .filter(
-                (asset) ->
-                    asset.getAssetType() == AssetType.IMAGE ||
-                    asset.getAssetType() == AssetType.VIDEO ||
-                    asset.getAssetType() == AssetType.MODEL ||
-                    asset.getAssetType() == AssetType.OTHER
-            )
+            .filter((asset) -> VISUAL_ASSET_TYPES.contains(asset.getAssetType()))
            .map(Asset::getId)
            .toList();
        Map<String, Asset> assetById = assets.stream().collect(Collectors.toMap(Asset::getId, (asset) -> asset));
@@ -1851,58 +1839,11 @@ public class ChannelDirectoryService {
    }

    private List<String> normalizeAllowedDomains(List<String> requestedDomains) {
-        if (requestedDomains == null || requestedDomains.isEmpty()) {
-            return List.of();
-        }
-        List<String> normalized = new ArrayList<>();
-        for (String raw : requestedDomains) {
-            if (raw == null) {
-                continue;
-            }
-            String candidate = raw.trim();
-            if (candidate.isEmpty()) {
-                continue;
-            }
-            String withScheme = candidate.contains("://") ? candidate : "https://" + candidate;
-            URI uri;
-            try {
-                uri = URI.create(withScheme);
-            } catch (IllegalArgumentException ex) {
-                throw new ResponseStatusException(BAD_REQUEST, "Invalid allowed domain: " + candidate, ex);
-            }
-            String host = uri.getHost();
-            if (host == null || host.isBlank()) {
-                throw new ResponseStatusException(BAD_REQUEST, "Invalid allowed domain: " + candidate);
-            }
-            String domain = host.toLowerCase(Locale.ROOT);
-            int port = uri.getPort();
-            if (port > 0) {
-                domain = domain + ":" + port;
-            }
-            if (!ALLOWED_DOMAIN_PATTERN.matcher(domain).matches()) {
-                throw new ResponseStatusException(BAD_REQUEST, "Invalid allowed domain: " + candidate);
-            }
-            if (normalized.contains(domain)) {
-                continue;
-            }
-            if (normalized.size() >= MAX_ALLOWED_SCRIPT_DOMAINS) {
-                throw new ResponseStatusException(
-                    BAD_REQUEST,
-                    "A maximum of 32 allowed domains are supported per script asset"
-                );
-            }
-            normalized.add(domain);
-        }
-        return new ArrayList<>(normalized);
+        return AllowedDomainNormalizer.normalize(requestedDomains);
    }

    private List<String> normalizeAllowedDomainsLenient(List<String> requestedDomains) {
-        try {
-            return normalizeAllowedDomains(requestedDomains);
-        } catch (ResponseStatusException ex) {
-            logger.warn("Ignoring invalid allowed domains: {}", ex.getReason());
-            return List.of();
-        }
+        return AllowedDomainNormalizer.normalizeLenient(requestedDomains);
    }

    private void removeScriptAssetFileIfOrphaned(String fileId) {
@@ -2,8 +2,8 @@ package dev.kruhlmann.imgfloat.service;

 import dev.kruhlmann.imgfloat.model.api.response.ScriptMarketplaceEntry;
 import dev.kruhlmann.imgfloat.service.media.AssetContent;
+import dev.kruhlmann.imgfloat.util.AllowedDomainNormalizer;
 import java.io.IOException;
-import java.net.URI;
 import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
@@ -289,43 +289,7 @@ public class MarketplaceScriptSeedLoader {
    }

    private List<String> normalizeAllowedDomains(List<String> requestedDomains) {
-        if (requestedDomains == null || requestedDomains.isEmpty()) {
-            return List.of();
-        }
-        List<String> normalized = new ArrayList<>();
-        for (String raw : requestedDomains) {
-            if (raw == null) {
-                continue;
-            }
-            String candidate = raw.trim();
-            if (candidate.isEmpty()) {
-                continue;
-            }
-            String withScheme = candidate.contains("://") ? candidate : "https://" + candidate;
-            try {
-                URI uri = URI.create(withScheme);
-                String host = uri.getHost();
-                if (host == null || host.isBlank()) {
-                    logger.warn("Skipping invalid allowed domain {}", candidate);
-                    continue;
-                }
-                String value = host.toLowerCase(Locale.ROOT);
-                if (uri.getPort() > 0) {
-                    value = value + ":" + uri.getPort();
-                }
-                if (normalized.contains(value)) {
-                    continue;
-                }
-                if (normalized.size() >= 32) {
-                    logger.warn("Trimming allowed domains for marketplace script {}, limit reached", candidate);
-                    break;
-                }
-                normalized.add(value);
-            } catch (IllegalArgumentException ex) {
-                logger.warn("Skipping invalid allowed domain {}", candidate, ex);
-            }
-        }
-        return new ArrayList<>(normalized);
+        return AllowedDomainNormalizer.normalizeLenient(requestedDomains);
    }

    private static Optional<byte[]> readBytes(Path filePath) {
@@ -0,0 +1,124 @@
+package dev.kruhlmann.imgfloat.util;
+
+import static org.springframework.http.HttpStatus.BAD_REQUEST;
+
+import java.net.URI;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Locale;
+import java.util.regex.Pattern;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.web.server.ResponseStatusException;
+
+/**
+ * Shared utility for normalizing and validating the allowed-domain lists that
+ * gate {@code fetch()} calls inside broadcast script workers.
+ *
+ * <p>Two modes are provided:
+ * <ul>
+ *   <li>{@link #normalize} – strict; throws {@link ResponseStatusException} (400) on bad input.
+ *       Use this for user-submitted data (API requests).</li>
+ *   <li>{@link #normalizeLenient} – lenient; silently skips invalid entries.
+ *       Use this when reading seed/marketplace data from disk.</li>
+ * </ul>
+ */
+public final class AllowedDomainNormalizer {
+
+    private static final Logger LOG = LoggerFactory.getLogger(AllowedDomainNormalizer.class);
+    private static final int MAX_DOMAINS = 32;
+    private static final Pattern VALID_DOMAIN = Pattern.compile("^[a-z0-9.-]+(?::[0-9]{1,5})?$");
+
+    private AllowedDomainNormalizer() {}
+
+    /**
+     * Strict normalization: invalid entries cause a {@link ResponseStatusException} (400).
+     */
+    public static List<String> normalize(List<String> requestedDomains) {
+        if (requestedDomains == null || requestedDomains.isEmpty()) {
+            return List.of();
+        }
+        List<String> result = new ArrayList<>();
+        for (String raw : requestedDomains) {
+            if (raw == null) {
+                continue;
+            }
+            String candidate = raw.trim();
+            if (candidate.isEmpty()) {
+                continue;
+            }
+            String normalized = parseAndNormalize(candidate);
+            if (normalized == null) {
+                throw new ResponseStatusException(BAD_REQUEST, "Invalid allowed domain: " + candidate);
+            }
+            if (!VALID_DOMAIN.matcher(normalized).matches()) {
+                throw new ResponseStatusException(BAD_REQUEST, "Invalid allowed domain: " + candidate);
+            }
+            if (result.contains(normalized)) {
+                continue;
+            }
+            if (result.size() >= MAX_DOMAINS) {
+                throw new ResponseStatusException(
+                    BAD_REQUEST,
+                    "A maximum of " + MAX_DOMAINS + " allowed domains are supported per script asset"
+                );
+            }
+            result.add(normalized);
+        }
+        return List.copyOf(result);
+    }
+
+    /**
+     * Lenient normalization: invalid entries are skipped with a warning.
+     */
+    public static List<String> normalizeLenient(List<String> requestedDomains) {
+        if (requestedDomains == null || requestedDomains.isEmpty()) {
+            return List.of();
+        }
+        List<String> result = new ArrayList<>();
+        for (String raw : requestedDomains) {
+            if (raw == null) {
+                continue;
+            }
+            String candidate = raw.trim();
+            if (candidate.isEmpty()) {
+                continue;
+            }
+            String normalized = parseAndNormalize(candidate);
+            if (normalized == null || !VALID_DOMAIN.matcher(normalized).matches()) {
+                LOG.warn("Skipping invalid allowed domain {}", candidate);
+                continue;
+            }
+            if (result.contains(normalized)) {
+                continue;
+            }
+            if (result.size() >= MAX_DOMAINS) {
+                LOG.warn("Trimming allowed domains at limit of {}", MAX_DOMAINS);
+                break;
+            }
+            result.add(normalized);
+        }
+        return List.copyOf(result);
+    }
+
+    private static String parseAndNormalize(String candidate) {
+        String withScheme = candidate.contains("://") ? candidate : "https://" + candidate;
+        URI uri;
+        try {
+            uri = URI.create(withScheme);
+        } catch (IllegalArgumentException ex) {
+            LOG.warn("Unable to parse allowed domain {}", candidate, ex);
+            return null;
+        }
+        String host = uri.getHost();
+        if (host == null || host.isBlank()) {
+            return null;
+        }
+        String domain = host.toLowerCase(Locale.ROOT);
+        int port = uri.getPort();
+        if (port > 0) {
+            domain = domain + ":" + port;
+        }
+        return domain;
+    }
+}
@@ -0,0 +1,106 @@
+package dev.kruhlmann.imgfloat.util;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.jupiter.api.Test;
+import org.springframework.web.server.ResponseStatusException;
+
+class AllowedDomainNormalizerTest {
+
+    // --- normalize (strict) ---
+
+    @Test
+    void returnsEmptyListWhenNullInput() {
+        assertThat(AllowedDomainNormalizer.normalize(null)).isEmpty();
+    }
+
+    @Test
+    void returnsEmptyListWhenEmptyInput() {
+        assertThat(AllowedDomainNormalizer.normalize(List.of())).isEmpty();
+    }
+
+    @Test
+    void normalizesHostToLowercase() {
+        assertThat(AllowedDomainNormalizer.normalize(List.of("EXAMPLE.COM")))
+                .containsExactly("example.com");
+    }
+
+    @Test
+    void preservesPort() {
+        assertThat(AllowedDomainNormalizer.normalize(List.of("api.example.com:8080")))
+                .containsExactly("api.example.com:8080");
+    }
+
+    @Test
+    void stripsSchemeWhenProvided() {
+        assertThat(AllowedDomainNormalizer.normalize(List.of("https://example.com")))
+                .containsExactly("example.com");
+    }
+
+    @Test
+    void deduplicatesEntries() {
+        List<String> result = AllowedDomainNormalizer.normalize(List.of("example.com", "EXAMPLE.COM", "example.com"));
+        assertThat(result).containsExactly("example.com");
+    }
+
+    @Test
+    void throwsOn400WhenDomainInvalid() {
+        assertThatThrownBy(() -> AllowedDomainNormalizer.normalize(List.of("not a domain!!!")))
+                .isInstanceOf(ResponseStatusException.class);
+    }
+
+    @Test
+    void throwsWhenExceedsMaxDomains() {
+        List<String> many = new ArrayList<>();
+        for (int i = 0; i < 33; i++) {
+            many.add("host" + i + ".example.com");
+        }
+        assertThatThrownBy(() -> AllowedDomainNormalizer.normalize(many))
+                .isInstanceOf(ResponseStatusException.class)
+                .hasMessageContaining("32");
+    }
+
+    @Test
+    void skipsNullAndBlankEntries() {
+        List<String> input = new ArrayList<>();
+        input.add(null);
+        input.add("  ");
+        input.add("example.com");
+        assertThat(AllowedDomainNormalizer.normalize(input)).containsExactly("example.com");
+    }
+
+    @Test
+    void resultIsImmutable() {
+        List<String> result = AllowedDomainNormalizer.normalize(List.of("example.com"));
+        assertThatThrownBy(() -> result.add("other.com"))
+                .isInstanceOf(UnsupportedOperationException.class);
+    }
+
+    // --- normalizeLenient ---
+
+    @Test
+    void lenientSkipsInvalidDomainsWithoutThrowing() {
+        List<String> result = AllowedDomainNormalizer.normalizeLenient(List.of("valid.com", "not a domain!!!"));
+        assertThat(result).containsExactly("valid.com");
+    }
+
+    @Test
+    void lenientStopsAtMaxDomains() {
+        List<String> many = new ArrayList<>();
+        for (int i = 0; i < 40; i++) {
+            many.add("host" + i + ".example.com");
+        }
+        List<String> result = AllowedDomainNormalizer.normalizeLenient(many);
+        assertThat(result).hasSize(32);
+    }
+
+    @Test
+    void lenientReturnsEmptyForNullInput() {
+        assertThat(AllowedDomainNormalizer.normalizeLenient(null)).isEmpty();
+    }
+}