Do not ignore commit in a group's history if the author is not parsed.
Some old versions of gerrit created commits in refs/groups/* using
server identity (e.g. "Gerrit Code Review <server@googlesource.com>).
Such commits contain a valid data which should be parsed and returned
by the groups audit log endpoint.
The options to keep the old behaviour is added in case some users wants
it.
Google-Bug-Id: b/218652717
Release-Notes: Groups Audit Log API - Gerrit now parses all commits in refs/groups/... branch.
Change-Id: I7eeb9537dd71e9ba5e65a696cf43d000f123d21c
(cherry picked from commit 6576d2c6947a1e044b7bd9a80d88e2925b1cfcdb)
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 2bfaa40..52d9da0 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2826,6 +2826,21 @@
[[groups]]
=== Section groups
+[[groups.auditLog.ignoreRecordsFromUnidentifiedUsers]]groups.auditLog.ignoreRecordsFromUnidentifiedUsers::
++
+Controls whether AuditLogReader should ignore commits created by unidentified users.
+If true, then AuditLogReader ignores commits in the refs/groups/* made by unidentified users (i.e.
+when the author of a commit can't be parsed as account id).
++
+The current version of Gerrit writes identified users as authors for new refs/groups/* commits.
+However, some old versions used a server identity as the author (e.g. "Gerrit Code Review
+<server@googlesource.com>") for such commits. Such string can't be converted to account id but
+usually the commit shouldn't be ignored.
++
+By default, false.
++
+Setting it to true may lead to some unexpected results in audit log and must be set carefully.
+
[[groups.includeExternalUsersInRegisteredUsersGroup]]groups.includeExternalUsersInRegisteredUsersGroup::
+
Controls whether external users (these are users we have sufficient
diff --git a/java/com/google/gerrit/server/group/db/AuditLogReader.java b/java/com/google/gerrit/server/group/db/AuditLogReader.java
index 8da2f50..ae2b2fc 100644
--- a/java/com/google/gerrit/server/group/db/AuditLogReader.java
+++ b/java/com/google/gerrit/server/group/db/AuditLogReader.java
@@ -36,6 +36,7 @@
import java.util.List;
import java.util.Optional;
import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
@@ -53,10 +54,14 @@
private final AllUsersName allUsersName;
private final NoteDbUtil noteDbUtil;
+ private final boolean ignoreRecordsFromUnidentifiedUsers;
+
@Inject
- public AuditLogReader(AllUsersName allUsersName, NoteDbUtil noteDbUtil) {
+ public AuditLogReader(AllUsersName allUsersName, NoteDbUtil noteDbUtil, Config cfg) {
this.allUsersName = allUsersName;
this.noteDbUtil = noteDbUtil;
+ ignoreRecordsFromUnidentifiedUsers =
+ cfg.getBoolean("groups", "auditLog", "ignoreRecordsFromUnidentifiedUsers", false);
}
// Having separate methods for reading the two types of audit records mirrors the split in
@@ -144,9 +149,12 @@
private Optional<ParsedCommit> parse(AccountGroup.UUID uuid, RevCommit c) {
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.
- return Optional.empty();
+ if (ignoreRecordsFromUnidentifiedUsers) {
+ // Only report audit events from identified users, since this was a non-nullable field in
+ // ReviewDb.
+ return Optional.empty();
+ }
+ authorId = Optional.of(Account.UNKNOWN_ACCOUNT_ID);
}
List<Account.Id> addedMembers = new ArrayList<>();
diff --git a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
index 5d9b15d..c4cc878 100644
--- a/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
+++ b/javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
@@ -28,7 +28,9 @@
import com.google.gerrit.server.account.externalids.storage.notedb.DisabledExternalIdCache;
import com.google.gerrit.server.notedb.NoteDbUtil;
import java.time.Instant;
+import java.util.Optional;
import java.util.Set;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.PersonIdent;
import org.junit.Before;
import org.junit.Test;
@@ -41,7 +43,8 @@
@Before
public void setUp() throws Exception {
auditLogReader =
- new AuditLogReader(allUsersName, new NoteDbUtil(SERVER_ID, new DisabledExternalIdCache()));
+ new AuditLogReader(
+ allUsersName, new NoteDbUtil(SERVER_ID, new DisabledExternalIdCache()), new Config());
}
@Test
@@ -89,6 +92,67 @@
}
@Test
+ public void addMemberByUnknownAuthorAndRemoveMemberByKnownAuthor() throws Exception {
+ InternalGroup group = createGroupAsUser(1, "test-group");
+ AccountGroup.UUID uuid = group.getGroupUUID();
+ AccountGroupMemberAudit expAudit1 =
+ createExpMemberAudit(group.getId(), userId, userId, getTipTimestamp(uuid));
+
+ // An unidentified user adds account 100002 to the group.
+ Account.Id id = Account.id(100002);
+ addMembers(
+ uuid,
+ ImmutableSet.of(id),
+ new PersonIdent("Test ident", "random@gerrit"),
+ Optional.of(Instant.ofEpochSecond(1)));
+ // Identified user removes account 100002 from the group.
+ removeMembers(uuid, ImmutableSet.of(id));
+
+ AccountGroupMemberAudit expAudit2 =
+ createExpMemberAudit(
+ group.getId(), id, Account.UNKNOWN_ACCOUNT_ID, Instant.ofEpochSecond(1))
+ .toBuilder()
+ .removed(userId, getTipTimestamp(uuid))
+ .build();
+ assertThat(auditLogReader.getMembersAudit(allUsersRepo, uuid))
+ .containsExactly(expAudit1, expAudit2)
+ .inOrder();
+ }
+
+ @Test
+ public void addMemberByUnknownAuthorAndRemoveMemberByKnownAuthor_ignoreUnidentifiedUser()
+ throws Exception {
+ // This test doesn't repeat the previous test, because it doesn't produce the correct result.
+ // Instead this test adds and removes uses by an unidentified user and expects nothing in the
+ // output.
+ Config cfg = new Config();
+ cfg.setBoolean("groups", "auditLog", "ignoreRecordsFromUnidentifiedUsers", true);
+ auditLogReader =
+ new AuditLogReader(
+ allUsersName, new NoteDbUtil(SERVER_ID, new DisabledExternalIdCache()), cfg);
+ InternalGroup group = createGroupAsUser(1, "test-group");
+ AccountGroup.UUID uuid = group.getGroupUUID();
+ AccountGroupMemberAudit expAudit1 =
+ createExpMemberAudit(group.getId(), userId, userId, getTipTimestamp(uuid));
+
+ // An unidentified user adds account 100002 to the group.
+ Account.Id id = Account.id(100002);
+ addMembers(
+ uuid,
+ ImmutableSet.of(id),
+ new PersonIdent("Test ident", "random@gerrit"),
+ Optional.of(Instant.ofEpochSecond(1)));
+ // Identified user removes account 100002 from the group.
+ removeMembers(
+ uuid,
+ ImmutableSet.of(id),
+ new PersonIdent("Test ident", "random@gerrit"),
+ Optional.of(Instant.ofEpochSecond(100)));
+
+ assertThat(auditLogReader.getMembersAudit(allUsersRepo, uuid)).containsExactly(expAudit1);
+ }
+
+ @Test
public void addMultiMembers() throws Exception {
InternalGroup group = createGroupAsUser(1, "test-group");
AccountGroup.Id groupId = group.getId();
@@ -268,26 +332,53 @@
}
private void updateGroup(AccountGroup.UUID uuid, GroupDelta groupDelta) throws Exception {
+ updateGroup(uuid, groupDelta, userIdent);
+ }
+
+ private void updateGroup(AccountGroup.UUID uuid, GroupDelta groupDelta, PersonIdent authorIdent)
+ throws Exception {
testRefAction(
() -> {
GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersName, allUsersRepo, uuid);
groupConfig.setGroupDelta(groupDelta, getAuditLogFormatter());
- groupConfig.commit(createMetaDataUpdate(userIdent));
+ groupConfig.commit(createMetaDataUpdate(authorIdent));
});
}
private void addMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids) throws Exception {
- GroupDelta groupDelta =
- GroupDelta.builder().setMemberModification(memberIds -> Sets.union(memberIds, ids)).build();
- updateGroup(groupUuid, groupDelta);
+ addMembers(groupUuid, ids, userIdent, Optional.empty());
+ }
+
+ private void addMembers(
+ AccountGroup.UUID groupUuid,
+ Set<Account.Id> ids,
+ PersonIdent authorIdent,
+ Optional<Instant> updatedOn)
+ throws Exception {
+ GroupDelta.Builder groupDelta =
+ GroupDelta.builder().setMemberModification(memberIds -> Sets.union(memberIds, ids));
+ if (updatedOn.isPresent()) {
+ groupDelta.setUpdatedOn(updatedOn.get());
+ }
+ updateGroup(groupUuid, groupDelta.build(), authorIdent);
}
private void removeMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids) throws Exception {
- GroupDelta groupDelta =
- GroupDelta.builder()
- .setMemberModification(memberIds -> Sets.difference(memberIds, ids))
- .build();
- updateGroup(groupUuid, groupDelta);
+ removeMembers(groupUuid, ids, userIdent, Optional.empty());
+ }
+
+ private void removeMembers(
+ AccountGroup.UUID groupUuid,
+ Set<Account.Id> ids,
+ PersonIdent authorIdent,
+ Optional<Instant> updatedOn)
+ throws Exception {
+ GroupDelta.Builder groupDelta =
+ GroupDelta.builder().setMemberModification(memberIds -> Sets.difference(memberIds, ids));
+ if (updatedOn.isPresent()) {
+ groupDelta.setUpdatedOn(updatedOn.get());
+ }
+ updateGroup(groupUuid, groupDelta.build(), authorIdent);
}
private void addSubgroups(AccountGroup.UUID groupUuid, Set<AccountGroup.UUID> uuids)
