opendev/gerrit
Code Issues Proposed changes

Merge "Add project scope feature to contributor-agreement enforcement."

Browse Source
This commit is contained in:
Patrick Hiesel
2018-10-18 07:14:41 +00:00
committed by Gerrit Code Review
parent d42d8545a0 c48da97cfd
commit 0f88f61f50
8 changed files with 173 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
Documentation/config-cla.txt
View File

@@ -25,13 +25,15 @@ Download the existing configuration from Gerrit:
----
Contributor agreements are defined as contributor-agreement sections in
`project.config`:
`project.config` of `All-Projects`:
----
[contributor-agreement "Individual"]
description = If you are going to be contributing code on your own, this is the one you want. You can sign this one online.
agreementUrl = static/cla_individual.html
autoVerify = group CLA Accepted - Individual
accepted = group CLA Accepted - Individual
matchProjects = ^/.*$
excludeProjects = ^/not/my/project/
----
Each `contributor-agreement` section within the `project.config` file must
@@ -75,6 +77,16 @@ List of groups that will be considered when verifying that a
contributor agreement has been accepted. The groups' UUID must also
appear in the `groups` file.
[[contributor-agreement.name.matchProjects]]contributor-agreement.<name>.matchProjects::
+
List of project regular expressions identifying projects where the
agreement is required. Defaults to every project when omitted.
[[contributor-agreement.name.excludeProjects]]contributor-agreement.<name>.excludeProjects::
+
List of project regular expressions identifying projects where the
agreement does not apply. Defaults to empty. i.e. no projects excluded.
GERRIT
------
Part of link:index.html[Gerrit Code Review]

16
java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -1245,14 +1245,14 @@ public abstract class AbstractDaemonTest {
protected ContributorAgreement configureContributorAgreement(boolean autoVerify)
throws Exception {
ContributorAgreement ca;
String g = createGroup(autoVerify ? "cla-test-group" : "cla-test-no-auto-verify-group");
GroupApi groupApi = gApi.groups().id(g);
groupApi.description("CLA test group");
InternalGroup caGroup = group(new AccountGroup.UUID(groupApi.detail().id));
GroupReference groupRef = new GroupReference(caGroup.getGroupUUID(), caGroup.getName());
PermissionRule rule = new PermissionRule(groupRef);
rule.setAction(PermissionRule.Action.ALLOW);
if (autoVerify) {
String g = createGroup("cla-test-group");
GroupApi groupApi = gApi.groups().id(g);
groupApi.description("CLA test group");
InternalGroup caGroup = group(new AccountGroup.UUID(groupApi.detail().id));
GroupReference groupRef = new GroupReference(caGroup.getGroupUUID(), caGroup.getName());
PermissionRule rule = new PermissionRule(groupRef);
rule.setAction(PermissionRule.Action.ALLOW);
ca = new ContributorAgreement("cla-test");
ca.setAutoVerify(groupRef);
ca.setAccepted(ImmutableList.of(rule));
@@ -1261,6 +1261,8 @@ public abstract class AbstractDaemonTest {
}
ca.setDescription("description");
ca.setAgreementUrl("agreement-url");
ca.setAccepted(ImmutableList.of(rule));
ca.setExcludeProjectsRegexes(ImmutableList.of("ExcludedProject"));
try (ProjectConfigUpdate u = updateProject(allProjects)) {
u.getConfig().replace(ca);

24
java/com/google/gerrit/common/data/ContributorAgreement.java
View File

@@ -25,6 +25,8 @@ public class ContributorAgreement implements Comparable<ContributorAgreement> {
protected List<PermissionRule> accepted;
protected GroupReference autoVerify;
protected String agreementUrl;
protected List<String> excludeProjectsRegexes;
protected List<String> matchProjectsRegexes;
protected ContributorAgreement() {}
@@ -75,6 +77,28 @@ public class ContributorAgreement implements Comparable<ContributorAgreement> {
this.agreementUrl = agreementUrl;
}
public List<String> getExcludeProjectsRegexes() {
if (excludeProjectsRegexes == null) {
excludeProjectsRegexes = new ArrayList<>();
}
return excludeProjectsRegexes;
}
public void setExcludeProjectsRegexes(List<String> excludeProjectsRegexes) {
this.excludeProjectsRegexes = excludeProjectsRegexes;
}
public List<String> getMatchProjectsRegexes() {
if (matchProjectsRegexes == null) {
matchProjectsRegexes = new ArrayList<>();
}
return matchProjectsRegexes;
}
public void setMatchProjectsRegexes(List<String> matchProjectsRegexes) {
this.matchProjectsRegexes = matchProjectsRegexes;
}
@Override
public int compareTo(ContributorAgreement o) {
return getName().compareTo(o.getName());

40
java/com/google/gerrit/server/project/ContributorAgreementsChecker.java
View File

@@ -14,6 +14,9 @@
package com.google.gerrit.server.project;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import com.google.gerrit.common.data.ContributorAgreement;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
@@ -34,6 +37,8 @@ import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@Singleton
public class ContributorAgreementsChecker {
@@ -93,6 +98,20 @@ public class ContributorAgreementsChecker {
List<AccountGroup.UUID> groupIds;
groupIds = okGroupIds;
// matchProjects defaults to match all projects when missing.
List<String> matchProjectsRegexes = ca.getMatchProjectsRegexes();
if (!matchProjectsRegexes.isEmpty()
&& !projectMatchesAnyPattern(project.get(), matchProjectsRegexes)) {
// Doesn't match, isn't checked.
continue;
}
// excludeProjects defaults to exclude no projects when missing.
List<String> excludeProjectsRegexes = ca.getExcludeProjectsRegexes();
if (!excludeProjectsRegexes.isEmpty()
&& projectMatchesAnyPattern(project.get(), excludeProjectsRegexes)) {
// Matches, isn't checked.
continue;
}
for (PermissionRule rule : ca.getAccepted()) {
if ((rule.getAction() == Action.ALLOW)
&& (rule.getGroup() != null)
@@ -102,7 +121,7 @@ public class ContributorAgreementsChecker {
}
}
if (!iUser.getEffectiveGroups().containsAnyOf(okGroupIds)) {
if (!okGroupIds.isEmpty() && !iUser.getEffectiveGroups().containsAnyOf(okGroupIds)) {
final StringBuilder msg = new StringBuilder();
msg.append("No Contributor Agreement on file for user ")
.append(iUser.getNameEmail())
@@ -114,4 +133,23 @@ public class ContributorAgreementsChecker {
throw new AuthException(msg.toString());
}
}
private boolean projectMatchesAnyPattern(String projectName, List<String> regexes) {
checkNotNull(regexes);
checkArgument(!regexes.isEmpty());
for (String patternString : regexes) {
Pattern pattern;
try {
pattern = Pattern.compile(patternString);
} catch (PatternSyntaxException e) {
// Should never happen: Regular expressions validated when reading project.config.
throw new IllegalStateException(
"Invalid matchProjects or excludeProjects clause in project.config", e);
}
if (pattern.matcher(projectName).find()) {
return true;
}
}
return false;
}
}

35
java/com/google/gerrit/server/project/ProjectConfig.java
View File

@@ -127,6 +127,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
private static final String KEY_ACCEPTED = "accepted";
private static final String KEY_AUTO_VERIFY = "autoVerify";
private static final String KEY_AGREEMENT_URL = "agreementUrl";
private static final String KEY_MATCH_PROJECTS = "matchProjects";
private static final String KEY_EXCLUDE_PROJECTS = "excludeProjects";
private static final String NOTIFY = "notify";
private static final String KEY_EMAIL = "email";
@@ -593,6 +595,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
ca.setAgreementUrl(rc.getString(CONTRIBUTOR_AGREEMENT, name, KEY_AGREEMENT_URL));
ca.setAccepted(
loadPermissionRules(rc, CONTRIBUTOR_AGREEMENT, name, KEY_ACCEPTED, groupsByName, false));
ca.setExcludeProjectsRegexes(
loadPatterns(rc, CONTRIBUTOR_AGREEMENT, name, KEY_EXCLUDE_PROJECTS));
ca.setMatchProjectsRegexes(loadPatterns(rc, CONTRIBUTOR_AGREEMENT, name, KEY_MATCH_PROJECTS));
List<PermissionRule> rules =
loadPermissionRules(
@@ -753,6 +758,22 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
}
}
private ImmutableList<String> loadPatterns(
Config rc, String section, String subsection, String varName) {
ImmutableList.Builder<String> patterns = ImmutableList.builder();
for (String patternString : rc.getStringList(section, subsection, varName)) {
try {
// While one could just use getStringList directly, compiling first will cause the server
// to fail fast if any of the patterns are invalid.
patterns.add(Pattern.compile(patternString).pattern());
} catch (PatternSyntaxException e) {
error(new ValidationError(PROJECT_CONFIG, "Invalid regular expression: " + e.getMessage()));
continue;
}
}
return patterns.build();
}
private ImmutableList<PermissionRule> loadPermissionRules(
Config rc,
String section,
@@ -1163,6 +1184,16 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
ca.getName(),
KEY_ACCEPTED,
ruleToStringList(ca.getAccepted(), keepGroups));
rc.setStringList(
CONTRIBUTOR_AGREEMENT,
ca.getName(),
KEY_EXCLUDE_PROJECTS,
patternToStringList(ca.getExcludeProjectsRegexes()));
rc.setStringList(
CONTRIBUTOR_AGREEMENT,
ca.getName(),
KEY_MATCH_PROJECTS,
patternToStringList(ca.getMatchProjectsRegexes()));
}
}
@@ -1206,6 +1237,10 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
}
}
private List<String> patternToStringList(List<String> list) {
return list;
}
private List<String> ruleToStringList(
List<PermissionRule> list, Set<AccountGroup.UUID> keepGroups) {
List<String> rules = new ArrayList<>();

33
javatests/com/google/gerrit/acceptance/api/accounts/AgreementsIT.java
View File

@@ -181,6 +181,28 @@ public class AgreementsIT extends AbstractDaemonTest {
gApi.changes().id(change.changeId).revert();
}
@Test
public void revertExcludedProjectChangeWithoutCLA() throws Exception {
// Contributor agreements configured with excludeProjects = ExcludedProject
// in AbstractDaemonTest.configureContributorAgreement(...)
assume().that(isContributorAgreementsEnabled()).isTrue();
// Create a change succeeds when agreement is not required
setUseContributorAgreements(InheritableBoolean.FALSE);
// Project name includes test method name which contains ExcludedProject
ChangeInfo change = gApi.changes().create(newChangeInput()).get();
// Approve and submit it
setApiUser(admin);
gApi.changes().id(change.changeId).current().review(ReviewInput.approve());
gApi.changes().id(change.changeId).current().submit(new SubmitInput());
// Revert in excluded project is allowed even when CLA is required but not signed
setApiUser(user);
setUseContributorAgreements(InheritableBoolean.TRUE);
gApi.changes().id(change.changeId).revert();
}
@Test
public void cherrypickChangeWithoutCLA() throws Exception {
assume().that(isContributorAgreementsEnabled()).isTrue();
@@ -240,6 +262,17 @@ public class AgreementsIT extends AbstractDaemonTest {
gApi.changes().create(newChangeInput());
}
@Test
public void createExcludedProjectChangeIgnoresCLA() throws Exception {
// Contributor agreements configured with excludeProjects = ExcludedProject
// in AbstractDaemonTest.configureContributorAgreement(...)
assume().that(isContributorAgreementsEnabled()).isTrue();
// Create a change in excluded project is allowed even when CLA is required but not signed.
setUseContributorAgreements(InheritableBoolean.TRUE);
gApi.changes().create(newChangeInput());
}
private void assertAgreement(AgreementInfo info, ContributorAgreement ca) {
assertThat(info.name).isEqualTo(ca.getName());
assertThat(info.description).isEqualTo(ca.getDescription());

1
javatests/com/google/gerrit/acceptance/edit/ChangeEditIT.java
View File

@@ -239,6 +239,7 @@ public class ChangeEditIT extends AbstractDaemonTest {
@Test
public void publishEditRestWithoutCLA() throws Exception {
configureContributorAgreement(true);
createArbitraryEditFor(changeId);
setUseContributorAgreements(InheritableBoolean.TRUE);
adminRestSession.post(urlPublish(changeId)).assertForbidden();

19
javatests/com/google/gerrit/server/project/ProjectConfigTest.java
View File

@@ -104,6 +104,13 @@ public class ProjectConfigTest extends GerritBaseTests {
+ " sameGroupVisibility = block group Staff\n"
+ "[contributor-agreement \"Individual\"]\n"
+ " description = A simple description\n"
+ " matchProjects = ^/ourproject\n"
+ " matchProjects = ^/ourotherproject\n"
+ " matchProjects = ^/someotherroot/ourproject\n"
+ " excludeProjects = ^/theirproject\n"
+ " excludeProjects = ^/theirotherproject\n"
+ " excludeProjects = ^/someotherroot/theirproject\n"
+ " excludeProjects = ^/someotherroot/theirotherproject\n"
+ " accepted = group Developers\n"
+ " accepted = group Staff\n"
+ " autoVerify = group Developers\n"
@@ -115,6 +122,14 @@ public class ProjectConfigTest extends GerritBaseTests {
ContributorAgreement ca = cfg.getContributorAgreement("Individual");
assertThat(ca.getName()).isEqualTo("Individual");
assertThat(ca.getDescription()).isEqualTo("A simple description");
assertThat(ca.getMatchProjectsRegexes())
.containsExactly("^/ourproject", "^/ourotherproject", "^/someotherroot/ourproject");
assertThat(ca.getExcludeProjectsRegexes())
.containsExactly(
"^/theirproject",
"^/theirotherproject",
"^/someotherroot/theirproject",
"^/someotherroot/theirotherproject");
assertThat(ca.getAgreementUrl()).isEqualTo("http://www.example.com/agree");
assertThat(ca.getAccepted()).hasSize(2);
assertThat(ca.getAccepted().get(0).getGroup()).isEqualTo(developers);
@@ -256,6 +271,7 @@ public class ProjectConfigTest extends GerritBaseTests {
+ " sameGroupVisibility = block group Staff\n"
+ "[contributor-agreement \"Individual\"]\n"
+ " description = A simple description\n"
+ " matchProjects = ^/ourproject\n"
+ " accepted = group Developers\n"
+ " autoVerify = group Developers\n"
+ " agreementUrl = http://www.example.com/agree\n"
@@ -273,6 +289,8 @@ public class ProjectConfigTest extends GerritBaseTests {
ContributorAgreement ca = cfg.getContributorAgreement("Individual");
ca.setAccepted(Collections.singletonList(new PermissionRule(cfg.resolve(staff))));
ca.setAutoVerify(null);
ca.setMatchProjectsRegexes(null);
ca.setExcludeProjectsRegexes(Collections.singletonList("^/theirproject"));
ca.setDescription("A new description");
rev = commit(cfg);
assertThat(text(rev, "project.config"))
@@ -289,6 +307,7 @@ public class ProjectConfigTest extends GerritBaseTests {
+ " description = A new description\n"
+ " accepted = group Staff\n"
+ " agreementUrl = http://www.example.com/agree\n"
+ "\texcludeProjects = ^/theirproject\n"
+ "[label \"CustomLabel\"]\n"
+ LABEL_SCORES_CONFIG
+ "\tfunction = MaxWithBlock\n" // label gets this function when it is created