From: Simon Brandhof Date: Wed, 20 Jun 2018 19:56:10 +0000 (+0200) Subject: Improve SystemPasscode X-Git-Tag: 7.5~905 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=addb50b1e98f50a37290a10e71b17d5eeb9829f6;p=sonarqube.git Improve SystemPasscode * remove unused method isConfigured() * testing passcode before user admin permission is faster when using the recommended passcode --- diff --git a/server/sonar-server/src/main/java/org/sonar/server/ce/ws/InfoAction.java b/server/sonar-server/src/main/java/org/sonar/server/ce/ws/InfoAction.java index cc2921b22ef..32f2df3d6a7 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/ce/ws/InfoAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/ce/ws/InfoAction.java @@ -55,7 +55,7 @@ public class InfoAction implements CeWsAction { @Override public void handle(Request request, Response response) throws Exception { - if (!userSession.isSystemAdministrator() && !systemPasscode.isValid(request)) { + if (!systemPasscode.isValid(request) && !userSession.isSystemAdministrator()) { throw AbstractUserSession.insufficientPrivilegesException(); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/ce/ws/PauseAction.java b/server/sonar-server/src/main/java/org/sonar/server/ce/ws/PauseAction.java index 769fcbafc0a..51137750b99 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/ce/ws/PauseAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/ce/ws/PauseAction.java @@ -53,7 +53,7 @@ public class PauseAction implements CeWsAction { @Override public void handle(Request request, Response response) throws Exception { - if (!userSession.isSystemAdministrator() && !systemPasscode.isValid(request)) { + if (!systemPasscode.isValid(request) && !userSession.isSystemAdministrator()) { throw AbstractUserSession.insufficientPrivilegesException(); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/ce/ws/ResumeAction.java b/server/sonar-server/src/main/java/org/sonar/server/ce/ws/ResumeAction.java index 53527f8f129..a4a0203dfa1 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/ce/ws/ResumeAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/ce/ws/ResumeAction.java @@ -53,7 +53,7 @@ public class ResumeAction implements CeWsAction { @Override public void handle(Request request, Response response) throws Exception { - if (!userSession.isSystemAdministrator() && !systemPasscode.isValid(request)) { + if (!systemPasscode.isValid(request) && !userSession.isSystemAdministrator()) { throw AbstractUserSession.insufficientPrivilegesException(); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/platform/ws/HealthAction.java b/server/sonar-server/src/main/java/org/sonar/server/platform/ws/HealthAction.java index 23b9eefb9d8..faf47b5cac8 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/platform/ws/HealthAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/platform/ws/HealthAction.java @@ -48,7 +48,7 @@ public class HealthAction implements SystemWsAction { @Override public void handle(Request request, Response response) throws Exception { - if (!isPassCodeAuthenticated(request) && !isSystemAdmin()) { + if (!systemPasscode.isValid(request) && !isSystemAdmin()) { throw new ForbiddenException("Insufficient privileges"); } @@ -63,8 +63,4 @@ public class HealthAction implements SystemWsAction { return userSession.isSystemAdministrator(); } - private boolean isPassCodeAuthenticated(Request request) { - return systemPasscode.isConfigured() && systemPasscode.isValid(request); - } - } diff --git a/server/sonar-server/src/main/java/org/sonar/server/platform/ws/SafeModeHealthAction.java b/server/sonar-server/src/main/java/org/sonar/server/platform/ws/SafeModeHealthAction.java index 70c32111503..64b746a054e 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/platform/ws/SafeModeHealthAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/platform/ws/SafeModeHealthAction.java @@ -42,7 +42,7 @@ public class SafeModeHealthAction implements SystemWsAction { @Override public void handle(Request request, Response response) throws Exception { - if (!systemPasscode.isConfigured() || !systemPasscode.isValid(request)) { + if (!systemPasscode.isValid(request)) { throw new ForbiddenException("Insufficient privileges"); } diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscode.java b/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscode.java index e178c792fb6..1cf628d6120 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscode.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscode.java @@ -25,18 +25,15 @@ import org.sonar.api.server.ws.Request; * Passcode for accessing some web services, usually for connecting * monitoring tools without using the credentials * of a system administrator. + * + * Important - the web services accepting passcode must be listed in + * {@link org.sonar.server.authentication.UserSessionInitializer#URL_USING_PASSCODE}. */ public interface SystemPasscode { /** - * Whether the system passcode is configured in sonar.properties or not. - * By default passcode is not defined and {@code false} is returned. - */ - boolean isConfigured(); - - /** - * Whether the configured system passcode is provided by the HTTP request or not. - * Returns {@code false} if {@link #isConfigured()} is {@code false}. + * Whether the system passcode is provided by the HTTP request or not. + * Returns {@code false} if passcode is not configured. */ boolean isValid(Request request); diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscodeImpl.java b/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscodeImpl.java index 806f4bf0119..d29d898317c 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscodeImpl.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/SystemPasscodeImpl.java @@ -40,11 +40,6 @@ public class SystemPasscodeImpl implements SystemPasscode, Startable { this.configuration = configuration; } - @Override - public boolean isConfigured() { - return configuredPasscode != null; - } - @Override public boolean isValid(Request request) { if (configuredPasscode == null) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/platform/ws/HealthActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/platform/ws/HealthActionTest.java index 0915ae91d3e..8c9033dce82 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/platform/ws/HealthActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/platform/ws/HealthActionTest.java @@ -32,7 +32,6 @@ import org.apache.commons.lang.RandomStringUtils; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.WebService; import org.sonar.process.cluster.health.NodeDetails; import org.sonar.process.cluster.health.NodeHealth; @@ -100,7 +99,7 @@ public class HealthActionTest { @Test public void request_fails_with_SystemPasscode_enabled_and_anonymous() { - when(systemPasscode.isConfigured()).thenReturn(true); + when(systemPasscode.isValid(any())).thenReturn(false); TestRequest request = underTest.newRequest(); expectForbiddenException(); @@ -110,8 +109,7 @@ public class HealthActionTest { @Test public void request_fails_with_SystemPasscode_enabled_but_no_passcode_and_user_is_not_system_administrator() { - when(systemPasscode.isConfigured()).thenReturn(true); - when(systemPasscode.isValid(any(Request.class))).thenReturn(false); + when(systemPasscode.isValid(any())).thenReturn(false); userSessionRule.logIn(); when(healthChecker.checkCluster()).thenReturn(randomStatusMinimalClusterHealth()); TestRequest request = underTest.newRequest(); @@ -123,8 +121,7 @@ public class HealthActionTest { @Test public void request_succeeds_with_SystemPasscode_enabled_and_passcode() { - when(systemPasscode.isConfigured()).thenReturn(true); - when(systemPasscode.isValid(any(Request.class))).thenReturn(true); + when(systemPasscode.isValid(any())).thenReturn(true); when(healthChecker.checkCluster()).thenReturn(randomStatusMinimalClusterHealth()); TestRequest request = underTest.newRequest(); @@ -132,30 +129,8 @@ public class HealthActionTest { } @Test - public void request_succeeds_with_SystemPasscode_disabled_and_user_is_system_administrator() { - when(systemPasscode.isConfigured()).thenReturn(false); - userSessionRule.logIn().setSystemAdministrator(); - when(healthChecker.checkCluster()).thenReturn(randomStatusMinimalClusterHealth()); - TestRequest request = underTest.newRequest(); - - request.execute(); - } - - @Test - public void request_succeeds_with_SystemPasscode_enabled_but_no_passcode_and_user_is_system_administrator() { - when(systemPasscode.isConfigured()).thenReturn(true); - when(systemPasscode.isValid(any(Request.class))).thenReturn(false); - userSessionRule.logIn().setSystemAdministrator(); - when(healthChecker.checkCluster()).thenReturn(randomStatusMinimalClusterHealth()); - TestRequest request = underTest.newRequest(); - - request.execute(); - } - - @Test - public void request_succeeds_with_SystemPasscode_enabled_and_passcode_and_user_is_system_administrator() { - when(systemPasscode.isConfigured()).thenReturn(true); - when(systemPasscode.isValid(any(Request.class))).thenReturn(true); + public void request_succeeds_with_SystemPasscode_incorrect_and_user_is_system_administrator() { + when(systemPasscode.isValid(any())).thenReturn(false); userSessionRule.logIn().setSystemAdministrator(); when(healthChecker.checkCluster()).thenReturn(randomStatusMinimalClusterHealth()); TestRequest request = underTest.newRequest(); @@ -381,11 +356,10 @@ public class HealthActionTest { */ private void authenticateWithRandomMethod() { if (random.nextBoolean()) { - when(systemPasscode.isConfigured()).thenReturn(true); if (random.nextBoolean()) { - when(systemPasscode.isValid(any(Request.class))).thenReturn(true); + when(systemPasscode.isValid(any())).thenReturn(true); } else { - when(systemPasscode.isValid(any(Request.class))).thenReturn(false); + when(systemPasscode.isValid(any())).thenReturn(false); userSessionRule.logIn().setSystemAdministrator(); } } else { diff --git a/server/sonar-server/src/test/java/org/sonar/server/platform/ws/SafeModeHealthActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/platform/ws/SafeModeHealthActionTest.java index 5f24446edda..d94ddf489b2 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/platform/ws/SafeModeHealthActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/platform/ws/SafeModeHealthActionTest.java @@ -26,7 +26,6 @@ import org.apache.commons.lang.RandomStringUtils; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import org.sonar.api.server.ws.Request; import org.sonar.api.server.ws.WebService; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.health.Health; @@ -68,9 +67,8 @@ public class SafeModeHealthActionTest { } @Test - public void request_fails_with_ForbiddenException_when_PassCode_disabled() { - when(systemPasscode.isConfigured()).thenReturn(false); - when(systemPasscode.isValid(any(Request.class))).thenReturn(random.nextBoolean()); + public void request_fails_with_ForbiddenException_when_PassCode_disabled_or_incorrect() { + when(systemPasscode.isValid(any())).thenReturn(false); TestRequest request = underTest.newRequest(); expectForbiddenException(); @@ -79,18 +77,7 @@ public class SafeModeHealthActionTest { } @Test - public void request_fails_with_ForbiddenException_when_PassCode_enabled_but_no_passcode() { - when(systemPasscode.isConfigured()).thenReturn(true); - when(systemPasscode.isValid(any(Request.class))).thenReturn(false); - TestRequest request = underTest.newRequest(); - - expectForbiddenException(); - - request.execute(); - } - - @Test - public void request_succeeds_when_PassCode_enabled_and_valid_passcode() { + public void request_succeeds_when_valid_passcode() { authenticateWithPasscode(); when(healthChecker.checkNode()) .thenReturn(newHealthCheckBuilder() @@ -156,8 +143,7 @@ public class SafeModeHealthActionTest { } private void authenticateWithPasscode() { - when(systemPasscode.isConfigured()).thenReturn(true); - when(systemPasscode.isValid(any(Request.class))).thenReturn(true); + when(systemPasscode.isValid(any())).thenReturn(true); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/SystemPasscodeImplTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/SystemPasscodeImplTest.java index 528596392e4..954949ab786 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/SystemPasscodeImplTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/SystemPasscodeImplTest.java @@ -42,26 +42,6 @@ public class SystemPasscodeImplTest { underTest.stop(); } - @Test - public void isConfigured_is_true_if_property_is_not_blank() { - verifyIsConfigured("foo", true); - } - - @Test - public void isConfigured_is_false_if_property_value_is_blank() { - verifyIsConfigured(" ", false); - } - - @Test - public void isConfigured_is_false_if_property_value_is_empty() { - verifyIsConfigured("", false); - } - - @Test - public void isConfigured_is_false_if_property_is_not_defined() { - assertThat(underTest.isConfigured()).isFalse(); - } - @Test public void startup_logs_show_that_feature_is_enabled() { configurePasscode("foo"); @@ -77,6 +57,14 @@ public class SystemPasscodeImplTest { assertThat(logTester.logs(LoggerLevel.INFO)).contains("System authentication by passcode is disabled"); } + @Test + public void passcode_is_disabled_if_blank_configuration() { + configurePasscode(""); + underTest.start(); + + assertThat(logTester.logs(LoggerLevel.INFO)).contains("System authentication by passcode is disabled"); + } + @Test public void isValid_is_true_if_request_header_matches_configured_passcode() { verifyIsValid(true, "foo", "foo"); @@ -111,11 +99,6 @@ public class SystemPasscodeImplTest { assertThat(underTest.isValid(request)).isEqualTo(expectedResult); } - private void verifyIsConfigured(String propertyValue, boolean expectedResult) { - configurePasscode(propertyValue); - assertThat(underTest.isConfigured()).isEqualTo(expectedResult); - } - private void configurePasscode(String propertyValue) { settings.setProperty("sonar.web.systemPasscode", propertyValue); underTest.start();