Resolve accounts by imported externalId
When an account's serverId does not match the one defined in
gerrit.config, then use the 'imported' external ids in the accounts
for reverse lookup of the accountId.
This allows to correctly understand and associate the imported
changes coming from repositories of other Gerrit servers to
the correct accounts locally.
Example scenario:
User John has account 100 on Gerrit A having serverId=server-id-a
and creates a Change A.
The same user registers later on Gerrit B having serverId=server-id-b
and is associated with the account 200.
Gerrit B's config set importedServerId=server-id-a,
the repository containing Change A is copied to Gerrit B and
the Gerrit admin runs a reindex on it.
By adding the additional externalId=imported:100@server-id-a the
ChangeNotesParser can lookup the correct account-id when parsing
the Change A.
When browsing Change A on Gerrit A or Gerrit B, John appears
to be the owner of the change on both of them, even if he has
two different accountIds on the two severs.
Release-Notes: Allows to lookup Change's accounts by importedServerIds.
Forward-Compatible: checked
Change-Id: I2f017af1cbb9f584e06152407cf864503738afb0
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 365a26e..be6e140 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -2549,9 +2549,14 @@
associated with the imported serverIds are indexed and displayed in the UI.
+
Specify multiple `gerrit.importedServerId` for allowing the import from multiple
-Gerrit servers with different serverIds. The servers associated with the imported
-serverIds need to have the same or a subset of the account Ids of the hosting
-Gerrit server.
+Gerrit servers with different serverIds.
++
+[NOTE]
+The account-ids referenced in the imported changes are used for looking up the
+associated account-id locally, using the `imported:` external-id.
+Example: the account-id 1000 from the imported server-id 59a4964e-6376-4ed9-beef
+will be looked up in the local accounts using the `imported:1000@59a4964e-6376-4ed9-beef`
+external-id.
+
If this value is not set, all changes imported from other Gerrit servers will be
ignored.
diff --git a/java/com/google/gerrit/entities/Account.java b/java/com/google/gerrit/entities/Account.java
index 303e79f..85dbdeb 100644
--- a/java/com/google/gerrit/entities/Account.java
+++ b/java/com/google/gerrit/entities/Account.java
@@ -46,6 +46,10 @@
*/
@AutoValue
public abstract class Account {
+
+ /** Placeholder for indicating an account-id that does not correspond to any local account */
+ public static final Id UNKNOWN_ACCOUNT_ID = id(0);
+
public static Id id(int id) {
return new AutoValue_Account_Id(id);
}
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
index 9f766d0..fe8feac 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
@@ -30,7 +30,7 @@
*
* <p>All returned collections are unmodifiable.
*/
-interface ExternalIdCache {
+public interface ExternalIdCache {
Optional<ExternalId> byKey(ExternalId.Key key) throws IOException;
ImmutableSet<ExternalId> byAccount(Account.Id accountId) throws IOException;
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
index 19044e6..0f2c877 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesCache.java
@@ -26,6 +26,7 @@
import com.google.gerrit.proto.Protos;
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.cache.CacheModule;
import com.google.gerrit.server.cache.proto.Cache.ChangeNotesKeyProto;
import com.google.gerrit.server.cache.serialize.CacheSerializer;
@@ -366,7 +367,13 @@
"Load change notes for change %s of project %s", key.changeId(), key.project());
ChangeNotesParser parser =
new ChangeNotesParser(
- key.changeId(), key.id(), walkSupplier.get(), args.changeNoteJson, args.metrics);
+ key.changeId(),
+ key.id(),
+ walkSupplier.get(),
+ args.changeNoteJson,
+ args.metrics,
+ args.serverId,
+ externalIdCache);
ChangeNotesState result = parser.parseAll();
// This assignment only happens if call() was actually called, which only
// happens when Cache#get(K, Callable<V>) incurs a cache miss.
@@ -377,11 +384,16 @@
private final Cache<Key, ChangeNotesState> cache;
private final Args args;
+ private final ExternalIdCache externalIdCache;
@Inject
- ChangeNotesCache(@Named(CACHE_NAME) Cache<Key, ChangeNotesState> cache, Args args) {
+ ChangeNotesCache(
+ @Named(CACHE_NAME) Cache<Key, ChangeNotesState> cache,
+ Args args,
+ ExternalIdCache externalIdCache) {
this.cache = cache;
this.args = args;
+ this.externalIdCache = externalIdCache;
}
Value get(
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 6e5e19c..f2a659d 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -79,6 +79,7 @@
import com.google.gerrit.server.ReviewerByEmailSet;
import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.ReviewerStatusUpdate;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.notedb.ChangeNoteUtil.ParsedPatchSetApproval;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.server.util.LabelVote;
@@ -199,18 +200,24 @@
// the latest record unsets the field).
private Optional<PatchSet.Id> cherryPickOf;
private Instant mergedOn;
+ private final ExternalIdCache externalIdCache;
+ private final String gerritServerId;
ChangeNotesParser(
Change.Id changeId,
ObjectId tip,
ChangeNotesRevWalk walk,
ChangeNoteJson changeNoteJson,
- NoteDbMetrics metrics) {
+ NoteDbMetrics metrics,
+ String gerritServerId,
+ ExternalIdCache externalIdCache) {
this.id = changeId;
this.tip = tip;
this.walk = walk;
this.changeNoteJson = changeNoteJson;
this.metrics = metrics;
+ this.externalIdCache = externalIdCache;
+ this.gerritServerId = gerritServerId;
approvals = new LinkedHashMap<>();
bufferedApprovals = new ArrayList<>();
reviewers = HashBasedTable.create();
@@ -1406,7 +1413,7 @@
}
private Account.Id parseIdent(PersonIdent ident) throws ConfigInvalidException {
- return NoteDbUtil.parseIdent(ident)
+ return NoteDbUtil.parseIdent(ident, gerritServerId, externalIdCache)
.orElseThrow(
() -> parseException("cannot retrieve account id: %s", ident.getEmailAddress()));
}
diff --git a/java/com/google/gerrit/server/notedb/NoteDbUtil.java b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
index 396e29b..64bf430 100644
--- a/java/com/google/gerrit/server/notedb/NoteDbUtil.java
+++ b/java/com/google/gerrit/server/notedb/NoteDbUtil.java
@@ -20,8 +20,12 @@
import com.google.common.primitives.Ints;
import com.google.gerrit.entities.Account;
import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
+import java.io.IOException;
import java.sql.Timestamp;
import java.util.Optional;
+import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.util.GitDateFormatter;
import org.eclipse.jgit.util.GitDateFormatter.Format;
@@ -48,6 +52,45 @@
return Optional.empty();
}
+ /**
+ * Returns an AccountId for the given email address and the current serverId. Reverse lookup the
+ * AccountId using the ExternalIdCache if the account has a foreign serverId.
+ *
+ * @param ident the accountId@serverId identity
+ * @param serverId the Gerrit's serverId
+ * @param externalIdCache reference to the cache for looking up the external ids
+ * @return a defined accountId if the account was found, {@link Account#UNKNOWN_ACCOUNT_ID} if the
+ * lookup via external-id did not return any account, or an empty value if the identity was
+ * malformed.
+ * @throws ConfigInvalidException when the lookup of the external-id failed
+ */
+ public static Optional<Account.Id> parseIdent(
+ PersonIdent ident, String serverId, ExternalIdCache externalIdCache)
+ throws ConfigInvalidException {
+ String email = ident.getEmailAddress();
+ int at = email.indexOf('@');
+ if (at >= 0) {
+ Integer id = Ints.tryParse(email.substring(0, at));
+ String accountServerId = email.substring(at + 1);
+ if (id != null) {
+ if (accountServerId.equals(serverId)) {
+ return Optional.of(Account.id(id));
+ }
+
+ ExternalId.Key extIdKey = ExternalId.Key.create(ExternalId.SCHEME_IMPORTED, email, false);
+ try {
+ return externalIdCache
+ .byKey(extIdKey)
+ .map(ExternalId::accountId)
+ .or(() -> Optional.of(Account.UNKNOWN_ACCOUNT_ID));
+ } catch (IOException e) {
+ throw new ConfigInvalidException("Unable to lookup external id from cache", e);
+ }
+ }
+ }
+ return Optional.empty();
+ }
+
public static String extractHostPartFromPersonIdent(PersonIdent ident) {
String email = ident.getEmailAddress();
int at = email.indexOf('@');
diff --git a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
index d17c3cc..1b286d1 100644
--- a/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java
@@ -40,6 +40,8 @@
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.account.externalids.DisabledExternalIdCache;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.approval.PatchSetApprovalUuidGenerator;
import com.google.gerrit.server.approval.testing.TestPatchSetApprovalUuidGenerator;
import com.google.gerrit.server.config.AllUsersName;
@@ -70,6 +72,7 @@
import com.google.inject.Guice;
import com.google.inject.Inject;
import com.google.inject.Injector;
+import com.google.inject.Module;
import com.google.inject.TypeLiteral;
import java.time.Instant;
import java.time.ZoneId;
@@ -119,6 +122,8 @@
@Inject @GerritServerId protected String serverId;
+ @Inject protected ExternalIdCache externalIdCache;
+
protected Injector injector;
private String systemTimeZone;
@@ -157,11 +162,17 @@
protected Injector createTestInjector(String serverId, String... importedServerIds)
throws Exception {
+ return createTestInjector(DisabledExternalIdCache.module(), serverId, importedServerIds);
+ }
+
+ protected Injector createTestInjector(
+ Module extraGuiceModule, String serverId, String... importedServerIds) throws Exception {
return Guice.createInjector(
new FactoryModule() {
@Override
public void configure() {
+ install(extraGuiceModule);
install(new GitModule());
install(new DefaultUrlFormatterModule());
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
index 666b8fc..9445f4a 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesCommitTest.java
@@ -98,7 +98,8 @@
private ChangeNotesParser newParser(ObjectId tip) throws Exception {
walk.reset();
ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class);
- return new ChangeNotesParser(newChange().getId(), tip, walk, changeNoteJson, args.metrics);
+ return new ChangeNotesParser(
+ newChange().getId(), tip, walk, changeNoteJson, args.metrics, serverId, externalIdCache);
}
private RevCommit writeCommit(String body) throws Exception {
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
index 276f093..4543b50 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesParserTest.java
@@ -201,11 +201,11 @@
+ "Branch: refs/heads/master\n"
+ "Change-id: I577fb248e474018276351785930358ec0450e9f7\n"
+ "Patch-set: 1\n"
- + "Copied-Label: Label1=+1 Account <1@gerrit>,Other Account <2@Gerrit>\n"
+ + "Copied-Label: Label1=+1 Account <1@gerrit>,Other Account <2@gerrit>\n"
+ "Copied-Label: Label2=+1 Account <1@gerrit>\n"
- + "Copied-Label: Label3=+1 Account <1@gerrit>,Other Account <2@Gerrit> :\"tag\"\n"
- + "Copied-Label: Label4=+1 Account <1@Gerrit> :\"tag with characters %^#@^( *::!\"\n"
- + "Copied-Label: -Label1 Account <1@gerrit>,Other Account <2@Gerrit>\\n"
+ + "Copied-Label: Label3=+1 Account <1@gerrit>,Other Account <2@gerrit> :\"tag\"\n"
+ + "Copied-Label: Label4=+1 Account <1@gerrit> :\"tag with characters %^#@^( *::!\"\n"
+ + "Copied-Label: -Label1 Account <1@gerrit>,Other Account <2@gerrit>\\n"
+ "Copied-Label: -Label1 Account <1@gerrit>\n"
+ "Subject: This is a test change\n");
@@ -782,6 +782,7 @@
private ChangeNotesParser newParser(ObjectId tip) throws Exception {
walk.reset();
ChangeNoteJson changeNoteJson = injector.getInstance(ChangeNoteJson.class);
- return new ChangeNotesParser(newChange().getId(), tip, walk, changeNoteJson, args.metrics);
+ return new ChangeNotesParser(
+ newChange().getId(), tip, walk, changeNoteJson, args.metrics, serverId, externalIdCache);
}
}
diff --git a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 8def660..4edfa8b4 100644
--- a/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -2084,7 +2084,13 @@
try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
ChangeNotesParser notesWithComments =
new ChangeNotesParser(
- c.getId(), commitWithComments.copy(), rw, changeNoteJson, args.metrics);
+ c.getId(),
+ commitWithComments.copy(),
+ rw,
+ changeNoteJson,
+ args.metrics,
+ serverId,
+ externalIdCache);
ChangeNotesState state = notesWithComments.parseAll();
assertThat(state.approvals()).isEmpty();
assertThat(state.publishedComments()).hasSize(1);
@@ -2093,7 +2099,13 @@
try (ChangeNotesRevWalk rw = ChangeNotesCommit.newRevWalk(repo)) {
ChangeNotesParser notesWithApprovals =
new ChangeNotesParser(
- c.getId(), commitWithApprovals.copy(), rw, changeNoteJson, args.metrics);
+ c.getId(),
+ commitWithApprovals.copy(),
+ rw,
+ changeNoteJson,
+ args.metrics,
+ serverId,
+ externalIdCache);
ChangeNotesState state = notesWithApprovals.parseAll();
assertThat(state.approvals()).hasSize(1);
diff --git a/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java b/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java
index 1eb08ce..bb49a6d 100644
--- a/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java
+++ b/javatests/com/google/gerrit/server/notedb/ImportedChangeNotesTest.java
@@ -16,9 +16,18 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.git.RepositoryCaseMismatchException;
+import com.google.inject.AbstractModule;
+import java.util.Optional;
import org.eclipse.jgit.errors.RepositoryNotFoundException;
import org.junit.Before;
import org.junit.Test;
@@ -28,6 +37,8 @@
private static final String FOREIGN_SERVER_ID = "foreign-server-id";
private static final String IMPORTED_SERVER_ID = "gerrit-imported-1";
+ private ExternalIdCache externalIdCacheMock;
+
@Before
@Override
public void setUpTestEnvironment() throws Exception {
@@ -36,7 +47,17 @@
private void initServerIds(String serverId, String... importedServerIds)
throws Exception, RepositoryCaseMismatchException, RepositoryNotFoundException {
- injector = createTestInjector(serverId, importedServerIds);
+ externalIdCacheMock = mock(ExternalIdCache.class);
+ injector =
+ createTestInjector(
+ new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(ExternalIdCache.class).toInstance(externalIdCacheMock);
+ }
+ },
+ serverId,
+ importedServerIds);
injector.injectMembers(this);
createAllUsers(injector);
}
@@ -45,6 +66,17 @@
public void allowChangeFromImportedServerId() throws Exception {
initServerIds(LOCAL_SERVER_ID, IMPORTED_SERVER_ID);
+ ExternalId.Key importedAccountIdKey =
+ ExternalId.Key.create(
+ ExternalId.SCHEME_IMPORTED,
+ changeOwner.getAccountId() + "@" + IMPORTED_SERVER_ID,
+ false);
+ ExternalId importedAccountId =
+ ExternalId.create(importedAccountIdKey, changeOwner.getAccountId(), null, null, null);
+
+ when(externalIdCacheMock.byKey(eq(importedAccountIdKey)))
+ .thenReturn(Optional.of(importedAccountId));
+
Change importedChange = newChange(createTestInjector(IMPORTED_SERVER_ID), false);
Change localChange = newChange();
@@ -55,6 +87,8 @@
@Test
public void rejectChangeWithForeignServerId() throws Exception {
initServerIds(LOCAL_SERVER_ID);
+ when(externalIdCacheMock.byKey(any())).thenReturn(Optional.empty());
+
Change foreignChange = newChange(createTestInjector(FOREIGN_SERVER_ID), false);
InvalidServerIdException invalidServerIdEx =
@@ -64,4 +98,17 @@
assertThat(invalidServerIdMessage).contains("expected " + LOCAL_SERVER_ID);
assertThat(invalidServerIdMessage).contains("actual: " + FOREIGN_SERVER_ID);
}
+
+ @Test
+ public void changeFromImportedServerIdWithUnknownAccountId() throws Exception {
+ initServerIds(LOCAL_SERVER_ID, IMPORTED_SERVER_ID);
+
+ when(externalIdCacheMock.byKey(any())).thenReturn(Optional.empty());
+
+ Change importedChange = newChange(createTestInjector(IMPORTED_SERVER_ID), false);
+ assertThat(newNotes(importedChange).getServerId()).isEqualTo(IMPORTED_SERVER_ID);
+
+ assertThat(newNotes(importedChange).getChange().getOwner())
+ .isEqualTo(Account.UNKNOWN_ACCOUNT_ID);
+ }
}
