Repository: gocd/gocd
Issue: HTTP 500 when using external authorization plugins and GoCD site urls are blank

##### Issue Type

- Bug Report

##### Summary

When using GoCD GitLab or GitHub authentication plugins, the server site URLs must be set properly in the server configuration, otherwise the GoCD will fail with HTTP 500.

##### Environment

Discovered on 21.2.0, Java 15.
I suppose it is not related to the GoCD or Java version.

###### Basic environment details

* Go Version: 21.2.0 
* JAVA Version: 15.0.2
* OS: Linux 4.19.0-17-amd64

##### Steps to Reproduce

1. Put a GitLab or GitHub plugin JAR into the appropriate folder and restart the server
2. Create a new authorization configuration with the desired plugin
3. Make sure that both server site urls are not set OR at least the second one (Secure site URL) is not set
4. Try to login via plugin

##### Expected Results

Should be redirected to the external IDP properly

##### Actual Results

GoCD throws an HTTP500

##### Possible Fix

Use at least the site url which is set or something default, like http://localhost:8153

##### Log snippets

In the go-server.log you can see following:

```
2022-01-03 10:25:48,470 WARN  [qtp679525351-29] HttpChannel:673 - handleException /go/plugin/cd.go.authorization.gitlab/login java.net.MalformedURLException: no protocol:
```



Changed files:
config/config-api/src/main/java/com/thoughtworks/go/domain/ServerSiteUrlConfig.java
config/config-api/src/main/java/com/thoughtworks/go/config/ServerConfig.java
server/src/main/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProvider.java
config/config-api/src/test/java/com/thoughtworks/go/config/ServerConfigTest.java
server/src/test-fast/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProviderTest.java
config/config-api/src/test/java/com/thoughtworks/go/domain/ServerSiteUrlConfigTest.java

diff --git a/config/config-api/src/main/java/com/thoughtworks/go/config/ServerConfig.java b/config/config-api/src/main/java/com/thoughtworks/go/config/ServerConfig.java
index 0fef04a87e..51f01b601b 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/config/ServerConfig.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/config/ServerConfig.java
@@ -294,15 +294,12 @@ public class ServerConfig implements Validatable {
 
 
     public ServerSiteUrlConfig getSiteUrlPreferablySecured() {
-        SiteUrl siteUrl = getSiteUrl();
-        SecureSiteUrl secureSiteUrlConfig = getSecureSiteUrl();
-        if (secureSiteUrlConfig.hasNonNullUrl()) {
-            return secureSiteUrlConfig;
-        }
-        if (!secureSiteUrlConfig.hasNonNullUrl()) {
-            return siteUrl;
+        SecureSiteUrl secureSiteUrl = getSecureSiteUrl();
+        if (!secureSiteUrl.isBlank()) {
+            return secureSiteUrl;
+        } else {
+            return getSiteUrl();
         }
-        return new SiteUrl();
     }
 
     public ServerSiteUrlConfig getHttpsUrl() {
@@ -311,7 +308,7 @@ public class ServerConfig implements Validatable {
     }
 
     public boolean hasAnyUrlConfigured() {
-        return getSiteUrl().hasNonNullUrl() || getSecureSiteUrl().hasNonNullUrl();
+        return !getSiteUrl().isBlank() || !getSecureSiteUrl().isBlank();
     }
 
     public Double getPurgeStart() {
diff --git a/config/config-api/src/main/java/com/thoughtworks/go/domain/ServerSiteUrlConfig.java b/config/config-api/src/main/java/com/thoughtworks/go/domain/ServerSiteUrlConfig.java
index b350abcc9b..c978d4c873 100644
--- a/config/config-api/src/main/java/com/thoughtworks/go/domain/ServerSiteUrlConfig.java
+++ b/config/config-api/src/main/java/com/thoughtworks/go/domain/ServerSiteUrlConfig.java
@@ -16,6 +16,7 @@
 package com.thoughtworks.go.domain;
 
 import com.thoughtworks.go.config.ConfigValue;
+import org.apache.commons.lang3.StringUtils;
 
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -32,8 +33,8 @@ public abstract class ServerSiteUrlConfig {
         this.url = url;
     }
 
-    public boolean hasNonNullUrl() {
-        return getUrl() != null;
+    public boolean isBlank() {
+        return StringUtils.isBlank(url);
     }
 
     public String getUrl() {
@@ -68,14 +69,22 @@ public abstract class ServerSiteUrlConfig {
     }
 
     public String siteUrlFor(String givenUrl, boolean honorGivenHostName) throws URISyntaxException {
-        if (url == null || isPath(givenUrl)) {
+        if (isBlank() || isPath(givenUrl)) {
             return givenUrl; //it is a path
         }
 
         URI baseUri = new URI(url);
         URI givenUri = new URI(givenUrl);
 
-        return new URI(baseUri.getScheme(), getOrDefault(givenUri, baseUri, URI::getUserInfo), honorGivenHostName ? givenUri.getHost() : baseUri.getHost(), baseUri.getPort(), getOrDefault(givenUri, baseUri, URI::getPath), getOrDefault(givenUri, baseUri, URI::getQuery), getOrDefault(givenUri, baseUri, URI::getFragment)).toString();
+        return new URI(
+            baseUri.getScheme(),
+            getOrDefault(givenUri, baseUri, URI::getUserInfo),
+            honorGivenHostName ? givenUri.getHost() : baseUri.getHost(),
+            baseUri.getPort(),
+            getOrDefault(givenUri, baseUri, URI::getPath),
+            getOrDefault(givenUri, baseUri, URI::getQuery),
+            getOrDefault(givenUri, baseUri, URI::getFragment)
+        ).toString();
     }
 
     private boolean isPath(String givenUrl) {
@@ -88,7 +97,7 @@ public abstract class ServerSiteUrlConfig {
     }
 
     public boolean isAHttpsUrl() {
-        return url != null && url.matches(HTTPS_URL_REGEX);
+        return !isBlank() && url.matches(HTTPS_URL_REGEX);
     }
 
     interface Getter {
@@ -97,7 +106,7 @@ public abstract class ServerSiteUrlConfig {
 
     @Override
     public String toString() {
-        return hasNonNullUrl() ? url : "";
+        return isBlank() ? "" : url;
     }
 }
 
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/config/ServerConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/config/ServerConfigTest.java
index ccc451b193..14968fde88 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/config/ServerConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/config/ServerConfigTest.java
@@ -61,11 +61,11 @@ public class ServerConfigTest {
     public void shouldReturnBlankUrlBothSiteUrlAndSecureSiteUrlIsNotDefined() {
         defaultServerConfig.setSiteUrl(null);
         defaultServerConfig.setSecureSiteUrl(null);
-        assertThat(defaultServerConfig.getSiteUrlPreferablySecured().hasNonNullUrl(), is(false));
+        assertThat(defaultServerConfig.getSiteUrlPreferablySecured().isBlank(), is(true));
     }
 
     @Test
-    public void shouldReturnAnEmptyForSecureSiteUrlIfOnlySiteUrlIsConfigured() throws Exception {
+    public void shouldReturnAnEmptyForSecureSiteUrlIfOnlySiteUrlIsConfigured() {
         ServerConfig serverConfig = new ServerConfig(null, null, new SiteUrl("http://foo.bar:813"), new SecureSiteUrl());
         assertThat(serverConfig.getHttpsUrl(), is(new SecureSiteUrl()));
     }
diff --git a/config/config-api/src/test/java/com/thoughtworks/go/domain/ServerSiteUrlConfigTest.java b/config/config-api/src/test/java/com/thoughtworks/go/domain/ServerSiteUrlConfigTest.java
index 46826f047d..dfb2286558 100644
--- a/config/config-api/src/test/java/com/thoughtworks/go/domain/ServerSiteUrlConfigTest.java
+++ b/config/config-api/src/test/java/com/thoughtworks/go/domain/ServerSiteUrlConfigTest.java
@@ -16,11 +16,14 @@
 package com.thoughtworks.go.domain;
 
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import java.net.URISyntaxException;
 
-import static org.hamcrest.Matchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
 
 public class ServerSiteUrlConfigTest {
     @Test
@@ -77,9 +80,14 @@ public class ServerSiteUrlConfigTest {
         assertThat(url.toString(), is("http://someurl.com"));
     }
 
-    @Test
-    public void shouldReturnEmptyStringForToStringWhenTheUrlIsNotSet() throws Exception {
-        ServerSiteUrlConfig url = new SiteUrl();
+    @ParameterizedTest
+    @NullSource
+    @ValueSource(strings = {"", " "})
+    public void shouldHandleBlankUrlsConsistently(String input) throws Exception {
+        ServerSiteUrlConfig url = new SiteUrl(input);
         assertThat(url.toString(), is(""));
+        assertThat(url.isBlank(), is(true));
+        assertThat(url.isAHttpsUrl(), is(false));
+        assertThat(url.siteUrlFor("http://test.host/foo/bar?foo=bar#quux"), is("http://test.host/foo/bar?foo=bar#quux"));
     }
 }
diff --git a/server/src/main/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProvider.java b/server/src/main/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProvider.java
index daf9d31e81..ef23185c10 100644
--- a/server/src/main/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProvider.java
+++ b/server/src/main/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProvider.java
@@ -88,12 +88,12 @@ public class WebBasedPluginAuthenticationProvider extends AbstractPluginAuthenti
         return new AuthenticationToken<>(userPrinciple, credentials, pluginId, clock.currentTimeMillis(), authConfigId);
     }
 
-    private String getRootUrl(String string) {
+    private String rootUrlFrom(String urlString) {
         try {
-            final URL url = new URL(string);
+            final URL url = new URL(urlString);
             return new URL(url.getProtocol(), url.getHost(), url.getPort(), "").toString();
         } catch (MalformedURLException e) {
-            throw new RuntimeException(e);
+            throw new RuntimeException(String.format("Configured siteUrl [%s] does not appear to be a valid URL", urlString), e);
         }
     }
 
@@ -107,11 +107,12 @@ public class WebBasedPluginAuthenticationProvider extends AbstractPluginAuthenti
         return goConfigService.security().securityAuthConfigs().findByPluginId(pluginId);
     }
 
-    public String getAuthorizationServerUrl(String pluginId, String rootURL) {
-        if (goConfigService.serverConfig().hasAnyUrlConfigured()) {
-            rootURL = getRootUrl(goConfigService.serverConfig().getSiteUrlPreferablySecured().getUrl());
-        }
-        return authorizationExtension.getAuthorizationServerUrl(pluginId, getAuthConfigs(pluginId), rootURL);
+    public String getAuthorizationServerUrl(String pluginId, String alternateRootUrl) {
+        String chosenRootUrl =
+            goConfigService.serverConfig().hasAnyUrlConfigured()
+                ? rootUrlFrom(goConfigService.serverConfig().getSiteUrlPreferablySecured().getUrl())
+                : alternateRootUrl;
+        return authorizationExtension.getAuthorizationServerUrl(pluginId, getAuthConfigs(pluginId), chosenRootUrl);
     }
 
 }
diff --git a/server/src/test-fast/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProviderTest.java b/server/src/test-fast/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProviderTest.java
index 03988934a9..3c63a1d1b5 100644
--- a/server/src/test-fast/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProviderTest.java
+++ b/server/src/test-fast/java/com/thoughtworks/go/server/newsecurity/providers/WebBasedPluginAuthenticationProviderTest.java
@@ -41,6 +41,8 @@ import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.ArgumentCaptor;
 import org.mockito.InOrder;
 
+import java.net.MalformedURLException;
+
 import static com.thoughtworks.go.server.security.GoAuthority.ROLE_USER;
 import static java.util.Arrays.asList;
 import static java.util.Collections.*;
@@ -322,6 +324,19 @@ class WebBasedPluginAuthenticationProviderTest {
         verify(authorizationExtension).getAuthorizationServerUrl(PLUGIN_ID, singletonList(githubSecurityAuthconfig), "https://example.com");
     }
 
+    @Test
+    void shouldThrowUsefulErrorIfAuthorizationServerUrlIsBad() {
+        final ServerConfig serverConfig = mock(ServerConfig.class);
+        when(goConfigService.serverConfig()).thenReturn(serverConfig);
+        when(serverConfig.hasAnyUrlConfigured()).thenReturn(true);
+        when(serverConfig.getSiteUrlPreferablySecured()).thenReturn(new SecureSiteUrl("https://badurl:3434:"));
+
+        assertThatThrownBy(() -> authenticationProvider.getAuthorizationServerUrl(PLUGIN_ID, "https://example.com"))
+            .isInstanceOf(RuntimeException.class)
+            .hasMessageContaining("does not appear to be a valid URL")
+            .hasCauseInstanceOf(MalformedURLException.class);
+    }
+
     @Test
     void shouldFetchAccessTokenFromPlugin() {
         when(authorizationExtension.fetchAccessToken(PLUGIN_ID, emptyMap(), singletonMap("code", "some-code"), singletonList(githubSecurityAuthconfig))).thenReturn(singletonMap("access_token", "some-access-token"));