opendev/gerrit
Code Issues Proposed changes

Make ProjectControl package-private

Browse Source 
This commit removes the last occurences of ProjectControl outside of
its package. It replaces ProjectControl#canPushToAtLeastOneRef() by a
new permission and defers the decision what to do about this shortcut
to a later point in time.

Change-Id: I4ef6c523b6f118a07dab98e1295a579ed61fd3c2
This commit is contained in:
Patrick Hiesel
2017-10-12 14:50:18 +02:00
parent e8633370c0
commit edeb3bb410
8 changed files with 43 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/InProcessProtocol.java
View File

@@ -356,7 +356,7 @@ class InProcessProtocol extends TestProtocol<Context> {
rp.setPostReceiveHook(PostReceiveHookChain.newChain(Lists.newArrayList(postReceiveHooks)));
return rp;
} catch (IOException e) {
} catch (IOException | PermissionBackendException e) {
throw new RuntimeException(e);
}
}

3
gerrit-httpd/src/main/java/com/google/gerrit/httpd/GitOverHttpServlet.java
View File

@@ -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,

2
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
View File

@@ -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);
}
}

24
gerrit-server/src/main/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java
View File

@@ -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<LazyPostReceiveHookChain> 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 {

5
gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java
View File

@@ -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;

55
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
View File

@@ -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<PerRequestProjectControlCache> userCache;
@Inject
Factory(Provider<PerRequestProjectControlCache> 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:

5
gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
View File

@@ -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) {

10
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/Receive.java
View File

@@ -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();