opendev/gerrit
Code Issues Proposed changes

Consolidate reachable commit in CommitsCollection

Browse Source 
Moving the logic to determine if a commit is reachable into
CommitsCollection pulls most of it out of the legacy ProjectControl
and RefControl.

Improve the heads or tags code slightly by determining the size of the
two collections and presizing the map for that count.  This avoids an
intermediate ArrayList copy.

Rename the related tests into CommitsCollectionTest.

Change-Id: I4c4a7624a4b50335509034a968c31da90e981795
This commit is contained in:
Shawn Pearce 2017-04-29 16:54:32 -07:00 committed by Patrick Hiesel
parent cc8de7756b
commit 97015b0c1c
12 changed files with 138 additions and 126 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java
View File

@ -23,6 +23,7 @@ import com.google.gerrit.extensions.api.projects.CommentLinkInfo;
import com.google.gerrit.extensions.config.FactoryModule;
import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.rules.PrologModule;
import com.google.gerrit.server.CurrentUser;
@ -63,6 +64,7 @@ import com.google.gerrit.server.notedb.NoteDbModule;
import com.google.gerrit.server.patch.DiffExecutorModule;
import com.google.gerrit.server.patch.PatchListCacheImpl;
import com.google.gerrit.server.project.CommentLinkProvider;
import com.google.gerrit.server.project.CommitResource;
import com.google.gerrit.server.project.DefaultPermissionBackendModule;
import com.google.gerrit.server.project.ProjectCacheImpl;
import com.google.gerrit.server.project.ProjectState;
@ -115,6 +117,8 @@ public class BatchProgramModule extends FactoryModule {
.in(SINGLETON);
bind(new TypeLiteral<DynamicMap<ChangeQueryProcessor.ChangeAttributeFactory>>() {})
.toInstance(DynamicMap.<ChangeQueryProcessor.ChangeAttributeFactory>emptyMap());
bind(new TypeLiteral<DynamicMap<RestView<CommitResource>>>() {})
.toInstance(DynamicMap.<RestView<CommitResource>>emptyMap());
bind(String.class)
.annotatedWith(CanonicalWebUrl.class)
.toProvider(CanonicalWebUrlProvider.class);

10
gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java
View File

@ -56,9 +56,11 @@ import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.CommitsCollection;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectResource;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.ProjectsCollection;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
@ -100,6 +102,7 @@ public class CreateChange
private final PermissionBackend permissionBackend;
private final Provider<CurrentUser> user;
private final ProjectsCollection projectsCollection;
private final CommitsCollection commits;
private final ChangeInserter.Factory changeInserterFactory;
private final ChangeJson.Factory jsonFactory;
private final ChangeFinder changeFinder;
@ -121,6 +124,7 @@ public class CreateChange
PermissionBackend permissionBackend,
Provider<CurrentUser> user,
ProjectsCollection projectsCollection,
CommitsCollection commits,
ChangeInserter.Factory changeInserterFactory,
ChangeJson.Factory json,
ChangeFinder changeFinder,
@ -139,6 +143,7 @@ public class CreateChange
this.permissionBackend = permissionBackend;
this.user = user;
this.projectsCollection = projectsCollection;
this.commits = commits;
this.changeInserterFactory = changeInserterFactory;
this.jsonFactory = json;
this.changeFinder = changeFinder;
@ -311,12 +316,13 @@ public class CreateChange
throw new BadRequestException("merge.source must be non-empty");
}
ProjectState state = projectControl.getProjectState();
RevCommit sourceCommit = MergeUtil.resolveCommit(repo, rw, merge.source);
if (!projectControl.canReadCommit(db.get(), repo, sourceCommit)) {
if (!commits.canRead(state, repo, sourceCommit)) {
throw new BadRequestException("do not have read permission for: " + merge.source);
}
MergeUtil mergeUtil = mergeUtilFactory.create(projectControl.getProjectState());
MergeUtil mergeUtil = mergeUtilFactory.create(state);
// default merge strategy from project settings
String mergeStrategy =
MoreObjects.firstNonNull(

9
gerrit-server/src/main/java/com/google/gerrit/server/change/CreateMergePatchSet.java
View File

@ -43,8 +43,10 @@ import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.permissions.ChangePermission;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.CommitsCollection;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.update.RetryHelper;
import com.google.gerrit.server.update.RetryingRestModifyView;
@ -69,9 +71,9 @@ import org.eclipse.jgit.util.ChangeIdUtil;
@Singleton
public class CreateMergePatchSet
extends RetryingRestModifyView<ChangeResource, MergePatchSetInput, Response<ChangeInfo>> {
private final Provider<ReviewDb> db;
private final GitRepositoryManager gitManager;
private final CommitsCollection commits;
private final TimeZone serverTimeZone;
private final Provider<CurrentUser> user;
private final ChangeJson.Factory jsonFactory;
@ -83,6 +85,7 @@ public class CreateMergePatchSet
CreateMergePatchSet(
Provider<ReviewDb> db,
GitRepositoryManager gitManager,
CommitsCollection commits,
@GerritPersonIdent PersonIdent myIdent,
Provider<CurrentUser> user,
ChangeJson.Factory json,
@ -93,6 +96,7 @@ public class CreateMergePatchSet
super(retryHelper);
this.db = db;
this.gitManager = gitManager;
this.commits = commits;
this.serverTimeZone = myIdent.getTimeZone();
this.user = user;
this.jsonFactory = json;
@ -116,6 +120,7 @@ public class CreateMergePatchSet
ChangeControl ctl = rsrc.getControl();
PatchSet ps = psUtil.current(db.get(), ctl.getNotes());
ProjectControl projectControl = ctl.getProjectControl();
ProjectState state = projectControl.getProjectState();
Change change = ctl.getChange();
Project.NameKey project = change.getProject();
Branch.NameKey dest = change.getDest();
@ -125,7 +130,7 @@ public class CreateMergePatchSet
RevWalk rw = new RevWalk(reader)) {
RevCommit sourceCommit = MergeUtil.resolveCommit(git, rw, merge.source);
if (!projectControl.canReadCommit(db.get(), git, sourceCommit)) {
if (!commits.canRead(state, git, sourceCommit)) {
throw new ResourceNotFoundException(
"cannot find source commit: " + merge.source + " to merge.");
}

11
gerrit-server/src/main/java/com/google/gerrit/server/project/CheckMergeability.java
View File

@ -19,13 +19,11 @@ import com.google.gerrit.extensions.common.MergeableInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.InMemoryInserter;
import com.google.gerrit.server.git.MergeUtil;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectInserter;
@ -39,11 +37,9 @@ import org.kohsuke.args4j.Option;
/** Check the mergeability at current branch for a git object references expression. */
public class CheckMergeability implements RestReadView<BranchResource> {
private String source;
private String strategy;
private SubmitType submitType;
private final Provider<ReviewDb> db;
@Option(
name = "--source",
@ -68,14 +64,15 @@ public class CheckMergeability implements RestReadView<BranchResource> {
}
private final GitRepositoryManager gitManager;
private final CommitsCollection commits;
@Inject
CheckMergeability(
GitRepositoryManager gitManager, @GerritServerConfig Config cfg, Provider<ReviewDb> db) {
GitRepositoryManager gitManager, CommitsCollection commits, @GerritServerConfig Config cfg) {
this.gitManager = gitManager;
this.commits = commits;
this.strategy = MergeUtil.getMergeStrategy(cfg).getName();
this.submitType = cfg.getEnum("project", null, "submitType", SubmitType.MERGE_IF_NECESSARY);
this.db = db;
}
@Override
@ -102,7 +99,7 @@ public class CheckMergeability implements RestReadView<BranchResource> {
RevCommit targetCommit = rw.parseCommit(destRef.getObjectId());
RevCommit sourceCommit = MergeUtil.resolveCommit(git, rw, source);
if (!resource.getControl().canReadCommit(db.get(), git, sourceCommit)) {
if (!commits.canRead(resource.getProjectState(), git, sourceCommit)) {
throw new BadRequestException("do not have read permission for: " + source);
}

58
gerrit-server/src/main/java/com/google/gerrit/server/project/CommitsCollection.java
View File

@ -19,33 +19,48 @@ import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestView;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.change.IncludedInResolver;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.VisibleRefFilter;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import java.util.List;
import java.util.Map;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
import org.eclipse.jgit.errors.MissingObjectException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@Singleton
public class CommitsCollection implements ChildCollection<ProjectResource, CommitResource> {
private static final Logger log = LoggerFactory.getLogger(CommitsCollection.class);
private final DynamicMap<RestView<CommitResource>> views;
private final GitRepositoryManager repoManager;
private final Provider<ReviewDb> db;
private final VisibleRefFilter.Factory refFilter;
private final Provider<InternalChangeQuery> queryProvider;
@Inject
public CommitsCollection(
DynamicMap<RestView<CommitResource>> views,
GitRepositoryManager repoManager,
Provider<ReviewDb> db) {
VisibleRefFilter.Factory refFilter,
Provider<InternalChangeQuery> queryProvider) {
this.views = views;
this.repoManager = repoManager;
this.db = db;
this.refFilter = refFilter;
this.queryProvider = queryProvider;
}
@Override
@ -67,7 +82,7 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
RevWalk rw = new RevWalk(repo)) {
RevCommit commit = rw.parseCommit(objectId);
rw.parseBody(commit);
if (!parent.getControl().canReadCommit(db.get(), repo, commit)) {
if (!canRead(parent.getProjectState(), repo, commit)) {
throw new ResourceNotFoundException(id);
}
for (int i = 0; i < commit.getParentCount(); i++) {
@ -83,4 +98,37 @@ public class CommitsCollection implements ChildCollection<ProjectResource, Commi
public DynamicMap<RestView<CommitResource>> views() {
return views;
}
/** @return true if {@code commit} is visible to the caller. */
public boolean canRead(ProjectState state, Repository repo, RevCommit commit) {
Project.NameKey project = state.getProject().getNameKey();
// Look for changes associated with the commit.
try {
List<ChangeData> changes =
queryProvider.get().enforceVisibility(true).byProjectCommit(project, commit);
if (!changes.isEmpty()) {
return true;
}
} catch (OrmException e) {
log.error("Cannot look up change for commit " + commit.name() + " in " + project, e);
}
return isReachableFrom(state, repo, commit, repo.getAllRefs());
}
public boolean isReachableFrom(
ProjectState state, Repository repo, RevCommit commit, Map<String, Ref> refs) {
try (RevWalk rw = new RevWalk(repo)) {
refs = refFilter.create(state, repo).filter(refs, true);
return IncludedInResolver.includedInAny(repo, rw, commit, refs.values());
} catch (IOException e) {
log.error(
String.format(
"Cannot verify permissions to commit object %s in repository %s",
commit.name(), state.getProject().getNameKey()),
e);
return false;
}
}
}

12
gerrit-server/src/main/java/com/google/gerrit/server/project/GetHead.java
View File

@ -17,10 +17,8 @@ package com.google.gerrit.server.project;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
import org.eclipse.jgit.errors.IncorrectObjectTypeException;
@ -34,13 +32,13 @@ import org.eclipse.jgit.revwalk.RevWalk;
@Singleton
public class GetHead implements RestReadView<ProjectResource> {
private GitRepositoryManager repoManager;
private Provider<ReviewDb> db;
private final GitRepositoryManager repoManager;
private final CommitsCollection commits;
@Inject
GetHead(GitRepositoryManager repoManager, Provider<ReviewDb> db) {
GetHead(GitRepositoryManager repoManager, CommitsCollection commits) {
this.repoManager = repoManager;
this.db = db;
this.commits = commits;
}
@Override
@ -59,7 +57,7 @@ public class GetHead implements RestReadView<ProjectResource> {
} else if (head.getObjectId() != null) {
try (RevWalk rw = new RevWalk(repo)) {
RevCommit commit = rw.parseCommit(head.getObjectId());
if (rsrc.getControl().canReadCommit(db.get(), repo, commit)) {
if (commits.canRead(rsrc.getProjectState(), repo, commit)) {
return head.getObjectId().name();
}
throw new AuthException("not allowed to see HEAD");

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

@ -16,6 +16,7 @@ package com.google.gerrit.server.project;
import static com.google.common.base.Preconditions.checkArgument;
import com.google.common.collect.Iterables;
import com.google.common.collect.Maps;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.PageLinks;
@ -39,11 +40,9 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.change.IncludedInResolver;
import com.google.gerrit.server.config.CanonicalWebUrl;
import com.google.gerrit.server.config.GitReceivePackGroups;
import com.google.gerrit.server.config.GitUploadPackGroups;
import com.google.gerrit.server.git.VisibleRefFilter;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.FailedPermissionBackend;
@ -55,7 +54,6 @@ import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@ -71,10 +69,11 @@ import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefDatabase;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@ -132,16 +131,14 @@ public class ProjectControl {
private final Set<AccountGroup.UUID> uploadGroups;
private final Set<AccountGroup.UUID> receiveGroups;
private final String canonicalWebUrl;
private final PermissionBackend.WithUser perm;
private final CurrentUser user;
private final ProjectState state;
private final CommitsCollection commits;
private final ChangeControl.Factory changeControlFactory;
private final PermissionCollection.Factory permissionFilter;
private final VisibleRefFilter.Factory refFilter;
private final Collection<ContributorAgreement> contributorAgreements;
private final Provider<InternalChangeQuery> queryProvider;
private final Metrics metrics;
private List<SectionMatcher> allSections;
@ -156,22 +153,20 @@ public class ProjectControl {
@GitReceivePackGroups Set<AccountGroup.UUID> receiveGroups,
ProjectCache pc,
PermissionCollection.Factory permissionFilter,
CommitsCollection commits,
ChangeControl.Factory changeControlFactory,
VisibleRefFilter.Factory refFilter,
Provider<InternalChangeQuery> queryProvider,
@CanonicalWebUrl @Nullable String canonicalWebUrl,
PermissionBackend permissionBackend,
@Assisted CurrentUser who,
@Assisted ProjectState ps,
Metrics metrics) {
this.changeControlFactory = changeControlFactory;
this.refFilter = refFilter;
this.uploadGroups = uploadGroups;
this.receiveGroups = receiveGroups;
this.permissionFilter = permissionFilter;
this.commits = commits;
this.contributorAgreements = pc.getAllProjects().getConfig().getContributorAgreements();
this.canonicalWebUrl = canonicalWebUrl;
this.queryProvider = queryProvider;
this.metrics = metrics;
this.perm = permissionBackend.user(who);
user = who;
@ -483,50 +478,26 @@ public class ProjectControl {
return false;
}
/** @return whether a commit is visible to user. */
public boolean canReadCommit(ReviewDb db, Repository repo, RevCommit commit) {
// Look for changes associated with the commit.
boolean isReachableFromHeadsOrTags(Repository repo, RevCommit commit) {
try {
List<ChangeData> changes =
queryProvider.get().byProjectCommit(getProject().getNameKey(), commit);
for (ChangeData change : changes) {
if (controlFor(db, change.change()).isVisible(db)) {
return true;
}
RefDatabase refdb = repo.getRefDatabase();
Collection<Ref> heads = refdb.getRefs(Constants.R_HEADS).values();
Collection<Ref> tags = refdb.getRefs(Constants.R_TAGS).values();
Map<String, Ref> refs = Maps.newHashMapWithExpectedSize(heads.size() + tags.size());
for (Ref r : Iterables.concat(heads, tags)) {
refs.put(r.getName(), r);
}
} catch (OrmException e) {
log.error(
"Cannot look up change for commit " + commit.name() + " in " + getProject().getName(), e);
}
// Scan all visible refs.
return canReadCommitFromVisibleRef(repo, commit);
}
private boolean canReadCommitFromVisibleRef(Repository repo, RevCommit commit) {
try (RevWalk rw = new RevWalk(repo)) {
return isMergedIntoVisibleRef(repo, rw, commit, repo.getAllRefs().values());
return commits.isReachableFrom(state, repo, commit, refs);
} catch (IOException e) {
String msg =
log.error(
String.format(
"Cannot verify permissions to commit object %s in repository %s",
commit.name(), getProject().getNameKey());
log.error(msg, e);
commit.name(), getProject().getNameKey()),
e);
return false;
}
}
boolean isMergedIntoVisibleRef(
Repository repo, RevWalk rw, RevCommit commit, Collection<Ref> unfilteredRefs)
throws IOException {
VisibleRefFilter filter = refFilter.create(state, repo);
Map<String, Ref> m = Maps.newHashMapWithExpectedSize(unfilteredRefs.size());
for (Ref r : unfilteredRefs) {
m.put(r.getName(), r);
}
Map<String, Ref> refs = filter.filter(m, true);
return IncludedInResolver.includedInAny(repo, rw, commit, refs.values());
}
ForProject asForProject() {
return new ForProjectImpl();
}

19
gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
View File

@ -45,9 +45,7 @@ import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.eclipse.jgit.lib.Constants;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevObject;
@ -347,7 +345,7 @@ public class RefControl {
// If the user has push permissions, they can create the ref regardless
// of whether they are pushing any new objects along with the create.
return null;
} else if (isMergedIntoBranchOrTag(repo, commit)) {
} else if (projectControl.isReachableFromHeadsOrTags(repo, commit)) {
// If the user has no push permissions, check whether the object is
// merged into a branch or tag readable by this user. If so, they are
// not effectively "pushing" more objects, so they can create the ref
@ -357,21 +355,6 @@ public class RefControl {
return userId + " lacks permission " + Permission.PUSH + " for creating new commit object";
}
private boolean isMergedIntoBranchOrTag(Repository repo, RevCommit commit) {
try (RevWalk rw = new RevWalk(repo)) {
List<Ref> refs = new ArrayList<>(repo.getRefDatabase().getRefs(Constants.R_HEADS).values());
refs.addAll(repo.getRefDatabase().getRefs(Constants.R_TAGS).values());
return projectControl.isMergedIntoVisibleRef(repo, rw, commit, refs);
} catch (IOException e) {
String msg =
String.format(
"Cannot verify permissions to commit object %s in repository %s",
commit.name(), projectControl.getProject().getNameKey());
log.error(msg, e);
}
return false;
}
/**
* Determines whether the user can delete the Git ref controlled by this object.
*

52
gerrit-server/src/test/java/com/google/gerrit/server/project/ProjectControlTest.java → gerrit-server/src/test/java/com/google/gerrit/server/project/CommitsCollectionTest.java
View File

@ -55,19 +55,19 @@ import org.junit.After;
import org.junit.Before;
import org.junit.Test;
/** Unit tests for {@link ProjectControl}. */
public class ProjectControlTest {
/** Unit tests for {@link CommitsCollection}. */
public class CommitsCollectionTest {
@Inject private AccountManager accountManager;
@Inject private IdentifiedUser.GenericFactory userFactory;
@Inject private InMemoryDatabase schemaFactory;
@Inject private InMemoryRepositoryManager repoManager;
@Inject private ProjectControl.GenericFactory projectControlFactory;
@Inject private SchemaCreator schemaCreator;
@Inject private ThreadLocalRequestContext requestContext;
@Inject protected ProjectCache projectCache;
@Inject protected MetaDataUpdate.Server metaDataUpdateFactory;
@Inject protected AllProjectsName allProjects;
@Inject protected GroupCache groupCache;
@Inject private CommitsCollection commits;
private LifecycleManager lifecycle;
private ReviewDb db;
@ -135,11 +135,11 @@ public class ProjectControlTest {
public void canReadCommitWhenAllRefsVisible() throws Exception {
allow(project, READ, REGISTERED_USERS, "refs/*");
ObjectId id = repo.branch("master").commit().create();
ProjectControl pc = newProjectControl();
ProjectState state = readProjectState();
RevWalk rw = repo.getRevWalk();
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id)));
assertTrue(commits.canRead(state, r, rw.parseCommit(id)));
}
@Test
@ -150,12 +150,12 @@ public class ProjectControlTest {
ObjectId id1 = repo.branch("branch1").commit().create();
ObjectId id2 = repo.branch("branch2").commit().create();
ProjectControl pc = newProjectControl();
ProjectState state = readProjectState();
RevWalk rw = repo.getRevWalk();
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id2)));
assertTrue(commits.canRead(state, r, rw.parseCommit(id1)));
assertTrue(commits.canRead(state, r, rw.parseCommit(id2)));
}
@Test
@ -166,12 +166,12 @@ public class ProjectControlTest {
ObjectId id1 = repo.branch("branch1").commit().create();
ObjectId id2 = repo.branch("branch2").commit().create();
ProjectControl pc = newProjectControl();
ProjectState state = readProjectState();
RevWalk rw = repo.getRevWalk();
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(id2)));
assertTrue(commits.canRead(state, r, rw.parseCommit(id1)));
assertFalse(commits.canRead(state, r, rw.parseCommit(id2)));
}
@Test
@ -185,11 +185,11 @@ public class ProjectControlTest {
RevCommit parent2 = repo.commit().create();
repo.branch("branch2").commit().parent(parent2).create();
ProjectControl pc = newProjectControl();
ProjectState state = readProjectState();
RevWalk rw = repo.getRevWalk();
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(parent2)));
assertTrue(commits.canRead(state, r, rw.parseCommit(parent1)));
assertFalse(commits.canRead(state, r, rw.parseCommit(parent2)));
}
@Test
@ -199,16 +199,16 @@ public class ProjectControlTest {
RevCommit parent1 = repo.commit().create();
ObjectId id1 = repo.branch("branch1").commit().parent(parent1).create();
ProjectControl pc = newProjectControl();
ProjectState state = readProjectState();
RevWalk rw = repo.getRevWalk();
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertTrue(commits.canRead(state, r, rw.parseCommit(parent1)));
assertTrue(commits.canRead(state, r, rw.parseCommit(id1)));
repo.branch("branch1").update(parent1);
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertTrue(commits.canRead(state, r, rw.parseCommit(parent1)));
assertFalse(commits.canRead(state, r, rw.parseCommit(id1)));
}
@Test
@ -218,20 +218,20 @@ public class ProjectControlTest {
RevCommit parent1 = repo.commit().create();
ObjectId id1 = repo.branch("branch1").commit().parent(parent1).create();
ProjectControl pc = newProjectControl();
ProjectState state = readProjectState();
RevWalk rw = repo.getRevWalk();
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertTrue(commits.canRead(state, r, rw.parseCommit(parent1)));
assertTrue(commits.canRead(state, r, rw.parseCommit(id1)));
repo.branch("branch1").update(parent1);
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertTrue(commits.canRead(state, r, rw.parseCommit(parent1)));
assertFalse(commits.canRead(state, r, rw.parseCommit(id1)));
}
private ProjectControl newProjectControl() throws Exception {
return projectControlFactory.controlFor(project.getName(), user);
private ProjectState readProjectState() throws Exception {
return projectCache.get(project.getName());
}
protected void allow(ProjectConfig project, String permission, AccountGroup.UUID id, String ref)

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

@ -60,7 +60,6 @@ import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.schema.SchemaCreator;
import com.google.gerrit.server.util.RequestContext;
import com.google.gerrit.server.util.ThreadLocalRequestContext;
@ -212,7 +211,6 @@ public class RefControlTest {
@Inject private SingleVersionListener singleVersionListener;
@Inject private InMemoryDatabase schemaFactory;
@Inject private ThreadLocalRequestContext requestContext;
@Inject private Provider<InternalChangeQuery> queryProvider;
@Inject private ProjectControl.Metrics metrics;
@Before
@ -899,17 +897,14 @@ public class RefControlTest {
}
private ProjectControl user(ProjectConfig local, String name, AccountGroup.UUID... memberOf) {
String canonicalWebUrl = "http://localhost";
return new ProjectControl(
Collections.<AccountGroup.UUID>emptySet(),
Collections.<AccountGroup.UUID>emptySet(),
projectCache,
sectionSorter,
null, // commitsCollection
changeControlFactory,
null, // refFilter
queryProvider,
canonicalWebUrl,
"http://localhost", // canonicalWebUrl
permissionBackend,
new MockUser(name, memberOf),
newProjectState(local),

9
gerrit-sshd/src/main/java/com/google/gerrit/sshd/AbstractGitCommand.java
View File

@ -20,6 +20,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.sshd.SshScope.Context;
import com.google.inject.Inject;
import java.io.IOException;
@ -45,6 +46,8 @@ public abstract class AbstractGitCommand extends BaseCommand {
@Inject private IdentifiedUser.GenericFactory userFactory;
protected Repository repo;
protected ProjectState state;
protected Project.NameKey projectName;
protected Project project;
@Override
@ -86,10 +89,12 @@ public abstract class AbstractGitCommand extends BaseCommand {
}
private void service() throws IOException, PermissionBackendException, Failure {
project = projectControl.getProjectState().getProject();
state = projectControl.getProjectState();
project = state.getProject();
projectName = project.getNameKey();
try {
repo = repoManager.openRepository(project.getNameKey());
repo = repoManager.openRepository(projectName);
} catch (RepositoryNotFoundException e) {
throw new Failure(1, "fatal: '" + project.getName() + "': not a git archive", e);
}

8
gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/UploadArchive.java
View File

@ -18,13 +18,13 @@ import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.AllowedFormats;
import com.google.gerrit.server.change.ArchiveFormat;
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.CommitsCollection;
import com.google.gerrit.sshd.AbstractGitCommand;
import com.google.inject.Inject;
import java.io.IOException;
@ -121,9 +121,9 @@ public class UploadArchive extends AbstractGitCommand {
}
@Inject private PermissionBackend permissionBackend;
@Inject private CommitsCollection commits;
@Inject private IdentifiedUser user;
@Inject private AllowedFormats allowedFormats;
@Inject private ReviewDb db;
private Options options = new Options();
/**
@ -244,13 +244,13 @@ public class UploadArchive extends AbstractGitCommand {
private boolean canRead(ObjectId revId) throws IOException, PermissionBackendException {
try {
permissionBackend.user(user).project(project.getNameKey()).check(ProjectPermission.READ);
permissionBackend.user(user).project(projectName).check(ProjectPermission.READ);
return true;
} catch (AuthException e) {
// Check reachability of the specific revision.
try (RevWalk rw = new RevWalk(repo)) {
RevCommit commit = rw.parseCommit(revId);
return projectControl.canReadCommit(db, repo, commit);
return commits.canRead(state, repo, commit);
}
}
}