From b70b92ca57b1bfc2e7361e973c7ff5d041293ff8 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Oct 2017 09:55:22 +0200 Subject: [PATCH 1/5] Fall-back to loading groups from ReviewDb if group index is not available In Gerrit slaves indexes are not available and hence trying to query groups from the group index fails. This prevents loading of accounts via the account cache. As result of this the account cache returns inactive account instances to represent the missing accounts. This prevents users from authenticating with SSH and SSH requests are rejected with "Permission denied (publickey)." To fix this fall-back to loading groups from ReviewDb if the group index is not available. This reverts change I86f85b6e41 partially. Bug: Issue 7340 Change-Id: I196c313f75f0a2f2bcb97445347bfe7113283198 Signed-off-by: Edwin Kempin --- .../server/account/AccountCacheImpl.java | 3 ++- .../gerrit/server/account/GroupCacheImpl.java | 21 +++++++++++++++-- .../google/gerrit/server/group/Groups.java | 23 +++++++++++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java index adca6fb6d3..d062842441 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountCacheImpl.java @@ -224,7 +224,8 @@ public class AccountCacheImpl implements AccountCache { private ImmutableSet getGroupsWithMember(ReviewDb db, Account.Id memberId) throws OrmException { Stream internalGroupStream; - if (groupIndexProvider.get().getSchema().hasField(GroupField.MEMBER)) { + if (groupIndexProvider.get() != null + && groupIndexProvider.get().getSchema().hasField(GroupField.MEMBER)) { internalGroupStream = groupQueryProvider.get().byMember(memberId).stream(); } else { internalGroupStream = diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java index 81996ab411..7887929f4a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java @@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.cache.CacheModule; import com.google.gerrit.server.group.Groups; import com.google.gerrit.server.group.InternalGroup; +import com.google.gerrit.server.index.group.GroupIndexCollection; import com.google.gerrit.server.index.group.GroupIndexer; import com.google.gerrit.server.query.group.InternalGroupQuery; import com.google.gwtorm.server.SchemaFactory; @@ -147,16 +148,32 @@ public class GroupCacheImpl implements GroupCache { } static class ByIdLoader extends CacheLoader> { + private final SchemaFactory schema; + private final Groups groups; + private final boolean hasGroupIndex; private final Provider groupQueryProvider; @Inject - ByIdLoader(Provider groupQueryProvider) { + ByIdLoader( + SchemaFactory schema, + Groups groups, + GroupIndexCollection groupIndexCollection, + Provider groupQueryProvider) { + this.schema = schema; + this.groups = groups; + this.hasGroupIndex = groupIndexCollection.getSearchIndex() != null; this.groupQueryProvider = groupQueryProvider; } @Override public Optional load(AccountGroup.Id key) throws Exception { - return groupQueryProvider.get().byId(key); + if (hasGroupIndex) { + return groupQueryProvider.get().byId(key); + } + + try (ReviewDb db = schema.open()) { + return groups.getGroup(db, key); + } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java index a2660f2672..233f36b385 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/group/Groups.java @@ -46,6 +46,29 @@ import java.util.stream.Stream; @Singleton public class Groups { + /** + * Returns the {@code AccountGroup} for the specified ID if it exists. + * + * @param db the {@code ReviewDb} instance to use for lookups + * @param groupId the ID of the group + * @return the found {@code AccountGroup} if it exists, or else an empty {@code Optional} + * @throws OrmException if the group couldn't be retrieved from ReviewDb + */ + public Optional getGroup(ReviewDb db, AccountGroup.Id groupId) + throws OrmException, NoSuchGroupException { + Optional accountGroup = Optional.ofNullable(db.accountGroups().get(groupId)); + + if (!accountGroup.isPresent()) { + return Optional.empty(); + } + + AccountGroup.UUID groupUuid = accountGroup.get().getGroupUUID(); + ImmutableSet members = getMembers(db, groupUuid).collect(toImmutableSet()); + ImmutableSet subgroups = + getSubgroups(db, groupUuid).collect(toImmutableSet()); + return accountGroup.map(group -> InternalGroup.create(group, members, subgroups)); + } + /** * Returns the {@code InternalGroup} for the specified UUID if it exists. * From b8b763781f3f05e946ab2044cc74ed52cb0ad6e9 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 5 Oct 2017 18:06:43 +0200 Subject: [PATCH 2/5] InitAdminUser: Reindex admin group after adding the user Since change Iff64365630 group members are now stored in the group index. Hence after adding a group member the group must be reindexed. Since the group reindex was skipped the membership of the initial admin user in the Administrators group was not effective until a next reindex of the group occurred. Bug: Issue 7358 Change-Id: Idb5b91a92c682f03d74a5327ec96442f63999d14 Signed-off-by: Edwin Kempin --- .../google/gerrit/pgm/init/InitAdminUser.java | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java index 86468c9032..9a81c52153 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/InitAdminUser.java @@ -17,6 +17,7 @@ package com.google.gerrit.pgm.init; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.client.AuthType; import com.google.gerrit.pgm.init.api.AllUsersNameOnInitProvider; @@ -31,8 +32,11 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.group.InternalGroup; import com.google.gerrit.server.index.account.AccountIndex; import com.google.gerrit.server.index.account.AccountIndexCollection; +import com.google.gerrit.server.index.group.GroupIndex; +import com.google.gerrit.server.index.group.GroupIndexCollection; import com.google.gwtorm.server.SchemaFactory; import com.google.inject.Inject; import java.io.IOException; @@ -55,7 +59,8 @@ public class InitAdminUser implements InitStep { private final SequencesOnInit sequencesOnInit; private final GroupsOnInit groupsOnInit; private SchemaFactory dbFactory; - private AccountIndexCollection indexCollection; + private AccountIndexCollection accountIndexCollection; + private GroupIndexCollection groupIndexCollection; @Inject InitAdminUser( @@ -86,8 +91,13 @@ public class InitAdminUser implements InitStep { } @Inject(optional = true) - void set(AccountIndexCollection indexCollection) { - this.indexCollection = indexCollection; + void set(AccountIndexCollection accountIndexCollection) { + this.accountIndexCollection = accountIndexCollection; + } + + @Inject(optional = true) + void set(GroupIndexCollection groupIndexCollection) { + this.groupIndexCollection = groupIndexCollection; } @Override @@ -138,9 +148,15 @@ public class InitAdminUser implements InitStep { Collections.singleton(adminGroup.getGroupUUID()), extIds, new HashMap<>()); - for (AccountIndex accountIndex : indexCollection.getWriteIndexes()) { + for (AccountIndex accountIndex : accountIndexCollection.getWriteIndexes()) { accountIndex.replace(as); } + + InternalGroup adminInternalGroup = + InternalGroup.create(adminGroup, ImmutableSet.of(id), ImmutableSet.of()); + for (GroupIndex groupIndex : groupIndexCollection.getWriteIndexes()) { + groupIndex.replace(adminInternalGroup); + } } } } From 0426e1ee637fa3e18a8040435f0e1ea07adbe03d Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Mon, 9 Oct 2017 01:30:29 +0200 Subject: [PATCH 3/5] Update JGit to 4.9.0.201710071750-r Change-Id: I5d74d5487a0c67dbd6380cea6aab166af5fff8bf --- lib/jgit/jgit.bzl | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lib/jgit/jgit.bzl b/lib/jgit/jgit.bzl index 63761f220b..1d964b8291 100644 --- a/lib/jgit/jgit.bzl +++ b/lib/jgit/jgit.bzl @@ -1,12 +1,12 @@ load("//tools/bzl:maven_jar.bzl", "GERRIT", "MAVEN_LOCAL", "MAVEN_CENTRAL", "maven_jar") -_JGIT_VERS = "4.8.0.201706111038-r.71-g45da0fc6f" +_JGIT_VERS = "4.9.0.201710071750-r" -_DOC_VERS = "4.8.0.201706111038-r" # Set to _JGIT_VERS unless using a snapshot +_DOC_VERS = _JGIT_VERS # Set to _JGIT_VERS unless using a snapshot JGIT_DOC_URL = "http://download.eclipse.org/jgit/site/" + _DOC_VERS + "/apidocs" -_JGIT_REPO = GERRIT # Leave here even if set to MAVEN_CENTRAL. +_JGIT_REPO = MAVEN_CENTRAL # Leave here even if set to MAVEN_CENTRAL. # set this to use a local version. # "/home//projects/jgit" @@ -26,28 +26,28 @@ def jgit_maven_repos(): name = "jgit_lib", artifact = "org.eclipse.jgit:org.eclipse.jgit:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "7248b0a7d7f76dd4f7e55ed081b981cf4d8aa26e", - src_sha1 = "6ed203c95decc3f795f44ca17149e7554b92212d", + sha1 = "69d8510b335d4d33d551a133505a4141311f970a", + src_sha1 = "6fd1eb331447b6163898b4d10aa769e2ac193740", unsign = True, ) maven_jar( name = "jgit_servlet", artifact = "org.eclipse.jgit:org.eclipse.jgit.http.server:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "f21fc0c651cc9475db92061432d919ba28b7a7ad", + sha1 = "93fb0075988b9c6bb97c725c03706f2341965b6b", unsign = True, ) maven_jar( name = "jgit_archive", artifact = "org.eclipse.jgit:org.eclipse.jgit.archive:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "0f179321f527840dfc8ca79894eba2f6b255dbab", + sha1 = "a15aee805c758516ad7e9fa3f16e27bb9f4a1c2e", ) maven_jar( name = "jgit_junit", artifact = "org.eclipse.jgit:org.eclipse.jgit.junit:" + _JGIT_VERS, repository = _JGIT_REPO, - sha1 = "7e5225064cf14115bddaae9448246c83c89f78ad", + sha1 = "b6e712e743ea5798134f54547ae80456fad07f76", unsign = True, ) From 4d598b33428689932f82e83dfc4f8be0ebbce673 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 6 Oct 2017 14:46:52 +0200 Subject: [PATCH 4/5] Update git submodules * Update plugins/replication from branch 'stable-2.15' - Document which branches must be replicated to replicate the account data Change-Id: I2c9dc06927cd8e9e541eb032829029bdfac8db6e Signed-off-by: Edwin Kempin (cherry picked from commit 93ab1609a4ba32e9466ff0649317b84a211285ad) --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index 2540766109..8bc97a106a 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit 25407661098f4848e771395d62da2feca1135793 +Subproject commit 8bc97a106a46a0f32350396e74769af11ec7b98e From 733976750f61b696226fe9175ac3fe46301f24d6 Mon Sep 17 00:00:00 2001 From: Changcheng Xiao Date: Mon, 9 Oct 2017 09:15:42 +0200 Subject: [PATCH 5/5] Keep "refs/publish/*" as a synonym of "refs/for/*" There are some tools, e.g. git-review, which are depending on this magic branch. We have to treat "refs/publish/*" as a synonym of "refs/for/*" in 2.15. See the corresponding change in master for details on further work. Change-Id: Ifcd35dfdc8eefe78e7173386ae6b8a8b2efa158d (cherry picked from commit 732e075ce860df57bcda239c08315e137011ede8) --- .../acceptance/git/AbstractPushForReview.java | 10 ++++++++++ .../server/git/receive/ReceiveCommits.java | 8 ++++++++ .../google/gerrit/server/util/MagicBranch.java | 18 ++++++++++++++++-- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 95e4e16efd..928cd7ed7c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -1852,6 +1852,16 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { .isEqualTo(Iterables.getLast(commits).name()); } + @Test + public void pushToPublishMagicBranchIsAllowed() throws Exception { + // Push to "refs/publish/*" will be a synonym of "refs/for/*". + createChange("refs/publish/master"); + PushOneCommit.Result result = pushTo("refs/publish/master"); + result.assertOkStatus(); + assertThat(result.getMessage()) + .endsWith("Pushing to refs/publish/* is deprecated, use refs/for/* instead.\n"); + } + private DraftInput newDraft(String path, int line, String message) { DraftInput d = new DraftInput(); d.path = path; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index b7aa416c1f..3b27c06835 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -669,6 +669,11 @@ class ReceiveCommits { } addMessage(""); } + + // TODO(xchangcheng): remove after migrating tools which are using this magic branch. + if (magicBranch != null && magicBranch.publish) { + addMessage("Pushing to refs/publish/* is deprecated, use refs/for/* instead."); + } } private void insertChangesAndPatchSets() { @@ -1165,6 +1170,8 @@ class ReceiveCommits { ) boolean draft; + boolean publish; + @Option(name = "--private", usage = "mark new/updated change as private") boolean isPrivate; @@ -1289,6 +1296,7 @@ class ReceiveCommits { NotesMigration notesMigration) { this.cmd = cmd; this.draft = cmd.getRefName().startsWith(MagicBranch.NEW_DRAFT_CHANGE); + this.publish = cmd.getRefName().startsWith(MagicBranch.NEW_PUBLISH_CHANGE); this.labelTypes = labelTypes; this.notesMigration = notesMigration; GeneralPreferencesInfo prefs = user.getAccount().getGeneralPreferencesInfo(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java b/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java index 2088409ef5..e757d77a63 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/util/MagicBranch.java @@ -27,21 +27,27 @@ public final class MagicBranch { private static final Logger log = LoggerFactory.getLogger(MagicBranch.class); public static final String NEW_CHANGE = "refs/for/"; - // TODO: remove after 'repo' supports private/wip changes. + // TODO(xchangcheng): remove after 'repo' supports private/wip changes. public static final String NEW_DRAFT_CHANGE = "refs/drafts/"; + // TODO(xchangcheng): remove after migrating tools which are using this magic branch. + public static final String NEW_PUBLISH_CHANGE = "refs/publish/"; /** Extracts the destination from a ref name */ public static String getDestBranchName(String refName) { String magicBranch = NEW_CHANGE; if (refName.startsWith(NEW_DRAFT_CHANGE)) { magicBranch = NEW_DRAFT_CHANGE; + } else if (refName.startsWith(NEW_PUBLISH_CHANGE)) { + magicBranch = NEW_PUBLISH_CHANGE; } return refName.substring(magicBranch.length()); } /** Checks if the supplied ref name is a magic branch */ public static boolean isMagicBranch(String refName) { - return refName.startsWith(NEW_DRAFT_CHANGE) || refName.startsWith(NEW_CHANGE); + return refName.startsWith(NEW_DRAFT_CHANGE) + || refName.startsWith(NEW_PUBLISH_CHANGE) + || refName.startsWith(NEW_CHANGE); } /** Returns the ref name prefix for a magic branch, {@code null} if the branch is not magic */ @@ -49,6 +55,9 @@ public final class MagicBranch { if (refName.startsWith(NEW_DRAFT_CHANGE)) { return NEW_DRAFT_CHANGE; } + if (refName.startsWith(NEW_PUBLISH_CHANGE)) { + return NEW_PUBLISH_CHANGE; + } if (refName.startsWith(NEW_CHANGE)) { return NEW_CHANGE; } @@ -72,6 +81,11 @@ public final class MagicBranch { if (result != Capable.OK) { return result; } + result = checkMagicBranchRef(NEW_PUBLISH_CHANGE, repo, project); + if (result != Capable.OK) { + return result; + } + return Capable.OK; }