opendev/gerrit
Code Issues Proposed changes

Merge changes I6be4658b,Iece1e1ac

Browse Source 
* changes:
  Don't store a reference to ProjectApi in AccessIT
  Make ProjectPermission.READ_CONFIG check if the user can READ r/m/c
This commit is contained in:
Patrick Hiesel
2018-02-21 07:16:48 +00:00
committed by Gerrit Code Review
parent 25ea50afa4 f52dd5b4be
commit 26bd38b313
2 changed files with 48 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
java/com/google/gerrit/server/permissions/ProjectControl.java
View File

@@ -24,6 +24,7 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.GroupMembership;
@@ -418,9 +419,11 @@ class ProjectControl {
case PUSH_AT_LEAST_ONE_REF: case PUSH_AT_LEAST_ONE_REF:
return canPushToAtLeastOneRef(); return canPushToAtLeastOneRef();
case READ_CONFIG:
return controlForRef(RefNames.REFS_CONFIG).isVisible();
case BAN_COMMIT: case BAN_COMMIT:
case READ_REFLOG: case READ_REFLOG:
case READ_CONFIG:
case WRITE_CONFIG: case WRITE_CONFIG:
return isOwner(); return isOwner();
} }

87
javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
View File

@@ -16,6 +16,7 @@ package com.google.gerrit.acceptance.rest.project;
import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth8.assertThat; import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES; import static com.google.gerrit.extensions.client.ListChangesOption.MESSAGES;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil; import com.google.gerrit.acceptance.GitUtil;
@@ -69,20 +70,18 @@ public class AccessIT extends AbstractDaemonTest {
private static final String LABEL_CODE_REVIEW = "Code-Review"; private static final String LABEL_CODE_REVIEW = "Code-Review";
private String newProjectName; private Project.NameKey newProjectName;
private ProjectApi pApi;
@Inject private DynamicSet<FileHistoryWebLink> fileHistoryWebLinkDynamicSet; @Inject private DynamicSet<FileHistoryWebLink> fileHistoryWebLinkDynamicSet;
@Before @Before
public void setUp() throws Exception { public void setUp() throws Exception {
newProjectName = createProject(PROJECT_NAME).get(); newProjectName = createProject(PROJECT_NAME);
pApi = gApi.projects().name(newProjectName);
} }
@Test @Test
public void getDefaultInheritance() throws Exception { public void getDefaultInheritance() throws Exception {
String inheritedName = pApi.access().inheritsFrom.name; String inheritedName = pApi().access().inheritsFrom.name;
assertThat(inheritedName).isEqualTo(AllProjectsNameProvider.DEFAULT); assertThat(inheritedName).isEqualTo(AllProjectsNameProvider.DEFAULT);
} }
@@ -99,7 +98,7 @@ public class AccessIT extends AbstractDaemonTest {
} }
}); });
try { try {
ProjectAccessInfo info = pApi.access(); ProjectAccessInfo info = pApi().access();
assertThat(info.configWebLinks).hasSize(1); assertThat(info.configWebLinks).hasSize(1);
assertThat(info.configWebLinks.get(0).url) assertThat(info.configWebLinks.get(0).url)
.isEqualTo("http://view/" + newProjectName + "/project.config"); .isEqualTo("http://view/" + newProjectName + "/project.config");
@@ -120,13 +119,13 @@ public class AccessIT extends AbstractDaemonTest {
"name", "imageURL", "http://view/" + projectName + "/" + fileName); "name", "imageURL", "http://view/" + projectName + "/" + fileName);
} }
}); });
try (Repository repo = repoManager.openRepository(new Project.NameKey(newProjectName))) { try (Repository repo = repoManager.openRepository(newProjectName)) {
RefUpdate u = repo.updateRef(RefNames.REFS_CONFIG); RefUpdate u = repo.updateRef(RefNames.REFS_CONFIG);
u.setForceUpdate(true); u.setForceUpdate(true);
assertThat(u.delete()).isEqualTo(Result.FORCED); assertThat(u.delete()).isEqualTo(Result.FORCED);
// This should not crash. // This should not crash.
pApi.access(); pApi().access();
} finally { } finally {
handle.remove(); handle.remove();
} }
@@ -134,34 +133,34 @@ public class AccessIT extends AbstractDaemonTest {
@Test @Test
public void addAccessSection() throws Exception { public void addAccessSection() throws Exception {
Project.NameKey p = new Project.NameKey(newProjectName); RevCommit initialHead = getRemoteHead(newProjectName, RefNames.REFS_CONFIG);
RevCommit initialHead = getRemoteHead(p, RefNames.REFS_CONFIG);
ProjectAccessInput accessInput = newProjectAccessInput(); ProjectAccessInput accessInput = newProjectAccessInput();
AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo(); AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
accessInput.add.put(REFS_HEADS, accessSectionInfo); accessInput.add.put(REFS_HEADS, accessSectionInfo);
pApi.access(accessInput); pApi().access(accessInput);
assertThat(pApi.access().local).isEqualTo(accessInput.add); assertThat(pApi().access().local).isEqualTo(accessInput.add);
RevCommit updatedHead = getRemoteHead(p, RefNames.REFS_CONFIG); RevCommit updatedHead = getRemoteHead(newProjectName, RefNames.REFS_CONFIG);
eventRecorder.assertRefUpdatedEvents( eventRecorder.assertRefUpdatedEvents(
p.get(), RefNames.REFS_CONFIG, null, initialHead, initialHead, updatedHead); newProjectName.get(), RefNames.REFS_CONFIG, null, initialHead, initialHead, updatedHead);
} }
@Test @Test
public void createAccessChangeNop() throws Exception { public void createAccessChangeNop() throws Exception {
ProjectAccessInput accessInput = newProjectAccessInput(); ProjectAccessInput accessInput = newProjectAccessInput();
exception.expect(BadRequestException.class); exception.expect(BadRequestException.class);
pApi.accessChange(accessInput); pApi().accessChange(accessInput);
} }
@Test @Test
public void createAccessChange() throws Exception { public void createAccessChange() throws Exception {
allow(newProjectName, RefNames.REFS_CONFIG, Permission.READ, REGISTERED_USERS);
// User can see the branch // User can see the branch
setApiUser(user); setApiUser(user);
gApi.projects().name(newProjectName).branch("refs/heads/master").get(); pApi().branch("refs/heads/master").get();
ProjectAccessInput accessInput = newProjectAccessInput(); ProjectAccessInput accessInput = newProjectAccessInput();
@@ -176,9 +175,9 @@ public class AccessIT extends AbstractDaemonTest {
accessInput.add.put(REFS_HEADS, accessSection); accessInput.add.put(REFS_HEADS, accessSection);
setApiUser(user); setApiUser(user);
ChangeInfo out = pApi.accessChange(accessInput); ChangeInfo out = pApi().accessChange(accessInput);
assertThat(out.project).isEqualTo(newProjectName); assertThat(out.project).isEqualTo(newProjectName.get());
assertThat(out.branch).isEqualTo(RefNames.REFS_CONFIG); assertThat(out.branch).isEqualTo(RefNames.REFS_CONFIG);
assertThat(out.status).isEqualTo(ChangeStatus.NEW); assertThat(out.status).isEqualTo(ChangeStatus.NEW);
assertThat(out.submitted).isNull(); assertThat(out.submitted).isNull();
@@ -196,7 +195,7 @@ public class AccessIT extends AbstractDaemonTest {
// check that the change took effect. // check that the change took effect.
setApiUser(user); setApiUser(user);
try { try {
BranchInfo info = gApi.projects().name(newProjectName).branch("refs/heads/master").get(); BranchInfo info = pApi().branch("refs/heads/master").get();
fail("wanted failure, got " + newGson().toJson(info)); fail("wanted failure, got " + newGson().toJson(info));
} catch (ResourceNotFoundException e) { } catch (ResourceNotFoundException e) {
// OK. // OK.
@@ -207,17 +206,15 @@ public class AccessIT extends AbstractDaemonTest {
accessInput.remove.put(REFS_HEADS, accessSection); accessInput.remove.put(REFS_HEADS, accessSection);
setApiUser(user); setApiUser(user);
pApi.accessChange(accessInput);
setApiUser(admin); setApiUser(admin);
out = pApi.accessChange(accessInput); out = pApi().accessChange(accessInput);
gApi.changes().id(out._number).current().review(reviewIn); gApi.changes().id(out._number).current().review(reviewIn);
gApi.changes().id(out._number).current().submit(); gApi.changes().id(out._number).current().submit();
// Now it works again. // Now it works again.
setApiUser(user); setApiUser(user);
gApi.projects().name(newProjectName).branch("refs/heads/master").get(); pApi().branch("refs/heads/master").get();
} }
@Test @Test
@@ -227,7 +224,7 @@ public class AccessIT extends AbstractDaemonTest {
AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo(); AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
accessInput.add.put(REFS_HEADS, accessSectionInfo); accessInput.add.put(REFS_HEADS, accessSectionInfo);
pApi.access(accessInput); pApi().access(accessInput);
// Remove specific permission // Remove specific permission
AccessSectionInfo accessSectionToRemove = newAccessSectionInfo(); AccessSectionInfo accessSectionToRemove = newAccessSectionInfo();
@@ -235,13 +232,13 @@ public class AccessIT extends AbstractDaemonTest {
Permission.LABEL + LABEL_CODE_REVIEW, newPermissionInfo()); Permission.LABEL + LABEL_CODE_REVIEW, newPermissionInfo());
ProjectAccessInput removal = newProjectAccessInput(); ProjectAccessInput removal = newProjectAccessInput();
removal.remove.put(REFS_HEADS, accessSectionToRemove); removal.remove.put(REFS_HEADS, accessSectionToRemove);
pApi.access(removal); pApi().access(removal);
// Remove locally // Remove locally
accessInput.add.get(REFS_HEADS).permissions.remove(Permission.LABEL + LABEL_CODE_REVIEW); accessInput.add.get(REFS_HEADS).permissions.remove(Permission.LABEL + LABEL_CODE_REVIEW);
// Check // Check
assertThat(pApi.access().local).isEqualTo(accessInput.add); assertThat(pApi().access().local).isEqualTo(accessInput.add);
} }
@Test @Test
@@ -251,7 +248,7 @@ public class AccessIT extends AbstractDaemonTest {
AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo(); AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
accessInput.add.put(REFS_HEADS, accessSectionInfo); accessInput.add.put(REFS_HEADS, accessSectionInfo);
pApi.access(accessInput); pApi().access(accessInput);
// Remove specific permission rule // Remove specific permission rule
AccessSectionInfo accessSectionToRemove = newAccessSectionInfo(); AccessSectionInfo accessSectionToRemove = newAccessSectionInfo();
@@ -262,7 +259,7 @@ public class AccessIT extends AbstractDaemonTest {
accessSectionToRemove.permissions.put(Permission.LABEL + LABEL_CODE_REVIEW, codeReview); accessSectionToRemove.permissions.put(Permission.LABEL + LABEL_CODE_REVIEW, codeReview);
ProjectAccessInput removal = newProjectAccessInput(); ProjectAccessInput removal = newProjectAccessInput();
removal.remove.put(REFS_HEADS, accessSectionToRemove); removal.remove.put(REFS_HEADS, accessSectionToRemove);
pApi.access(removal); pApi().access(removal);
// Remove locally // Remove locally
accessInput accessInput
@@ -274,7 +271,7 @@ public class AccessIT extends AbstractDaemonTest {
.remove(SystemGroupBackend.REGISTERED_USERS.get()); .remove(SystemGroupBackend.REGISTERED_USERS.get());
// Check // Check
assertThat(pApi.access().local).isEqualTo(accessInput.add); assertThat(pApi().access().local).isEqualTo(accessInput.add);
} }
@Test @Test
@@ -284,7 +281,7 @@ public class AccessIT extends AbstractDaemonTest {
AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo(); AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
accessInput.add.put(REFS_HEADS, accessSectionInfo); accessInput.add.put(REFS_HEADS, accessSectionInfo);
pApi.access(accessInput); pApi().access(accessInput);
// Remove specific permission rules // Remove specific permission rules
AccessSectionInfo accessSectionToRemove = newAccessSectionInfo(); AccessSectionInfo accessSectionToRemove = newAccessSectionInfo();
@@ -297,13 +294,13 @@ public class AccessIT extends AbstractDaemonTest {
accessSectionToRemove.permissions.put(Permission.LABEL + LABEL_CODE_REVIEW, codeReview); accessSectionToRemove.permissions.put(Permission.LABEL + LABEL_CODE_REVIEW, codeReview);
ProjectAccessInput removal = newProjectAccessInput(); ProjectAccessInput removal = newProjectAccessInput();
removal.remove.put(REFS_HEADS, accessSectionToRemove); removal.remove.put(REFS_HEADS, accessSectionToRemove);
pApi.access(removal); pApi().access(removal);
// Remove locally // Remove locally
accessInput.add.get(REFS_HEADS).permissions.remove(Permission.LABEL + LABEL_CODE_REVIEW); accessInput.add.get(REFS_HEADS).permissions.remove(Permission.LABEL + LABEL_CODE_REVIEW);
// Check // Check
assertThat(pApi.access().local).isEqualTo(accessInput.add); assertThat(pApi().access().local).isEqualTo(accessInput.add);
} }
@Test @Test
@@ -314,11 +311,11 @@ public class AccessIT extends AbstractDaemonTest {
// Disallow READ // Disallow READ
accessInput.add.put(REFS_ALL, accessSectionInfo); accessInput.add.put(REFS_ALL, accessSectionInfo);
pApi.access(accessInput); pApi().access(accessInput);
setApiUser(user); setApiUser(user);
exception.expect(ResourceNotFoundException.class); exception.expect(ResourceNotFoundException.class);
gApi.projects().name(newProjectName).access(); pApi().access();
} }
@Test @Test
@@ -329,7 +326,7 @@ public class AccessIT extends AbstractDaemonTest {
// Disallow READ // Disallow READ
accessInput.add.put(REFS_ALL, accessSectionInfo); accessInput.add.put(REFS_ALL, accessSectionInfo);
pApi.access(accessInput); pApi().access(accessInput);
// Create a change to apply // Create a change to apply
ProjectAccessInput accessInfoToApply = newProjectAccessInput(); ProjectAccessInput accessInfoToApply = newProjectAccessInput();
@@ -338,7 +335,7 @@ public class AccessIT extends AbstractDaemonTest {
setApiUser(user); setApiUser(user);
exception.expect(ResourceNotFoundException.class); exception.expect(ResourceNotFoundException.class);
gApi.projects().name(newProjectName).access(); pApi().access();
} }
@Test @Test
@@ -358,7 +355,7 @@ public class AccessIT extends AbstractDaemonTest {
accessSection.permissions.put(Permission.READ, read); accessSection.permissions.put(Permission.READ, read);
accessInput.add.put(REFS_ALL, accessSection); accessInput.add.put(REFS_ALL, accessSection);
ProjectAccessInfo result = pApi.access(accessInput); ProjectAccessInfo result = pApi().access(accessInput);
assertThat(result.groups.keySet()) assertThat(result.groups.keySet())
.containsExactly( .containsExactly(
SystemGroupBackend.PROJECT_OWNERS.get(), SystemGroupBackend.ANONYMOUS_USERS.get()); SystemGroupBackend.PROJECT_OWNERS.get(), SystemGroupBackend.ANONYMOUS_USERS.get());
@@ -371,7 +368,7 @@ public class AccessIT extends AbstractDaemonTest {
assertThat(result.groups.get(SystemGroupBackend.PROJECT_OWNERS.get()).id).isNull(); assertThat(result.groups.get(SystemGroupBackend.PROJECT_OWNERS.get()).id).isNull();
// Get call returns groups too. // Get call returns groups too.
ProjectAccessInfo loggedInResult = pApi.access(); ProjectAccessInfo loggedInResult = pApi().access();
assertThat(loggedInResult.groups.keySet()) assertThat(loggedInResult.groups.keySet())
.containsExactly( .containsExactly(
SystemGroupBackend.PROJECT_OWNERS.get(), SystemGroupBackend.ANONYMOUS_USERS.get()); SystemGroupBackend.PROJECT_OWNERS.get(), SystemGroupBackend.ANONYMOUS_USERS.get());
@@ -384,7 +381,7 @@ public class AccessIT extends AbstractDaemonTest {
// PROJECT_OWNERS is invisible to anonymous user, but GetAccess disregards visibility. // PROJECT_OWNERS is invisible to anonymous user, but GetAccess disregards visibility.
setApiUserAnonymous(); setApiUserAnonymous();
ProjectAccessInfo anonResult = pApi.access(); ProjectAccessInfo anonResult = pApi().access();
assertThat(anonResult.groups.keySet()) assertThat(anonResult.groups.keySet())
.containsExactly( .containsExactly(
SystemGroupBackend.PROJECT_OWNERS.get(), SystemGroupBackend.ANONYMOUS_USERS.get()); SystemGroupBackend.PROJECT_OWNERS.get(), SystemGroupBackend.ANONYMOUS_USERS.get());
@@ -402,7 +399,7 @@ public class AccessIT extends AbstractDaemonTest {
setApiUser(user); setApiUser(user);
exception.expect(AuthException.class); exception.expect(AuthException.class);
exception.expectMessage("administrate server not permitted"); exception.expectMessage("administrate server not permitted");
gApi.projects().name(newProjectName).access(accessInput); pApi().access(accessInput);
} }
@Test @Test
@@ -414,9 +411,9 @@ public class AccessIT extends AbstractDaemonTest {
ProjectAccessInput accessInput = newProjectAccessInput(); ProjectAccessInput accessInput = newProjectAccessInput();
accessInput.parent = newParentProjectName; accessInput.parent = newParentProjectName;
gApi.projects().name(newProjectName).access(accessInput); pApi().access(accessInput);
assertThat(pApi.access().inheritsFrom.name).isEqualTo(newParentProjectName); assertThat(pApi().access().inheritsFrom.name).isEqualTo(newParentProjectName);
} }
@Test @Test
@@ -457,7 +454,7 @@ public class AccessIT extends AbstractDaemonTest {
accessInput.add.put(AccessSection.GLOBAL_CAPABILITIES, accessSectionInfo); accessInput.add.put(AccessSection.GLOBAL_CAPABILITIES, accessSectionInfo);
exception.expect(BadRequestException.class); exception.expect(BadRequestException.class);
pApi.access(accessInput); pApi().access(accessInput);
} }
@Test @Test
@@ -632,6 +629,10 @@ public class AccessIT extends AbstractDaemonTest {
assertThat(permissions2.keySet()).containsExactly(Permission.READ); assertThat(permissions2.keySet()).containsExactly(Permission.READ);
} }
private ProjectApi pApi() throws Exception {
return gApi.projects().name(newProjectName.get());
}
private ProjectAccessInput newProjectAccessInput() { private ProjectAccessInput newProjectAccessInput() {
ProjectAccessInput p = new ProjectAccessInput(); ProjectAccessInput p = new ProjectAccessInput();
p.add = new HashMap<>(); p.add = new HashMap<>();