refactor(web): authoritative Content-Length, typed Optional<AppVersion> in controller
This commit is contained in:
@@ -2,7 +2,6 @@ package com.cameleer.server.app.security;
|
|||||||
|
|
||||||
import com.cameleer.server.app.web.ArtifactDownloadTokenSigner;
|
import com.cameleer.server.app.web.ArtifactDownloadTokenSigner;
|
||||||
import org.springframework.beans.factory.InitializingBean;
|
import org.springframework.beans.factory.InitializingBean;
|
||||||
import org.springframework.beans.factory.annotation.Value;
|
|
||||||
import org.springframework.boot.context.properties.EnableConfigurationProperties;
|
import org.springframework.boot.context.properties.EnableConfigurationProperties;
|
||||||
import org.springframework.context.annotation.Bean;
|
import org.springframework.context.annotation.Bean;
|
||||||
import org.springframework.context.annotation.Configuration;
|
import org.springframework.context.annotation.Configuration;
|
||||||
@@ -39,9 +38,8 @@ public class SecurityBeanConfig {
|
|||||||
}
|
}
|
||||||
|
|
||||||
@Bean
|
@Bean
|
||||||
public ArtifactDownloadTokenSigner artifactDownloadTokenSigner(
|
public ArtifactDownloadTokenSigner artifactDownloadTokenSigner(SecurityProperties properties) {
|
||||||
@Value("${cameleer.server.security.jwtsecret}") String jwtSecret) {
|
return new ArtifactDownloadTokenSigner(properties.getJwtSecret(), Clock.systemUTC());
|
||||||
return new ArtifactDownloadTokenSigner(jwtSecret, Clock.systemUTC());
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@Bean
|
@Bean
|
||||||
|
|||||||
@@ -43,6 +43,11 @@ public class FilesystemArtifactStore implements ArtifactStore {
|
|||||||
return Files.newInputStream(pathOf(coords));
|
return Files.newInputStream(pathOf(coords));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public long size(ArtifactCoordinates coords) throws IOException {
|
||||||
|
return Files.size(pathOf(coords));
|
||||||
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
public boolean exists(ArtifactCoordinates coords) {
|
public boolean exists(ArtifactCoordinates coords) {
|
||||||
return Files.exists(pathOf(coords));
|
return Files.exists(pathOf(coords));
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package com.cameleer.server.app.web;
|
|||||||
|
|
||||||
import com.cameleer.server.core.runtime.AppService;
|
import com.cameleer.server.core.runtime.AppService;
|
||||||
import com.cameleer.server.core.runtime.AppVersion;
|
import com.cameleer.server.core.runtime.AppVersion;
|
||||||
|
import com.cameleer.server.core.storage.ArtifactCoordinates;
|
||||||
import com.cameleer.server.core.storage.ArtifactStore;
|
import com.cameleer.server.core.storage.ArtifactStore;
|
||||||
import org.springframework.core.io.InputStreamResource;
|
import org.springframework.core.io.InputStreamResource;
|
||||||
import org.springframework.http.MediaType;
|
import org.springframework.http.MediaType;
|
||||||
@@ -14,6 +15,7 @@ import org.springframework.web.bind.annotation.RestController;
|
|||||||
|
|
||||||
import java.io.IOException;
|
import java.io.IOException;
|
||||||
import java.io.InputStream;
|
import java.io.InputStream;
|
||||||
|
import java.util.Optional;
|
||||||
import java.util.UUID;
|
import java.util.UUID;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -44,16 +46,17 @@ public class ArtifactDownloadController {
|
|||||||
if (!signer.verify(appVersionId, exp, sig)) {
|
if (!signer.verify(appVersionId, exp, sig)) {
|
||||||
return ResponseEntity.status(401).build();
|
return ResponseEntity.status(401).build();
|
||||||
}
|
}
|
||||||
AppVersion version;
|
Optional<AppVersion> versionOpt = appService.findVersion(appVersionId);
|
||||||
try {
|
if (versionOpt.isEmpty()) {
|
||||||
version = appService.getVersion(appVersionId);
|
|
||||||
} catch (IllegalArgumentException e) {
|
|
||||||
return ResponseEntity.notFound().build();
|
return ResponseEntity.notFound().build();
|
||||||
}
|
}
|
||||||
InputStream in = artifactStore.get(appService.coordinatesFor(version));
|
AppVersion version = versionOpt.get();
|
||||||
|
ArtifactCoordinates coords = appService.coordinatesFor(version);
|
||||||
|
long size = artifactStore.size(coords);
|
||||||
|
InputStream in = artifactStore.get(coords);
|
||||||
return ResponseEntity.ok()
|
return ResponseEntity.ok()
|
||||||
.contentType(MediaType.parseMediaType("application/java-archive"))
|
.contentType(MediaType.parseMediaType("application/java-archive"))
|
||||||
.contentLength(version.jarSizeBytes())
|
.contentLength(size)
|
||||||
.body(new InputStreamResource(in));
|
.body(new InputStreamResource(in));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -72,6 +72,15 @@ class FilesystemArtifactStoreTest {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void sizeReturnsActualOnDiskBytes(@TempDir Path tmp) throws Exception {
|
||||||
|
FilesystemArtifactStore store = new FilesystemArtifactStore(tmp.toString());
|
||||||
|
ArtifactCoordinates c = new ArtifactCoordinates("default", UUID.randomUUID(), 1);
|
||||||
|
byte[] payload = new byte[1234];
|
||||||
|
store.put(c, new java.io.ByteArrayInputStream(payload), payload.length);
|
||||||
|
assertThat(store.size(c)).isEqualTo(1234L);
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void deleteLeavesAppDirAloneWhenSiblingVersionExists(@TempDir Path tmp) throws Exception {
|
void deleteLeavesAppDirAloneWhenSiblingVersionExists(@TempDir Path tmp) throws Exception {
|
||||||
FilesystemArtifactStore store = new FilesystemArtifactStore(tmp.toString());
|
FilesystemArtifactStore store = new FilesystemArtifactStore(tmp.toString());
|
||||||
|
|||||||
@@ -10,6 +10,7 @@ import org.springframework.test.web.servlet.MockMvc;
|
|||||||
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
|
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
|
||||||
|
|
||||||
import java.io.ByteArrayInputStream;
|
import java.io.ByteArrayInputStream;
|
||||||
|
import java.util.Optional;
|
||||||
import java.util.UUID;
|
import java.util.UUID;
|
||||||
|
|
||||||
import static org.mockito.ArgumentMatchers.any;
|
import static org.mockito.ArgumentMatchers.any;
|
||||||
@@ -54,8 +55,9 @@ class ArtifactDownloadControllerTest {
|
|||||||
UUID id = UUID.randomUUID();
|
UUID id = UUID.randomUUID();
|
||||||
when(signer.verify(eq(id), eq(123L), eq("good"))).thenReturn(true);
|
when(signer.verify(eq(id), eq(123L), eq("good"))).thenReturn(true);
|
||||||
AppVersion v = new AppVersion(id, UUID.randomUUID(), 1, "loc", "h", "a.jar", 5L, null, null, null);
|
AppVersion v = new AppVersion(id, UUID.randomUUID(), 1, "loc", "h", "a.jar", 5L, null, null, null);
|
||||||
when(appService.getVersion(id)).thenReturn(v);
|
when(appService.findVersion(id)).thenReturn(Optional.of(v));
|
||||||
when(appService.coordinatesFor(v)).thenReturn(new ArtifactCoordinates("default", v.appId(), 1));
|
when(appService.coordinatesFor(v)).thenReturn(new ArtifactCoordinates("default", v.appId(), 1));
|
||||||
|
when(artifactStore.size(any())).thenReturn(5L);
|
||||||
when(artifactStore.get(any())).thenReturn(new ByteArrayInputStream("hello".getBytes()));
|
when(artifactStore.get(any())).thenReturn(new ByteArrayInputStream("hello".getBytes()));
|
||||||
|
|
||||||
mvc.perform(get("/api/v1/artifacts/{id}", id).param("exp", "123").param("sig", "good"))
|
mvc.perform(get("/api/v1/artifacts/{id}", id).param("exp", "123").param("sig", "good"))
|
||||||
@@ -73,14 +75,14 @@ class ArtifactDownloadControllerTest {
|
|||||||
.andExpect(status().isUnauthorized());
|
.andExpect(status().isUnauthorized());
|
||||||
|
|
||||||
// Defence-in-depth: bad sig must short-circuit before any data lookup.
|
// Defence-in-depth: bad sig must short-circuit before any data lookup.
|
||||||
verify(appService, never()).getVersion(any());
|
verify(appService, never()).findVersion(any());
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void returns404WhenArtifactMissing() throws Exception {
|
void returns404WhenArtifactMissing() throws Exception {
|
||||||
UUID id = UUID.randomUUID();
|
UUID id = UUID.randomUUID();
|
||||||
when(signer.verify(any(), anyLong(), any())).thenReturn(true);
|
when(signer.verify(any(), anyLong(), any())).thenReturn(true);
|
||||||
when(appService.getVersion(id)).thenThrow(new IllegalArgumentException("AppVersion not found"));
|
when(appService.findVersion(id)).thenReturn(Optional.empty());
|
||||||
|
|
||||||
mvc.perform(get("/api/v1/artifacts/{id}", id).param("exp", "123").param("sig", "ok"))
|
mvc.perform(get("/api/v1/artifacts/{id}", id).param("exp", "123").param("sig", "ok"))
|
||||||
.andExpect(status().isNotFound());
|
.andExpect(status().isNotFound());
|
||||||
|
|||||||
@@ -13,6 +13,7 @@ import java.security.MessageDigest;
|
|||||||
import java.util.HexFormat;
|
import java.util.HexFormat;
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
|
import java.util.Optional;
|
||||||
import java.util.UUID;
|
import java.util.UUID;
|
||||||
import java.util.regex.Pattern;
|
import java.util.regex.Pattern;
|
||||||
|
|
||||||
@@ -57,6 +58,13 @@ public class AppService {
|
|||||||
.orElseThrow(() -> new IllegalArgumentException("AppVersion not found: " + versionId));
|
.orElseThrow(() -> new IllegalArgumentException("AppVersion not found: " + versionId));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Typed Optional accessor for callers that distinguish missing-rows from
|
||||||
|
* other failures without relying on the message-based exception convention
|
||||||
|
* of {@link #getVersion(UUID)}. */
|
||||||
|
public Optional<AppVersion> findVersion(UUID versionId) {
|
||||||
|
return versionRepo.findById(versionId);
|
||||||
|
}
|
||||||
|
|
||||||
/** Coordinate corresponding to an AppVersion. Used by callers that need to
|
/** Coordinate corresponding to an AppVersion. Used by callers that need to
|
||||||
* pass the artifact identity to {@link ArtifactStore} (download, delete, exists). */
|
* pass the artifact identity to {@link ArtifactStore} (download, delete, exists). */
|
||||||
public ArtifactCoordinates coordinatesFor(AppVersion version) {
|
public ArtifactCoordinates coordinatesFor(AppVersion version) {
|
||||||
|
|||||||
@@ -22,6 +22,11 @@ public interface ArtifactStore {
|
|||||||
/** Open the artifact for reading. Caller closes. */
|
/** Open the artifact for reading. Caller closes. */
|
||||||
InputStream get(ArtifactCoordinates coords) throws IOException;
|
InputStream get(ArtifactCoordinates coords) throws IOException;
|
||||||
|
|
||||||
|
/** Size of the stored artifact in bytes. Authoritative — read from the backend
|
||||||
|
* at serve time so a stale {@code AppVersion.jarSizeBytes} cannot silently
|
||||||
|
* truncate downloads. */
|
||||||
|
long size(ArtifactCoordinates coords) throws IOException;
|
||||||
|
|
||||||
/** True if an artifact is currently stored under {@code coords}. */
|
/** True if an artifact is currently stored under {@code coords}. */
|
||||||
boolean exists(ArtifactCoordinates coords);
|
boolean exists(ArtifactCoordinates coords);
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user