Move addition of refs/users/self symlink out of filtering logic
Gerrit moved to a model where it wraps around RefDatabase to tell JGit which refs are visible to a user. This was done in I0bb85de2b. This meant moving filtering code from a AdvertiseRefsHook into a wrapper of RefDatabase. There is one case in which the logic that got moved would not only filter refs but add a ref in addition that is not present in the input: when authenticated users interact with All-Users. In this case, Gerrit adds a refs/users/self symbolic reference to make it easier to interact with one's account data. This led to a problem with the new inferface and smelled as a whole since concerns weren't clearly separated. The problem with the new interface would occur when using RefDatabase#exactRef - a method that expects {0,1} refs but got two for All-Users since refs/users/self was added. This change fixes the issue by separating the addition of the symref into an AdvertiseRefsHook and leaving ref filtering by just that. RefAdvertisementIT has different tests for testing the advertisement of refs/users/self and PushAccountIT covers push cases. The tests succeeded before, because the problem could only be encountered when the in-memory ref cache on the push path is not used. We add a config to run push tests without in-memory ref cache to make sure the code path that failed previosuly gets executed now. Bug: Issue 12027 Change-Id: I07777b698ddb86ad5cec1407ac5671874866fee5
This commit is contained in:

committed by
David Pursehouse

parent
e4821b1285
commit
3ceb65aaf0
@@ -35,6 +35,7 @@ import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
|
|||||||
import com.google.gerrit.server.git.ReceivePackInitializer;
|
import com.google.gerrit.server.git.ReceivePackInitializer;
|
||||||
import com.google.gerrit.server.git.TransferConfig;
|
import com.google.gerrit.server.git.TransferConfig;
|
||||||
import com.google.gerrit.server.git.UploadPackInitializer;
|
import com.google.gerrit.server.git.UploadPackInitializer;
|
||||||
|
import com.google.gerrit.server.git.UsersSelfAdvertiseRefsHook;
|
||||||
import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
|
import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
|
||||||
import com.google.gerrit.server.git.validators.UploadValidators;
|
import com.google.gerrit.server.git.validators.UploadValidators;
|
||||||
import com.google.gerrit.server.permissions.PermissionBackend;
|
import com.google.gerrit.server.permissions.PermissionBackend;
|
||||||
@@ -200,6 +201,7 @@ class InProcessProtocol extends TestProtocol<Context> {
|
|||||||
private final ThreadLocalRequestContext threadContext;
|
private final ThreadLocalRequestContext threadContext;
|
||||||
private final ProjectCache projectCache;
|
private final ProjectCache projectCache;
|
||||||
private final PermissionBackend permissionBackend;
|
private final PermissionBackend permissionBackend;
|
||||||
|
private final UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook;
|
||||||
|
|
||||||
@Inject
|
@Inject
|
||||||
Upload(
|
Upload(
|
||||||
@@ -209,7 +211,8 @@ class InProcessProtocol extends TestProtocol<Context> {
|
|||||||
UploadValidators.Factory uploadValidatorsFactory,
|
UploadValidators.Factory uploadValidatorsFactory,
|
||||||
ThreadLocalRequestContext threadContext,
|
ThreadLocalRequestContext threadContext,
|
||||||
ProjectCache projectCache,
|
ProjectCache projectCache,
|
||||||
PermissionBackend permissionBackend) {
|
PermissionBackend permissionBackend,
|
||||||
|
UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook) {
|
||||||
this.transferConfig = transferConfig;
|
this.transferConfig = transferConfig;
|
||||||
this.uploadPackInitializers = uploadPackInitializers;
|
this.uploadPackInitializers = uploadPackInitializers;
|
||||||
this.preUploadHooks = preUploadHooks;
|
this.preUploadHooks = preUploadHooks;
|
||||||
@@ -217,6 +220,7 @@ class InProcessProtocol extends TestProtocol<Context> {
|
|||||||
this.threadContext = threadContext;
|
this.threadContext = threadContext;
|
||||||
this.projectCache = projectCache;
|
this.projectCache = projectCache;
|
||||||
this.permissionBackend = permissionBackend;
|
this.permissionBackend = permissionBackend;
|
||||||
|
this.usersSelfAdvertiseRefsHook = usersSelfAdvertiseRefsHook;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
@@ -250,6 +254,9 @@ class InProcessProtocol extends TestProtocol<Context> {
|
|||||||
UploadPack up = new UploadPack(permissionAwareRepository);
|
UploadPack up = new UploadPack(permissionAwareRepository);
|
||||||
up.setPackConfig(transferConfig.getPackConfig());
|
up.setPackConfig(transferConfig.getPackConfig());
|
||||||
up.setTimeout(transferConfig.getTimeout());
|
up.setTimeout(transferConfig.getTimeout());
|
||||||
|
if (projectState.isAllUsers()) {
|
||||||
|
up.setAdvertiseRefsHook(usersSelfAdvertiseRefsHook);
|
||||||
|
}
|
||||||
List<PreUploadHook> hooks = Lists.newArrayList(preUploadHooks);
|
List<PreUploadHook> hooks = Lists.newArrayList(preUploadHooks);
|
||||||
hooks.add(
|
hooks.add(
|
||||||
uploadValidatorsFactory.create(
|
uploadValidatorsFactory.create(
|
||||||
|
@@ -35,6 +35,7 @@ import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
|
|||||||
import com.google.gerrit.server.git.TracingHook;
|
import com.google.gerrit.server.git.TracingHook;
|
||||||
import com.google.gerrit.server.git.TransferConfig;
|
import com.google.gerrit.server.git.TransferConfig;
|
||||||
import com.google.gerrit.server.git.UploadPackInitializer;
|
import com.google.gerrit.server.git.UploadPackInitializer;
|
||||||
|
import com.google.gerrit.server.git.UsersSelfAdvertiseRefsHook;
|
||||||
import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
|
import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
|
||||||
import com.google.gerrit.server.git.validators.UploadValidators;
|
import com.google.gerrit.server.git.validators.UploadValidators;
|
||||||
import com.google.gerrit.server.group.GroupAuditService;
|
import com.google.gerrit.server.group.GroupAuditService;
|
||||||
@@ -355,6 +356,7 @@ public class GitOverHttpServlet extends GitServlet {
|
|||||||
private final GroupAuditService groupAuditService;
|
private final GroupAuditService groupAuditService;
|
||||||
private final Metrics metrics;
|
private final Metrics metrics;
|
||||||
private final PluginSetContext<RequestListener> requestListeners;
|
private final PluginSetContext<RequestListener> requestListeners;
|
||||||
|
private final UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook;
|
||||||
|
|
||||||
@Inject
|
@Inject
|
||||||
UploadFilter(
|
UploadFilter(
|
||||||
@@ -363,13 +365,15 @@ public class GitOverHttpServlet extends GitServlet {
|
|||||||
Provider<CurrentUser> userProvider,
|
Provider<CurrentUser> userProvider,
|
||||||
GroupAuditService groupAuditService,
|
GroupAuditService groupAuditService,
|
||||||
Metrics metrics,
|
Metrics metrics,
|
||||||
PluginSetContext<RequestListener> requestListeners) {
|
PluginSetContext<RequestListener> requestListeners,
|
||||||
|
UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook) {
|
||||||
this.uploadValidatorsFactory = uploadValidatorsFactory;
|
this.uploadValidatorsFactory = uploadValidatorsFactory;
|
||||||
this.permissionBackend = permissionBackend;
|
this.permissionBackend = permissionBackend;
|
||||||
this.userProvider = userProvider;
|
this.userProvider = userProvider;
|
||||||
this.groupAuditService = groupAuditService;
|
this.groupAuditService = groupAuditService;
|
||||||
this.metrics = metrics;
|
this.metrics = metrics;
|
||||||
this.requestListeners = requestListeners;
|
this.requestListeners = requestListeners;
|
||||||
|
this.usersSelfAdvertiseRefsHook = usersSelfAdvertiseRefsHook;
|
||||||
}
|
}
|
||||||
|
|
||||||
@Override
|
@Override
|
||||||
@@ -416,6 +420,9 @@ public class GitOverHttpServlet extends GitServlet {
|
|||||||
up.setPreUploadHook(
|
up.setPreUploadHook(
|
||||||
PreUploadHookChain.newChain(
|
PreUploadHookChain.newChain(
|
||||||
Lists.newArrayList(up.getPreUploadHook(), uploadValidators)));
|
Lists.newArrayList(up.getPreUploadHook(), uploadValidators)));
|
||||||
|
if (state.isAllUsers()) {
|
||||||
|
up.setAdvertiseRefsHook(usersSelfAdvertiseRefsHook);
|
||||||
|
}
|
||||||
|
|
||||||
try (TracingHook tracingHook = new TracingHook()) {
|
try (TracingHook tracingHook = new TracingHook()) {
|
||||||
up.setProtocolV2Hook(tracingHook);
|
up.setProtocolV2Hook(tracingHook);
|
||||||
|
@@ -21,6 +21,7 @@ import java.util.Map;
|
|||||||
import org.eclipse.jgit.lib.Ref;
|
import org.eclipse.jgit.lib.Ref;
|
||||||
import org.eclipse.jgit.transport.ReceivePack;
|
import org.eclipse.jgit.transport.ReceivePack;
|
||||||
import org.eclipse.jgit.transport.ServiceMayNotContinueException;
|
import org.eclipse.jgit.transport.ServiceMayNotContinueException;
|
||||||
|
import org.eclipse.jgit.transport.UploadPack;
|
||||||
|
|
||||||
/** Static utilities for writing git protocol hooks. */
|
/** Static utilities for writing git protocol hooks. */
|
||||||
public class HookUtil {
|
public class HookUtil {
|
||||||
@@ -51,5 +52,32 @@ public class HookUtil {
|
|||||||
return refs;
|
return refs;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Scan and advertise all refs in the repo if refs have not already been advertised; otherwise,
|
||||||
|
* just return the advertised map.
|
||||||
|
*
|
||||||
|
* @param up upload-pack handler.
|
||||||
|
* @return map of refs that were advertised.
|
||||||
|
* @throws ServiceMayNotContinueException if a problem occurred.
|
||||||
|
*/
|
||||||
|
public static Map<String, Ref> ensureAllRefsAdvertised(UploadPack up)
|
||||||
|
throws ServiceMayNotContinueException {
|
||||||
|
Map<String, Ref> refs = up.getAdvertisedRefs();
|
||||||
|
if (refs != null) {
|
||||||
|
return refs;
|
||||||
|
}
|
||||||
|
try {
|
||||||
|
refs =
|
||||||
|
up.getRepository().getRefDatabase().getRefs().stream()
|
||||||
|
.collect(toMap(Ref::getName, r -> r));
|
||||||
|
} catch (ServiceMayNotContinueException e) {
|
||||||
|
throw e;
|
||||||
|
} catch (IOException e) {
|
||||||
|
throw new ServiceMayNotContinueException(e);
|
||||||
|
}
|
||||||
|
up.setAdvertisedRefs(refs);
|
||||||
|
return refs;
|
||||||
|
}
|
||||||
|
|
||||||
private HookUtil() {}
|
private HookUtil() {}
|
||||||
}
|
}
|
||||||
|
@@ -0,0 +1,93 @@
|
|||||||
|
// Copyright (C) 2020 The Android Open Source Project
|
||||||
|
//
|
||||||
|
// Licensed under the Apache License, Version 2.0 (the "License");
|
||||||
|
// you may not use this file except in compliance with the License.
|
||||||
|
// You may obtain a copy of the License at
|
||||||
|
//
|
||||||
|
// http://www.apache.org/licenses/LICENSE-2.0
|
||||||
|
//
|
||||||
|
// Unless required by applicable law or agreed to in writing, software
|
||||||
|
// distributed under the License is distributed on an "AS IS" BASIS,
|
||||||
|
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
|
||||||
|
// See the License for the specific language governing permissions and
|
||||||
|
// limitations under the License.
|
||||||
|
|
||||||
|
package com.google.gerrit.server.git;
|
||||||
|
|
||||||
|
import com.google.common.flogger.FluentLogger;
|
||||||
|
import com.google.gerrit.entities.Account;
|
||||||
|
import com.google.gerrit.entities.RefNames;
|
||||||
|
import com.google.gerrit.server.CurrentUser;
|
||||||
|
import java.io.IOException;
|
||||||
|
import java.util.Map;
|
||||||
|
import javax.inject.Inject;
|
||||||
|
import javax.inject.Provider;
|
||||||
|
import javax.inject.Singleton;
|
||||||
|
import org.eclipse.jgit.lib.Ref;
|
||||||
|
import org.eclipse.jgit.lib.RefDatabase;
|
||||||
|
import org.eclipse.jgit.lib.SymbolicRef;
|
||||||
|
import org.eclipse.jgit.transport.AdvertiseRefsHook;
|
||||||
|
import org.eclipse.jgit.transport.ReceivePack;
|
||||||
|
import org.eclipse.jgit.transport.ServiceMayNotContinueException;
|
||||||
|
import org.eclipse.jgit.transport.UploadPack;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Advertises {@code refs/users/self} for authenticated users when interacting with the {@code
|
||||||
|
* All-Users} repository.
|
||||||
|
*/
|
||||||
|
@Singleton
|
||||||
|
public class UsersSelfAdvertiseRefsHook implements AdvertiseRefsHook {
|
||||||
|
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
|
||||||
|
|
||||||
|
private final Provider<CurrentUser> userProvider;
|
||||||
|
|
||||||
|
@Inject
|
||||||
|
public UsersSelfAdvertiseRefsHook(Provider<CurrentUser> userProvider) {
|
||||||
|
this.userProvider = userProvider;
|
||||||
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public void advertiseRefs(UploadPack uploadPack) throws ServiceMayNotContinueException {
|
||||||
|
CurrentUser user = userProvider.get();
|
||||||
|
if (!user.isIdentifiedUser()) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
addSelfSymlinkIfNecessary(
|
||||||
|
uploadPack.getRepository().getRefDatabase(),
|
||||||
|
HookUtil.ensureAllRefsAdvertised(uploadPack),
|
||||||
|
user.getAccountId());
|
||||||
|
}
|
||||||
|
|
||||||
|
@Override
|
||||||
|
public void advertiseRefs(ReceivePack receivePack) throws ServiceMayNotContinueException {
|
||||||
|
CurrentUser user = userProvider.get();
|
||||||
|
if (!user.isIdentifiedUser()) {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
addSelfSymlinkIfNecessary(
|
||||||
|
receivePack.getRepository().getRefDatabase(),
|
||||||
|
HookUtil.ensureAllRefsAdvertised(receivePack),
|
||||||
|
user.getAccountId());
|
||||||
|
}
|
||||||
|
|
||||||
|
private static void addSelfSymlinkIfNecessary(
|
||||||
|
RefDatabase refDatabase, Map<String, Ref> advertisedRefs, Account.Id accountId)
|
||||||
|
throws ServiceMayNotContinueException {
|
||||||
|
String refName = RefNames.refsUsers(accountId);
|
||||||
|
try {
|
||||||
|
Ref r = refDatabase.exactRef(refName);
|
||||||
|
if (r == null) {
|
||||||
|
logger.atWarning().log("User ref %s not found", refName);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
SymbolicRef s = new SymbolicRef(RefNames.REFS_USERS_SELF, r);
|
||||||
|
advertisedRefs.put(s.getName(), s);
|
||||||
|
logger.atFinest().log("Added %s as alias for user ref %s", RefNames.REFS_USERS_SELF, refName);
|
||||||
|
} catch (IOException e) {
|
||||||
|
throw new ServiceMayNotContinueException(e);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
@@ -33,6 +33,7 @@ import com.google.gerrit.metrics.Histogram1;
|
|||||||
import com.google.gerrit.metrics.MetricMaker;
|
import com.google.gerrit.metrics.MetricMaker;
|
||||||
import com.google.gerrit.metrics.Timer1;
|
import com.google.gerrit.metrics.Timer1;
|
||||||
import com.google.gerrit.server.IdentifiedUser;
|
import com.google.gerrit.server.IdentifiedUser;
|
||||||
|
import com.google.gerrit.server.config.AllUsersName;
|
||||||
import com.google.gerrit.server.config.ConfigUtil;
|
import com.google.gerrit.server.config.ConfigUtil;
|
||||||
import com.google.gerrit.server.config.GerritServerConfig;
|
import com.google.gerrit.server.config.GerritServerConfig;
|
||||||
import com.google.gerrit.server.config.ReceiveCommitsExecutor;
|
import com.google.gerrit.server.config.ReceiveCommitsExecutor;
|
||||||
@@ -40,6 +41,7 @@ import com.google.gerrit.server.git.MultiProgressMonitor;
|
|||||||
import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
|
import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
|
||||||
import com.google.gerrit.server.git.ProjectRunnable;
|
import com.google.gerrit.server.git.ProjectRunnable;
|
||||||
import com.google.gerrit.server.git.TransferConfig;
|
import com.google.gerrit.server.git.TransferConfig;
|
||||||
|
import com.google.gerrit.server.git.UsersSelfAdvertiseRefsHook;
|
||||||
import com.google.gerrit.server.logging.Metadata;
|
import com.google.gerrit.server.logging.Metadata;
|
||||||
import com.google.gerrit.server.permissions.PermissionBackend;
|
import com.google.gerrit.server.permissions.PermissionBackend;
|
||||||
import com.google.gerrit.server.permissions.PermissionBackendException;
|
import com.google.gerrit.server.permissions.PermissionBackendException;
|
||||||
@@ -267,6 +269,8 @@ public class AsyncReceiveCommits implements PreReceiveHook {
|
|||||||
ContributorAgreementsChecker contributorAgreements,
|
ContributorAgreementsChecker contributorAgreements,
|
||||||
Metrics metrics,
|
Metrics metrics,
|
||||||
QuotaBackend quotaBackend,
|
QuotaBackend quotaBackend,
|
||||||
|
UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook,
|
||||||
|
AllUsersName allUsersName,
|
||||||
@Named(TIMEOUT_NAME) long timeoutMillis,
|
@Named(TIMEOUT_NAME) long timeoutMillis,
|
||||||
@Assisted ProjectState projectState,
|
@Assisted ProjectState projectState,
|
||||||
@Assisted IdentifiedUser user,
|
@Assisted IdentifiedUser user,
|
||||||
@@ -310,7 +314,8 @@ public class AsyncReceiveCommits implements PreReceiveHook {
|
|||||||
|
|
||||||
allRefsWatcher = new AllRefsWatcher();
|
allRefsWatcher = new AllRefsWatcher();
|
||||||
receivePack.setAdvertiseRefsHook(
|
receivePack.setAdvertiseRefsHook(
|
||||||
ReceiveCommitsAdvertiseRefsHookChain.create(allRefsWatcher, queryProvider, projectName));
|
ReceiveCommitsAdvertiseRefsHookChain.create(
|
||||||
|
allRefsWatcher, usersSelfAdvertiseRefsHook, allUsersName, queryProvider, projectName));
|
||||||
resultChangeIds = new ResultChangeIds();
|
resultChangeIds = new ResultChangeIds();
|
||||||
receiveCommits =
|
receiveCommits =
|
||||||
factory.create(
|
factory.create(
|
||||||
|
@@ -16,8 +16,13 @@ package com.google.gerrit.server.git.receive;
|
|||||||
|
|
||||||
import com.google.common.annotations.VisibleForTesting;
|
import com.google.common.annotations.VisibleForTesting;
|
||||||
import com.google.gerrit.entities.Project;
|
import com.google.gerrit.entities.Project;
|
||||||
|
import com.google.gerrit.server.CurrentUser;
|
||||||
|
import com.google.gerrit.server.config.AllUsersName;
|
||||||
|
import com.google.gerrit.server.config.AllUsersNameProvider;
|
||||||
|
import com.google.gerrit.server.git.UsersSelfAdvertiseRefsHook;
|
||||||
import com.google.gerrit.server.query.change.InternalChangeQuery;
|
import com.google.gerrit.server.query.change.InternalChangeQuery;
|
||||||
import com.google.inject.Provider;
|
import com.google.inject.Provider;
|
||||||
|
import com.google.inject.util.Providers;
|
||||||
import java.util.ArrayList;
|
import java.util.ArrayList;
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
import org.eclipse.jgit.transport.AdvertiseRefsHook;
|
import org.eclipse.jgit.transport.AdvertiseRefsHook;
|
||||||
@@ -34,9 +39,17 @@ public class ReceiveCommitsAdvertiseRefsHookChain {
|
|||||||
*/
|
*/
|
||||||
public static AdvertiseRefsHook create(
|
public static AdvertiseRefsHook create(
|
||||||
AllRefsWatcher allRefsWatcher,
|
AllRefsWatcher allRefsWatcher,
|
||||||
|
UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook,
|
||||||
|
AllUsersName allUsersName,
|
||||||
Provider<InternalChangeQuery> queryProvider,
|
Provider<InternalChangeQuery> queryProvider,
|
||||||
Project.NameKey projectName) {
|
Project.NameKey projectName) {
|
||||||
return create(allRefsWatcher, queryProvider, projectName, false);
|
return create(
|
||||||
|
allRefsWatcher,
|
||||||
|
usersSelfAdvertiseRefsHook,
|
||||||
|
allUsersName,
|
||||||
|
queryProvider,
|
||||||
|
projectName,
|
||||||
|
false);
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -47,12 +60,20 @@ public class ReceiveCommitsAdvertiseRefsHookChain {
|
|||||||
*/
|
*/
|
||||||
@VisibleForTesting
|
@VisibleForTesting
|
||||||
public static AdvertiseRefsHook createForTest(
|
public static AdvertiseRefsHook createForTest(
|
||||||
Provider<InternalChangeQuery> queryProvider, Project.NameKey projectName) {
|
Provider<InternalChangeQuery> queryProvider, Project.NameKey projectName, CurrentUser user) {
|
||||||
return create(new AllRefsWatcher(), queryProvider, projectName, true);
|
return create(
|
||||||
|
new AllRefsWatcher(),
|
||||||
|
new UsersSelfAdvertiseRefsHook(Providers.of(user)),
|
||||||
|
new AllUsersName(AllUsersNameProvider.DEFAULT),
|
||||||
|
queryProvider,
|
||||||
|
projectName,
|
||||||
|
true);
|
||||||
}
|
}
|
||||||
|
|
||||||
private static AdvertiseRefsHook create(
|
private static AdvertiseRefsHook create(
|
||||||
AllRefsWatcher allRefsWatcher,
|
AllRefsWatcher allRefsWatcher,
|
||||||
|
UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook,
|
||||||
|
AllUsersName allUsersName,
|
||||||
Provider<InternalChangeQuery> queryProvider,
|
Provider<InternalChangeQuery> queryProvider,
|
||||||
Project.NameKey projectName,
|
Project.NameKey projectName,
|
||||||
boolean skipHackPushNegotiateHook) {
|
boolean skipHackPushNegotiateHook) {
|
||||||
@@ -62,6 +83,9 @@ public class ReceiveCommitsAdvertiseRefsHookChain {
|
|||||||
if (!skipHackPushNegotiateHook) {
|
if (!skipHackPushNegotiateHook) {
|
||||||
advHooks.add(new HackPushNegotiateHook());
|
advHooks.add(new HackPushNegotiateHook());
|
||||||
}
|
}
|
||||||
|
if (projectName.equals(allUsersName)) {
|
||||||
|
advHooks.add(usersSelfAdvertiseRefsHook);
|
||||||
|
}
|
||||||
return AdvertiseRefsHookChain.newChain(advHooks);
|
return AdvertiseRefsHookChain.newChain(advHooks);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
@@ -68,7 +68,6 @@ import org.eclipse.jgit.lib.Config;
|
|||||||
import org.eclipse.jgit.lib.Constants;
|
import org.eclipse.jgit.lib.Constants;
|
||||||
import org.eclipse.jgit.lib.Ref;
|
import org.eclipse.jgit.lib.Ref;
|
||||||
import org.eclipse.jgit.lib.Repository;
|
import org.eclipse.jgit.lib.Repository;
|
||||||
import org.eclipse.jgit.lib.SymbolicRef;
|
|
||||||
|
|
||||||
class DefaultRefFilter {
|
class DefaultRefFilter {
|
||||||
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
|
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
|
||||||
@@ -206,10 +205,6 @@ class DefaultRefFilter {
|
|||||||
throws PermissionBackendException {
|
throws PermissionBackendException {
|
||||||
logger.atFinest().log("Filter refs (refs = %s)", refs);
|
logger.atFinest().log("Filter refs (refs = %s)", refs);
|
||||||
|
|
||||||
if (projectState.isAllUsers()) {
|
|
||||||
refs = addUsersSelfSymref(repo, refs);
|
|
||||||
}
|
|
||||||
|
|
||||||
// TODO(hiesel): Remove when optimization is done.
|
// TODO(hiesel): Remove when optimization is done.
|
||||||
boolean hasReadOnRefsStar =
|
boolean hasReadOnRefsStar =
|
||||||
checkProjectPermission(permissionBackendForProject, ProjectPermission.READ);
|
checkProjectPermission(permissionBackendForProject, ProjectPermission.READ);
|
||||||
@@ -395,32 +390,6 @@ class DefaultRefFilter {
|
|||||||
return refs;
|
return refs;
|
||||||
}
|
}
|
||||||
|
|
||||||
private Map<String, Ref> addUsersSelfSymref(Repository repo, Map<String, Ref> refs)
|
|
||||||
throws PermissionBackendException {
|
|
||||||
if (user.isIdentifiedUser()) {
|
|
||||||
// User self symref is already there
|
|
||||||
if (refs.containsKey(REFS_USERS_SELF)) {
|
|
||||||
return refs;
|
|
||||||
}
|
|
||||||
String refName = RefNames.refsUsers(user.getAccountId());
|
|
||||||
try {
|
|
||||||
Ref r = repo.exactRef(refName);
|
|
||||||
if (r == null) {
|
|
||||||
logger.atWarning().log("User ref %s not found", refName);
|
|
||||||
return refs;
|
|
||||||
}
|
|
||||||
|
|
||||||
SymbolicRef s = new SymbolicRef(REFS_USERS_SELF, r);
|
|
||||||
refs = new HashMap<>(refs);
|
|
||||||
refs.put(s.getName(), s);
|
|
||||||
logger.atFinest().log("Added %s as alias for user ref %s", REFS_USERS_SELF, refName);
|
|
||||||
} catch (IOException e) {
|
|
||||||
throw new PermissionBackendException(e);
|
|
||||||
}
|
|
||||||
}
|
|
||||||
return refs;
|
|
||||||
}
|
|
||||||
|
|
||||||
private boolean visible(Repository repo, Change.Id changeId) throws PermissionBackendException {
|
private boolean visible(Repository repo, Change.Id changeId) throws PermissionBackendException {
|
||||||
if (visibleChanges == null) {
|
if (visibleChanges == null) {
|
||||||
if (changeCache == null) {
|
if (changeCache == null) {
|
||||||
|
@@ -24,6 +24,7 @@ import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
|
|||||||
import com.google.gerrit.server.git.TracingHook;
|
import com.google.gerrit.server.git.TracingHook;
|
||||||
import com.google.gerrit.server.git.TransferConfig;
|
import com.google.gerrit.server.git.TransferConfig;
|
||||||
import com.google.gerrit.server.git.UploadPackInitializer;
|
import com.google.gerrit.server.git.UploadPackInitializer;
|
||||||
|
import com.google.gerrit.server.git.UsersSelfAdvertiseRefsHook;
|
||||||
import com.google.gerrit.server.git.validators.UploadValidationException;
|
import com.google.gerrit.server.git.validators.UploadValidationException;
|
||||||
import com.google.gerrit.server.git.validators.UploadValidators;
|
import com.google.gerrit.server.git.validators.UploadValidators;
|
||||||
import com.google.gerrit.server.logging.TraceContext;
|
import com.google.gerrit.server.logging.TraceContext;
|
||||||
@@ -52,6 +53,7 @@ final class Upload extends AbstractGitCommand {
|
|||||||
@Inject private PluginSetContext<RequestListener> requestListeners;
|
@Inject private PluginSetContext<RequestListener> requestListeners;
|
||||||
@Inject private UploadValidators.Factory uploadValidatorsFactory;
|
@Inject private UploadValidators.Factory uploadValidatorsFactory;
|
||||||
@Inject private PermissionBackend permissionBackend;
|
@Inject private PermissionBackend permissionBackend;
|
||||||
|
@Inject private UsersSelfAdvertiseRefsHook usersSelfAdvertiseRefsHook;
|
||||||
|
|
||||||
private PackStatistics stats;
|
private PackStatistics stats;
|
||||||
|
|
||||||
@@ -73,6 +75,9 @@ final class Upload extends AbstractGitCommand {
|
|||||||
up.setPackConfig(config.getPackConfig());
|
up.setPackConfig(config.getPackConfig());
|
||||||
up.setTimeout(config.getTimeout());
|
up.setTimeout(config.getTimeout());
|
||||||
up.setPostUploadHook(PostUploadHookChain.newChain(Lists.newArrayList(postUploadHooks)));
|
up.setPostUploadHook(PostUploadHookChain.newChain(Lists.newArrayList(postUploadHooks)));
|
||||||
|
if (projectState.isAllUsers()) {
|
||||||
|
up.setAdvertiseRefsHook(usersSelfAdvertiseRefsHook);
|
||||||
|
}
|
||||||
if (extraParameters != null) {
|
if (extraParameters != null) {
|
||||||
up.setExtraParameters(ImmutableList.copyOf(extraParameters));
|
up.setExtraParameters(ImmutableList.copyOf(extraParameters));
|
||||||
}
|
}
|
||||||
|
@@ -41,6 +41,7 @@ import static com.google.gerrit.truth.ConfigSubject.assertThat;
|
|||||||
import static java.nio.charset.StandardCharsets.UTF_8;
|
import static java.nio.charset.StandardCharsets.UTF_8;
|
||||||
import static java.util.Objects.requireNonNull;
|
import static java.util.Objects.requireNonNull;
|
||||||
import static java.util.concurrent.TimeUnit.SECONDS;
|
import static java.util.concurrent.TimeUnit.SECONDS;
|
||||||
|
import static java.util.stream.Collectors.toList;
|
||||||
import static java.util.stream.Collectors.toSet;
|
import static java.util.stream.Collectors.toSet;
|
||||||
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
|
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
|
||||||
|
|
||||||
@@ -129,8 +130,6 @@ import com.google.gerrit.server.git.meta.MetaDataUpdate;
|
|||||||
import com.google.gerrit.server.index.account.AccountIndexer;
|
import com.google.gerrit.server.index.account.AccountIndexer;
|
||||||
import com.google.gerrit.server.index.account.StalenessChecker;
|
import com.google.gerrit.server.index.account.StalenessChecker;
|
||||||
import com.google.gerrit.server.notedb.Sequences;
|
import com.google.gerrit.server.notedb.Sequences;
|
||||||
import com.google.gerrit.server.permissions.PermissionBackend;
|
|
||||||
import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
|
|
||||||
import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
|
import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics;
|
||||||
import com.google.gerrit.server.plugincontext.PluginSetContext;
|
import com.google.gerrit.server.plugincontext.PluginSetContext;
|
||||||
import com.google.gerrit.server.project.ProjectConfig;
|
import com.google.gerrit.server.project.ProjectConfig;
|
||||||
@@ -164,6 +163,7 @@ import java.util.concurrent.atomic.AtomicInteger;
|
|||||||
import org.bouncycastle.bcpg.ArmoredOutputStream;
|
import org.bouncycastle.bcpg.ArmoredOutputStream;
|
||||||
import org.bouncycastle.openpgp.PGPPublicKey;
|
import org.bouncycastle.openpgp.PGPPublicKey;
|
||||||
import org.bouncycastle.openpgp.PGPPublicKeyRing;
|
import org.bouncycastle.openpgp.PGPPublicKeyRing;
|
||||||
|
import org.eclipse.jgit.api.Git;
|
||||||
import org.eclipse.jgit.api.errors.TransportException;
|
import org.eclipse.jgit.api.errors.TransportException;
|
||||||
import org.eclipse.jgit.errors.ConfigInvalidException;
|
import org.eclipse.jgit.errors.ConfigInvalidException;
|
||||||
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
|
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
|
||||||
@@ -213,7 +213,6 @@ public class AccountIT extends AbstractDaemonTest {
|
|||||||
@Inject private Sequences seq;
|
@Inject private Sequences seq;
|
||||||
@Inject private StalenessChecker stalenessChecker;
|
@Inject private StalenessChecker stalenessChecker;
|
||||||
@Inject private VersionedAuthorizedKeys.Accessor authorizedKeys;
|
@Inject private VersionedAuthorizedKeys.Accessor authorizedKeys;
|
||||||
@Inject private PermissionBackend permissionBackend;
|
|
||||||
@Inject private ExtensionRegistry extensionRegistry;
|
@Inject private ExtensionRegistry extensionRegistry;
|
||||||
|
|
||||||
@Inject protected Emails emails;
|
@Inject protected Emails emails;
|
||||||
@@ -1470,14 +1469,11 @@ public class AccountIT extends AbstractDaemonTest {
|
|||||||
|
|
||||||
@Test
|
@Test
|
||||||
public void refsUsersSelfIsAdvertised() throws Exception {
|
public void refsUsersSelfIsAdvertised() throws Exception {
|
||||||
try (Repository allUsersRepo = repoManager.openRepository(allUsers)) {
|
TestRepository<?> testRepository = cloneProject(allUsers, user);
|
||||||
assertThat(
|
try (Git git = testRepository.git()) {
|
||||||
permissionBackend
|
List<String> advertisedRefs =
|
||||||
.currentUser()
|
git.lsRemote().call().stream().map(Ref::getName).collect(toList());
|
||||||
.project(allUsers)
|
assertThat(advertisedRefs).contains(RefNames.REFS_USERS_SELF);
|
||||||
.filter(ImmutableList.of(), allUsersRepo, RefFilterOptions.defaults())
|
|
||||||
.keySet())
|
|
||||||
.containsExactly(RefNames.REFS_USERS_SELF);
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@@ -62,6 +62,20 @@ public class PushAccountIT extends AbstractDaemonTest {
|
|||||||
|
|
||||||
@ConfigSuite.Default
|
@ConfigSuite.Default
|
||||||
public static Config enableSignedPushConfig() {
|
public static Config enableSignedPushConfig() {
|
||||||
|
return defaultConfig();
|
||||||
|
}
|
||||||
|
|
||||||
|
@ConfigSuite.Config
|
||||||
|
public static Config disableInMemoryRefCache() {
|
||||||
|
// Run these tests for both enabled and disabled in-memory ref caches. This is an implementation
|
||||||
|
// detail of ReceiveCommits that makes the logic either base it's computation on previously
|
||||||
|
// advertised refs or a make it query a ref database.
|
||||||
|
Config cfg = defaultConfig();
|
||||||
|
cfg.setBoolean("receive", null, "enableInMemoryRefCache", false);
|
||||||
|
return cfg;
|
||||||
|
}
|
||||||
|
|
||||||
|
private static Config defaultConfig() {
|
||||||
Config cfg = new Config();
|
Config cfg = new Config();
|
||||||
cfg.setBoolean("receive", null, "enableSignedPush", true);
|
cfg.setBoolean("receive", null, "enableSignedPush", true);
|
||||||
|
|
||||||
|
@@ -1345,7 +1345,6 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
|
|||||||
|
|
||||||
List<String> expectedNonMetaRefs =
|
List<String> expectedNonMetaRefs =
|
||||||
ImmutableList.of(
|
ImmutableList.of(
|
||||||
RefNames.REFS_USERS_SELF,
|
|
||||||
RefNames.refsUsers(admin.id()),
|
RefNames.refsUsers(admin.id()),
|
||||||
RefNames.refsUsers(user.id()),
|
RefNames.refsUsers(user.id()),
|
||||||
RefNames.REFS_EXTERNAL_IDS,
|
RefNames.REFS_EXTERNAL_IDS,
|
||||||
@@ -1446,7 +1445,8 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
|
|||||||
private TestRefAdvertiser.Result getReceivePackRefs() throws Exception {
|
private TestRefAdvertiser.Result getReceivePackRefs() throws Exception {
|
||||||
try (Repository repo = repoManager.openRepository(project)) {
|
try (Repository repo = repoManager.openRepository(project)) {
|
||||||
AdvertiseRefsHook adv =
|
AdvertiseRefsHook adv =
|
||||||
ReceiveCommitsAdvertiseRefsHookChain.createForTest(queryProvider, project);
|
ReceiveCommitsAdvertiseRefsHookChain.createForTest(
|
||||||
|
queryProvider, project, identifiedUserFactory.create(admin.id()));
|
||||||
ReceivePack rp = new ReceivePack(repo);
|
ReceivePack rp = new ReceivePack(repo);
|
||||||
rp.setAdvertiseRefsHook(adv);
|
rp.setAdvertiseRefsHook(adv);
|
||||||
TestRefAdvertiser advertiser = new TestRefAdvertiser(repo);
|
TestRefAdvertiser advertiser = new TestRefAdvertiser(repo);
|
||||||
|
Reference in New Issue
Block a user