opendev/gerrit
Code Issues Proposed changes

GetPreferences: Don't return unsupported download scheme

Browse Source 
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>
This commit is contained in:
Edwin Kempin 2018-10-19 10:18:19 +02:00
parent adbef6be8e
commit 779680dda5
2 changed files with 102 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
java/com/google/gerrit/server/restapi/account/GetPreferences.java
View File

@ -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;
}
}

70
javatests/com/google/gerrit/acceptance/api/accounts/GeneralPreferencesIT.java
View File

@ -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;
}
}
}