From 160953521fcd4af8dbf4d3249f33f0317b457cf7 Mon Sep 17 00:00:00 2001 From: Changcheng Xiao Date: Mon, 18 Dec 2017 10:03:08 +0100 Subject: [PATCH] Add config to disable private changes This commit adds a config option so that we can disable private changes. With this config enabled, users will not be able to create private changes through UI or Git push. Users can't set a change to be private through the PostPrivate REST endpoint, either. Change-Id: I2794c140f37bd3940b0a2fad68abbced1145c843 --- Documentation/config-gerrit.txt | 6 + Documentation/rest-api-config.txt | 2 + .../extensions/common/ChangeConfigInfo.java | 1 + .../server/git/receive/ReceiveCommits.java | 20 ++- .../server/git/receive/ReceiveConfig.java | 2 + .../server/restapi/change/CreateChange.java | 13 +- .../server/restapi/change/PostPrivate.java | 14 ++- .../server/restapi/config/GetServerInfo.java | 2 + .../api/change/DisablePrivateChangesIT.java | 118 ++++++++++++++++++ .../rest/change/PrivateByDefaultIT.java | 41 ++++++ .../acceptance/rest/config/ServerInfoIT.java | 3 + 11 files changed, 212 insertions(+), 10 deletions(-) create mode 100644 javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 3fde9e46b4..5b3dbfa04e 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -1252,6 +1252,12 @@ Zero or negative values allow robot comments of unlimited size. + The default limit is 1024kB. +[[change.disablePrivateChanges]]change.disablePrivateChanges:: ++ +If set to true, users are not allowed to create private changes. ++ +The default is false. + [[changeCleanup]] === Section changeCleanup diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt index 6e02786075..8aa1f429b8 100644 --- a/Documentation/rest-api-config.txt +++ b/Documentation/rest-api-config.txt @@ -1398,6 +1398,8 @@ interface should poll for updates to the currently open change]. |`submit_whole_topic` || link:config-gerrit.html#change.submitWholeTopic[A configuration if the whole topic is submitted]. +|`disable_private_changes` |not set if `false`| +Returns true if private changes are disabled. |============================= [[check-account-external-ids-input]] diff --git a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java index b710121ec7..9e02ae51c6 100644 --- a/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java +++ b/java/com/google/gerrit/extensions/common/ChangeConfigInfo.java @@ -18,6 +18,7 @@ public class ChangeConfigInfo { public Boolean allowBlame; public Boolean showAssigneeInChangesTable; public Boolean allowDrafts; + public Boolean disablePrivateChanges; public int largeChange; public String replyLabel; public String replyTooltip; diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index a058aec797..d8c0469479 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -376,6 +376,7 @@ class ReceiveCommits { private MagicBranchInput magicBranch; private boolean newChangeForAllNotInTarget; private String setFullNameTo; + private boolean setChangeAsPrivate; // Handles for outputting back over the wire to the end user. private Task newProgress; @@ -1521,6 +1522,18 @@ class ReceiveCommits { return; } + boolean privateByDefault = + projectCache.get(project.getNameKey()).is(BooleanProjectConfig.PRIVATE_BY_DEFAULT); + setChangeAsPrivate = + magicBranch.draft + || magicBranch.isPrivate + || (privateByDefault && !magicBranch.removePrivate); + + if (receiveConfig.disablePrivateChanges && setChangeAsPrivate) { + reject(cmd, "private changes are disabled"); + return; + } + if (magicBranch.workInProgress && magicBranch.ready) { reject(cmd, "the options 'wip' and 'ready' are mutually exclusive"); return; @@ -2133,18 +2146,13 @@ class ReceiveCommits { } private void setChangeId(int id) { - boolean privateByDefault = - projectCache.get(project.getNameKey()).is(BooleanProjectConfig.PRIVATE_BY_DEFAULT); changeId = new Change.Id(id); ins = changeInserterFactory .create(changeId, commit, refName) .setTopic(magicBranch.topic) - .setPrivate( - magicBranch.draft - || magicBranch.isPrivate - || (privateByDefault && !magicBranch.removePrivate)) + .setPrivate(setChangeAsPrivate) .setWorkInProgress(magicBranch.workInProgress) // Changes already validated in validateNewCommits. .setValidate(false); diff --git a/java/com/google/gerrit/server/git/receive/ReceiveConfig.java b/java/com/google/gerrit/server/git/receive/ReceiveConfig.java index 89158d30fa..cdbf3108c8 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveConfig.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveConfig.java @@ -28,6 +28,7 @@ class ReceiveConfig { final boolean checkMagicRefs; final boolean checkReferencedObjectsAreReachable; final int maxBatchCommits; + final boolean disablePrivateChanges; private final int systemMaxBatchChanges; private final AccountLimits.Factory limitsFactory; @@ -38,6 +39,7 @@ class ReceiveConfig { config.getBoolean("receive", null, "checkReferencedObjectsAreReachable", true); maxBatchCommits = config.getInt("receive", null, "maxBatchCommits", 10000); systemMaxBatchChanges = config.getInt("receive", "maxBatchChanges", 0); + disablePrivateChanges = config.getBoolean("change", null, "disablePrivateChanges", false); this.limitsFactory = limitsFactory; } diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java index d4e1c405d3..de9e71231a 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateChange.java +++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java @@ -28,6 +28,7 @@ import com.google.gerrit.extensions.common.ChangeInput; import com.google.gerrit.extensions.common.MergeInput; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; @@ -114,6 +115,7 @@ public class CreateChange private final SubmitType submitType; private final NotifyUtil notifyUtil; private final ContributorAgreementsChecker contributorAgreements; + private final boolean disablePrivateChanges; @Inject CreateChange( @@ -152,6 +154,7 @@ public class CreateChange this.changeFinder = changeFinder; this.psUtil = psUtil; this.submitType = config.getEnum("project", null, "submitType", SubmitType.MERGE_IF_NECESSARY); + this.disablePrivateChanges = config.getBoolean("change", null, "disablePrivateChanges", false); this.mergeUtilFactory = mergeUtilFactory; this.notifyUtil = notifyUtil; this.contributorAgreements = contributorAgreements; @@ -181,6 +184,13 @@ public class CreateChange } ProjectResource rsrc = projectsCollection.parse(input.project); + boolean privateByDefault = rsrc.getProjectState().is(BooleanProjectConfig.PRIVATE_BY_DEFAULT); + boolean isPrivate = input.isPrivate == null ? privateByDefault : input.isPrivate; + + if (isPrivate && disablePrivateChanges) { + throw new MethodNotAllowedException("private changes are disabled"); + } + contributorAgreements.check(rsrc.getNameKey(), rsrc.getUser()); Project.NameKey project = rsrc.getNameKey(); @@ -257,7 +267,6 @@ public class CreateChange c = newCommit(oi, rw, author, mergeTip, commitMessage); } - boolean privateByDefault = rsrc.getProjectState().is(BooleanProjectConfig.PRIVATE_BY_DEFAULT); Change.Id changeId = new Change.Id(seq.nextChangeId()); ChangeInserter ins = changeInserterFactory.create(changeId, c, refName); ins.setMessage(String.format("Uploaded patch set %s.", ins.getPatchSetId().get())); @@ -266,7 +275,7 @@ public class CreateChange topic = Strings.emptyToNull(topic.trim()); } ins.setTopic(topic); - ins.setPrivate(input.isPrivate == null ? privateByDefault : input.isPrivate); + ins.setPrivate(isPrivate); ins.setWorkInProgress(input.workInProgress != null && input.workInProgress); ins.setGroups(groups); ins.setNotify(input.notify); diff --git a/java/com/google/gerrit/server/restapi/change/PostPrivate.java b/java/com/google/gerrit/server/restapi/change/PostPrivate.java index 9f02fa8dbe..5a13346069 100644 --- a/java/com/google/gerrit/server/restapi/change/PostPrivate.java +++ b/java/com/google/gerrit/server/restapi/change/PostPrivate.java @@ -20,6 +20,7 @@ import static com.google.gerrit.extensions.conditions.BooleanCondition.or; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.extensions.conditions.BooleanCondition; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.webui.UiAction; @@ -27,6 +28,7 @@ import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.change.ChangeResource; +import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.update.BatchUpdate; @@ -36,6 +38,7 @@ import com.google.gerrit.server.update.UpdateException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; +import org.eclipse.jgit.lib.Config; @Singleton public class PostPrivate @@ -45,6 +48,7 @@ public class PostPrivate private final Provider dbProvider; private final PermissionBackend permissionBackend; private final SetPrivateOp.Factory setPrivateOpFactory; + private final boolean disablePrivateChanges; @Inject PostPrivate( @@ -52,18 +56,24 @@ public class PostPrivate RetryHelper retryHelper, ChangeMessagesUtil cmUtil, PermissionBackend permissionBackend, - SetPrivateOp.Factory setPrivateOpFactory) { + SetPrivateOp.Factory setPrivateOpFactory, + @GerritServerConfig Config config) { super(retryHelper); this.dbProvider = dbProvider; this.cmUtil = cmUtil; this.permissionBackend = permissionBackend; this.setPrivateOpFactory = setPrivateOpFactory; + this.disablePrivateChanges = config.getBoolean("change", null, "disablePrivateChanges", false); } @Override public Response applyImpl( BatchUpdate.Factory updateFactory, ChangeResource rsrc, SetPrivateOp.Input input) throws RestApiException, UpdateException { + if (disablePrivateChanges) { + throw new MethodNotAllowedException("private changes are disabled"); + } + if (!canSetPrivate(rsrc).value()) { throw new AuthException("not allowed to mark private"); } @@ -88,7 +98,7 @@ public class PostPrivate return new UiAction.Description() .setLabel("Mark private") .setTitle("Mark change as private") - .setVisible(and(!change.isPrivate(), canSetPrivate(rsrc))); + .setVisible(and(!disablePrivateChanges && !change.isPrivate(), canSetPrivate(rsrc))); } private BooleanCondition canSetPrivate(ChangeResource rsrc) { diff --git a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java index 4c23f59f86..f31277d812 100644 --- a/java/com/google/gerrit/server/restapi/config/GetServerInfo.java +++ b/java/com/google/gerrit/server/restapi/config/GetServerInfo.java @@ -246,6 +246,8 @@ public class GetServerInfo implements RestReadView { info.updateDelay = (int) ConfigUtil.getTimeUnit(cfg, "change", null, "updateDelay", 300, TimeUnit.SECONDS); info.submitWholeTopic = MergeSuperSet.wholeTopicEnabled(cfg); + info.disablePrivateChanges = + toBoolean(config.getBoolean("change", null, "disablePrivateChanges", false)); return info; } diff --git a/javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java b/javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java new file mode 100644 index 0000000000..0ff6fb2c24 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/api/change/DisablePrivateChangesIT.java @@ -0,0 +1,118 @@ +// 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.acceptance.api.change; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GerritConfig; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.extensions.common.ChangeInput; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import org.eclipse.jgit.revwalk.RevCommit; +import org.junit.Test; + +public class DisablePrivateChangesIT extends AbstractDaemonTest { + + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void createPrivateChangeWithDisablePrivateChangesTrue() throws Exception { + ChangeInput input = new ChangeInput(project.get(), "master", "empty change"); + input.isPrivate = true; + exception.expect(MethodNotAllowedException.class); + exception.expectMessage("private changes are disabled"); + gApi.changes().create(input); + } + + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void createNonPrivateChangeWithDisablePrivateChangesTrue() throws Exception { + ChangeInput input = new ChangeInput(project.get(), "master", "empty change"); + assertThat(gApi.changes().create(input).get().isPrivate).isNull(); + } + + @Test + public void createPrivateChangeWithDisablePrivateChangesFalse() throws Exception { + ChangeInput input = new ChangeInput(project.get(), "master", "empty change"); + input.isPrivate = true; + assertThat(gApi.changes().create(input).get().isPrivate).isEqualTo(true); + } + + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void pushPrivatesWithDisablePrivateChangesTrue() throws Exception { + PushOneCommit.Result result = + pushFactory.create(db, admin.getIdent(), testRepo).to("refs/for/master%private"); + result.assertErrorStatus(); + } + + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void pushDraftsWithDisablePrivateChangesTrue() throws Exception { + RevCommit initialHead = getRemoteHead(); + PushOneCommit.Result result = + pushFactory.create(db, admin.getIdent(), testRepo).to("refs/for/master%draft"); + result.assertErrorStatus(); + + testRepo.reset(initialHead); + result = pushFactory.create(db, admin.getIdent(), testRepo).to("refs/drafts/master"); + result.assertErrorStatus(); + } + + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void pushWithDisablePrivateChangesTrue() throws Exception { + PushOneCommit.Result result = + pushFactory.create(db, admin.getIdent(), testRepo).to("refs/for/master"); + result.assertOkStatus(); + assertThat(result.getChange().change().isPrivate()).isFalse(); + } + + @Test + public void pushPrivatesWithDisablePrivateChangesFalse() throws Exception { + PushOneCommit.Result result = + pushFactory.create(db, admin.getIdent(), testRepo).to("refs/for/master%private"); + assertThat(result.getChange().change().isPrivate()).isEqualTo(true); + } + + @Test + public void pushDraftsWithDisablePrivateChangesFalse() throws Exception { + RevCommit initialHead = getRemoteHead(); + PushOneCommit.Result result = + pushFactory.create(db, admin.getIdent(), testRepo).to("refs/for/master%draft"); + assertThat(result.getChange().change().isPrivate()).isEqualTo(true); + + testRepo.reset(initialHead); + result = pushFactory.create(db, admin.getIdent(), testRepo).to("refs/drafts/master"); + assertThat(result.getChange().change().isPrivate()).isEqualTo(true); + } + + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void setPrivateWithDisablePrivateChangesTrue() throws Exception { + PushOneCommit.Result result = createChange(); + + exception.expect(MethodNotAllowedException.class); + exception.expectMessage("private changes are disabled"); + gApi.changes().id(result.getChangeId()).setPrivate(true, "set private"); + } + + @Test + public void setPrivateWithDisablePrivateChangesFalse() throws Exception { + PushOneCommit.Result result = createChange(); + gApi.changes().id(result.getChangeId()).setPrivate(true, "set private"); + assertThat(gApi.changes().id(result.getChangeId()).get().isPrivate).isTrue(); + } +} diff --git a/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java b/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java index a10062ca1e..993c144618 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/PrivateByDefaultIT.java @@ -17,14 +17,17 @@ package com.google.gerrit.acceptance.rest.change; import static com.google.common.truth.Truth.assertThat; import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.GerritConfig; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.extensions.api.projects.ConfigInput; import com.google.gerrit.extensions.client.InheritableBoolean; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.extensions.common.ChangeInput; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.reviewdb.client.Project; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; +import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Before; import org.junit.Test; @@ -69,6 +72,17 @@ public class PrivateByDefaultIT extends AbstractDaemonTest { assertThat(info.isPrivate).isTrue(); } + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void createChangeWithPrivateByDefaultAndDisablePrivateChangesTrue() throws Exception { + setPrivateByDefault(project2, InheritableBoolean.TRUE); + + ChangeInput input = new ChangeInput(project2.get(), "master", "empty change"); + exception.expect(MethodNotAllowedException.class); + exception.expectMessage("private changes are disabled"); + gApi.changes().create(input); + } + @Test public void pushWithPrivateByDefaultEnabled() throws Exception { setPrivateByDefault(project2, InheritableBoolean.TRUE); @@ -97,6 +111,33 @@ public class PrivateByDefaultIT extends AbstractDaemonTest { assertThat(createChange(project2).getChange().change().isPrivate()).isEqualTo(true); } + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void pushPrivatesWithPrivateByDefaultAndDisablePrivateChangesTrue() throws Exception { + setPrivateByDefault(project2, InheritableBoolean.TRUE); + + TestRepository testRepo = cloneProject(project2); + PushOneCommit.Result result = + pushFactory.create(db, admin.getIdent(), testRepo).to("refs/for/master%private"); + result.assertErrorStatus(); + } + + @Test + @GerritConfig(name = "change.disablePrivateChanges", value = "true") + public void pushDraftsWithPrivateByDefaultAndDisablePrivateChangesTrue() throws Exception { + setPrivateByDefault(project2, InheritableBoolean.TRUE); + + RevCommit initialHead = getRemoteHead(); + TestRepository testRepo = cloneProject(project2); + PushOneCommit.Result result = + pushFactory.create(db, admin.getIdent(), testRepo).to("refs/for/master%draft"); + result.assertErrorStatus(); + + testRepo.reset(initialHead); + result = pushFactory.create(db, admin.getIdent(), testRepo).to("refs/drafts/master"); + result.assertErrorStatus(); + } + private void setPrivateByDefault(Project.NameKey proj, InheritableBoolean value) throws Exception { ConfigInput input = new ConfigInput(); diff --git a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java index 38462c043e..8fc5312a4a 100644 --- a/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/config/ServerInfoIT.java @@ -61,6 +61,7 @@ public class ServerInfoIT extends AbstractDaemonTest { @GerritConfig(name = "change.replyTooltip", value = "Publish votes and draft comments") @GerritConfig(name = "change.replyLabel", value = "Vote") @GerritConfig(name = "change.updateDelay", value = "50s") + @GerritConfig(name = "change.disablePrivateChanges", value = "true") // download @GerritConfig( @@ -105,6 +106,7 @@ public class ServerInfoIT extends AbstractDaemonTest { assertThat(i.change.replyTooltip).startsWith("Publish votes and draft comments"); assertThat(i.change.replyLabel).isEqualTo("Vote\u2026"); assertThat(i.change.updateDelay).isEqualTo(50); + assertThat(i.change.disablePrivateChanges).isTrue(); // download assertThat(i.download.archives).containsExactly("tar", "tbz2", "tgz", "txz"); @@ -178,6 +180,7 @@ public class ServerInfoIT extends AbstractDaemonTest { assertThat(i.change.replyTooltip).startsWith("Reply and score"); assertThat(i.change.replyLabel).isEqualTo("Reply\u2026"); assertThat(i.change.updateDelay).isEqualTo(300); + assertThat(i.change.disablePrivateChanges).isNull(); // download assertThat(i.download.archives).containsExactly("tar", "tbz2", "tgz", "txz");