diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java index b6eaa22244..629c6bd23f 100644 --- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java +++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java @@ -356,7 +356,7 @@ class InProcessProtocol extends TestProtocol { rp.setPostReceiveHook(PostReceiveHookChain.newChain(Lists.newArrayList(postReceiveHooks))); return rp; - } catch (IOException e) { + } catch (IOException | PermissionBackendException e) { throw new RuntimeException(e); } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java index efb8fc508f..4d472dae75 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java @@ -358,11 +358,13 @@ public class GitOverHttpServlet extends GitServlet { rp.getAdvertiseRefsHook().advertiseRefs(rp); ProjectState state = (ProjectState) request.getAttribute(ATT_STATE); + Capable s; try { permissionBackend .user(userProvider) .project(state.getNameKey()) .check(ProjectPermission.RUN_RECEIVE_PACK); + s = arc.canUpload(); } catch (AuthException e) { GitSmartHttpTools.sendError( (HttpServletRequest) request, @@ -374,7 +376,6 @@ public class GitOverHttpServlet extends GitServlet { throw new RuntimeException(e); } - Capable s = arc.canUpload(); if (s != Capable.OK) { GitSmartHttpTools.sendError( (HttpServletRequest) request, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java index 5d88ec0569..3c9168a51b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java @@ -20,7 +20,6 @@ import com.google.gerrit.extensions.config.FactoryModule; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.RequestCleanup; import com.google.gerrit.server.project.PerRequestProjectControlCache; -import com.google.gerrit.server.project.ProjectControl; import com.google.inject.servlet.RequestScoped; /** Bindings for {@link RequestScoped} entities. */ @@ -32,6 +31,5 @@ public class GerritRequestModule extends FactoryModule { bind(IdentifiedUser.RequestFactory.class).in(SINGLETON); bind(PerRequestProjectControlCache.class).in(RequestScoped.class); - bind(ProjectControl.Factory.class).in(SINGLETON); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java index cfd9595572..6eba282aab 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java @@ -33,8 +33,6 @@ import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.project.ContributorAgreementsChecker; -import com.google.gerrit.server.project.NoSuchProjectException; -import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.util.MagicBranch; @@ -167,12 +165,12 @@ public class AsyncReceiveCommits implements PreReceiveHook { } private final ReceiveCommits.Factory factory; + private final PermissionBackend.ForProject perm; private final ReceivePack rp; private final ExecutorService executor; private final RequestScopePropagator scopePropagator; private final ReceiveConfig receiveConfig; private final ContributorAgreementsChecker contributorAgreements; - private final ProjectControl.GenericFactory projectControlFactory; private final long timeoutMillis; private final ProjectState projectState; private final IdentifiedUser user; @@ -193,7 +191,6 @@ public class AsyncReceiveCommits implements PreReceiveHook { TransferConfig transferConfig, Provider lazyPostReceive, ContributorAgreementsChecker contributorAgreements, - ProjectControl.GenericFactory projectControlFactory, @Named(TIMEOUT_NAME) long timeoutMillis, @Assisted ProjectState projectState, @Assisted IdentifiedUser user, @@ -206,7 +203,6 @@ public class AsyncReceiveCommits implements PreReceiveHook { this.scopePropagator = scopePropagator; this.receiveConfig = receiveConfig; this.contributorAgreements = contributorAgreements; - this.projectControlFactory = projectControlFactory; this.timeoutMillis = timeoutMillis; this.projectState = projectState; this.user = user; @@ -230,8 +226,9 @@ public class AsyncReceiveCommits implements PreReceiveHook { // 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 { - permissionBackend.user(user).project(projectName).check(ProjectPermission.READ); + this.perm.check(ProjectPermission.READ); } catch (AuthException e) { rp.setCheckReferencedObjectsAreReachable(receiveConfig.checkReferencedObjectsAreReachable); } @@ -246,18 +243,11 @@ public class AsyncReceiveCommits implements PreReceiveHook { } /** Determine if the user can upload commits. */ - public Capable canUpload() throws IOException { - // TODO(hiesel): Remove dependency on ProjectControl - ProjectControl projectControl; + public Capable canUpload() throws IOException, PermissionBackendException { try { - projectControl = projectControlFactory.controlFor(projectState.getNameKey(), user); - } catch (NoSuchProjectException e) { - throw new IOException(e); - } - - Capable result = projectControl.canPushToAtLeastOneRef(); - if (result != Capable.OK) { - return result; + perm.check(ProjectPermission.PUSH_AT_LEAST_ONE_REF); + } catch (AuthException e) { + return new Capable("Upload denied for project '" + projectState.getName() + "'"); } try { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java index 2a6bb8b547..6627f766d6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java @@ -88,7 +88,10 @@ public enum ProjectPermission { BAN_COMMIT, /** Allow accessing the project's reflog. */ - READ_REFLOG; + READ_REFLOG, + + /** Can push to at least one reference within the repository. */ + PUSH_AT_LEAST_ONE_REF; private final String name; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index f189fb72af..0e62b3fdc5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Maps; import com.google.gerrit.common.data.AccessSection; -import com.google.gerrit.common.data.Capable; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.extensions.restapi.AuthException; @@ -50,7 +49,6 @@ import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; -import com.google.inject.Provider; import com.google.inject.Singleton; import com.google.inject.assistedinject.Assisted; import java.io.IOException; @@ -71,10 +69,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** Access control management for a user accessing a project's data. */ -public class ProjectControl { +class ProjectControl { private static final Logger log = LoggerFactory.getLogger(ProjectControl.class); - public static class GenericFactory { + static class GenericFactory { private final ProjectCache projectCache; @Inject @@ -82,7 +80,7 @@ public class ProjectControl { projectCache = pc; } - public ProjectControl controlFor(Project.NameKey nameKey, CurrentUser user) + ProjectControl controlFor(Project.NameKey nameKey, CurrentUser user) throws NoSuchProjectException, IOException { final ProjectState p = projectCache.checkedGet(nameKey); if (p == null) { @@ -92,20 +90,7 @@ public class ProjectControl { } } - public static class Factory { - private final Provider userCache; - - @Inject - Factory(Provider uc) { - userCache = uc; - } - - public ProjectControl controlFor(Project.NameKey nameKey) throws NoSuchProjectException { - return userCache.get().get(nameKey); - } - } - - public interface AssistedFactory { + interface AssistedFactory { ProjectControl create(CurrentUser who, ProjectState ps); } @@ -155,27 +140,27 @@ public class ProjectControl { state = ps; } - public ProjectControl forUser(CurrentUser who) { + ProjectControl forUser(CurrentUser who) { ProjectControl r = state.controlFor(who); // Not per-user, and reusing saves lookup time. r.allSections = allSections; return r; } - public ChangeControl controlFor(ReviewDb db, Change change) throws OrmException { + ChangeControl controlFor(ReviewDb db, Change change) throws OrmException { return changeControlFactory.create( controlForRef(change.getDest()), db, change.getProject(), change.getId()); } - public ChangeControl controlFor(ChangeNotes notes) { + ChangeControl controlFor(ChangeNotes notes) { return changeControlFactory.create(controlForRef(notes.getChange().getDest()), notes); } - public RefControl controlForRef(Branch.NameKey ref) { + RefControl controlForRef(Branch.NameKey ref) { return controlForRef(ref.get()); } - public RefControl controlForRef(String refName) { + RefControl controlForRef(String refName) { if (refControls == null) { refControls = new HashMap<>(); } @@ -188,15 +173,15 @@ public class ProjectControl { return ctl; } - public CurrentUser getUser() { + CurrentUser getUser() { return user; } - public ProjectState getProjectState() { + ProjectState getProjectState() { return state; } - public Project getProject() { + Project getProject() { return state.getProject(); } @@ -209,13 +194,10 @@ public class ProjectControl { * @return {@code Capable.OK} if the user can upload to at least one reference. Does not check * Contributor Agreements. */ - public Capable canPushToAtLeastOneRef() { - if (!canPerformOnAnyRef(Permission.PUSH) - && !canPerformOnAnyRef(Permission.CREATE_TAG) - && !isOwner()) { - return new Capable("Upload denied for project '" + state.getName() + "'"); - } - return Capable.OK; + boolean canPushToAtLeastOneRef() { + return canPerformOnAnyRef(Permission.PUSH) + || canPerformOnAnyRef(Permission.CREATE_TAG) + || isOwner(); } /** Can the user run upload pack? */ @@ -390,7 +372,7 @@ public class ProjectControl { } } - public boolean canRead() { + boolean canRead() { return !isHidden() && allRefsAreVisible(Collections.emptySet()); } @@ -475,6 +457,9 @@ public class ProjectControl { case RUN_UPLOAD_PACK: return canRunUploadPack(); + case PUSH_AT_LEAST_ONE_REF: + return canPushToAtLeastOneRef(); + case BAN_COMMIT: case READ_REFLOG: case READ_CONFIG: diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java index f7ce73f6cc..049b86b0ca 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java @@ -35,7 +35,6 @@ import static com.google.gerrit.testutil.InMemoryRepositoryManager.newRepository import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.collect.Lists; -import com.google.gerrit.common.data.Capable; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.PermissionRule; @@ -133,7 +132,7 @@ public class RefControlTest { } private void assertCanUpload(ProjectControl u) { - assertThat(u.canPushToAtLeastOneRef()).named("can upload").isEqualTo(Capable.OK); + assertThat(u.canPushToAtLeastOneRef()).named("can upload").isTrue(); } private void assertCreateChange(String ref, ProjectControl u) { @@ -142,7 +141,7 @@ public class RefControlTest { } private void assertCannotUpload(ProjectControl u) { - assertThat(u.canPushToAtLeastOneRef()).named("cannot upload").isNotEqualTo(Capable.OK); + assertThat(u.canPushToAtLeastOneRef()).named("cannot upload").isFalse(); } private void assertCannotCreateChange(String ref, ProjectControl u) { diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java index 06fbcfc590..b199349892 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java @@ -95,9 +95,13 @@ final class Receive extends AbstractGitCommand { AsyncReceiveCommits arc = factory.create(projectState, currentUser, repo, null, reviewers); - Capable r = arc.canUpload(); - if (r != Capable.OK) { - throw die(r.getMessage()); + try { + Capable r = arc.canUpload(); + if (r != Capable.OK) { + throw die(r.getMessage()); + } + } catch (PermissionBackendException e) { + throw die(e.getMessage()); } ReceivePack rp = arc.getReceivePack();