Implement PermissionAwareRefDatabase

Gerrit had some trouble with enabling Git Protocol V2 with refs-in-wants
support at the end of 2018. The origin for this trouble was that JGit
and Gerrit maintainers had no defined API for how visiblity is enforced.
In fact, JGit doesn't promise any visibility enforcements.

Gerrit previously used an AdvertiseRefsHook to filter refs by visibility
before advertising them. Only advertised refs were fetchable (given that
no additional settings were provided). With the Git Protocol advancing,
this assumption is not safe anymore and we need stronger guarantees to
make sure Gerrit and JGit play well together when it comes to security.

This change is a proposal to solve the dilemma by providing a custom
repository to JGit. The repository provides a RefDatabase that is
permission-aware and will only ever return refs that the user has access
to.

This way, Gerrit is resilient against whatever changes are made in JGit
because we never expose invisible refs or rely on any JGit filter
functionality.

JGit's Repository object was unfortunately not meant to be extended and
wrapped. It is an abstract class (not an interface) that has two direct
implementations in JGit: FileRepository and DFSRepository (abstract). We
can either subclass Repository directly - which is what this change is
doing - or subclass the two implementations separately.

Best case, we would change Repository to be an interface instead and
stop bothering, but this seems unfeasible at least for now.

This change preserves advertise refs hook: DefaultAdvertiseRefsHook. So
that the checks are performed twice. Follow-up change will introduce a
configuration option to select between DefaultAdvertiseRefsHook or
permission aware ref database.

Change-Id: Id10f74f20b79a2b7851fc462d6f64acb0c7413a4

java/com/google/gerrit/acceptance/InProcessProtocol.java

@@ -32,6 +32,7 @@ import com.google.gerrit.server.RemotePeer;
 import com.google.gerrit.server.RequestCleanup;
 import com.google.gerrit.server.config.GerritRequestModule;
 import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
+import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
 import com.google.gerrit.server.git.ReceivePackInitializer;
 import com.google.gerrit.server.git.TransferConfig;
 import com.google.gerrit.server.git.UploadPackInitializer;
@@ -247,12 +248,15 @@ class InProcessProtocol extends TestProtocol<Context> {
       if (projectState == null) {
         throw new RuntimeException("can't load project state for " + req.project.get());
       }
-      UploadPack up = new UploadPack(repo);
+      Repository permissionAwareRepository = PermissionAwareRepositoryManager.wrap(repo, perm);
+      UploadPack up = new UploadPack(permissionAwareRepository);
       up.setPackConfig(transferConfig.getPackConfig());
       up.setTimeout(transferConfig.getTimeout());
       up.setAdvertiseRefsHook(new DefaultAdvertiseRefsHook(perm, RefFilterOptions.defaults()));
       List<PreUploadHook> hooks = Lists.newArrayList(preUploadHooks);
-      hooks.add(uploadValidatorsFactory.create(projectState.getProject(), repo, "localhost-test"));
+      hooks.add(
+          uploadValidatorsFactory.create(
+              projectState.getProject(), permissionAwareRepository, "localhost-test"));
       up.setPreUploadHook(PreUploadHookChain.newChain(hooks));
       uploadPackInitializers.runEach(initializer -> initializer.init(req.project, up));
       return up;

java/com/google/gerrit/httpd/GitOverHttpServlet.java

@@ -32,6 +32,7 @@ import com.google.gerrit.server.audit.HttpAuditEvent;
 import com.google.gerrit.server.cache.CacheModule;
 import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
 import com.google.gerrit.server.git.GitRepositoryManager;
+import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
 import com.google.gerrit.server.git.TransferConfig;
 import com.google.gerrit.server.git.UploadPackInitializer;
 import com.google.gerrit.server.git.receive.AsyncReceiveCommits;
@@ -310,27 +311,33 @@ public class GitOverHttpServlet extends GitServlet {
     private final DynamicSet<PreUploadHook> preUploadHooks;
     private final DynamicSet<PostUploadHook> postUploadHooks;
     private final PluginSetContext<UploadPackInitializer> uploadPackInitializers;
+    private final PermissionBackend permissionBackend;
 
     @Inject
     UploadFactory(
         TransferConfig tc,
         DynamicSet<PreUploadHook> preUploadHooks,
         DynamicSet<PostUploadHook> postUploadHooks,
-        PluginSetContext<UploadPackInitializer> uploadPackInitializers) {
+        PluginSetContext<UploadPackInitializer> uploadPackInitializers,
+        PermissionBackend permissionBackend) {
       this.config = tc;
       this.preUploadHooks = preUploadHooks;
       this.postUploadHooks = postUploadHooks;
       this.uploadPackInitializers = uploadPackInitializers;
+      this.permissionBackend = permissionBackend;
     }
 
     @Override
     public UploadPack create(HttpServletRequest req, Repository repo) {
-      UploadPack up = new UploadPack(repo);
+      ProjectState state = (ProjectState) req.getAttribute(ATT_STATE);
+      UploadPack up =
+          new UploadPack(
+              PermissionAwareRepositoryManager.wrap(
+                  repo, permissionBackend.currentUser().project(state.getNameKey())));
       up.setPackConfig(config.getPackConfig());
       up.setTimeout(config.getTimeout());
       up.setPreUploadHook(PreUploadHookChain.newChain(Lists.newArrayList(preUploadHooks)));
       up.setPostUploadHook(PostUploadHookChain.newChain(Lists.newArrayList(postUploadHooks)));
-      ProjectState state = (ProjectState) req.getAttribute(ATT_STATE);
       uploadPackInitializers.runEach(initializer -> initializer.init(state.getNameKey(), up));
       return up;
     }

java/com/google/gerrit/server/git/DelegateRefDatabase.java

@@ -0,0 +1,86 @@
+// Copyright (C) 2019 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 java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefDatabase;
+import org.eclipse.jgit.lib.RefRename;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Wrapper around {@link RefDatabase} that delegates all calls to the wrapped {@link RefDatabase}.
+ */
+public class DelegateRefDatabase extends RefDatabase {
+
+  private Repository delegate;
+
+  DelegateRefDatabase(Repository delegate) {
+    this.delegate = delegate;
+  }
+
+  @Override
+  public void create() throws IOException {
+    delegate.getRefDatabase().create();
+  }
+
+  @Override
+  public void close() {
+    delegate.close();
+  }
+
+  @Override
+  public boolean isNameConflicting(String name) throws IOException {
+    return delegate.getRefDatabase().isNameConflicting(name);
+  }
+
+  @Override
+  public RefUpdate newUpdate(String name, boolean detach) throws IOException {
+    return delegate.getRefDatabase().newUpdate(name, detach);
+  }
+
+  @Override
+  public RefRename newRename(String fromName, String toName) throws IOException {
+    return delegate.getRefDatabase().newRename(fromName, toName);
+  }
+
+  @Override
+  public Ref exactRef(String name) throws IOException {
+    return delegate.getRefDatabase().exactRef(name);
+  }
+
+  @SuppressWarnings("deprecation")
+  @Override
+  public Map<String, Ref> getRefs(String prefix) throws IOException {
+    return delegate.getRefDatabase().getRefs(prefix);
+  }
+
+  @Override
+  public List<Ref> getAdditionalRefs() throws IOException {
+    return delegate.getRefDatabase().getAdditionalRefs();
+  }
+
+  @Override
+  public Ref peel(Ref ref) throws IOException {
+    return delegate.getRefDatabase().peel(ref);
+  }
+
+  Repository getDelegate() {
+    return delegate;
+  }
+}

java/com/google/gerrit/server/git/DelegateRepository.java

@@ -0,0 +1,89 @@
+// Copyright (C) 2019 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 java.io.IOException;
+import org.eclipse.jgit.attributes.AttributesNodeProvider;
+import org.eclipse.jgit.lib.BaseRepositoryBuilder;
+import org.eclipse.jgit.lib.ObjectDatabase;
+import org.eclipse.jgit.lib.RefDatabase;
+import org.eclipse.jgit.lib.ReflogReader;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.lib.StoredConfig;
+
+/** Wrapper around {@link Repository} that delegates all calls to the wrapped {@link Repository}. */
+class DelegateRepository extends Repository {
+
+  private final Repository delegate;
+
+  DelegateRepository(Repository delegate) {
+    super(toBuilder(delegate));
+    this.delegate = delegate;
+  }
+
+  @Override
+  public void create(boolean bare) throws IOException {
+    delegate.create(bare);
+  }
+
+  @Override
+  public String getIdentifier() {
+    return delegate.getIdentifier();
+  }
+
+  @Override
+  public ObjectDatabase getObjectDatabase() {
+    return delegate.getObjectDatabase();
+  }
+
+  @Override
+  public RefDatabase getRefDatabase() {
+    return delegate.getRefDatabase();
+  }
+
+  @Override
+  public StoredConfig getConfig() {
+    return delegate.getConfig();
+  }
+
+  @Override
+  public AttributesNodeProvider createAttributesNodeProvider() {
+    return delegate.createAttributesNodeProvider();
+  }
+
+  @Override
+  public void scanForRepoChanges() throws IOException {
+    delegate.scanForRepoChanges();
+  }
+
+  @Override
+  public void notifyIndexChanged(boolean internal) {
+    delegate.notifyIndexChanged(internal);
+  }
+
+  @Override
+  public ReflogReader getReflogReader(String refName) throws IOException {
+    return delegate.getReflogReader(refName);
+  }
+
+  @SuppressWarnings("rawtypes")
+  private static BaseRepositoryBuilder toBuilder(Repository repo) {
+    if (!repo.isBare()) {
+      throw new IllegalArgumentException("non-bare repository is not supported");
+    }
+
+    return new BaseRepositoryBuilder<>().setFS(repo.getFS()).setGitDir(repo.getDirectory());
+  }
+}

java/com/google/gerrit/server/git/PermissionAwareReadOnlyRefDatabase.java

@@ -0,0 +1,159 @@
+// Copyright (C) 2019 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 static java.util.stream.Collectors.toList;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.inject.Inject;
+import java.io.IOException;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.eclipse.jgit.annotations.NonNull;
+import org.eclipse.jgit.annotations.Nullable;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefRename;
+import org.eclipse.jgit.lib.RefUpdate;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Wrapper around {@link DelegateRefDatabase} that filters all refs using {@link
+ * com.google.gerrit.server.permissions.PermissionBackend}.
+ */
+public class PermissionAwareReadOnlyRefDatabase extends DelegateRefDatabase {
+
+  private final PermissionBackend.ForProject forProject;
+
+  @Inject
+  PermissionAwareReadOnlyRefDatabase(
+      Repository delegateRepository, PermissionBackend.ForProject forProject) {
+    super(delegateRepository);
+    this.forProject = forProject;
+  }
+
+  @Override
+  public boolean isNameConflicting(String name) {
+    throw new UnsupportedOperationException("PermissionAwareReadOnlyRefDatabase is read-only");
+  }
+
+  @Override
+  public RefUpdate newUpdate(String name, boolean detach) {
+    throw new UnsupportedOperationException("PermissionAwareReadOnlyRefDatabase is read-only");
+  }
+
+  @Override
+  public RefRename newRename(String fromName, String toName) {
+    throw new UnsupportedOperationException("PermissionAwareReadOnlyRefDatabase is read-only");
+  }
+
+  @Override
+  public Ref exactRef(String name) throws IOException {
+    Ref ref = getDelegate().getRefDatabase().exactRef(name);
+    if (ref == null) {
+      return null;
+    }
+
+    Map<String, Ref> result;
+    try {
+      result =
+          forProject.filter(ImmutableMap.of(name, ref), getDelegate(), RefFilterOptions.defaults());
+    } catch (PermissionBackendException e) {
+      if (e.getCause() instanceof IOException) {
+        throw (IOException) e.getCause();
+      }
+      throw new IOException(e);
+    }
+    if (result.isEmpty()) {
+      return null;
+    }
+
+    Preconditions.checkState(
+        result.size() == 1, "Only one element expected, but was: " + result.size());
+    return Iterables.getOnlyElement(result.values());
+  }
+
+  @SuppressWarnings("deprecation")
+  @Override
+  public Map<String, Ref> getRefs(String prefix) throws IOException {
+    Map<String, Ref> refs = getDelegate().getRefDatabase().getRefs(prefix);
+    if (refs.isEmpty()) {
+      return refs;
+    }
+
+    Map<String, Ref> result;
+    try {
+      result = forProject.filter(refs, getDelegate(), RefFilterOptions.defaults());
+    } catch (PermissionBackendException e) {
+      throw new IOException("");
+    }
+    return result;
+  }
+
+  @Override
+  public List<Ref> getRefsByPrefix(String prefix) throws IOException {
+    Map<String, Ref> coarseRefs;
+    int lastSlash = prefix.lastIndexOf('/');
+    if (lastSlash == -1) {
+      coarseRefs = getRefs(ALL);
+    } else {
+      coarseRefs = getRefs(prefix.substring(0, lastSlash + 1));
+    }
+
+    List<Ref> result;
+    if (lastSlash + 1 == prefix.length()) {
+      result = coarseRefs.values().stream().collect(toList());
+    } else {
+      String p = prefix.substring(lastSlash + 1);
+      result =
+          coarseRefs.entrySet().stream()
+              .filter(e -> e.getKey().startsWith(p))
+              .map(e -> e.getValue())
+              .collect(toList());
+    }
+    return Collections.unmodifiableList(result);
+  }
+
+  @Override
+  @NonNull
+  public Map<String, Ref> exactRef(String... refs) throws IOException {
+    Map<String, Ref> result = new HashMap<>(refs.length);
+    for (String name : refs) {
+      Ref ref = exactRef(name);
+      if (ref != null) {
+        result.put(name, ref);
+      }
+    }
+    return result;
+  }
+
+  @Override
+  @Nullable
+  public Ref firstExactRef(String... refs) throws IOException {
+    for (String name : refs) {
+      Ref ref = exactRef(name);
+      if (ref != null) {
+        return ref;
+      }
+    }
+    return null;
+  }
+}

java/com/google/gerrit/server/git/PermissionAwareRepository.java

@@ -0,0 +1,39 @@
+// Copyright (C) 2019 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.gerrit.server.permissions.PermissionBackend;
+import org.eclipse.jgit.lib.RefDatabase;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Wrapper around {@link DelegateRepository} that overwrites {@link #getRefDatabase()} to return a
+ * {@link PermissionAwareReadOnlyRefDatabase}.
+ */
+public class PermissionAwareRepository extends DelegateRepository {
+
+  private final PermissionAwareReadOnlyRefDatabase permissionAwareReadOnlyRefDatabase;
+
+  public PermissionAwareRepository(Repository delegate, PermissionBackend.ForProject forProject) {
+    super(delegate);
+    this.permissionAwareReadOnlyRefDatabase =
+        new PermissionAwareReadOnlyRefDatabase(delegate, forProject);
+  }
+
+  @Override
+  public RefDatabase getRefDatabase() {
+    return permissionAwareReadOnlyRefDatabase;
+  }
+}

java/com/google/gerrit/server/git/PermissionAwareRepositoryManager.java

@@ -0,0 +1,32 @@
+// Copyright (C) 2019 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.base.Preconditions;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import org.eclipse.jgit.lib.Repository;
+
+/**
+ * Wraps and unwraps existing repositories and makes them permission-aware by returning a {@link
+ * PermissionAwareReadOnlyRefDatabase}.
+ */
+public class PermissionAwareRepositoryManager {
+  public static Repository wrap(Repository delegate, PermissionBackend.ForProject forProject) {
+    Preconditions.checkState(
+        !(delegate instanceof PermissionAwareRepository),
+        "Cannot wrap PermissionAwareRepository instance");
+    return new PermissionAwareRepository(delegate, forProject);
+  }
+}

java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java

@@ -37,6 +37,7 @@ import com.google.gerrit.server.config.ConfigUtil;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.config.ReceiveCommitsExecutor;
 import com.google.gerrit.server.git.MultiProgressMonitor;
+import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
 import com.google.gerrit.server.git.ProjectRunnable;
 import com.google.gerrit.server.git.TransferConfig;
 import com.google.gerrit.server.logging.Metadata;
@@ -281,9 +282,11 @@ public class AsyncReceiveCommits implements PreReceiveHook {
     this.user = user;
     this.repo = repo;
     this.metrics = metrics;
-
+    // If the user lacks READ permission, some references may be filtered and hidden from view.
+    // Check objects mentioned inside the incoming pack file are reachable from visible refs.
     Project.NameKey projectName = projectState.getNameKey();
-    receivePack = new ReceivePack(repo);
+    this.perm = permissionBackend.user(user).project(projectName);
+    receivePack = new ReceivePack(PermissionAwareRepositoryManager.wrap(repo, perm));
     receivePack.setAllowCreates(true);
     receivePack.setAllowDeletes(true);
     receivePack.setAllowNonFastForwards(true);
@@ -296,9 +299,6 @@ public class AsyncReceiveCommits implements PreReceiveHook {
     receivePack.setPreReceiveHook(this);
     receivePack.setPostReceiveHook(lazyPostReceive.create(user, projectName));
 
-    // If the user lacks READ permission, some references may be filtered and hidden from view.
-    // Check objects mentioned inside the incoming pack file are reachable from visible refs.
-    this.perm = permissionBackend.user(user).project(projectName);
     try {
       projectState.checkStatePermitsRead();
       this.perm.check(ProjectPermission.READ);
@@ -314,7 +314,7 @@ public class AsyncReceiveCommits implements PreReceiveHook {
     resultChangeIds = new ResultChangeIds();
     receiveCommits =
         factory.create(
-            projectState, user, receivePack, allRefsWatcher, messageSender, resultChangeIds);
+            projectState, user, receivePack, repo, allRefsWatcher, messageSender, resultChangeIds);
     receiveCommits.init();
     QuotaResponse.Aggregated availableTokens =
         quotaBackend.user(user).project(projectName).availableTokens(REPOSITORY_SIZE_GROUP);

java/com/google/gerrit/server/git/receive/ReceiveCommits.java

@@ -248,6 +248,7 @@ class ReceiveCommits {
         ProjectState projectState,
         IdentifiedUser user,
         ReceivePack receivePack,
+        Repository repository,
         AllRefsWatcher allRefsWatcher,
         MessageSender messageSender,
         ResultChangeIds resultChangeIds);
@@ -422,6 +423,7 @@ class ReceiveCommits {
       @Assisted ProjectState projectState,
       @Assisted IdentifiedUser user,
       @Assisted ReceivePack rp,
+      @Assisted Repository repository,
       @Assisted AllRefsWatcher allRefsWatcher,
       @Nullable @Assisted MessageSender messageSender,
       @Assisted ResultChangeIds resultChangeIds)
@@ -471,13 +473,15 @@ class ReceiveCommits {
     this.projectState = projectState;
     this.user = user;
     this.receivePack = rp;
+    // This repository instance in unwrapped, while the repository instance in
+    // receivePack.getRepo() is wrapped in PermissionAwareRepository instance.
+    this.repo = repository;
 
     // Immutable fields derived from constructor arguments.
-    repo = rp.getRepository();
     project = projectState.getProject();
     labelTypes = projectState.getLabelTypes();
     permissions = permissionBackend.user(user).project(project.getNameKey());
-    rejectCommits = BanCommit.loadRejectCommitsMap(rp.getRepository(), rp.getRevWalk());
+    rejectCommits = BanCommit.loadRejectCommitsMap(repo, rp.getRevWalk());
 
     // Collections populated during processing.
     errors = MultimapBuilder.linkedHashKeys().arrayListValues().build();

java/com/google/gerrit/server/permissions/DefaultRefFilter.java

@@ -399,6 +399,10 @@ class DefaultRefFilter {
   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);

java/com/google/gerrit/sshd/commands/Upload.java

@@ -20,6 +20,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
 import com.google.gerrit.server.RequestInfo;
 import com.google.gerrit.server.RequestListener;
 import com.google.gerrit.server.git.DefaultAdvertiseRefsHook;
+import com.google.gerrit.server.git.PermissionAwareRepositoryManager;
 import com.google.gerrit.server.git.TransferConfig;
 import com.google.gerrit.server.git.UploadPackInitializer;
 import com.google.gerrit.server.git.validators.UploadValidationException;
@@ -35,6 +36,7 @@ import com.google.gerrit.sshd.SshSession;
 import com.google.inject.Inject;
 import java.io.IOException;
 import java.util.List;
+import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.transport.PostUploadHook;
 import org.eclipse.jgit.transport.PostUploadHookChain;
 import org.eclipse.jgit.transport.PreUploadHook;
@@ -65,7 +67,8 @@ final class Upload extends AbstractGitCommand {
       throw new Failure(1, "fatal: unable to check permissions " + e);
     }
 
-    final UploadPack up = new UploadPack(repo);
+    Repository permissionAwareRepository = PermissionAwareRepositoryManager.wrap(repo, perm);
+    final UploadPack up = new UploadPack(permissionAwareRepository);
     up.setAdvertiseRefsHook(new DefaultAdvertiseRefsHook(perm, RefFilterOptions.defaults()));
     up.setPackConfig(config.getPackConfig());
     up.setTimeout(config.getTimeout());
@@ -73,7 +76,8 @@ final class Upload extends AbstractGitCommand {
 
     List<PreUploadHook> allPreUploadHooks = Lists.newArrayList(preUploadHooks);
     allPreUploadHooks.add(
-        uploadValidatorsFactory.create(project, repo, session.getRemoteAddressAsString()));
+        uploadValidatorsFactory.create(
+            project, permissionAwareRepository, session.getRemoteAddressAsString()));
     up.setPreUploadHook(PreUploadHookChain.newChain(allPreUploadHooks));
     for (UploadPackInitializer initializer : uploadPackInitializers) {
       initializer.init(projectState.getNameKey(), up);

javatests/com/google/gerrit/acceptance/rest/project/GetCommitIT.java

@@ -43,11 +43,7 @@ public class GetCommitIT extends AbstractDaemonTest {
   @Before
   public void setUp() throws Exception {
     repo = GitUtil.newTestRepository(repoManager.openRepository(project));
-    projectOperations
-        .project(project)
-        .forUpdate()
-        .add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
-        .update();
+    blockRead();
   }
 
   @After
@@ -117,8 +113,17 @@ public class GetCommitIT extends AbstractDaemonTest {
 
   @Test
   public void getOpenChange_NotFound() throws Exception {
+    // Need to unblock read to allow the push operation to succeed if not, when retrieving the
+    // advertised refs during
+    // the push, the client won't be sent the initial commit and will send it again as part of the
+    // change.
+    unblockRead();
+
     PushOneCommit.Result r = pushFactory.create(admin.newIdent(), testRepo).to("refs/for/master");
     r.assertOkStatus();
+
+    // Re-blocking the read
+    blockRead();
     assertNotFound(r.getCommit());
   }
 
@@ -129,6 +134,14 @@ public class GetCommitIT extends AbstractDaemonTest {
     }
   }
 
+  private void blockRead() {
+    projectOperations
+        .project(project)
+        .forUpdate()
+        .add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS))
+        .update();
+  }
+
   private void assertNotFound(ObjectId id) throws Exception {
     userRestSession.get("/projects/" + project.get() + "/commits/" + id.name()).assertNotFound();
   }