opendev/gerrit
Code Issues Proposed changes

Refactor ProjectControl#canReadCommmit to take repository as a parameter

Browse Source 
Two reasons:
1. Reusing an existing RevWalk to call isMergedInto or similar on it can
corrupt the state in the caller, see I0d242362.
2. The repository is already open before calling this method.

Pass the Repo instead of RevWalk and create a new clean RevWalk from the
Repo.

Change-Id: I00a6d9061d4c690c236e739b29b66b79427cd47b
This commit is contained in:
Zhen Chen
2016-06-03 15:02:08 -07:00
parent be0d2d89d1
commit 10c815d2e1
8 changed files with 40 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -1060,8 +1060,7 @@ public class ReceiveCommits {
}
RefControl ctl = projectControl.controlForRef(cmd.getRefName());
rp.getRevWalk().reset();
if (ctl.canCreate(db, rp.getRevWalk(), obj)) {
if (ctl.canCreate(db, rp.getRepository(), obj)) {
validateNewCommits(ctl, cmd);
batch.addCommand(cmd);
} else {

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

@@ -69,7 +69,7 @@ public class CommitsCollection implements
RevWalk rw = new RevWalk(repo)) {
RevCommit commit = rw.parseCommit(objectId);
rw.parseBody(commit);
if (!parent.getControl().canReadCommit(db.get(), rw, commit)) {
if (!parent.getControl().canReadCommit(db.get(), repo, commit)) {
throw new ResourceNotFoundException(id);
}
for (int i = 0; i < commit.getParentCount(); i++) {

3
gerrit-server/src/main/java/com/google/gerrit/server/project/CreateBranch.java
View File

@@ -113,8 +113,7 @@ public class CreateBranch implements RestModifyView<ProjectResource, BranchInput
}
}
rw.reset();
if (!refControl.canCreate(db.get(), rw, object)) {
if (!refControl.canCreate(db.get(), repo, object)) {
throw new AuthException("Cannot create \"" + ref + "\"");
}

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

@@ -62,7 +62,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(), rw, commit)) {
if (rsrc.getControl().canReadCommit(db.get(), repo, commit)) {
return head.getObjectId().name();
}
throw new AuthException("not allowed to see HEAD");

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

@@ -516,8 +516,8 @@ public class ProjectControl {
return false;
}
public boolean canReadCommit(ReviewDb db, RevWalk rw, RevCommit commit) {
try (Repository repo = openRepository()) {
public boolean canReadCommit(ReviewDb db, Repository repo, RevCommit commit) {
try (RevWalk rw = new RevWalk(repo)) {
return isMergedIntoVisibleRef(repo, db, rw, commit,
repo.getAllRefs().values());
} catch (IOException e) {
@@ -541,8 +541,4 @@ public class ProjectControl {
return !refs.isEmpty()
&& IncludedInResolver.includedInOne(repo, rw, commit, refs.values());
}
Repository openRepository() throws IOException {
return repoManager.openRepository(getProject().getNameKey());
}
}

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

@@ -236,12 +236,11 @@ public class RefControl {
* Determines whether the user can create a new Git ref.
*
* @param db db for checking change visibility.
* @param rw revision pool {@code object} was parsed in; must be reset before
* calling this method.
* @param repo repository on which user want to create
* @param object the object the user will start the reference with.
* @return {@code true} if the user specified can create a new Git ref
*/
public boolean canCreate(ReviewDb db, RevWalk rw, RevObject object) {
public boolean canCreate(ReviewDb db, Repository repo, RevObject object) {
if (!canWrite()) {
return false;
}
@@ -274,7 +273,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 true;
} else if (isMergedIntoBranchOrTag(db, rw, (RevCommit) object)) {
} else if (isMergedIntoBranchOrTag(db, repo, (RevCommit) object)) {
// 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
@@ -284,7 +283,7 @@ public class RefControl {
return false;
} else if (object instanceof RevTag) {
final RevTag tag = (RevTag) object;
try {
try (RevWalk rw = new RevWalk(repo)) {
rw.parseBody(tag);
} catch (IOException e) {
return false;
@@ -318,9 +317,9 @@ public class RefControl {
}
}
private boolean isMergedIntoBranchOrTag(ReviewDb db, RevWalk rw,
private boolean isMergedIntoBranchOrTag(ReviewDb db, Repository repo,
RevCommit commit) {
try (Repository repo = projectControl.openRepository()) {
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());

42
gerrit-server/src/test/java/com/google/gerrit/server/project/ProjectControlTest.java
View File

@@ -49,6 +49,7 @@ import com.google.inject.util.Providers;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.junit.After;
@@ -137,7 +138,9 @@ public class ProjectControlTest {
ObjectId id = repo.branch("master").commit().create();
ProjectControl pc = newProjectControl();
RevWalk rw = repo.getRevWalk();
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(id)));
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id)));
}
@Test
@@ -150,8 +153,10 @@ public class ProjectControlTest {
ProjectControl pc = newProjectControl();
RevWalk rw = repo.getRevWalk();
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(id1)));
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(id2)));
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id2)));
}
@Test
@@ -164,8 +169,10 @@ public class ProjectControlTest {
ProjectControl pc = newProjectControl();
RevWalk rw = repo.getRevWalk();
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(id1)));
assertFalse(pc.canReadCommit(db, rw, rw.parseCommit(id2)));
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(id2)));
}
@Test
@@ -181,8 +188,9 @@ public class ProjectControlTest {
ProjectControl pc = newProjectControl();
RevWalk rw = repo.getRevWalk();
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, rw, rw.parseCommit(parent2)));
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(parent2)));
}
@Test
@@ -194,12 +202,14 @@ public class ProjectControlTest {
ProjectControl pc = newProjectControl();
RevWalk rw = repo.getRevWalk();
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(parent1)));
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(id1)));
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
repo.branch("branch1").update(parent1);
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, rw, rw.parseCommit(id1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(id1)));
}
@Test
@@ -211,12 +221,14 @@ public class ProjectControlTest {
ProjectControl pc = newProjectControl();
RevWalk rw = repo.getRevWalk();
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(parent1)));
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(id1)));
Repository r = repo.getRepository();
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(id1)));
repo.branch("branch1").update(parent1);
assertTrue(pc.canReadCommit(db, rw, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, rw, rw.parseCommit(id1)));
assertTrue(pc.canReadCommit(db, r, rw.parseCommit(parent1)));
assertFalse(pc.canReadCommit(db, r, rw.parseCommit(id1)));
}
private ProjectControl newProjectControl() throws Exception {

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

@@ -217,7 +217,7 @@ public class UploadArchive extends AbstractGitCommand {
private boolean canRead(ObjectId revId) throws IOException {
try (RevWalk rw = new RevWalk(repo)) {
RevCommit commit = rw.parseCommit(revId);
return projectControl.canReadCommit(db, rw, commit);
return projectControl.canReadCommit(db, repo, commit);
}
}
}