diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 2976e96dac..1efe839a8e 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -912,12 +912,15 @@ project. Plugins can refer to groups so that when they are renamed, the project config will also be updated in this section. The proper format to use is -the string representation of a GroupReference, as shown below. +the same as for any other group reference in the `project.config`, as shown below. ---- -Group[group_name / group_uuid] +group group_name ---- +The file `groups` must also contains the mapping of the group name and its UUID, +refer to link:config-project-config.html#file-groups[file groups] + [[project-specific-configuration]] == Project Specific Configuration in own config file diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java index 8362281086..dc22d62bc4 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/GroupReference.java @@ -14,10 +14,14 @@ package com.google.gerrit.common.data; +import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.AccountGroup; /** Describes a group within a projects {@link AccessSection}s. */ public class GroupReference implements Comparable { + + private static final String PREFIX = "group "; + /** @return a new reference to the given group description. */ public static GroupReference forGroup(AccountGroup group) { return new GroupReference(group.getGroupUUID(), group.getName()); @@ -27,10 +31,16 @@ public class GroupReference implements Comparable { return new GroupReference(group.getGroupUUID(), group.getName()); } - public static GroupReference fromString(String ref) { - String name = ref.substring(ref.indexOf("[") + 1, ref.lastIndexOf("/")).trim(); - String uuid = ref.substring(ref.lastIndexOf("/") + 1, ref.lastIndexOf("]")).trim(); - return new GroupReference(new AccountGroup.UUID(uuid), name); + public static boolean isGroupReference(String configValue) { + return configValue != null && configValue.startsWith(PREFIX); + } + + @Nullable + public static String extractGroupName(String configValue) { + if (!isGroupReference(configValue)) { + return null; + } + return configValue.substring(PREFIX.length()).trim(); } protected String uuid; @@ -78,6 +88,10 @@ public class GroupReference implements Comparable { return o instanceof GroupReference && compareTo((GroupReference) o) == 0; } + public String toConfigValue() { + return PREFIX + name; + } + @Override public String toString() { return "Group[" + getName() + " / " + getUUID() + "]"; diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java index 92658303f1..9098ec3451 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRule.java @@ -204,8 +204,7 @@ public class PermissionRule implements Comparable { r.append(' '); } - r.append("group "); - r.append(getGroup().getName()); + r.append(getGroup().toConfigValue()); return r.toString(); } @@ -238,7 +237,7 @@ public class PermissionRule implements Comparable { src = src.substring("+force ".length()).trim(); } - if (mightUseRange && !src.startsWith("group ")) { + if (mightUseRange && !GroupReference.isGroupReference(src)) { int sp = src.indexOf(' '); String range = src.substring(0, sp); @@ -254,10 +253,10 @@ public class PermissionRule implements Comparable { src = src.substring(sp + 1).trim(); } - if (src.startsWith("group ")) { - src = src.substring(6).trim(); + String groupName = GroupReference.extractGroupName(src); + if (groupName != null) { GroupReference group = new GroupReference(); - group.setName(src); + group.setName(groupName); rule.setGroup(group); } else { throw new IllegalArgumentException("Rule must include group: " + orig); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java index 1b124959b3..fd1a12c081 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/PluginConfig.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.config; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import com.google.common.collect.Iterables; +import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.project.ProjectState; import java.util.Arrays; @@ -56,11 +57,16 @@ public class PluginConfig { cfg = copyConfig(cfg); for (String name : parentPluginConfig.cfg.getNames(PLUGIN, pluginName)) { if (!allNames.contains(name)) { - cfg.setStringList( - PLUGIN, - pluginName, - name, - Arrays.asList(parentPluginConfig.cfg.getStringList(PLUGIN, pluginName, name))); + List values = + Arrays.asList(parentPluginConfig.cfg.getStringList(PLUGIN, pluginName, name)); + for (String value : values) { + GroupReference groupRef = + parentPluginConfig.projectConfig.getGroup(GroupReference.extractGroupName(value)); + if (groupRef != null) { + projectConfig.resolve(groupRef); + } + } + cfg.setStringList(PLUGIN, pluginName, name, values); } } } @@ -152,4 +158,13 @@ public class PluginConfig { public Set getNames() { return cfg.getNames(PLUGIN, pluginName, true); } + + public GroupReference getGroupReference(String name) { + return projectConfig.getGroup(GroupReference.extractGroupName(getString(name))); + } + + public void setGroupReference(String name, GroupReference value) { + GroupReference groupRef = projectConfig.resolve(value); + setString(name, groupRef.toConfigValue()); + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index 61d8cfecf8..e3299d9c7f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -182,6 +182,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. private boolean checkReceivedObjects; private Set sectionsWithUnknownPermissions; private boolean hasLegacyPermissions; + private Map groupsByName; public static ProjectConfig read(MetaDataUpdate update) throws IOException, ConfigInvalidException { @@ -402,7 +403,13 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } public GroupReference resolve(GroupReference group) { - return groupList.resolve(group); + GroupReference groupRef = groupList.resolve(group); + if (groupRef != null + && groupRef.getUUID() != null + && !groupsByName.containsKey(groupRef.getName())) { + groupsByName.put(groupRef.getName(), groupRef); + } + return groupRef; } /** @return the group reference, if the group is used by at least one rule. */ @@ -410,6 +417,14 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. return groupList.byUUID(uuid); } + /** + * @return the group reference corresponding to the specified group name if the group is used by + * at least one rule or plugin value. + */ + public GroupReference getGroup(String groupName) { + return groupsByName.get(groupName); + } + /** @return set of all groups used by this configuration. */ public Set getAllGroupUUIDs() { return groupList.uuids(); @@ -473,7 +488,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. @Override protected void onLoad() throws IOException, ConfigInvalidException { readGroupList(); - Map groupsByName = mapGroupReferences(); + groupsByName = mapGroupReferences(); rulesId = getObjectId("rules.pl"); Config rc = readConfig(PROJECT_CONFIG); @@ -515,11 +530,11 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. p.setDefaultDashboard(rc.getString(DASHBOARD, null, KEY_DEFAULT)); p.setLocalDefaultDashboard(rc.getString(DASHBOARD, null, KEY_LOCAL_DEFAULT)); - loadAccountsSection(rc, groupsByName); - loadContributorAgreements(rc, groupsByName); - loadAccessSections(rc, groupsByName); + loadAccountsSection(rc); + loadContributorAgreements(rc); + loadAccessSections(rc); loadBranchOrderSection(rc); - loadNotifySections(rc, groupsByName); + loadNotifySections(rc); loadLabelSections(rc); loadCommentLinkSections(rc); loadSubscribeSections(rc); @@ -528,13 +543,13 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. loadReceiveSection(rc); } - private void loadAccountsSection(Config rc, Map groupsByName) { + private void loadAccountsSection(Config rc) { accountsSection = new AccountsSection(); accountsSection.setSameGroupVisibility( loadPermissionRules(rc, ACCOUNTS, null, KEY_SAME_GROUP_VISIBILITY, groupsByName, false)); } - private void loadContributorAgreements(Config rc, Map groupsByName) { + private void loadContributorAgreements(Config rc) { contributorAgreements = new HashMap<>(); for (String name : rc.getSubsections(CONTRIBUTOR_AGREEMENT)) { ContributorAgreement ca = getContributorAgreement(name, true); @@ -594,7 +609,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. * type = submitted_changes * */ - private void loadNotifySections(Config rc, Map groupsByName) { + private void loadNotifySections(Config rc) { notifySections = new HashMap<>(); for (String sectionName : rc.getSubsections(NOTIFY)) { NotifyConfig n = new NotifyConfig(); @@ -607,8 +622,8 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. n.setHeader(rc.getEnum(NOTIFY, sectionName, KEY_HEADER, NotifyConfig.Header.BCC)); for (String dst : rc.getStringList(NOTIFY, sectionName, KEY_EMAIL)) { - if (dst.startsWith("group ")) { - String groupName = dst.substring(6).trim(); + String groupName = GroupReference.extractGroupName(dst); + if (groupName != null) { GroupReference ref = groupsByName.get(groupName); if (ref == null) { ref = new GroupReference(null, groupName); @@ -639,7 +654,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } } - private void loadAccessSections(Config rc, Map groupsByName) { + private void loadAccessSections(Config rc) { accessSections = new HashMap<>(); sectionsWithUnknownPermissions = new HashSet<>(); for (String refName : rc.getSubsections(ACCESS)) { @@ -940,17 +955,15 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. pluginConfigs.put(plugin, pluginConfig); for (String name : rc.getNames(PLUGIN, plugin)) { String value = rc.getString(PLUGIN, plugin, name); - if (value.startsWith("Group[")) { - GroupReference refFromString = GroupReference.fromString(value); - GroupReference ref = groupList.byUUID(refFromString.getUUID()); + String groupName = GroupReference.extractGroupName(value); + if (groupName != null) { + GroupReference ref = groupsByName.get(groupName); if (ref == null) { - ref = refFromString; error( new ValidationError( - PROJECT_CONFIG, - "group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME)); + PROJECT_CONFIG, "group \"" + groupName + "\" not in " + GroupList.FILE_NAME)); } - rc.setString(PLUGIN, plugin, name, ref.toString()); + rc.setString(PLUGIN, plugin, name, value); } pluginConfig.setStringList( PLUGIN, plugin, name, Arrays.asList(rc.getStringList(PLUGIN, plugin, name))); @@ -1354,11 +1367,12 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. Config pluginConfig = e.getValue(); for (String name : pluginConfig.getNames(PLUGIN, plugin)) { String value = pluginConfig.getString(PLUGIN, plugin, name); - if (value.startsWith("Group[")) { - GroupReference ref = resolve(GroupReference.fromString(value)); - if (ref.getUUID() != null) { + String groupName = GroupReference.extractGroupName(value); + if (groupName != null) { + GroupReference ref = groupsByName.get(groupName); + if (ref != null && ref.getUUID() != null) { keepGroups.add(ref.getUUID()); - pluginConfig.setString(PLUGIN, plugin, name, ref.toString()); + pluginConfig.setString(PLUGIN, plugin, name, "group " + ref.getName()); } } rc.setStringList( diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java index ab68c10a92..2d927be4d5 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/git/ProjectConfigTest.java @@ -27,8 +27,10 @@ import com.google.gerrit.common.data.PermissionRule; 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.PluginConfig; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import java.io.IOException; +import java.util.Arrays; import java.util.Collections; import java.util.Map; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -325,6 +327,155 @@ public class ProjectConfigTest extends LocalDiskRepositoryTestCase { + " read = group Developers\n"); } + @Test + public void readExistingPluginConfig() throws Exception { + RevCommit rev = + util.commit( + util.tree( // + util.file( + "project.config", + util.blob( + "" // + + "[plugin \"somePlugin\"]\n" // + + " key1 = value1\n" // + + " key2 = value2a\n" + + " key2 = value2b\n")) // + )); + update(rev); + + ProjectConfig cfg = read(rev); + PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin"); + assertThat(pluginCfg.getNames().size()).isEqualTo(2); + assertThat(pluginCfg.getString("key1")).isEqualTo("value1"); + assertThat(pluginCfg.getStringList(("key2"))).isEqualTo(new String[] {"value2a", "value2b"}); + } + + @Test + public void readUnexistingPluginConfig() throws Exception { + ProjectConfig cfg = new ProjectConfig(new Project.NameKey("test")); + cfg.load(db); + PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin"); + assertThat(pluginCfg.getNames()).isEmpty(); + } + + @Test + public void editPluginConfig() throws Exception { + RevCommit rev = + util.commit( + util.tree( // + util.file( + "project.config", + util.blob( + "" // + + "[plugin \"somePlugin\"]\n" // + + " key1 = value1\n" // + + " key2 = value2a\n" // + + " key2 = value2b\n")) // + )); + update(rev); + + ProjectConfig cfg = read(rev); + PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin"); + pluginCfg.setString("key1", "updatedValue1"); + pluginCfg.setStringList("key2", Arrays.asList("updatedValue2a", "updatedValue2b")); + rev = commit(cfg); + assertThat(text(rev, "project.config")) + .isEqualTo( + "" // + + "[plugin \"somePlugin\"]\n" // + + "\tkey1 = updatedValue1\n" // + + "\tkey2 = updatedValue2a\n" // + + "\tkey2 = updatedValue2b\n"); + } + + @Test + public void readPluginConfigGroupReference() throws Exception { + RevCommit rev = + util.commit( + util.tree( // + util.file("groups", util.blob(group(developers))), // + util.file( + "project.config", + util.blob( + "" // + + "[plugin \"somePlugin\"]\n" // + + "key1 = " + + developers.toConfigValue() + + "\n")) // + )); + update(rev); + + ProjectConfig cfg = read(rev); + PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin"); + assertThat(pluginCfg.getNames().size()).isEqualTo(1); + assertThat(pluginCfg.getGroupReference("key1")).isEqualTo(developers); + } + + @Test + public void readPluginConfigGroupReferenceNotInGroupsFile() throws Exception { + RevCommit rev = + util.commit( + util.tree( // + util.file("groups", util.blob(group(developers))), // + util.file( + "project.config", + util.blob( + "" // + + "[plugin \"somePlugin\"]\n" // + + "key1 = " + + staff.toConfigValue() + + "\n")) // + )); + update(rev); + + ProjectConfig cfg = read(rev); + assertThat(cfg.getValidationErrors()).hasSize(1); + assertThat(Iterables.getOnlyElement(cfg.getValidationErrors()).getMessage()) + .isEqualTo( + "project.config: group \"" + staff.getName() + "\" not in " + GroupList.FILE_NAME); + } + + @Test + public void editPluginConfigGroupReference() throws Exception { + RevCommit rev = + util.commit( + util.tree( // + util.file("groups", util.blob(group(developers))), // + util.file( + "project.config", + util.blob( + "" // + + "[plugin \"somePlugin\"]\n" // + + "key1 = " + + developers.toConfigValue() + + "\n")) // + )); + update(rev); + + ProjectConfig cfg = read(rev); + PluginConfig pluginCfg = cfg.getPluginConfig("somePlugin"); + assertThat(pluginCfg.getNames().size()).isEqualTo(1); + assertThat(pluginCfg.getGroupReference("key1")).isEqualTo(developers); + + pluginCfg.setGroupReference("key1", staff); + rev = commit(cfg); + assertThat(text(rev, "project.config")) + .isEqualTo( + "" // + + "[plugin \"somePlugin\"]\n" // + + "\tkey1 = " + + staff.toConfigValue() + + "\n"); + assertThat(text(rev, "groups")) + .isEqualTo( + "# UUID\tGroup Name\n" // + + "#\n" // + + staff.getUUID().get() + + " \t" + + staff.getName() + + "\n"); + } + private ProjectConfig read(RevCommit rev) throws IOException, ConfigInvalidException { ProjectConfig cfg = new ProjectConfig(new Project.NameKey("test")); cfg.load(db, rev);