From 579b58ba5fcaf64732ae32a2f830555dc1176fa0 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Mon, 3 May 2021 19:04:08 +0200 Subject: [PATCH] Show hg binary verification error messages (#1637) Show hg verification error messages on global hg config page if trying to save invalid hg binary. --- .../hg_verification_error_messages.yaml | 2 + .../api/v2/resources/HgConfigResource.java | 13 ++++- .../scm/repository/HgRepositoryHandler.java | 2 - .../java/sonia/scm/repository/HgVerifier.java | 55 +++++++++++++----- .../v2/resources/HgConfigResourceTest.java | 13 ++++- .../autoconfig/PosixAutoConfiguratorTest.java | 6 +- .../sonia/scm/repository/HgVerifierTest.java | 57 +++++++++++-------- .../src/config/Configuration.tsx | 13 +++-- 8 files changed, 113 insertions(+), 48 deletions(-) create mode 100644 gradle/changelog/hg_verification_error_messages.yaml diff --git a/gradle/changelog/hg_verification_error_messages.yaml b/gradle/changelog/hg_verification_error_messages.yaml new file mode 100644 index 0000000000..fb0819ebbc --- /dev/null +++ b/gradle/changelog/hg_verification_error_messages.yaml @@ -0,0 +1,2 @@ +- type: added + description: Show hg binary verification error messages ([#1637](https://github.com/scm-manager/scm-manager/pull/1637)) diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/api/v2/resources/HgConfigResource.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/api/v2/resources/HgConfigResource.java index 41764948df..b110348e43 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/api/v2/resources/HgConfigResource.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/api/v2/resources/HgConfigResource.java @@ -35,6 +35,7 @@ import io.swagger.v3.oas.annotations.tags.Tag; import sonia.scm.config.ConfigurationPermissions; import sonia.scm.repository.HgGlobalConfig; import sonia.scm.repository.HgRepositoryHandler; +import sonia.scm.repository.HgVerifier; import sonia.scm.web.HgVndMediaType; import sonia.scm.web.VndMediaType; @@ -48,6 +49,8 @@ import javax.ws.rs.Path; import javax.ws.rs.Produces; import javax.ws.rs.core.Response; +import static sonia.scm.ScmConstraintViolationException.Builder.doThrow; + /** * RESTful Web Service Resource to manage the configuration of the hg plugin. */ @@ -144,6 +147,7 @@ public class HgConfigResource { responseCode = "204", description = "update success" ) + @ApiResponse(responseCode = "400", description = "invalid configuration") @ApiResponse(responseCode = "401", description = "not authenticated / invalid credentials") @ApiResponse(responseCode = "403", description = "not authorized, the current user does not have the \"configuration:write:hg\" privilege") @ApiResponse( @@ -154,11 +158,16 @@ public class HgConfigResource { schema = @Schema(implementation = ErrorDto.class) )) public Response update(@Valid HgGlobalGlobalConfigDto configDto) { - HgGlobalConfig config = dtoToConfigMapper.map(configDto); - ConfigurationPermissions.write(config).check(); + if (config.getHgBinary() != null) { + HgVerifier.HgVerifyStatus verifyStatus = new HgVerifier().verify(config.getHgBinary()); + doThrow() + .violation(verifyStatus.getDescription()) + .when(verifyStatus != HgVerifier.HgVerifyStatus.VALID); + } + repositoryHandler.setConfig(config); repositoryHandler.storeConfig(); diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgRepositoryHandler.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgRepositoryHandler.java index 4498301e65..3710ffbaa1 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgRepositoryHandler.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgRepositoryHandler.java @@ -24,8 +24,6 @@ package sonia.scm.repository; -//~--- non-JDK imports -------------------------------------------------------- - import com.google.inject.Inject; import com.google.inject.Singleton; import org.slf4j.Logger; diff --git a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgVerifier.java b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgVerifier.java index ef332b8770..df4de270a9 100644 --- a/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgVerifier.java +++ b/scm-plugins/scm-hg-plugin/src/main/java/sonia/scm/repository/HgVerifier.java @@ -29,6 +29,9 @@ import org.slf4j.LoggerFactory; import sonia.scm.io.SimpleCommand; import sonia.scm.io.SimpleCommandResult; +import javax.xml.bind.annotation.XmlEnum; +import javax.xml.bind.annotation.XmlEnumValue; +import javax.xml.bind.annotation.XmlType; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; @@ -48,23 +51,31 @@ public class HgVerifier { } public boolean isValid(HgGlobalConfig config) { - return isValid(config.getHgBinary()); + return verify(config) == HgVerifyStatus.VALID; } - public boolean isValid(String hg) { - return isValid(Paths.get(hg)); + public boolean isValid(Path path) { + return verify(path) == HgVerifyStatus.VALID; } - public boolean isValid(Path hg) { + public HgVerifyStatus verify(HgGlobalConfig config) { + return verify(config.getHgBinary()); + } + + public HgVerifyStatus verify(String hg) { + return verify(Paths.get(hg)); + } + + public HgVerifyStatus verify(Path hg) { LOG.trace("check if hg binary {} is valid", hg); if (!Files.isRegularFile(hg)) { LOG.warn("{} is not a regular file", hg); - return false; + return HgVerifyStatus.NOT_REGULAR_FILE; } if (!Files.isExecutable(hg)) { LOG.warn("{} is not executable", hg); - return false; + return HgVerifyStatus.NOT_EXECUTABLE; } try { @@ -72,31 +83,31 @@ public class HgVerifier { return isVersionValid(hg, version); } catch (IOException ex) { LOG.warn("failed to resolve version of {}: ", hg, ex); - return false; + return HgVerifyStatus.COULD_NOT_RESOLVE_VERSION; } catch (InterruptedException ex) { Thread.currentThread().interrupt(); LOG.warn("failed to resolve version of {}: ", hg, ex); - return false; + return HgVerifyStatus.COULD_NOT_RESOLVE_VERSION; } } - private boolean isVersionValid(Path hg, String version) { + private HgVerifyStatus isVersionValid(Path hg, String version) { String[] parts = version.split("\\."); if (parts.length < 2) { LOG.warn("{} returned invalid version: {}", hg, version); - return false; + return HgVerifyStatus.INVALID_VERSION; } try { int major = Integer.parseInt(parts[0]); if (major < 4) { LOG.warn("{} is too old, we need at least mercurial 4.x", hg); - return false; + return HgVerifyStatus.VERSION_TOO_OLD; } } catch (NumberFormatException ex) { LOG.warn("{} returned invalid version {}", hg, version); - return false; + return HgVerifyStatus.INVALID_VERSION; } - return true; + return HgVerifyStatus.VALID; } private VersionResolver defaultVersionResolver() { @@ -115,4 +126,22 @@ public class HgVerifier { String resolveVersion(Path hg) throws IOException, InterruptedException; } + public enum HgVerifyStatus { + VALID("hg binary is valid"), + NOT_REGULAR_FILE("hg binary is not a regular file"), + NOT_EXECUTABLE("hg binary is not executable"), + INVALID_VERSION("hg binary returned invalid version"), + VERSION_TOO_OLD("hg binary version is too old, we need at least 4.x"), + COULD_NOT_RESOLVE_VERSION("failed to resolve version of hg binary"); + + private final String description; + + HgVerifyStatus(String description) { + this.description = description; + } + + public String getDescription() { + return description; + } + } } diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/api/v2/resources/HgConfigResourceTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/api/v2/resources/HgConfigResourceTest.java index 0a13590130..597a865de6 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/api/v2/resources/HgConfigResourceTest.java +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/api/v2/resources/HgConfigResourceTest.java @@ -155,6 +155,13 @@ public class HgConfigResourceTest { assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); } + @Test + @SubjectAware(username = "writeOnly") + public void shouldNotUpdateConfigForInvalidBinary() throws URISyntaxException { + MockHttpResponse response = put("{\"hgBinary\":\"3.2.1\"}"); + assertEquals(400, response.getStatus()); + } + @Test @SubjectAware(username = "readOnly") public void shouldNotUpdateConfigWhenNotAuthorized() throws URISyntaxException, UnsupportedEncodingException { @@ -172,9 +179,13 @@ public class HgConfigResourceTest { } private MockHttpResponse put() throws URISyntaxException { + return put("{\"disabled\":true}"); + } + + private MockHttpResponse put(String config) throws URISyntaxException { MockHttpRequest request = MockHttpRequest.put("/" + HgConfigResource.HG_CONFIG_PATH_V2) .contentType(HgVndMediaType.CONFIG) - .content("{\"disabled\":true}".getBytes()); + .content(config.getBytes()); MockHttpResponse response = new MockHttpResponse(); dispatcher.invoke(request, response); diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/autoconfig/PosixAutoConfiguratorTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/autoconfig/PosixAutoConfiguratorTest.java index c9ac5be62c..ee7e552db3 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/autoconfig/PosixAutoConfiguratorTest.java +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/autoconfig/PosixAutoConfiguratorTest.java @@ -104,9 +104,9 @@ class PosixAutoConfiguratorTest { PosixAutoConfigurator configurator = new PosixAutoConfigurator( verifier, createEnv(one), ImmutableList.of( - two.toAbsolutePath().toString(), - three.toAbsolutePath().toString() - ) + two.toAbsolutePath().toString(), + three.toAbsolutePath().toString() + ) ); HgGlobalConfig config = new HgGlobalConfig(); diff --git a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/HgVerifierTest.java b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/HgVerifierTest.java index 707997e124..b733242acd 100644 --- a/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/HgVerifierTest.java +++ b/scm-plugins/scm-hg-plugin/src/test/java/sonia/scm/repository/HgVerifierTest.java @@ -51,22 +51,22 @@ class HgVerifierTest { private final HgVerifier verifier = new HgVerifier(); @Test - void shouldReturnFalseIfFileDoesNotExists(@TempDir Path directory) { + void shouldReturnNotRegularFileIfFileDoesNotExists(@TempDir Path directory) { Path hg = directory.resolve("hg"); - boolean isValid = verify(hg); + HgVerifier.HgVerifyStatus verifyStatus = verify(hg); - assertThat(isValid).isFalse(); + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.NOT_REGULAR_FILE); } @Test - void shouldReturnFalseIfFileIsADirectory(@TempDir Path directory) throws IOException { + void shouldReturnNotRegularFileIfFileIsADirectory(@TempDir Path directory) throws IOException { Path hg = directory.resolve("hg"); Files.createDirectories(hg); - boolean isValid = verify(hg); + HgVerifier.HgVerifyStatus verifyStatus = verify(hg); - assertThat(isValid).isFalse(); + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.NOT_REGULAR_FILE); } @Test @@ -74,21 +74,21 @@ class HgVerifierTest { Path hg = directory.resolve("hg"); Files.createFile(hg); - boolean isValid = verify(hg); + HgVerifier.HgVerifyStatus verifyStatus = verify(hg); - assertThat(isValid).isFalse(); + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.NOT_EXECUTABLE); } @Test - void shouldReturnTrueForIfMercurialIsAvailable() { + void shouldReturnValidIfMercurialIsAvailable() { Path hg = findHg(); // skip test if we could not find mercurial Assumptions.assumeTrue(hg != null, "skip test, because we could not find mercurial on path"); - boolean isValid = verify(hg); + HgVerifier.HgVerifyStatus verifyStatus = verify(hg); - assertThat(isValid).isTrue(); + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.VALID); } private Path findHg() { @@ -106,48 +106,59 @@ class HgVerifierTest { return null; } - private boolean verify(Path hg) { + private HgVerifier.HgVerifyStatus verify(Path hg) { HgGlobalConfig config = new HgGlobalConfig(); config.setHgBinary(hg.toString()); - return verifier.isValid(config); + return verifier.verify(config); } } @ParameterizedTest - @ValueSource(strings = { "3-2-1", "x.y.z", "3.2.0" }) - void shouldReturnFalseForInvalidVersions(String version, @TempDir Path directory) throws IOException { + @ValueSource(strings = { "3-2-1", "x.y.z" }) + void shouldReturnInvalidVersions(String version, @TempDir Path directory) throws IOException { HgVerifier verifier = new HgVerifier(hg -> version); Path hg = createHg(directory); - boolean isValid = verifier.isValid(hg); + HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg); - assertThat(isValid).isFalse(); + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.INVALID_VERSION); } @Test - void shouldReturnFalseOnIOException(@TempDir Path directory) throws IOException { + void shouldReturnTooOldVersion(@TempDir Path directory) throws IOException { + HgVerifier verifier = new HgVerifier(hg -> "3.2.1"); + + Path hg = createHg(directory); + + HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg); + + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.VERSION_TOO_OLD); + } + + @Test + void shouldReturnCouldNotResolveOnIOException(@TempDir Path directory) throws IOException { HgVerifier verifier = new HgVerifier(hg -> { throw new IOException("failed"); }); Path hg = createHg(directory); - boolean isValid = verifier.isValid(hg); + HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg); - assertThat(isValid).isFalse(); + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.COULD_NOT_RESOLVE_VERSION); } @Test - void shouldReturnTrue(@TempDir Path directory) throws IOException { + void shouldReturnValid(@TempDir Path directory) throws IOException { HgVerifier verifier = new HgVerifier(hg -> "4.2.0"); Path hg = createHg(directory); - boolean isValid = verifier.isValid(hg); + HgVerifier.HgVerifyStatus verifyStatus = verifier.verify(hg); - assertThat(isValid).isTrue(); + assertThat(verifyStatus).isEqualTo(HgVerifier.HgVerifyStatus.VALID); } @Nonnull diff --git a/scm-ui/ui-components/src/config/Configuration.tsx b/scm-ui/ui-components/src/config/Configuration.tsx index 02f8f3b061..c3f891e033 100644 --- a/scm-ui/ui-components/src/config/Configuration.tsx +++ b/scm-ui/ui-components/src/config/Configuration.tsx @@ -135,7 +135,8 @@ class Configuration extends React.Component { event.preventDefault(); this.setState({ - modifying: true + modifying: true, + error: undefined }); const { modifiedConfiguration } = this.state; @@ -182,9 +183,7 @@ class Configuration extends React.Component { const { t } = this.props; const { fetching, error, configuration, modifying, valid } = this.state; - if (error) { - return ; - } else if (fetching || !configuration) { + if (fetching || !configuration) { return ; } else { const readOnly = this.isReadOnly(); @@ -200,6 +199,12 @@ class Configuration extends React.Component { {this.renderConfigChangedNotification()}
{this.props.render(renderProps)} + {error && ( + <> +
+ + + )}
}