Fix updating replication credentials using te RepicationRemotesApi
The implementation of the ReplicationRemotesApi for updating a remote
introduced with I8003ec6c827 has wrongly saved the credentials (username)
in the repication.config rather than in the SecureStore, causing
the generation of non-functioning replication configs.
The credentials are read from the secure.config (or other SecureStore)
as documented in [1].
[1] https://gerrit.googlesource.com/plugins/replication/+/refs/heads/master/src/main/resources/Documentation/config.md#file-2
Change-Id: I644d73bc69589189a8762de98d1c58c263abcfd4
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiImpl.java b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiImpl.java
index e4ef318..cc6bcad 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiImpl.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiImpl.java
@@ -59,19 +59,19 @@
"configuration update must have at least one 'remote' section");
}
- SeparatedRemoteConfigs configs = onlyRemoteSectionsWithSeparatedPasswords(remoteConfig);
- persistRemotesPasswords(configs);
+ SeparatedRemoteConfigs configs = onlyRemoteSectionsWithSeparatedCredentials(remoteConfig);
+ persistRemotesCredentials(configs);
mergedConfigResource.update(configs.remotes);
}
- private SeparatedRemoteConfigs onlyRemoteSectionsWithSeparatedPasswords(Config configUpdates) {
+ private SeparatedRemoteConfigs onlyRemoteSectionsWithSeparatedCredentials(Config configUpdates) {
SeparatedRemoteConfigs configs = new SeparatedRemoteConfigs();
for (String subSection : configUpdates.getSubsections("remote")) {
for (String name : configUpdates.getNames("remote", subSection)) {
List<String> values = List.of(configUpdates.getStringList("remote", subSection, name));
- if ("password".equals(name)) {
- configs.passwords.setStringList("remote", subSection, "password", values);
+ if ("password".equals(name) || "username".equals(name)) {
+ configs.credentials.setStringList("remote", subSection, name, values);
} else {
configs.remotes.setStringList("remote", subSection, name, values);
}
@@ -81,16 +81,20 @@
return configs;
}
- private void persistRemotesPasswords(SeparatedRemoteConfigs configs) {
- for (String subSection : configs.passwords.getSubsections("remote")) {
- List<String> values =
- List.of(configs.passwords.getStringList("remote", subSection, "password"));
- secureStore.setList("remote", subSection, "password", values);
+ private void persistRemotesCredentials(SeparatedRemoteConfigs configs) {
+ for (String subSection : configs.credentials.getSubsections("remote")) {
+ copyRemoteStringList(configs.credentials, subSection, "username");
+ copyRemoteStringList(configs.credentials, subSection, "password");
}
}
+ private void copyRemoteStringList(Config config, String subSection, String key) {
+ secureStore.setList(
+ "remote", subSection, key, List.of(config.getStringList("remote", subSection, key)));
+ }
+
private static class SeparatedRemoteConfigs {
private final Config remotes = new Config();
- private final Config passwords = new Config();
+ private final Config credentials = new Config();
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiTest.java
index a806875..40d9846 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationRemotesApiTest.java
@@ -148,9 +148,9 @@
}
@Test
- public void shouldEncryptPasswordButNotStoreInConfig() throws Exception {
+ public void shouldSetPasswordViaSecureStoreButNotStoreInConfig() throws Exception {
Config update = new Config();
- String password = "my_secret_password";
+ String password = "encrypted-password";
String remoteName = "site";
setRemoteSite(update, remoteName, "password", password);
ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
@@ -163,6 +163,22 @@
assertRemoteSite(objectUnderTest.get(remoteName), remoteName, "password").isNull();
}
+ @Test
+ public void shouldSetUsernameViaSecureStoreButNotStoreInConfig() throws Exception {
+ Config update = new Config();
+ String username = "encrypted-username";
+ String remoteName = "site";
+ setRemoteSite(update, remoteName, "username", username);
+ ReplicationRemotesApi objectUnderTest = getReplicationRemotesApi();
+
+ objectUnderTest.update(update);
+
+ verify(secureStoreMock).setList("remote", remoteName, "username", List.of(username));
+ assertRemoteSite(baseConfig.getConfig(), remoteName, "username").isNull();
+ assertRemoteSite(testOverrides.get().getConfig(), remoteName, "username").isNull();
+ assertRemoteSite(objectUnderTest.get(remoteName), remoteName, "username").isNull();
+ }
+
private void setRemoteSite(Config config, String remoteName, String name, String value) {
config.setString("remote", remoteName, name, value);
}
