Add project scope feature to contributor-agreement enforcement.

Two new lists are added to the contributor-agreement config section:

1. matchProjects will apply the enforcement only to matching projects.
   When omitted, enforcement applies to all projects.
2. excludeProjects will cause the enforcement to skip matches.
   When omitted, enforcement applies to all projects per matchProjects.

Change-Id: Ide2d8c4718ffcfb3cf1701465fe08bea58e9d7c6
This commit is contained in:
Bob Badour 2018-10-12 14:32:57 -07:00
parent 2f7e0e4b07
commit c48da97cfd
8 changed files with 173 additions and 9 deletions
Documentation
java/com/google/gerrit
javatests/com/google/gerrit
acceptance
server/project

@ -25,13 +25,15 @@ Download the existing configuration from Gerrit:
---- ----
Contributor agreements are defined as contributor-agreement sections in Contributor agreements are defined as contributor-agreement sections in
`project.config`: `project.config` of `All-Projects`:
---- ----
[contributor-agreement "Individual"] [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. 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 agreementUrl = static/cla_individual.html
autoVerify = group CLA Accepted - Individual autoVerify = group CLA Accepted - Individual
accepted = group CLA Accepted - Individual accepted = group CLA Accepted - Individual
matchProjects = ^/.*$
excludeProjects = ^/not/my/project/
---- ----
Each `contributor-agreement` section within the `project.config` file must 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 contributor agreement has been accepted. The groups' UUID must also
appear in the `groups` file. 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 GERRIT
------ ------
Part of link:index.html[Gerrit Code Review] Part of link:index.html[Gerrit Code Review]

@ -1245,14 +1245,14 @@ public abstract class AbstractDaemonTest {
protected ContributorAgreement configureContributorAgreement(boolean autoVerify) protected ContributorAgreement configureContributorAgreement(boolean autoVerify)
throws Exception { throws Exception {
ContributorAgreement ca; 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) { 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 = new ContributorAgreement("cla-test");
ca.setAutoVerify(groupRef); ca.setAutoVerify(groupRef);
ca.setAccepted(ImmutableList.of(rule)); ca.setAccepted(ImmutableList.of(rule));
@ -1261,6 +1261,8 @@ public abstract class AbstractDaemonTest {
} }
ca.setDescription("description"); ca.setDescription("description");
ca.setAgreementUrl("agreement-url"); ca.setAgreementUrl("agreement-url");
ca.setAccepted(ImmutableList.of(rule));
ca.setExcludeProjectsRegexes(ImmutableList.of("ExcludedProject"));
try (ProjectConfigUpdate u = updateProject(allProjects)) { try (ProjectConfigUpdate u = updateProject(allProjects)) {
u.getConfig().replace(ca); u.getConfig().replace(ca);

@ -25,6 +25,8 @@ public class ContributorAgreement implements Comparable<ContributorAgreement> {
protected List<PermissionRule> accepted; protected List<PermissionRule> accepted;
protected GroupReference autoVerify; protected GroupReference autoVerify;
protected String agreementUrl; protected String agreementUrl;
protected List<String> excludeProjectsRegexes;
protected List<String> matchProjectsRegexes;
protected ContributorAgreement() {} protected ContributorAgreement() {}
@ -75,6 +77,28 @@ public class ContributorAgreement implements Comparable<ContributorAgreement> {
this.agreementUrl = agreementUrl; 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 @Override
public int compareTo(ContributorAgreement o) { public int compareTo(ContributorAgreement o) {
return getName().compareTo(o.getName()); return getName().compareTo(o.getName());

@ -14,6 +14,9 @@
package com.google.gerrit.server.project; 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.ContributorAgreement;
import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action; import com.google.gerrit.common.data.PermissionRule.Action;
@ -34,6 +37,8 @@ import java.io.IOException;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Collection; import java.util.Collection;
import java.util.List; import java.util.List;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@Singleton @Singleton
public class ContributorAgreementsChecker { public class ContributorAgreementsChecker {
@ -93,6 +98,20 @@ public class ContributorAgreementsChecker {
List<AccountGroup.UUID> groupIds; List<AccountGroup.UUID> groupIds;
groupIds = okGroupIds; 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()) { for (PermissionRule rule : ca.getAccepted()) {
if ((rule.getAction() == Action.ALLOW) if ((rule.getAction() == Action.ALLOW)
&& (rule.getGroup() != null) && (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(); final StringBuilder msg = new StringBuilder();
msg.append("No Contributor Agreement on file for user ") msg.append("No Contributor Agreement on file for user ")
.append(iUser.getNameEmail()) .append(iUser.getNameEmail())
@ -114,4 +133,23 @@ public class ContributorAgreementsChecker {
throw new AuthException(msg.toString()); 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;
}
} }

@ -127,6 +127,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
private static final String KEY_ACCEPTED = "accepted"; private static final String KEY_ACCEPTED = "accepted";
private static final String KEY_AUTO_VERIFY = "autoVerify"; private static final String KEY_AUTO_VERIFY = "autoVerify";
private static final String KEY_AGREEMENT_URL = "agreementUrl"; 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 NOTIFY = "notify";
private static final String KEY_EMAIL = "email"; 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.setAgreementUrl(rc.getString(CONTRIBUTOR_AGREEMENT, name, KEY_AGREEMENT_URL));
ca.setAccepted( ca.setAccepted(
loadPermissionRules(rc, CONTRIBUTOR_AGREEMENT, name, KEY_ACCEPTED, groupsByName, false)); 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 = List<PermissionRule> rules =
loadPermissionRules( 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( private ImmutableList<PermissionRule> loadPermissionRules(
Config rc, Config rc,
String section, String section,
@ -1163,6 +1184,16 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError.
ca.getName(), ca.getName(),
KEY_ACCEPTED, KEY_ACCEPTED,
ruleToStringList(ca.getAccepted(), keepGroups)); 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( private List<String> ruleToStringList(
List<PermissionRule> list, Set<AccountGroup.UUID> keepGroups) { List<PermissionRule> list, Set<AccountGroup.UUID> keepGroups) {
List<String> rules = new ArrayList<>(); List<String> rules = new ArrayList<>();

@ -181,6 +181,28 @@ public class AgreementsIT extends AbstractDaemonTest {
gApi.changes().id(change.changeId).revert(); 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 @Test
public void cherrypickChangeWithoutCLA() throws Exception { public void cherrypickChangeWithoutCLA() throws Exception {
assume().that(isContributorAgreementsEnabled()).isTrue(); assume().that(isContributorAgreementsEnabled()).isTrue();
@ -240,6 +262,17 @@ public class AgreementsIT extends AbstractDaemonTest {
gApi.changes().create(newChangeInput()); 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) { private void assertAgreement(AgreementInfo info, ContributorAgreement ca) {
assertThat(info.name).isEqualTo(ca.getName()); assertThat(info.name).isEqualTo(ca.getName());
assertThat(info.description).isEqualTo(ca.getDescription()); assertThat(info.description).isEqualTo(ca.getDescription());

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

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