opendev/gerrit
Code Issues Proposed changes

ChangeNotesParser: Hoist server id check to ChangeNotes

Browse Source 
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
This commit is contained in:
David Ostrovsky
2019-08-03 17:05:19 +02:00
parent f5d9ab7e59
commit f8367a8da5
10 changed files with 76 additions and 24 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
java/com/google/gerrit/server/group/db/AuditLogReader.java
View File

@@ -27,7 +27,6 @@ import com.google.gerrit.reviewdb.client.AccountGroupByIdAudit;
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 @@ import org.eclipse.jgit.util.RawParseUtils;
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 @@ public class AuditLogReader {
}
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 @@ public class AuditLogReader {
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);
}

6
java/com/google/gerrit/server/notedb/AbstractChangeNotes.java
View File

@@ -25,6 +25,7 @@ import com.google.gerrit.metrics.Timer1;
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 abstract class AbstractChangeNotes<T> {
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 @@ public abstract class AbstractChangeNotes<T> {
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 @@ public abstract class AbstractChangeNotes<T> {
this.changeNoteJson = changeNoteJson;
this.metrics = metrics;
this.cache = cache;
this.serverId = serverId;
}
}

14
java/com/google/gerrit/server/notedb/ChangeNotes.java
View File

@@ -22,6 +22,7 @@ import static java.util.Objects.requireNonNull;
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 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
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();
}

6
java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@@ -135,6 +135,7 @@ class ChangeNotesParser {
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 @@ class ChangeNotesParser {
createdOn,
lastUpdatedOn,
ownerId,
serverId,
branch,
buildCurrentPatchSetId(),
subject,
@@ -334,6 +336,10 @@ class ChangeNotesParser {
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);

12
java/com/google/gerrit/server/notedb/ChangeNotesState.java
View File

@@ -96,6 +96,7 @@ public abstract class ChangeNotesState {
Timestamp createdOn,
Timestamp lastUpdatedOn,
Account.Id owner,
String serverId,
String branch,
@Nullable PatchSet.Id currentPatchSetId,
String subject,
@@ -154,6 +155,7 @@ public abstract class ChangeNotesState {
.build())
.pastAssignees(pastAssignees)
.hashtags(hashtags)
.serverId(serverId)
.patchSets(patchSets.entrySet())
.approvals(approvals.entries())
.reviewers(reviewers)
@@ -276,6 +278,9 @@ public abstract class ChangeNotesState {
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 @@ public abstract class ChangeNotesState {
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 @@ public abstract class ChangeNotesState {
.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 @@ public abstract class ChangeNotesState {
.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())

7
java/com/google/gerrit/server/notedb/LegacyChangeNoteRead.java
View File

@@ -50,14 +50,11 @@ public class LegacyChangeNoteRead {
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) {

25
java/com/google/gerrit/server/notedb/NoteDbUtil.java
View File

@@ -37,25 +37,28 @@ public class NoteDbUtil {
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.

2
javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
View File

@@ -37,7 +37,7 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
@Before
public void setUp() throws Exception {
auditLogReader = new AuditLogReader(SERVER_ID, allUsersName);
auditLogReader = new AuditLogReader(allUsersName);
}
@Test

16
javatests/com/google/gerrit/server/notedb/ChangeNotesStateTest.java
View File

@@ -56,6 +56,7 @@ import java.sql.Timestamp;
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 @@ public class ChangeNotesStateTest {
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 @@ public class ChangeNotesStateTest {
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 @@ public class ChangeNotesStateTest {
.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));
}

3
proto/cache.proto
View File

@@ -186,6 +186,9 @@ message ChangeNotesStateProto {
// Number of updates to the change's meta ref.
int32 update_count = 19;
string server_id = 20;
bool has_server_id = 21;
}