Make AccountSshKey immutable

Change-Id: I787fe021ed6a0a481d74e8ef8a92f66f24ad6b10
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2018-02-06 12:53:37 +01:00
parent f4c2ddeffe
commit 932b9f91fc
17 changed files with 121 additions and 165 deletions

View File

@ -145,7 +145,7 @@ public class InitAdminUser implements InitStep {
if (sshKey != null) {
VersionedAuthorizedKeysOnInit authorizedKeys = authorizedKeysFactory.create(id).load();
authorizedKeys.addKey(sshKey.getSshPublicKey());
authorizedKeys.addKey(sshKey.sshPublicKey());
authorizedKeys.save("Add SSH key for initial admin user\n");
}
@ -165,8 +165,8 @@ public class InitAdminUser implements InitStep {
private String readEmail(AccountSshKey sshKey) {
String defaultEmail = "admin@example.com";
if (sshKey != null && sshKey.getComment() != null) {
String c = sshKey.getComment().trim();
if (sshKey != null && sshKey.comment() != null) {
String c = sshKey.comment().trim();
if (EmailValidator.getInstance().isValid(c)) {
defaultEmail = c;
}
@ -199,6 +199,6 @@ public class InitAdminUser implements InitStep {
throw new IOException(String.format("Cannot add public SSH key: %s is not a file", keyFile));
}
String content = new String(Files.readAllBytes(p), UTF_8);
return new AccountSshKey(new AccountSshKey.Id(id, 1), content);
return AccountSshKey.create(id, 1, content);
}
}

View File

@ -66,8 +66,8 @@ public class VersionedAuthorizedKeysOnInit extends VersionedMetaDataOnInit {
public AccountSshKey addKey(String pub) {
checkState(keys != null, "SSH keys not loaded yet");
int seq = keys.isEmpty() ? 1 : keys.size() + 1;
AccountSshKey.Id keyId = new AccountSshKey.Id(accountId, seq);
AccountSshKey key = new VersionedAuthorizedKeys.SimpleSshKeyCreator().create(keyId, pub);
AccountSshKey key =
new VersionedAuthorizedKeys.SimpleSshKeyCreator().create(accountId, seq, pub);
keys.add(Optional.of(key));
return key;
}

View File

@ -14,76 +14,50 @@
package com.google.gerrit.server.account;
import com.google.auto.value.AutoValue;
import com.google.common.base.Splitter;
import com.google.gerrit.reviewdb.client.Account;
import java.io.Serializable;
import java.util.List;
import java.util.Objects;
/** An SSH key approved for use by an {@link Account}. */
public final class AccountSshKey {
public static class Id implements Serializable {
private static final long serialVersionUID = 2L;
private Account.Id accountId;
private int seq;
public Id(Account.Id a, int s) {
accountId = a;
seq = s;
}
public Account.Id getParentKey() {
return accountId;
}
public int get() {
return seq;
}
public boolean isValid() {
return seq > 0;
}
@Override
public int hashCode() {
return Objects.hash(accountId, seq);
}
@Override
public boolean equals(Object obj) {
if (!(obj instanceof Id)) {
return false;
}
Id otherId = (Id) obj;
return Objects.equals(accountId, otherId.accountId) && Objects.equals(seq, otherId.seq);
}
@AutoValue
public abstract class AccountSshKey {
public static AccountSshKey create(Account.Id accountId, int seq, String sshPublicKey) {
return create(accountId, seq, sshPublicKey, true);
}
private AccountSshKey.Id id;
private String sshPublicKey;
private boolean valid;
public AccountSshKey(AccountSshKey.Id i, String pub) {
id = i;
sshPublicKey = pub.replace("\n", "").replace("\r", "");
valid = id.isValid();
public static AccountSshKey createInvalid(Account.Id accountId, int seq, String sshPublicKey) {
return create(accountId, seq, sshPublicKey, false);
}
public Account.Id getAccount() {
return id.accountId;
public static AccountSshKey createInvalid(AccountSshKey key) {
return create(key.accountId(), key.seq(), key.sshPublicKey(), false);
}
public AccountSshKey.Id getKey() {
return id;
public static AccountSshKey create(
Account.Id accountId, int seq, String sshPublicKey, boolean valid) {
return new AutoValue_AccountSshKey.Builder()
.setAccountId(accountId)
.setSeq(seq)
.setSshPublicKey(stripOffNewLines(sshPublicKey))
.setValid(valid && seq > 0)
.build();
}
public String getSshPublicKey() {
return sshPublicKey;
private static String stripOffNewLines(String s) {
return s.replace("\n", "").replace("\r", "");
}
private String getPublicKeyPart(int index, String defaultValue) {
String s = getSshPublicKey();
public abstract Account.Id accountId();
public abstract int seq();
public abstract String sshPublicKey();
public abstract boolean valid();
private String publicKeyPart(int index, String defaultValue) {
String s = sshPublicKey();
if (s != null && s.length() > 0) {
List<String> parts = Splitter.on(' ').splitToList(s);
if (parts.size() > index) {
@ -93,39 +67,28 @@ public final class AccountSshKey {
return defaultValue;
}
public String getAlgorithm() {
return getPublicKeyPart(0, "none");
public String algorithm() {
return publicKeyPart(0, "none");
}
public String getEncodedKey() {
return getPublicKeyPart(1, null);
public String encodedKey() {
return publicKeyPart(1, null);
}
public String getComment() {
return getPublicKeyPart(2, "");
public String comment() {
return publicKeyPart(2, "");
}
public boolean isValid() {
return valid && id.isValid();
}
@AutoValue.Builder
abstract static class Builder {
public abstract Builder setAccountId(Account.Id accountId);
public void setInvalid() {
valid = false;
}
public abstract Builder setSeq(int seq);
@Override
public boolean equals(Object o) {
if (o instanceof AccountSshKey) {
AccountSshKey other = (AccountSshKey) o;
return Objects.equals(id, other.id)
&& Objects.equals(sshPublicKey, other.sshPublicKey)
&& Objects.equals(valid, other.valid);
}
return false;
}
public abstract Builder setSshPublicKey(String sshPublicKey);
@Override
public int hashCode() {
return Objects.hash(id, sshPublicKey, valid);
public abstract Builder setValid(boolean valid);
public abstract AccountSshKey build();
}
}

View File

@ -41,8 +41,7 @@ public class AuthorizedKeys {
continue;
} else if (line.startsWith(INVALID_KEY_COMMENT_PREFIX)) {
String pub = line.substring(INVALID_KEY_COMMENT_PREFIX.length());
AccountSshKey key = new AccountSshKey(new AccountSshKey.Id(accountId, seq++), pub);
key.setInvalid();
AccountSshKey key = AccountSshKey.createInvalid(accountId, seq++, pub);
keys.add(Optional.of(key));
} else if (line.startsWith(DELETED_KEY_COMMENT)) {
keys.add(Optional.empty());
@ -50,7 +49,7 @@ public class AuthorizedKeys {
} else if (line.startsWith("#")) {
continue;
} else {
AccountSshKey key = new AccountSshKey(new AccountSshKey.Id(accountId, seq++), line);
AccountSshKey key = AccountSshKey.create(accountId, seq++, line);
keys.add(Optional.of(key));
}
}
@ -61,10 +60,10 @@ public class AuthorizedKeys {
StringBuilder b = new StringBuilder();
for (Optional<AccountSshKey> key : keys) {
if (key.isPresent()) {
if (!key.get().isValid()) {
if (!key.get().valid()) {
b.append(INVALID_KEY_COMMENT_PREFIX);
}
b.append(key.get().getSshPublicKey().trim());
b.append(key.get().sshPublicKey().trim());
} else {
b.append(DELETED_KEY_COMMENT);
}

View File

@ -24,7 +24,6 @@ import com.google.gerrit.common.errors.InvalidSshKeyException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountSshKey.Id;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
@ -139,8 +138,8 @@ public class VersionedAuthorizedKeys extends VersionedMetaData {
public static class SimpleSshKeyCreator implements SshKeyCreator {
@Override
public AccountSshKey create(Id id, String encoded) {
return new AccountSshKey(id, encoded);
public AccountSshKey create(Account.Id accountId, int seq, String encoded) {
return AccountSshKey.create(accountId, seq, encoded);
}
}
@ -211,14 +210,13 @@ public class VersionedAuthorizedKeys extends VersionedMetaData {
checkLoaded();
for (Optional<AccountSshKey> key : keys) {
if (key.isPresent() && key.get().getSshPublicKey().trim().equals(pub.trim())) {
if (key.isPresent() && key.get().sshPublicKey().trim().equals(pub.trim())) {
return key.get();
}
}
int seq = keys.size() + 1;
AccountSshKey.Id keyId = new AccountSshKey.Id(accountId, seq);
AccountSshKey key = sshKeyCreator.create(keyId, pub);
AccountSshKey key = sshKeyCreator.create(accountId, seq, pub);
keys.add(Optional.of(key));
return key;
}
@ -249,9 +247,10 @@ public class VersionedAuthorizedKeys extends VersionedMetaData {
*/
private boolean markKeyInvalid(int seq) {
checkLoaded();
AccountSshKey key = getKey(seq);
if (key != null && key.isValid()) {
key.setInvalid();
Optional<AccountSshKey> key = keys.get(seq - 1);
if (key.isPresent() && key.get().valid()) {
keys.set(seq - 1, Optional.of(AccountSshKey.createInvalid(key.get())));
return true;
}
return false;
@ -265,10 +264,10 @@ public class VersionedAuthorizedKeys extends VersionedMetaData {
* @param newKeys the new public SSH keys
*/
public void setKeys(Collection<AccountSshKey> newKeys) {
Ordering<AccountSshKey> o = Ordering.from(comparing(k -> k.getKey().get()));
keys = new ArrayList<>(Collections.nCopies(o.max(newKeys).getKey().get(), Optional.empty()));
Ordering<AccountSshKey> o = Ordering.from(comparing(k -> k.seq()));
keys = new ArrayList<>(Collections.nCopies(o.max(newKeys).seq(), Optional.empty()));
for (AccountSshKey key : newKeys) {
keys.set(key.getKey().get() - 1, Optional.of(key));
keys.set(key.seq() - 1, Optional.of(key));
}
}

View File

@ -127,7 +127,7 @@ public class AddKeySender extends OutgoingEmail {
}
public String getSshKey() {
return (sshKey != null) ? sshKey.getSshPublicKey() + "\n" : null;
return (sshKey != null) ? sshKey.sshPublicKey() + "\n" : null;
}
public String getGpgKeys() {

View File

@ -61,7 +61,7 @@ public class DeleteSshKey implements RestModifyView<AccountResource.SshKey, Inpu
permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
}
authorizedKeys.deleteKey(rsrc.getUser().getAccountId(), rsrc.getSshKey().getKey().get());
authorizedKeys.deleteKey(rsrc.getUser().getAccountId(), rsrc.getSshKey().seq());
rsrc.getUser().getUserName().ifPresent(sshKeyCache::evict);
return Response.none();

View File

@ -70,12 +70,12 @@ public class GetSshKeys implements RestReadView<AccountResource> {
public static SshKeyInfo newSshKeyInfo(AccountSshKey sshKey) {
SshKeyInfo info = new SshKeyInfo();
info.seq = sshKey.getKey().get();
info.sshPublicKey = sshKey.getSshPublicKey();
info.encodedKey = sshKey.getEncodedKey();
info.algorithm = sshKey.getAlgorithm();
info.comment = Strings.emptyToNull(sshKey.getComment());
info.valid = sshKey.isValid();
info.seq = sshKey.seq();
info.sshPublicKey = sshKey.sshPublicKey();
info.encodedKey = sshKey.encodedKey();
info.algorithm = sshKey.algorithm();
info.comment = Strings.emptyToNull(sshKey.comment());
info.valid = sshKey.valid();
return info;
}
}

View File

@ -84,11 +84,8 @@ public class Schema_124 extends SchemaVersion {
Account.Id accountId = new Account.Id(rs.getInt(1));
int seq = rs.getInt(2);
String sshPublicKey = rs.getString(3);
AccountSshKey key = new AccountSshKey(new AccountSshKey.Id(accountId, seq), sshPublicKey);
boolean valid = toBoolean(rs.getString(4));
if (!valid) {
key.setInvalid();
}
AccountSshKey key = AccountSshKey.create(accountId, seq, sshPublicKey, valid);
imports.put(accountId, key);
}
}
@ -122,15 +119,13 @@ public class Schema_124 extends SchemaVersion {
}
private Collection<AccountSshKey> fixInvalidSequenceNumbers(Collection<AccountSshKey> keys) {
Ordering<AccountSshKey> o = Ordering.from(comparing(k -> k.getKey().get()));
Ordering<AccountSshKey> o = Ordering.from(comparing(k -> k.seq()));
List<AccountSshKey> fixedKeys = new ArrayList<>(keys);
AccountSshKey minKey = o.min(keys);
while (minKey.getKey().get() <= 0) {
while (minKey.seq() <= 0) {
AccountSshKey fixedKey =
new AccountSshKey(
new AccountSshKey.Id(
minKey.getKey().getParentKey(), Math.max(o.max(keys).getKey().get() + 1, 1)),
minKey.getSshPublicKey());
AccountSshKey.create(
minKey.accountId(), Math.max(o.max(keys).seq() + 1, 1), minKey.sshPublicKey());
Collections.replaceAll(fixedKeys, minKey, fixedKey);
minKey = o.min(fixedKeys);
}

View File

@ -15,6 +15,7 @@
package com.google.gerrit.server.ssh;
import com.google.gerrit.common.errors.InvalidSshKeyException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountSshKey;
import com.google.inject.AbstractModule;
import com.google.inject.Module;
@ -37,7 +38,8 @@ public class NoSshKeyCache implements SshKeyCache, SshKeyCreator {
public void evict(String username) {}
@Override
public AccountSshKey create(AccountSshKey.Id id, String encoded) throws InvalidSshKeyException {
public AccountSshKey create(Account.Id accountId, int seq, String encoded)
throws InvalidSshKeyException {
throw new InvalidSshKeyException();
}
}

View File

@ -15,8 +15,9 @@
package com.google.gerrit.server.ssh;
import com.google.gerrit.common.errors.InvalidSshKeyException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountSshKey;
public interface SshKeyCreator {
AccountSshKey create(AccountSshKey.Id id, String encoded) throws InvalidSshKeyException;
AccountSshKey create(Account.Id accountId, int seq, String encoded) throws InvalidSshKeyException;
}

View File

@ -15,20 +15,19 @@
package com.google.gerrit.sshd;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountSshKey;
import java.security.PublicKey;
class SshKeyCacheEntry {
private final AccountSshKey.Id id;
private final Account.Id accountId;
private final PublicKey publicKey;
SshKeyCacheEntry(AccountSshKey.Id i, PublicKey k) {
id = i;
publicKey = k;
SshKeyCacheEntry(Account.Id accountId, PublicKey publicKey) {
this.accountId = accountId;
this.publicKey = publicKey;
}
Account.Id getAccount() {
return id.getParentKey();
return accountId;
}
boolean match(PublicKey inkey) {

View File

@ -109,7 +109,7 @@ public class SshKeyCacheImpl implements SshKeyCache {
List<SshKeyCacheEntry> kl = new ArrayList<>(4);
for (AccountSshKey k : authorizedKeys.getKeys(user.get().accountId())) {
if (k.isValid()) {
if (k.valid()) {
add(kl, k);
}
}
@ -122,7 +122,7 @@ public class SshKeyCacheImpl implements SshKeyCache {
private void add(List<SshKeyCacheEntry> kl, AccountSshKey k) {
try {
kl.add(new SshKeyCacheEntry(k.getKey(), SshUtil.parse(k)));
kl.add(new SshKeyCacheEntry(k.accountId(), SshUtil.parse(k)));
} catch (OutOfMemoryError e) {
// This is the only case where we assume the problem has nothing
// to do with the key object, and instead we must abort this load.
@ -135,11 +135,11 @@ public class SshKeyCacheImpl implements SshKeyCache {
private void markInvalid(AccountSshKey k) {
try {
log.info("Flagging SSH key " + k.getKey() + " invalid");
authorizedKeys.markKeyInvalid(k.getAccount(), k.getKey().get());
k.setInvalid();
log.info("Flagging SSH key " + k.seq() + " of account " + k.accountId() + " invalid");
authorizedKeys.markKeyInvalid(k.accountId(), k.seq());
} catch (IOException | ConfigInvalidException e) {
log.error("Failed to mark SSH key" + k.getKey() + " invalid", e);
log.error(
"Failed to mark SSH key " + k.seq() + " of account " + k.accountId() + " invalid", e);
}
}
}

View File

@ -15,6 +15,7 @@
package com.google.gerrit.sshd;
import com.google.gerrit.common.errors.InvalidSshKeyException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.AccountSshKey;
import com.google.gerrit.server.ssh.SshKeyCreator;
import java.security.NoSuchAlgorithmException;
@ -27,9 +28,10 @@ public class SshKeyCreatorImpl implements SshKeyCreator {
private static final Logger log = LoggerFactory.getLogger(SshKeyCreatorImpl.class);
@Override
public AccountSshKey create(AccountSshKey.Id id, String encoded) throws InvalidSshKeyException {
public AccountSshKey create(Account.Id accountId, int seq, String encoded)
throws InvalidSshKeyException {
try {
AccountSshKey key = new AccountSshKey(id, SshUtil.toOpenSshPublicKey(encoded));
AccountSshKey key = AccountSshKey.create(accountId, seq, SshUtil.toOpenSshPublicKey(encoded));
SshUtil.parse(key);
return key;
} catch (NoSuchAlgorithmException | InvalidKeySpecException e) {

View File

@ -51,7 +51,7 @@ public class SshUtil {
public static PublicKey parse(AccountSshKey key)
throws NoSuchAlgorithmException, InvalidKeySpecException, NoSuchProviderException {
try {
final String s = key.getEncodedKey();
final String s = key.encodedKey();
if (s == null) {
throw new InvalidKeySpecException("No key string");
}

View File

@ -263,8 +263,7 @@ final class SetAccountCommand extends SshCommand {
private void deleteSshKey(SshKeyInfo i)
throws AuthException, OrmException, RepositoryNotFoundException, IOException,
ConfigInvalidException, PermissionBackendException {
AccountSshKey sshKey =
new AccountSshKey(new AccountSshKey.Id(user.getAccountId(), i.seq), i.sshPublicKey);
AccountSshKey sshKey = AccountSshKey.create(user.getAccountId(), i.seq, i.sshPublicKey);
deleteSshKey.apply(new AccountResource.SshKey(user.asIdentifiedUser(), sshKey), null);
}

View File

@ -113,30 +113,30 @@ public class AuthorizedKeysTest {
@Test
public void validity() throws Exception {
AccountSshKey key = new AccountSshKey(new AccountSshKey.Id(accountId, -1), KEY1);
assertThat(key.isValid()).isFalse();
key = new AccountSshKey(new AccountSshKey.Id(accountId, 0), KEY1);
assertThat(key.isValid()).isFalse();
key = new AccountSshKey(new AccountSshKey.Id(accountId, 1), KEY1);
assertThat(key.isValid()).isTrue();
AccountSshKey key = AccountSshKey.create(accountId, -1, KEY1);
assertThat(key.valid()).isFalse();
key = AccountSshKey.create(accountId, 0, KEY1);
assertThat(key.valid()).isFalse();
key = AccountSshKey.create(accountId, 1, KEY1);
assertThat(key.valid()).isTrue();
}
@Test
public void getters() throws Exception {
AccountSshKey key = new AccountSshKey(new AccountSshKey.Id(accountId, 1), KEY1);
assertThat(key.getSshPublicKey()).isEqualTo(KEY1);
assertThat(key.getAlgorithm()).isEqualTo(KEY1.split(" ")[0]);
assertThat(key.getEncodedKey()).isEqualTo(KEY1.split(" ")[1]);
assertThat(key.getComment()).isEqualTo(KEY1.split(" ")[2]);
AccountSshKey key = AccountSshKey.create(accountId, 1, KEY1);
assertThat(key.sshPublicKey()).isEqualTo(KEY1);
assertThat(key.algorithm()).isEqualTo(KEY1.split(" ")[0]);
assertThat(key.encodedKey()).isEqualTo(KEY1.split(" ")[1]);
assertThat(key.comment()).isEqualTo(KEY1.split(" ")[2]);
}
@Test
public void keyWithNewLines() throws Exception {
AccountSshKey key = new AccountSshKey(new AccountSshKey.Id(accountId, 1), KEY1_WITH_NEWLINES);
assertThat(key.getSshPublicKey()).isEqualTo(KEY1);
assertThat(key.getAlgorithm()).isEqualTo(KEY1.split(" ")[0]);
assertThat(key.getEncodedKey()).isEqualTo(KEY1.split(" ")[1]);
assertThat(key.getComment()).isEqualTo(KEY1.split(" ")[2]);
AccountSshKey key = AccountSshKey.create(accountId, 1, KEY1_WITH_NEWLINES);
assertThat(key.sshPublicKey()).isEqualTo(KEY1);
assertThat(key.algorithm()).isEqualTo(KEY1.split(" ")[0]);
assertThat(key.encodedKey()).isEqualTo(KEY1.split(" ")[1]);
assertThat(key.comment()).isEqualTo(KEY1.split(" ")[2]);
}
private static String toWindowsLineEndings(String s) {
@ -157,8 +157,8 @@ public class AuthorizedKeysTest {
int seq = 1;
for (Optional<AccountSshKey> sshKey : parsedKeys) {
if (sshKey.isPresent()) {
assertThat(sshKey.get().getAccount()).isEqualTo(accountId);
assertThat(sshKey.get().getKey().get()).isEqualTo(seq);
assertThat(sshKey.get().accountId()).isEqualTo(accountId);
assertThat(sshKey.get().seq()).isEqualTo(seq);
}
seq++;
}
@ -170,10 +170,9 @@ public class AuthorizedKeysTest {
* @return the expected line for this key in the authorized_keys file
*/
private static String addKey(List<Optional<AccountSshKey>> keys, String pub) {
AccountSshKey.Id keyId = new AccountSshKey.Id(new Account.Id(1), keys.size() + 1);
AccountSshKey key = new AccountSshKey(keyId, pub);
AccountSshKey key = AccountSshKey.create(new Account.Id(1), keys.size() + 1, pub);
keys.add(Optional.of(key));
return key.getSshPublicKey() + "\n";
return key.sshPublicKey() + "\n";
}
/**
@ -182,11 +181,9 @@ public class AuthorizedKeysTest {
* @return the expected line for this key in the authorized_keys file
*/
private static String addInvalidKey(List<Optional<AccountSshKey>> keys, String pub) {
AccountSshKey.Id keyId = new AccountSshKey.Id(new Account.Id(1), keys.size() + 1);
AccountSshKey key = new AccountSshKey(keyId, pub);
key.setInvalid();
AccountSshKey key = AccountSshKey.createInvalid(new Account.Id(1), keys.size() + 1, pub);
keys.add(Optional.of(key));
return AuthorizedKeys.INVALID_KEY_COMMENT_PREFIX + key.getSshPublicKey() + "\n";
return AuthorizedKeys.INVALID_KEY_COMMENT_PREFIX + key.sshPublicKey() + "\n";
}
/**