ChangeNotesParser: Hoist server id check to ChangeNotes
In some contextes (e.g. analytics plugin) allow ChangeNotesParser to
return ChangeNotesStates objects containing the account instances from
foreign servers.
It would be bad if these objects escaped the analytics plugin and got
used elsewhere in other Gerrit APIs. Any solution that allows to parse
arbitrary serverIds from analytics plugin should also include some
safety provisions so it doesn't cause unintended consequences
elsewhere in Gerrit core/Gerrit plugin API.
Here is the plan:
* Add a serverId field to ChangeNotesState
* Modify ChangeNotesParser/NoteDbUtil to allow any serverId during the
parsing phase
* In ChangeNotes, reject any ChangeNotesState that has a serverId not
matching the serverId of the running server
* If serverId is not present, the cached entry was populated with an
earlier version and thus serverId has been already checked
* Since analytics plugin won't be using ChangeNotes, it doesn't need to
run the check that the serverId in the ChangeNotesState matches the
current server
* Outside of the NoteDb code, all or almost all Gerrit APIs use
ChangeNotes, not ChangeNotesState
* So with the above approach, it should be mostly impossible to ever
see notes in non-analytics contextes with a mismatched serverId.
Inspired-By: Dave Borowitz <dborowitz@google.com>
Feature: Issue 10174
Change-Id: I9b43f8479206b6373edad857251cecdfde917269
diff --git a/java/com/google/gerrit/server/group/db/AuditLogReader.java b/java/com/google/gerrit/server/group/db/AuditLogReader.java
index fb58577..77e0e0c 100644
--- a/java/com/google/gerrit/server/group/db/AuditLogReader.java
+++ b/java/com/google/gerrit/server/group/db/AuditLogReader.java
@@ -27,7 +27,6 @@
import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.notedb.NoteDbUtil;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -51,12 +50,10 @@
public class AuditLogReader {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- private final String serverId;
private final AllUsersName allUsersName;
@Inject
- public AuditLogReader(@GerritServerId String serverId, AllUsersName allUsersName) {
- this.serverId = serverId;
+ public AuditLogReader(AllUsersName allUsersName) {
this.allUsersName = allUsersName;
}
@@ -143,7 +140,7 @@
}
private Optional<ParsedCommit> parse(AccountGroup.UUID uuid, RevCommit c) {
- Optional<Account.Id> authorId = NoteDbUtil.parseIdent(c.getAuthorIdent(), serverId);
+ Optional<Account.Id> authorId = NoteDbUtil.parseIdent(c.getAuthorIdent());
if (!authorId.isPresent()) {
// Only report audit events from identified users, since this was a non-nullable field in
// ReviewDb. May be revisited.
@@ -179,7 +176,7 @@
private Optional<Account.Id> parseAccount(AccountGroup.UUID uuid, RevCommit c, FooterLine line) {
Optional<Account.Id> result =
Optional.ofNullable(RawParseUtils.parsePersonIdent(line.getValue()))
- .flatMap(ident -> NoteDbUtil.parseIdent(ident, serverId));
+ .flatMap(ident -> NoteDbUtil.parseIdent(ident));
if (!result.isPresent()) {
logInvalid(uuid, c, line);
}
diff --git a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
index 9cc1c84..10bae48 100644
--- a/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
@@ -25,6 +25,7 @@
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.project.NoSuchChangeException;
@@ -51,6 +52,7 @@
public final AllUsersName allUsers;
public final LegacyChangeNoteRead legacyChangeNoteRead;
public final NoteDbMetrics metrics;
+ public final String serverId;
// Providers required to avoid dependency cycles.
@@ -64,7 +66,8 @@
ChangeNoteJson changeNoteJson,
LegacyChangeNoteRead legacyChangeNoteRead,
NoteDbMetrics metrics,
- Provider<ChangeNotesCache> cache) {
+ Provider<ChangeNotesCache> cache,
+ @GerritServerId String serverId) {
this.failOnLoadForTest = new AtomicBoolean();
this.repoManager = repoManager;
this.allUsers = allUsers;
@@ -72,6 +75,7 @@
this.changeNoteJson = changeNoteJson;
this.metrics = metrics;
this.cache = cache;
+ this.serverId = serverId;
}
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index e1217c2..929974d 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -22,6 +22,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
@@ -503,6 +504,19 @@
ChangeNotesCache.Value v =
args.cache.get().get(getProjectName(), getChangeId(), rev, handle::walk);
state = v.state();
+
+ String stateServerId = state.serverId();
+ /**
+ * In earlier Gerrit versions serverId wasn't part of the change notes cache. That's why the
+ * earlier cached entries don't have the serverId attribute. That's fine because in earlier
+ * gerrit version serverId was already validated. Another approach to simplify the check would
+ * be to bump the cache version, but that would invalidate all persistent cache entries, what we
+ * rather try to avoid.
+ */
+ checkState(
+ Strings.isNullOrEmpty(stateServerId) || args.serverId.equals(stateServerId),
+ String.format("invalid server id, expected %s: actual: %s", args.serverId, stateServerId));
+
state.copyColumnsTo(change);
revisionNoteMap = v.revisionNoteMap();
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 27c2aa6..b5087ff 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -135,6 +135,7 @@
private Timestamp createdOn;
private Timestamp lastUpdatedOn;
private Account.Id ownerId;
+ private String serverId;
private String changeId;
private String subject;
private String originalSubject;
@@ -223,6 +224,7 @@
createdOn,
lastUpdatedOn,
ownerId,
+ serverId,
branch,
buildCurrentPatchSetId(),
subject,
@@ -334,6 +336,10 @@
Account.Id accountId = parseIdent(commit);
if (accountId != null) {
ownerId = accountId;
+ PersonIdent personIdent = commit.getAuthorIdent();
+ serverId = NoteDbUtil.extractHostPartFromPersonIdent(personIdent);
+ } else {
+ serverId = "UNKNOWN_SERVER_ID";
}
Account.Id realAccountId = parseRealAccountId(commit, accountId);
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index 2728516..5adc196 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -96,6 +96,7 @@
Timestamp createdOn,
Timestamp lastUpdatedOn,
Account.Id owner,
+ String serverId,
String branch,
@Nullable PatchSet.Id currentPatchSetId,
String subject,
@@ -154,6 +155,7 @@
.build())
.pastAssignees(pastAssignees)
.hashtags(hashtags)
+ .serverId(serverId)
.patchSets(patchSets.entrySet())
.approvals(approvals.entries())
.reviewers(reviewers)
@@ -276,6 +278,9 @@
abstract ImmutableSet<String> hashtags();
+ @Nullable
+ abstract String serverId();
+
abstract ImmutableList<Map.Entry<PatchSet.Id, PatchSet>> patchSets();
abstract ImmutableList<Map.Entry<PatchSet.Id, PatchSetApproval>> approvals();
@@ -376,6 +381,8 @@
abstract Builder columns(ChangeColumns columns);
+ abstract Builder serverId(String serverId);
+
abstract Builder pastAssignees(Set<Account.Id> pastAssignees);
abstract Builder hashtags(Iterable<String> hashtags);
@@ -427,6 +434,10 @@
.setChangeId(object.changeId().get())
.setColumns(toChangeColumnsProto(object.columns()));
+ if (object.serverId() != null) {
+ b.setServerId(object.serverId());
+ b.setHasServerId(true);
+ }
object.pastAssignees().forEach(a -> b.addPastAssignee(a.get()));
object.hashtags().forEach(b::addHashtag);
object
@@ -549,6 +560,7 @@
.metaId(ObjectIdConverter.create().fromByteString(proto.getMetaId()))
.changeId(changeId)
.columns(toChangeColumns(changeId, proto.getColumns()))
+ .serverId(proto.getHasServerId() ? proto.getServerId() : null)
.pastAssignees(
proto.getPastAssigneeList().stream().map(Account::id).collect(toImmutableSet()))
.hashtags(proto.getHashtagList())
diff --git a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java
index 36bfe47..7e301f7 100644
--- a/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java
+++ b/java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java
@@ -50,14 +50,11 @@
public Account.Id parseIdent(PersonIdent ident, Change.Id changeId)
throws ConfigInvalidException {
- return NoteDbUtil.parseIdent(ident, serverId)
+ return NoteDbUtil.parseIdent(ident)
.orElseThrow(
() ->
parseException(
- changeId,
- "invalid identity, expected <id>@%s: %s",
- serverId,
- ident.getEmailAddress()));
+ changeId, "cannot retrieve account id: %s", ident.getEmailAddress()));
}
private static boolean match(byte[] note, MutableInteger p, byte[] expected) {
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUtil.java b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
index c53f4b9..bfbdd18 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUtil.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
@@ -37,25 +37,28 @@
ImmutableSet.of(
"com.google.gerrit.httpd.restapi.RestApiServlet", RetryingRestModifyView.class.getName());
- /**
- * Returns an AccountId for the given email address. Returns empty if the address isn't on this
- * server.
- */
- public static Optional<Account.Id> parseIdent(PersonIdent ident, String serverId) {
+ /** Returns an AccountId for the given email address. */
+ public static Optional<Account.Id> parseIdent(PersonIdent ident) {
String email = ident.getEmailAddress();
int at = email.indexOf('@');
if (at >= 0) {
- String host = email.substring(at + 1);
- if (host.equals(serverId)) {
- Integer id = Ints.tryParse(email.substring(0, at));
- if (id != null) {
- return Optional.of(Account.id(id));
- }
+ Integer id = Ints.tryParse(email.substring(0, at));
+ if (id != null) {
+ return Optional.of(Account.id(id));
}
}
return Optional.empty();
}
+ public static String extractHostPartFromPersonIdent(PersonIdent ident) {
+ String email = ident.getEmailAddress();
+ int at = email.indexOf('@');
+ if (at >= 0) {
+ return email.substring(at + 1);
+ }
+ throw new IllegalArgumentException("No host part found: " + email);
+ }
+
public static String formatTime(PersonIdent ident, Timestamp t) {
GitDateFormatter dateFormatter = new GitDateFormatter(Format.DEFAULT);
// TODO(dborowitz): Use a ThreadLocal or use Joda.
diff --git a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
index a5b755e..81108ea 100644
--- a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
+++ b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
@@ -37,7 +37,7 @@
@Before
public void setUp() throws Exception {
- auditLogReader = new AuditLogReader(SERVER_ID, allUsersName);
+ auditLogReader = new AuditLogReader(allUsersName);
}
@Test
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
index 3e54863..a4602e19 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
@@ -56,6 +56,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
+import java.util.UUID;
import org.eclipse.jgit.lib.ObjectId;
import org.junit.Before;
import org.junit.Test;
@@ -66,6 +67,7 @@
ObjectId.fromString("1234567812345678123456781234567812345678");
private static final ByteString SHA_BYTES = ObjectIdConverter.create().toByteString(SHA);
private static final String CHANGE_KEY = "Iabcdabcdabcdabcdabcdabcdabcdabcdabcdabcd";
+ private static final String DEFAULT_SERVER_ID = UUID.randomUUID().toString();
private ChangeColumns cols;
private ChangeColumnsProto colsProto;
@@ -717,6 +719,7 @@
ImmutableMap.<String, Type>builder()
.put("metaId", ObjectId.class)
.put("changeId", Change.Id.class)
+ .put("serverId", String.class)
.put("columns", ChangeColumns.class)
.put("pastAssignees", new TypeLiteral<ImmutableSet<Account.Id>>() {}.getType())
.put("hashtags", new TypeLiteral<ImmutableSet<String>>() {}.getType())
@@ -918,6 +921,19 @@
.build());
}
+ @Test
+ public void serializeServerId() throws Exception {
+ assertRoundTrip(
+ newBuilder().serverId(DEFAULT_SERVER_ID).build(),
+ ChangeNotesStateProto.newBuilder()
+ .setMetaId(SHA_BYTES)
+ .setChangeId(ID.get())
+ .setServerId(DEFAULT_SERVER_ID)
+ .setHasServerId(true)
+ .setColumns(colsProto.toBuilder())
+ .build());
+ }
+
private static ChangeNotesStateProto toProto(ChangeNotesState state) throws Exception {
return ChangeNotesStateProto.parseFrom(Serializer.INSTANCE.serialize(state));
}
diff --git a/proto/cache.proto b/proto/cache.proto
index 77b6908..7e6abcc 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -186,6 +186,9 @@
// Number of updates to the change's meta ref.
int32 update_count = 19;
+
+ string server_id = 20;
+ bool has_server_id = 21;
}
