GetPreferences: Don't return unsupported download scheme

In the preferences users can set their preferred download scheme. When
setting the download scheme it is validated that the download scheme is
supported. A download scheme is supported if there is a plugin that
provides a DownloadScheme implementation for it that is enabled.
However this check does not guarantee that the download scheme that a
user has set in the preferences is always supported. It can happen that
a user sets a supported download scheme that later becomes unsupported
because the plugin is uninstalled or the download scheme is set to
disabled. In this case we don't want to return the unspported download
scheme to callers of the GetPreferences REST endpoint. E.g. if the
caller tries to use the current values for a SetPreferences call this
would be rejected due to the unsupported download scheme.

Bug: Issue 9857
Change-Id: Ic8c0313efa9fa9c22f3a6f71c1b34e979b200875
Signed-off-by: Edwin Kempin <ekempin@google.com>

2 files changed, 102 insertions, 5 deletions

diff --git a/java/com/google/gerrit/server/restapi/account/GetPreferences.java b/java/com/google/gerrit/server/restapi/account/GetPreferences.java
index a185898486..3d2064232f 100644
--- a/java/com/google/gerrit/server/restapi/account/GetPreferences.java
+++ b/java/com/google/gerrit/server/restapi/account/GetPreferences.java
@@ -15,6 +15,9 @@
 package com.google.gerrit.server.restapi.account;
 
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo;
+import com.google.gerrit.extensions.config.DownloadScheme;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.registration.Extension;
 import com.google.gerrit.extensions.restapi.IdString;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.RestApiException;
@@ -36,13 +39,18 @@ public class GetPreferences implements RestReadView<AccountResource> {
   private final Provider<CurrentUser> self;
   private final PermissionBackend permissionBackend;
   private final AccountCache accountCache;
+  private final DynamicMap<DownloadScheme> downloadSchemes;
 
   @Inject
   GetPreferences(
-      Provider<CurrentUser> self, PermissionBackend permissionBackend, AccountCache accountCache) {
+      Provider<CurrentUser> self,
+      PermissionBackend permissionBackend,
+      AccountCache accountCache,
+      DynamicMap<DownloadScheme> downloadSchemes) {
     this.self = self;
     this.permissionBackend = permissionBackend;
     this.accountCache = accountCache;
+    this.downloadSchemes = downloadSchemes;
   }
 
   @Override
@@ -53,9 +61,28 @@ public class GetPreferences implements RestReadView<AccountResource> {
     }
 
     Account.Id id = rsrc.getUser().getAccountId();
-    return accountCache
-        .get(id)
-        .map(AccountState::getGeneralPreferences)
-        .orElseThrow(() -> new ResourceNotFoundException(IdString.fromDecoded(id.toString())));
+    GeneralPreferencesInfo preferencesInfo =
+        accountCache
+            .get(id)
+            .map(AccountState::getGeneralPreferences)
+            .orElseThrow(() -> new ResourceNotFoundException(IdString.fromDecoded(id.toString())));
+    return unsetDownloadSchemeIfUnsupported(preferencesInfo);
+  }
+
+  private GeneralPreferencesInfo unsetDownloadSchemeIfUnsupported(
+      GeneralPreferencesInfo preferencesInfo) {
+    if (preferencesInfo.downloadScheme == null) {
+      return preferencesInfo;
+    }
+
+    for (Extension<DownloadScheme> e : downloadSchemes) {
+      if (e.getExportName().equals(preferencesInfo.downloadScheme)
+          && e.getProvider().get().isEnabled()) {
+        return preferencesInfo;
+      }
+    }
+
+    preferencesInfo.downloadScheme = null;
+    return preferencesInfo;
   }
 }
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
index 946e15c4e0..24040a4136 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
@@ -30,7 +30,13 @@ import com.google.gerrit.extensions.client.GeneralPreferencesInfo.EmailStrategy;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo.ReviewCategoryStrategy;
 import com.google.gerrit.extensions.client.GeneralPreferencesInfo.TimeFormat;
 import com.google.gerrit.extensions.client.MenuItem;
+import com.google.gerrit.extensions.config.DownloadScheme;
+import com.google.gerrit.extensions.registration.DynamicMap;
+import com.google.gerrit.extensions.registration.PrivateInternals_DynamicMapImpl;
+import com.google.gerrit.extensions.registration.RegistrationHandle;
 import com.google.gerrit.extensions.restapi.BadRequestException;
+import com.google.inject.Inject;
+import com.google.inject.util.Providers;
 import java.util.ArrayList;
 import java.util.HashMap;
 import org.junit.Before;
@@ -38,6 +44,8 @@ import org.junit.Test;
 
 @NoHttpd
 public class GeneralPreferencesIT extends AbstractDaemonTest {
+  @Inject private DynamicMap<DownloadScheme> downloadSchemes;
+
   private TestAccount user42;
 
   @Before
@@ -177,4 +185,66 @@ public class GeneralPreferencesIT extends AbstractDaemonTest {
     GeneralPreferencesInfo o = gApi.accounts().id(user42.getId().toString()).setPreferences(i);
     assertThat(o.my).containsExactly(new MenuItem("name", "url", "_blank", "id"));
   }
+
+  @Test
+  public void rejectUnsupportedDownloadScheme() throws Exception {
+    GeneralPreferencesInfo i = GeneralPreferencesInfo.defaults();
+    i.downloadScheme = "foo";
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("Unsupported download scheme: " + i.downloadScheme);
+    gApi.accounts().id(user42.getId().toString()).setPreferences(i);
+  }
+
+  @Test
+  public void setDownloadScheme() throws Exception {
+    String schemeName = "foo";
+    RegistrationHandle registrationHandle =
+        ((PrivateInternals_DynamicMapImpl<DownloadScheme>) downloadSchemes)
+            .put("myPlugin", schemeName, Providers.of(new TestDownloadScheme()));
+    try {
+      GeneralPreferencesInfo i = GeneralPreferencesInfo.defaults();
+      i.downloadScheme = schemeName;
+
+      GeneralPreferencesInfo o = gApi.accounts().id(user42.getId().toString()).setPreferences(i);
+      assertThat(o.downloadScheme).isEqualTo(schemeName);
+
+      o = gApi.accounts().id(user42.getId().toString()).getPreferences();
+      assertThat(o.downloadScheme).isEqualTo(schemeName);
+    } finally {
+      registrationHandle.remove();
+    }
+  }
+
+  @Test
+  public void unsupportedDownloadSchemeIsNotReturned() throws Exception {
+    // Set a download scheme and unregister the plugin that provides this download scheme so that it
+    // becomes unsupported.
+    setDownloadScheme();
+
+    GeneralPreferencesInfo o = gApi.accounts().id(user42.getId().toString()).getPreferences();
+    assertThat(o.downloadScheme).isNull();
+  }
+
+  private static class TestDownloadScheme extends DownloadScheme {
+    @Override
+    public String getUrl(String project) {
+      return "http://foo/" + project;
+    }
+
+    @Override
+    public boolean isAuthRequired() {
+      return false;
+    }
+
+    @Override
+    public boolean isAuthSupported() {
+      return false;
+    }
+
+    @Override
+    public boolean isEnabled() {
+      return true;
+    }
+  }
 }