opendev/gerrit
Code Issues Proposed changes

Enforce that All-Users must inherit from All-Projects

Browse Source 
For groups in NoteDb, we manage group ownership through permissions on
the group ref. During the migration, we prevent the mutation of
Gerrit-managed permissions for group refs to ensure the ReviewDb and
NoteDb data does not get out of sync.

These enforcements are in place for All-Users and All-Projects. To
prevent users from changing these properties by inheritance, we enforce
a fixed inheritance of All-Users <= All-Projects.

In addition, this commit adds a schema migration to migrate wrong
inheritance.

Change-Id: Id8e4f957d316cf401463dcce2042e604e19037bd
This commit is contained in:
Patrick Hiesel
2017-11-07 13:24:20 +01:00
parent b4a4c58120
commit 14959d1542
10 changed files with 258 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
java/com/google/gerrit/httpd/rpc/project/ProjectAccessHandler.java
View File

@@ -27,6 +27,7 @@ import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.common.errors.PermissionDeniedException;
import com.google.gerrit.common.errors.UpdateParentFailedException;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.httpd.rpc.Handler;
@@ -189,7 +190,7 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
+ "not an administrator. You may save the modifications for review "
+ "so that an administrator can approve them.",
e);
} catch (ResourceConflictException | UnprocessableEntityException e) {
} catch (ResourceConflictException | UnprocessableEntityException | BadRequestException e) {
throw new UpdateParentFailedException(e.getMessage(), e);
}
config.getProject().setParentName(parentProjectName);

8
java/com/google/gerrit/server/git/validators/CommitValidators.java
View File

@@ -427,6 +427,14 @@ public class CommitValidators {
throw new ConfigInvalidException("invalid project configuration");
}
}
if (allUsers.equals(receiveEvent.project.getNameKey())
&& !allProjects.equals(cfg.getProject().getParent(allProjects))) {
addError("Invalid project configuration:", messages);
addError(
String.format(" %s must inherit from %s", allUsers.get(), allProjects.get()),
messages);
throw new ConfigInvalidException("invalid project configuration");
}
} catch (ConfigInvalidException | IOException e) {
log.error(
"User "

10
java/com/google/gerrit/server/git/validators/MergeValidators.java
View File

@@ -117,6 +117,7 @@ public class MergeValidators {
+ "The change must be submitted by a Gerrit administrator.";
private final AllProjectsName allProjectsName;
private final AllUsersName allUsersName;
private final ProjectCache projectCache;
private final PermissionBackend permissionBackend;
private final DynamicMap<ProjectConfigEntry> pluginConfigEntries;
@@ -128,10 +129,12 @@ public class MergeValidators {
@Inject
public ProjectConfigValidator(
AllProjectsName allProjectsName,
AllUsersName allUsersName,
ProjectCache projectCache,
PermissionBackend permissionBackend,
DynamicMap<ProjectConfigEntry> pluginConfigEntries) {
this.allProjectsName = allProjectsName;
this.allUsersName = allUsersName;
this.projectCache = projectCache;
this.permissionBackend = permissionBackend;
this.pluginConfigEntries = pluginConfigEntries;
@@ -168,7 +171,12 @@ public class MergeValidators {
log.warn("Cannot check ADMINISTRATE_SERVER", e);
throw new MergeValidationException("validation unavailable");
}
if (allUsersName.equals(destProject.getNameKey())
&& !allProjectsName.equals(newParent)) {
throw new MergeValidationException(
String.format(
" %s must inherit from %s", allUsersName.get(), allProjectsName.get()));
}
if (projectCache.get(newParent) == null) {
throw new MergeValidationException(PARENT_NOT_FOUND);
}

5
java/com/google/gerrit/server/project/SetAccessUtil.java
View File

@@ -212,13 +212,14 @@ public class SetAccessUtil {
}
}
void setParentName(
public void setParentName(
IdentifiedUser identifiedUser,
ProjectConfig config,
Project.NameKey projectName,
Project.NameKey newParentProjectName,
boolean checkAdmin)
throws ResourceConflictException, AuthException, PermissionBackendException {
throws ResourceConflictException, AuthException, PermissionBackendException,
BadRequestException {
if (newParentProjectName != null
&& !config.getProject().getNameKey().equals(allProjects)
&& !config.getProject().getParent(allProjects).equals(newParentProjectName)) {

20
java/com/google/gerrit/server/project/SetParent.java
View File

@@ -21,6 +21,7 @@ import com.google.common.base.Strings;
import com.google.common.collect.Iterables;
import com.google.gerrit.extensions.api.projects.ParentInput;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -28,6 +29,7 @@ import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.permissions.GlobalPermission;
@@ -45,29 +47,34 @@ public class SetParent implements RestModifyView<ProjectResource, ParentInput> {
private final PermissionBackend permissionBackend;
private final MetaDataUpdate.Server updateFactory;
private final AllProjectsName allProjects;
private final AllUsersName allUsers;
@Inject
SetParent(
ProjectCache cache,
PermissionBackend permissionBackend,
MetaDataUpdate.Server updateFactory,
AllProjectsName allProjects) {
AllProjectsName allProjects,
AllUsersName allUsers) {
this.cache = cache;
this.permissionBackend = permissionBackend;
this.updateFactory = updateFactory;
this.allProjects = allProjects;
this.allUsers = allUsers;
}
@Override
public String apply(ProjectResource rsrc, ParentInput input)
throws AuthException, ResourceConflictException, ResourceNotFoundException,
UnprocessableEntityException, IOException, PermissionBackendException {
UnprocessableEntityException, IOException, PermissionBackendException,
BadRequestException {
return apply(rsrc, input, true);
}
public String apply(ProjectResource rsrc, ParentInput input, boolean checkIfAdmin)
throws AuthException, ResourceConflictException, ResourceNotFoundException,
UnprocessableEntityException, IOException, PermissionBackendException {
UnprocessableEntityException, IOException, PermissionBackendException,
BadRequestException {
IdentifiedUser user = rsrc.getUser().asIdentifiedUser();
String parentName =
MoreObjects.firstNonNull(Strings.emptyToNull(input.parent), allProjects.get());
@@ -102,11 +109,16 @@ public class SetParent implements RestModifyView<ProjectResource, ParentInput> {
public void validateParentUpdate(
Project.NameKey project, IdentifiedUser user, String newParent, boolean checkIfAdmin)
throws AuthException, ResourceConflictException, UnprocessableEntityException,
PermissionBackendException {
PermissionBackendException, BadRequestException {
if (checkIfAdmin) {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
}
if (project.equals(allUsers) && !allProjects.equals(newParent)) {
throw new BadRequestException(
String.format("%s must inherit from %s", allUsers.get(), allProjects.get()));
}
if (project.equals(allProjects)) {
throw new ResourceConflictException("cannot set parent of " + allProjects.get());
}

2
java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@@ -35,7 +35,7 @@ import java.util.concurrent.TimeUnit;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_161> C = Schema_161.class;
public static final Class<Schema_162> C = Schema_162.class;
public static int getBinaryVersion() {
return guessVersion(C);

71
java/com/google/gerrit/server/schema/Schema_162.java Normal file
View File

@@ -0,0 +1,71 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.schema;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import java.io.IOException;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
public class Schema_162 extends SchemaVersion {
private final GitRepositoryManager repoManager;
private final AllProjectsName allProjectsName;
private final AllUsersName allUsersName;
private final PersonIdent serverUser;
@Inject
Schema_162(
Provider<Schema_161> prior,
GitRepositoryManager repoManager,
AllProjectsName allProjectsName,
AllUsersName allUsersName,
@GerritPersonIdent PersonIdent serverUser) {
super(prior);
this.repoManager = repoManager;
this.allProjectsName = allProjectsName;
this.allUsersName = allUsersName;
this.serverUser = serverUser;
}
@Override
protected void migrateData(ReviewDb db, UpdateUI ui) throws OrmException {
try (Repository git = repoManager.openRepository(allUsersName);
MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, git)) {
ProjectConfig cfg = ProjectConfig.read(md);
if (allProjectsName.equals(cfg.getProject().getParent(allProjectsName))) {
return;
}
cfg.getProject().setParentName(allProjectsName);
md.getCommitBuilder().setAuthor(serverUser);
md.getCommitBuilder().setCommitter(serverUser);
md.setMessage(
String.format("Make %s inherit from %s", allUsersName.get(), allProjectsName.get()));
cfg.commit(md);
} catch (ConfigInvalidException | IOException ex) {
throw new OrmException(ex);
}
}
}

24
javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
View File

@@ -910,6 +910,30 @@ public class GroupsIT extends AbstractDaemonTest {
r.assertOkStatus();
}
@Test
public void pushCustomInheritanceForAllUsersFails() throws Exception {
TestRepository<InMemoryRepository> repo = cloneProject(allUsers, RefNames.REFS_CONFIG);
String config =
gApi.projects()
.name(allUsers.get())
.branch(RefNames.REFS_CONFIG)
.file("project.config")
.asString();
Config cfg = new Config();
cfg.fromText(config);
cfg.setString("access", null, "inheritFrom", project.get());
config = cfg.toText();
PushOneCommit.Result r =
pushFactory
.create(db, admin.getIdent(), repo, "Subject", "project.config", config)
.to(RefNames.REFS_CONFIG);
r.assertErrorStatus("invalid project configuration");
r.assertMessage("All-Users must inherit from All-Projects");
}
private void pushToGroupBranchForReviewAndSubmit(Project.NameKey project, String expectedError)
throws Exception {
grantLabel(

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

@@ -39,9 +39,13 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllProjectsNameProvider;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.inject.Inject;
import java.util.HashMap;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
@@ -60,6 +64,9 @@ public class AccessIT extends AbstractDaemonTest {
private static final String LABEL_CODE_REVIEW = "Code-Review";
@Inject private AllProjectsName allProjects;
@Inject private AllUsersName allUsers;
private String newProjectName;
private ProjectApi pApi;
@@ -480,7 +487,7 @@ public class AccessIT extends AbstractDaemonTest {
gApi.projects()
.name(allProjects.get())
.branch(RefNames.REFS_CONFIG)
.file("project.config")
.file(ProjectConfig.PROJECT_CONFIG)
.asString();
// Append and push unknown permission
@@ -490,7 +497,7 @@ public class AccessIT extends AbstractDaemonTest {
config = cfg.toText();
PushOneCommit push =
pushFactory.create(
db, admin.getIdent(), allProjectsRepo, "Subject", "project.config", config);
db, admin.getIdent(), allProjectsRepo, "Subject", ProjectConfig.PROJECT_CONFIG, config);
push.to(RefNames.REFS_CONFIG).assertOkStatus();
// Verify that unknownPermission is present
@@ -498,7 +505,7 @@ public class AccessIT extends AbstractDaemonTest {
gApi.projects()
.name(allProjects.get())
.branch(RefNames.REFS_CONFIG)
.file("project.config")
.file(ProjectConfig.PROJECT_CONFIG)
.asString();
cfg.fromText(config);
assertThat(cfg.getString(access, refsFor, unknownPermission)).isEqualTo(registeredUsers);
@@ -517,7 +524,7 @@ public class AccessIT extends AbstractDaemonTest {
gApi.projects()
.name(allProjects.get())
.branch(RefNames.REFS_CONFIG)
.file("project.config")
.file(ProjectConfig.PROJECT_CONFIG)
.asString();
cfg.fromText(config);
assertThat(cfg.getString(access, refsFor, unknownPermission)).isEqualTo(registeredUsers);
@@ -540,6 +547,15 @@ public class AccessIT extends AbstractDaemonTest {
gApi.projects().name(project.get()).access(accessInput);
}
@Test
public void allUsersCanOnlyInheritFromAllProjects() throws Exception {
ProjectAccessInput accessInput = newProjectAccessInput();
accessInput.parent = project.get();
exception.expect(BadRequestException.class);
exception.expectMessage(allUsers.get() + " must inherit from " + allProjects.get());
gApi.projects().name(allUsers.get()).access(accessInput);
}
private ProjectAccessInput newProjectAccessInput() {
ProjectAccessInput p = new ProjectAccessInput();
p.add = new HashMap<>();

104
javatests/com/google/gerrit/server/schema/Schema_161_to_162_Test.java Normal file
View File

@@ -0,0 +1,104 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.schema;
import static com.google.common.truth.Truth.assertThat;
import com.google.gerrit.extensions.api.GerritApi;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.testing.SchemaUpgradeTestEnvironment;
import com.google.gerrit.testing.TestUpdateUI;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import java.io.IOException;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
public class Schema_161_to_162_Test {
@Rule public SchemaUpgradeTestEnvironment testEnv = new SchemaUpgradeTestEnvironment();
@Inject private AllProjectsName allProjectsName;
@Inject private AllUsersName allUsersName;
@Inject private GerritApi gApi;
@Inject private GitRepositoryManager repoManager;
@Inject private Schema_162 schema162;
@Inject @GerritPersonIdent private PersonIdent serverUser;
private ReviewDb db;
@Before
public void setUp() throws Exception {
testEnv.getInjector().injectMembers(this);
db = testEnv.getDb();
}
@Test
public void skipCorrectInheritance() throws Exception {
assertThatAllUsersInheritsFrom(allProjectsName.get());
ObjectId oldHead;
try (Repository git = repoManager.openRepository(allUsersName)) {
oldHead = git.findRef(RefNames.REFS_CONFIG).getObjectId();
}
schema162.migrateData(db, new TestUpdateUI());
// Check that the parent remained unchanged and that no commit was made
assertThatAllUsersInheritsFrom(allProjectsName.get());
try (Repository git = repoManager.openRepository(allUsersName)) {
assertThat(oldHead).isEqualTo(git.findRef(RefNames.REFS_CONFIG).getObjectId());
}
}
@Test
public void fixIncorrectInheritance() throws Exception {
String testProject = gApi.projects().create("test").get().name;
assertThatAllUsersInheritsFrom(allProjectsName.get());
try (Repository git = repoManager.openRepository(allUsersName);
MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, allUsersName, git)) {
ProjectConfig cfg = ProjectConfig.read(md);
cfg.getProject().setParentName(testProject);
md.getCommitBuilder().setCommitter(serverUser);
md.getCommitBuilder().setAuthor(serverUser);
md.setMessage("Test");
cfg.commit(md);
} catch (ConfigInvalidException | IOException ex) {
throw new OrmException(ex);
}
assertThatAllUsersInheritsFrom(testProject);
schema162.migrateData(db, new TestUpdateUI());
assertThatAllUsersInheritsFrom(allProjectsName.get());
}
private void assertThatAllUsersInheritsFrom(String parent) throws Exception {
assertThat(gApi.projects().name(allUsersName.get()).access().inheritsFrom.name)
.isEqualTo(parent);
}
}