diff --git a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java index 89269b6e85..a3ddd98e68 100644 --- a/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java +++ b/java/com/google/gerrit/acceptance/testsuite/project/ProjectOperationsImpl.java @@ -151,15 +151,19 @@ public class ProjectOperationsImpl implements ProjectOperations { ProjectConfig projectConfig, ImmutableList removedPermissions) { for (TestProjectUpdate.TestPermissionKey p : removedPermissions) { - Permission permission = - projectConfig.getAccessSection(p.section(), true).getPermission(p.name(), true); - if (p.group().isPresent()) { - GroupReference group = GroupReference.create(p.group().get(), p.group().get().get()); - group = projectConfig.resolve(group); - permission.removeRule(group); - } else { - permission.clearRules(); - } + projectConfig.upsertAccessSection( + p.section(), + as -> { + Permission.Builder permission = as.upsertPermission(p.name()); + if (p.group().isPresent()) { + GroupReference group = + GroupReference.create(p.group().get(), p.group().get().get()); + group = projectConfig.resolve(group); + permission.removeRule(group); + } else { + permission.clearRules(); + } + }); } } @@ -168,10 +172,8 @@ public class ProjectOperationsImpl implements ProjectOperations { for (TestCapability c : addedCapabilities) { PermissionRule.Builder rule = newRule(projectConfig, c.group()); rule.setRange(c.min(), c.max()); - projectConfig - .getAccessSection(AccessSection.GLOBAL_CAPABILITIES, true) - .getPermission(c.name(), true) - .add(rule.build()); + projectConfig.upsertAccessSection( + AccessSection.GLOBAL_CAPABILITIES, as -> as.upsertPermission(c.name()).add(rule)); } } @@ -181,10 +183,7 @@ public class ProjectOperationsImpl implements ProjectOperations { PermissionRule.Builder rule = newRule(projectConfig, p.group()); rule.setAction(p.action()); rule.setForce(p.force()); - projectConfig - .getAccessSection(p.ref(), true) - .getPermission(p.name(), true) - .add(rule.build()); + projectConfig.upsertAccessSection(p.ref(), as -> as.upsertPermission(p.name()).add(rule)); } } @@ -196,9 +195,8 @@ public class ProjectOperationsImpl implements ProjectOperations { rule.setRange(p.min(), p.max()); String permissionName = p.impersonation() ? Permission.forLabelAs(p.name()) : Permission.forLabel(p.name()); - Permission permission = - projectConfig.getAccessSection(p.ref(), true).getPermission(permissionName, true); - permission.add(rule.build()); + projectConfig.upsertAccessSection( + p.ref(), as -> as.upsertPermission(permissionName).add(rule)); } } @@ -207,10 +205,9 @@ public class ProjectOperationsImpl implements ProjectOperations { ImmutableMap exclusiveGroupPermissions) { exclusiveGroupPermissions.forEach( (key, exclusive) -> - projectConfig - .getAccessSection(key.section(), true) - .getPermission(key.name(), true) - .setExclusiveGroup(exclusive)); + projectConfig.upsertAccessSection( + key.section(), + as -> as.upsertPermission(key.name()).setExclusiveGroup(exclusive))); } private RevCommit headOrNull(String branch) { diff --git a/java/com/google/gerrit/common/data/AccessSection.java b/java/com/google/gerrit/common/data/AccessSection.java index 0c9663b90c..18682e3155 100644 --- a/java/com/google/gerrit/common/data/AccessSection.java +++ b/java/com/google/gerrit/common/data/AccessSection.java @@ -14,18 +14,21 @@ package com.google.gerrit.common.data; +import static com.google.common.collect.ImmutableList.toImmutableList; import static java.util.Objects.requireNonNull; +import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Project; import java.util.ArrayList; -import java.util.HashSet; import java.util.List; -import java.util.Set; +import java.util.Optional; +import java.util.function.Consumer; /** Portion of a {@link Project} describing access rules. */ -public final class AccessSection implements Comparable { +@AutoValue +public abstract class AccessSection implements Comparable { /** Special name given to the global capabilities; not a valid reference. */ public static final String GLOBAL_CAPABILITIES = "GLOBAL_CAPABILITIES"; /** Pattern that matches all references in a project. */ @@ -38,12 +41,16 @@ public final class AccessSection implements Comparable { public static final String REGEX_PREFIX = "^"; /** Name of the access section. It could be a ref pattern or something else. */ - private String name; + public abstract String getName(); - private List permissions; + public abstract ImmutableList getPermissions(); - public AccessSection(String name) { - this.name = name; + public static AccessSection create(String name) { + return builder(name).build(); + } + + public static Builder builder(String name) { + return new AutoValue_AccessSection.Builder().setName(name).setPermissions(ImmutableList.of()); } /** @return true if the name is likely to be a valid reference section name. */ @@ -51,101 +58,19 @@ public final class AccessSection implements Comparable { return name.startsWith("refs/") || name.startsWith("^refs/"); } - public String getName() { - return name; - } - - public ImmutableList getPermissions() { - return permissions == null ? ImmutableList.of() : ImmutableList.copyOf(permissions); - } - - public void setPermissions(List list) { - requireNonNull(list); - - Set names = new HashSet<>(); - for (Permission p : list) { - if (!names.add(p.getName().toLowerCase())) { - throw new IllegalArgumentException(); - } - } - - permissions = new ArrayList<>(list); - } - @Nullable public Permission getPermission(String name) { - return getPermission(name, false); - } - - @Nullable - public Permission getPermission(String name, boolean create) { requireNonNull(name); - - if (permissions != null) { - for (Permission p : permissions) { - if (p.getName().equalsIgnoreCase(name)) { - return p; - } + for (Permission p : getPermissions()) { + if (p.getName().equalsIgnoreCase(name)) { + return p; } } - - if (create) { - if (permissions == null) { - permissions = new ArrayList<>(); - } - - Permission p = new Permission(name); - permissions.add(p); - return p; - } - return null; } - public void addPermission(Permission permission) { - requireNonNull(permission); - - if (permissions == null) { - permissions = new ArrayList<>(); - } - - for (Permission p : permissions) { - if (p.getName().equalsIgnoreCase(permission.getName())) { - throw new IllegalArgumentException(); - } - } - - permissions.add(permission); - } - - public void remove(Permission permission) { - requireNonNull(permission); - removePermission(permission.getName()); - } - - public void removePermission(String name) { - requireNonNull(name); - - if (permissions != null) { - permissions.removeIf(permission -> name.equalsIgnoreCase(permission.getName())); - } - } - - public void mergeFrom(AccessSection section) { - requireNonNull(section); - - for (Permission src : section.getPermissions()) { - Permission dst = getPermission(src.getName()); - if (dst != null) { - dst.mergeFrom(src); - } else { - permissions.add(src); - } - } - } - @Override - public int compareTo(AccessSection o) { + public final int compareTo(AccessSection o) { return comparePattern().compareTo(o.comparePattern()); } @@ -157,33 +82,85 @@ public final class AccessSection implements Comparable { } @Override - public String toString() { + public final String toString() { return "AccessSection[" + getName() + "]"; } - @Override - public boolean equals(Object obj) { - if (!(obj instanceof AccessSection)) { - return false; - } - - AccessSection other = (AccessSection) obj; - if (!getName().equals(other.getName())) { - return false; - } - return new HashSet<>(getPermissions()) - .equals(new HashSet<>(((AccessSection) obj).getPermissions())); + public Builder toBuilder() { + Builder b = autoToBuilder(); + b.getPermissions().stream().map(Permission::toBuilder).forEach(p -> b.addPermission(p)); + return b; } - @Override - public int hashCode() { - int hashCode = super.hashCode(); - if (permissions != null) { - for (Permission permission : permissions) { - hashCode += permission.hashCode(); - } + protected abstract Builder autoToBuilder(); + + @AutoValue.Builder + public abstract static class Builder { + private final List permissionBuilders; + + public Builder() { + permissionBuilders = new ArrayList<>(); } - hashCode += getName().hashCode(); - return hashCode; + + public abstract Builder setName(String name); + + public abstract String getName(); + + public Builder modifyPermissions(Consumer> modification) { + modification.accept(permissionBuilders); + return this; + } + + public Builder addPermission(Permission.Builder permission) { + requireNonNull(permission, "permission must be non-null"); + return modifyPermissions(p -> p.add(permission)); + } + + public Builder remove(Permission.Builder permission) { + requireNonNull(permission, "permission must be non-null"); + return removePermission(permission.getName()); + } + + public Builder removePermission(String name) { + requireNonNull(name, "name must be non-null"); + return modifyPermissions( + p -> p.removeIf(permissionBuilder -> name.equalsIgnoreCase(permissionBuilder.getName()))); + } + + public Permission.Builder upsertPermission(String permissionName) { + requireNonNull(permissionName, "permissionName must be non-null"); + + Optional maybePermission = + permissionBuilders.stream() + .filter(p -> p.getName().equalsIgnoreCase(permissionName)) + .findAny(); + if (maybePermission.isPresent()) { + return maybePermission.get(); + } + + Permission.Builder permission = Permission.builder(permissionName); + modifyPermissions(p -> p.add(permission)); + return permission; + } + + public AccessSection build() { + setPermissions( + permissionBuilders.stream().map(Permission.Builder::build).collect(toImmutableList())); + if (getPermissions().size() + > getPermissions().stream() + .map(Permission::getName) + .map(String::toLowerCase) + .distinct() + .count()) { + throw new IllegalArgumentException("duplicate permissions: " + getPermissions()); + } + return autoBuild(); + } + + protected abstract AccessSection autoBuild(); + + protected abstract ImmutableList getPermissions(); + + protected abstract Builder setPermissions(ImmutableList permissions); } } diff --git a/java/com/google/gerrit/common/data/Permission.java b/java/com/google/gerrit/common/data/Permission.java index 5aaffba4b2..93155d5780 100644 --- a/java/com/google/gerrit/common/data/Permission.java +++ b/java/com/google/gerrit/common/data/Permission.java @@ -14,14 +14,19 @@ package com.google.gerrit.common.data; +import static com.google.common.collect.ImmutableList.toImmutableList; + +import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; +import com.google.gerrit.common.Nullable; import java.util.ArrayList; -import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.function.Consumer; /** A single permission within an {@link AccessSection} of a project. */ -public class Permission implements Comparable { +@AutoValue +public abstract class Permission implements Comparable { public static final String ABANDON = "abandon"; public static final String ADD_PATCH_SET = "addPatchSet"; public static final String CREATE = "create"; @@ -133,18 +138,21 @@ public class Permission implements Comparable { return true; } - protected String name; - protected boolean exclusiveGroup; - protected List rules; + public abstract String getName(); - protected Permission() {} + protected abstract boolean isExclusiveGroup(); - public Permission(String name) { - this.name = name; + public abstract ImmutableList getRules(); + + public static Builder builder(String name) { + return new AutoValue_Permission.Builder() + .setName(name) + .setExclusiveGroup(false) + .setRules(ImmutableList.of()); } - public String getName() { - return name; + public static Permission create(String name) { + return builder(name).build(); } public String getLabel() { @@ -155,97 +163,32 @@ public class Permission implements Comparable { // Only permit exclusive group behavior on non OWNER permissions, // otherwise an owner might lose access to a delegated subspace. // - return exclusiveGroup && !OWNER.equals(getName()); - } - - public void setExclusiveGroup(boolean newExclusiveGroup) { - exclusiveGroup = newExclusiveGroup; - } - - public ImmutableList getRules() { - return rules == null ? ImmutableList.of() : ImmutableList.copyOf(rules); - } - - public void setRules(List list) { - rules = new ArrayList<>(list); - } - - public void add(PermissionRule rule) { - initRules(); - rules.add(rule); - } - - public void remove(PermissionRule rule) { - if (rule != null) { - removeRule(rule.getGroup()); - } - } - - public void removeRule(GroupReference group) { - if (rules != null) { - rules.removeIf(permissionRule -> sameGroup(permissionRule, group)); - } - } - - public void clearRules() { - if (rules != null) { - rules.clear(); - } + return isExclusiveGroup() && !OWNER.equals(getName()); } + @Nullable public PermissionRule getRule(GroupReference group) { - return getRule(group, false); - } - - public PermissionRule getRule(GroupReference group, boolean create) { - initRules(); - - for (PermissionRule r : rules) { + for (PermissionRule r : getRules()) { if (sameGroup(r, group)) { return r; } } - if (create) { - PermissionRule r = PermissionRule.create(group); - rules.add(r); - return r; - } return null; } - void mergeFrom(Permission src) { - for (PermissionRule srcRule : src.getRules()) { - PermissionRule dstRule = getRule(srcRule.getGroup()); - if (dstRule != null) { - rules.remove(dstRule); - rules.add(PermissionRule.merge(srcRule, dstRule)); - } else { - add(srcRule); - } - } - } - private static boolean sameGroup(PermissionRule rule, GroupReference group) { - if (group.getUUID() != null) { + if (group.getUUID() != null && rule.getGroup().getUUID() != null) { return group.getUUID().equals(rule.getGroup().getUUID()); - - } else if (group.getName() != null) { + } else if (group.getName() != null && rule.getGroup().getName() != null) { return group.getName().equals(rule.getGroup().getName()); - } else { return false; } } - private void initRules() { - if (rules == null) { - rules = new ArrayList<>(4); - } - } - @Override - public int compareTo(Permission b) { + public final int compareTo(Permission b) { int cmp = index(this) - index(b); if (cmp == 0) { cmp = getName().compareTo(b.getName()); @@ -265,28 +208,10 @@ public class Permission implements Comparable { } @Override - public boolean equals(Object obj) { - if (!(obj instanceof Permission)) { - return false; - } - - final Permission other = (Permission) obj; - if (!name.equals(other.name) || exclusiveGroup != other.exclusiveGroup) { - return false; - } - return new HashSet<>(getRules()).equals(new HashSet<>(other.getRules())); - } - - @Override - public int hashCode() { - return name.hashCode(); - } - - @Override - public String toString() { + public final String toString() { StringBuilder bldr = new StringBuilder(); - bldr.append(name).append(" "); - if (exclusiveGroup) { + bldr.append(getName()).append(" "); + if (isExclusiveGroup()) { bldr.append("[exclusive] "); } bldr.append("["); @@ -300,4 +225,67 @@ public class Permission implements Comparable { bldr.append("]"); return bldr.toString(); } + + protected abstract Builder autoToBuilder(); + + public Builder toBuilder() { + Builder b = autoToBuilder(); + getRules().stream().map(PermissionRule::toBuilder).forEach(r -> b.add(r)); + return b; + } + + @AutoValue.Builder + public abstract static class Builder { + private final List rulesBuilders; + + Builder() { + rulesBuilders = new ArrayList<>(); + } + + public abstract Builder setName(String value); + + public abstract String getName(); + + public abstract Builder setExclusiveGroup(boolean value); + + public Builder modifyRules(Consumer> modification) { + modification.accept(rulesBuilders); + return this; + } + + public Builder add(PermissionRule.Builder rule) { + return modifyRules(r -> r.add(rule)); + } + + public Builder remove(PermissionRule rule) { + if (rule != null) { + return removeRule(rule.getGroup()); + } + return this; + } + + public Builder removeRule(GroupReference group) { + return modifyRules(rules -> rules.removeIf(rule -> sameGroup(rule.build(), group))); + } + + public Builder clearRules() { + return modifyRules(r -> r.clear()); + } + + public Permission build() { + setRules( + rulesBuilders.stream().map(PermissionRule.Builder::build).collect(toImmutableList())); + return autoBuild(); + } + + public List getRulesBuilders() { + return rulesBuilders; + } + + protected abstract ImmutableList getRules(); + + protected abstract Builder setRules(ImmutableList rules); + + protected abstract Permission autoBuild(); + } } diff --git a/java/com/google/gerrit/common/data/PermissionRule.java b/java/com/google/gerrit/common/data/PermissionRule.java index c5437699b0..c4116526d4 100644 --- a/java/com/google/gerrit/common/data/PermissionRule.java +++ b/java/com/google/gerrit/common/data/PermissionRule.java @@ -263,6 +263,8 @@ public abstract class PermissionRule implements Comparable { public abstract Builder setMax(int max); + public abstract GroupReference getGroup(); + public abstract PermissionRule build(); } } diff --git a/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java b/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java index 17313e4f7b..7fbf7efb32 100644 --- a/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java +++ b/java/com/google/gerrit/server/CreateGroupPermissionSyncer.java @@ -14,8 +14,6 @@ package com.google.gerrit.server; -import static java.util.stream.Collectors.toList; - import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.data.AccessSection; @@ -101,23 +99,25 @@ public class CreateGroupPermissionSyncer implements ChangeMergedListener { try (MetaDataUpdate md = metaDataUpdateFactory.get().create(allUsers)) { ProjectConfig config = projectConfigFactory.read(md); - AccessSection createGroupAccessSection = - config.getAccessSection(RefNames.REFS_GROUPS + "*", true); - if (createGroupsGlobal.isEmpty()) { - createGroupAccessSection.setPermissions( - createGroupAccessSection.getPermissions().stream() - .filter(p -> !Permission.CREATE.equals(p.getName())) - .collect(toList())); - config.replace(createGroupAccessSection); - } else { - // The create permission is managed by Gerrit at this point only so there is no concern of - // overwriting user-defined permissions here. - Permission createGroupPermission = new Permission(Permission.CREATE); - createGroupAccessSection.remove(createGroupPermission); - createGroupAccessSection.addPermission(createGroupPermission); - createGroupsGlobal.forEach(createGroupPermission::add); - config.replace(createGroupAccessSection); - } + config.upsertAccessSection( + RefNames.REFS_GROUPS + "*", + refsGroupsAccessSectionBuilder -> { + if (createGroupsGlobal.isEmpty()) { + refsGroupsAccessSectionBuilder.modifyPermissions( + permissions -> { + permissions.removeIf(p -> Permission.CREATE.equals(p.getName())); + }); + } else { + // The create permission is managed by Gerrit at this point only so there is no + // concern of overwriting user-defined permissions here. + Permission.Builder createGroupPermission = Permission.builder(Permission.CREATE); + refsGroupsAccessSectionBuilder.remove(createGroupPermission); + refsGroupsAccessSectionBuilder.addPermission(createGroupPermission); + createGroupsGlobal.stream() + .map(p -> p.toBuilder()) + .forEach(createGroupPermission::add); + } + }); config.commit(md); projectCache.evict(config.getProject()); diff --git a/java/com/google/gerrit/server/account/CapabilityCollection.java b/java/com/google/gerrit/server/account/CapabilityCollection.java index 157f94612e..de522aeb8c 100644 --- a/java/com/google/gerrit/server/account/CapabilityCollection.java +++ b/java/com/google/gerrit/server/account/CapabilityCollection.java @@ -60,7 +60,7 @@ public class CapabilityCollection { this.systemGroupBackend = systemGroupBackend; if (section == null) { - section = new AccessSection(AccessSection.GLOBAL_CAPABILITIES); + section = AccessSection.create(AccessSection.GLOBAL_CAPABILITIES); } Map> tmp = new HashMap<>(); diff --git a/java/com/google/gerrit/server/project/ProjectConfig.java b/java/com/google/gerrit/server/project/ProjectConfig.java index cc13606264..5ff1d83331 100644 --- a/java/com/google/gerrit/server/project/ProjectConfig.java +++ b/java/com/google/gerrit/server/project/ProjectConfig.java @@ -350,16 +350,16 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. } public AccessSection getAccessSection(String name) { - return getAccessSection(name, false); + return accessSections.get(name); } - public AccessSection getAccessSection(String name, boolean create) { - AccessSection as = accessSections.get(name); - if (as == null && create) { - as = new AccessSection(name); - accessSections.put(name, as); - } - return as; + public void upsertAccessSection(String name, Consumer update) { + AccessSection.Builder accessSectionBuilder = + accessSections.containsKey(name) + ? accessSections.get(name).toBuilder() + : AccessSection.builder(name); + update.accept(accessSectionBuilder); + accessSections.put(name, accessSectionBuilder.build()); } public ImmutableSet getAccessSectionNames() { @@ -400,8 +400,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. if (section != null) { String name = section.getName(); if (sectionsWithUnknownPermissions.contains(name)) { - AccessSection a = accessSections.get(name); - a.setPermissions(new ArrayList<>()); + AccessSection.Builder a = accessSections.get(name).toBuilder(); + a.modifyPermissions(p -> p.clear()); + accessSections.put(name, a.build()); } else { accessSections.remove(name); } @@ -412,8 +413,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. if (permission == null) { remove(section); } else if (section != null) { - AccessSection a = accessSections.get(section.getName()); - a.remove(permission); + AccessSection a = + accessSections.get(section.getName()).toBuilder().remove(permission.toBuilder()).build(); + accessSections.put(section.getName(), a); if (a.getPermissions().isEmpty()) { remove(a); } @@ -432,28 +434,21 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. if (p == null) { return; } - p.remove(rule); - if (p.getRules().isEmpty()) { - a.remove(permission); + AccessSection.Builder accessSectionBuilder = a.toBuilder(); + Permission.Builder permissionBuilder = + accessSectionBuilder.upsertPermission(permission.getName()); + permissionBuilder.remove(rule); + if (permissionBuilder.build().getRules().isEmpty()) { + accessSectionBuilder.remove(permissionBuilder); } + a = accessSectionBuilder.build(); + accessSections.put(section.getName(), a); if (a.getPermissions().isEmpty()) { remove(a); } } } - public void replace(AccessSection section) { - for (Permission permission : section.getPermissions()) { - ImmutableList.Builder newRules = ImmutableList.builder(); - for (PermissionRule rule : permission.getRules()) { - newRules.add(rule.toBuilder().setGroup(resolve(rule.getGroup())).build()); - } - permission.setRules(newRules.build()); - } - - accessSections.put(section.getName(), section); - } - public ContributorAgreement getContributorAgreement(String name) { return contributorAgreements.get(name); } @@ -790,38 +785,41 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. sectionsWithUnknownPermissions = new HashSet<>(); for (String refName : rc.getSubsections(ACCESS)) { if (AccessSection.isValidRefSectionName(refName) && isValidRegex(refName)) { - AccessSection as = getAccessSection(refName, true); + upsertAccessSection( + refName, + as -> { + for (String varName : rc.getStringList(ACCESS, refName, KEY_GROUP_PERMISSIONS)) { + for (String n : Splitter.on(EXCLUSIVE_PERMISSIONS_SPLIT_PATTERN).split(varName)) { + n = convertLegacyPermission(n); + if (isCoreOrPluginPermission(n)) { + as.upsertPermission(n).setExclusiveGroup(true); + } + } + } - for (String varName : rc.getStringList(ACCESS, refName, KEY_GROUP_PERMISSIONS)) { - for (String n : Splitter.on(EXCLUSIVE_PERMISSIONS_SPLIT_PATTERN).split(varName)) { - n = convertLegacyPermission(n); - if (isCoreOrPluginPermission(n)) { - as.getPermission(n, true).setExclusiveGroup(true); - } - } - } - - for (String varName : rc.getNames(ACCESS, refName)) { - String convertedName = convertLegacyPermission(varName); - if (isCoreOrPluginPermission(convertedName)) { - Permission perm = as.getPermission(convertedName, true); - loadPermissionRules( - rc, ACCESS, refName, varName, perm, Permission.hasRange(convertedName)); - } else { - sectionsWithUnknownPermissions.add(as.getName()); - } - } + for (String varName : rc.getNames(ACCESS, refName)) { + String convertedName = convertLegacyPermission(varName); + if (isCoreOrPluginPermission(convertedName)) { + Permission.Builder perm = as.upsertPermission(convertedName); + loadPermissionRules( + rc, ACCESS, refName, varName, perm, Permission.hasRange(convertedName)); + } else { + sectionsWithUnknownPermissions.add(as.getName()); + } + } + }); } } - AccessSection capability = null; + AccessSection.Builder capability = null; for (String varName : rc.getNames(CAPABILITY)) { if (capability == null) { - capability = new AccessSection(AccessSection.GLOBAL_CAPABILITIES); - accessSections.put(AccessSection.GLOBAL_CAPABILITIES, capability); + capability = AccessSection.builder(AccessSection.GLOBAL_CAPABILITIES); + accessSections.put(AccessSection.GLOBAL_CAPABILITIES, capability.build()); } - Permission perm = capability.getPermission(varName, true); + Permission.Builder perm = capability.upsertPermission(varName); loadPermissionRules(rc, CAPABILITY, null, varName, perm, GlobalCapability.hasRange(varName)); + accessSections.put(AccessSection.GLOBAL_CAPABILITIES, capability.build()); } } @@ -874,9 +872,9 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. private ImmutableList loadPermissionRules( Config rc, String section, String subsection, String varName, boolean useRange) { - Permission perm = new Permission(varName); + Permission.Builder perm = Permission.builder(varName); loadPermissionRules(rc, section, subsection, varName, perm, useRange); - return ImmutableList.copyOf(perm.getRules()); + return ImmutableList.copyOf(perm.build().getRules()); } private void loadPermissionRules( @@ -884,7 +882,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. String section, String subsection, String varName, - Permission perm, + Permission.Builder perm, boolean useRange) { for (String ruleString : rc.getStringList(section, subsection, varName)) { PermissionRule rule; @@ -916,7 +914,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. PROJECT_CONFIG, "group \"" + ref.getName() + "\" not in " + GroupList.FILE_NAME)); } - perm.add(rule.toBuilder().setGroup(ref).build()); + perm.add(rule.toBuilder().setGroup(ref)); } } diff --git a/java/com/google/gerrit/server/project/ProjectCreator.java b/java/com/google/gerrit/server/project/ProjectCreator.java index 5ec4a58f91..fa0a262eb2 100644 --- a/java/com/google/gerrit/server/project/ProjectCreator.java +++ b/java/com/google/gerrit/server/project/ProjectCreator.java @@ -179,14 +179,17 @@ public class ProjectCreator { }); if (!args.ownerIds.isEmpty()) { - AccessSection all = config.getAccessSection(AccessSection.ALL, true); - for (AccountGroup.UUID ownerId : args.ownerIds) { - GroupDescription.Basic g = groupBackend.get(ownerId); - if (g != null) { - GroupReference group = config.resolve(GroupReference.forGroup(g)); - all.getPermission(Permission.OWNER, true).add(PermissionRule.create(group)); - } - } + config.upsertAccessSection( + AccessSection.ALL, + all -> { + for (AccountGroup.UUID ownerId : args.ownerIds) { + GroupDescription.Basic g = groupBackend.get(ownerId); + if (g != null) { + GroupReference group = config.resolve(GroupReference.forGroup(g)); + all.upsertPermission(Permission.OWNER).add(PermissionRule.builder(group)); + } + } + }); } md.setMessage("Created project\n"); diff --git a/java/com/google/gerrit/server/project/ProjectState.java b/java/com/google/gerrit/server/project/ProjectState.java index 6353103b81..986019cf2b 100644 --- a/java/com/google/gerrit/server/project/ProjectState.java +++ b/java/com/google/gerrit/server/project/ProjectState.java @@ -280,14 +280,16 @@ public class ProjectState { sm = new ArrayList<>(fromConfig.size()); for (AccessSection section : fromConfig) { if (isAllProjects) { - List copy = Lists.newArrayListWithCapacity(section.getPermissions().size()); + List copy = new ArrayList<>(); for (Permission p : section.getPermissions()) { if (Permission.canBeOnAllProjects(section.getName(), p.getName())) { - copy.add(p); + copy.add(p.toBuilder()); } } - section = new AccessSection(section.getName()); - section.setPermissions(copy); + section = + AccessSection.builder(section.getName()) + .modifyPermissions(permissions -> permissions.addAll(copy)) + .build(); } SectionMatcher matcher = SectionMatcher.wrap(getNameKey(), section); diff --git a/java/com/google/gerrit/server/restapi/project/GetAccess.java b/java/com/google/gerrit/server/restapi/project/GetAccess.java index 5a3dbcd28e..24167801a2 100644 --- a/java/com/google/gerrit/server/restapi/project/GetAccess.java +++ b/java/com/google/gerrit/server/restapi/project/GetAccess.java @@ -208,9 +208,9 @@ public class GetAccess implements RestReadView { // user is a member of, as well as groups they own or that // are visible to all users. - AccessSection dst = null; + AccessSection.Builder dst = null; for (Permission srcPerm : section.getPermissions()) { - Permission dstPerm = null; + Permission.Builder dstPerm = null; for (PermissionRule srcRule : srcPerm.getRules()) { AccountGroup.UUID groupId = srcRule.getGroup().getUUID(); @@ -221,12 +221,12 @@ public class GetAccess implements RestReadView { loadGroup(groups, groupId); if (dstPerm == null) { if (dst == null) { - dst = new AccessSection(name); - info.local.put(name, createAccessSection(groups, dst)); + dst = AccessSection.builder(name); + info.local.put(name, createAccessSection(groups, dst.build())); } - dstPerm = dst.getPermission(srcPerm.getName(), true); + dstPerm = dst.upsertPermission(srcPerm.getName()); } - dstPerm.add(srcRule); + dstPerm.add(srcRule.toBuilder()); } } } diff --git a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java index 0f7bc3df0b..d1511c1351 100644 --- a/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java +++ b/java/com/google/gerrit/server/restapi/project/SetAccessUtil.java @@ -78,14 +78,14 @@ public class SetAccessUtil { continue; } - AccessSection accessSection = new AccessSection(entry.getKey()); + AccessSection.Builder accessSection = AccessSection.builder(entry.getKey()); for (Map.Entry permissionEntry : entry.getValue().permissions.entrySet()) { if (permissionEntry.getValue().rules == null) { continue; } - Permission p = new Permission(permissionEntry.getKey()); + Permission.Builder p = Permission.builder(permissionEntry.getKey()); if (permissionEntry.getValue().exclusive != null) { p.setExclusiveGroup(permissionEntry.getValue().exclusive); } @@ -114,11 +114,11 @@ public class SetAccessUtil { r.setForce(pri.force); } } - p.add(r.build()); + p.add(r); } accessSection.addPermission(p); } - sections.add(accessSection); + sections.add(accessSection.build()); } return sections; } @@ -193,25 +193,23 @@ public class SetAccessUtil { // Apply additions for (AccessSection section : additions) { - AccessSection currentAccessSection = config.getAccessSection(section.getName()); - - if (currentAccessSection == null) { - // Add AccessSection - config.replace(section); - } else { - for (Permission p : section.getPermissions()) { - Permission currentPermission = currentAccessSection.getPermission(p.getName()); - if (currentPermission == null) { - // Add Permission - currentAccessSection.addPermission(p); - } else { - for (PermissionRule r : p.getRules()) { - // AddPermissionRule - currentPermission.add(r); + config.upsertAccessSection( + section.getName(), + existingAccessSection -> { + for (Permission p : section.getPermissions()) { + Permission currentPermission = + existingAccessSection.build().getPermission(p.getName()); + if (currentPermission == null) { + // Add Permission + existingAccessSection.addPermission(p.toBuilder()); + } else { + for (PermissionRule r : p.getRules()) { + // AddPermissionRule + existingAccessSection.upsertPermission(p.getName()).add(r.toBuilder()); + } + } } - } - } - } + }); } } diff --git a/java/com/google/gerrit/server/schema/AclUtil.java b/java/com/google/gerrit/server/schema/AclUtil.java index ed4a6ec7ef..e65568f90c 100644 --- a/java/com/google/gerrit/server/schema/AclUtil.java +++ b/java/com/google/gerrit/server/schema/AclUtil.java @@ -27,13 +27,16 @@ import com.google.gerrit.server.project.ProjectConfig; */ public class AclUtil { public static void grant( - ProjectConfig config, AccessSection section, String permission, GroupReference... groupList) { + ProjectConfig config, + AccessSection.Builder section, + String permission, + GroupReference... groupList) { grant(config, section, permission, false, groupList); } public static void grant( ProjectConfig config, - AccessSection section, + AccessSection.Builder section, String permission, boolean force, GroupReference... groupList) { @@ -42,35 +45,38 @@ public class AclUtil { public static void grant( ProjectConfig config, - AccessSection section, + AccessSection.Builder section, String permission, boolean force, Boolean exclusive, GroupReference... groupList) { - Permission p = section.getPermission(permission, true); + Permission.Builder p = section.upsertPermission(permission); if (exclusive != null) { p.setExclusiveGroup(exclusive); } for (GroupReference group : groupList) { if (group != null) { - p.add(rule(config, group).setForce(force).build()); + p.add(rule(config, group).setForce(force)); } } } public static void block( - ProjectConfig config, AccessSection section, String permission, GroupReference... groupList) { - Permission p = section.getPermission(permission, true); + ProjectConfig config, + AccessSection.Builder section, + String permission, + GroupReference... groupList) { + Permission.Builder p = section.upsertPermission(permission); for (GroupReference group : groupList) { if (group != null) { - p.add(rule(config, group).setBlock().build()); + p.add(rule(config, group).setBlock()); } } } public static void grant( ProjectConfig config, - AccessSection section, + AccessSection.Builder section, LabelType type, int min, int max, @@ -80,18 +86,18 @@ public class AclUtil { public static void grant( ProjectConfig config, - AccessSection section, + AccessSection.Builder section, LabelType type, int min, int max, boolean exclusive, GroupReference... groupList) { String name = Permission.LABEL + type.getName(); - Permission p = section.getPermission(name, true); + Permission.Builder p = section.upsertPermission(name); p.setExclusiveGroup(exclusive); for (GroupReference group : groupList) { if (group != null) { - p.add(rule(config, group).setRange(min, max).build()); + p.add(rule(config, group).setRange(min, max)); } } } @@ -101,8 +107,11 @@ public class AclUtil { } public static void remove( - ProjectConfig config, AccessSection section, String permission, GroupReference... groupList) { - Permission p = section.getPermission(permission, true); + ProjectConfig config, + AccessSection.Builder section, + String permission, + GroupReference... groupList) { + Permission.Builder p = section.upsertPermission(permission); for (GroupReference group : groupList) { if (group != null) { p.remove(rule(config, group).build()); diff --git a/java/com/google/gerrit/server/schema/AllProjectsCreator.java b/java/com/google/gerrit/server/schema/AllProjectsCreator.java index 908ce0a2c2..0fb282e7e6 100644 --- a/java/com/google/gerrit/server/schema/AllProjectsCreator.java +++ b/java/com/google/gerrit/server/schema/AllProjectsCreator.java @@ -144,79 +144,107 @@ public class AllProjectsCreator { } private void initDefaultAcls(ProjectConfig config, AllProjectsInput input) { - AccessSection capabilities = config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES, true); - AccessSection heads = config.getAccessSection(AccessSection.HEADS, true); - checkArgument(input.codeReviewLabel().isPresent()); LabelType codeReviewLabel = input.codeReviewLabel().get(); - initDefaultAclsForRegisteredUsers(heads, codeReviewLabel, config); + config.upsertAccessSection( + AccessSection.HEADS, + heads -> { + initDefaultAclsForRegisteredUsers(heads, codeReviewLabel, config); + }); - input - .batchUsersGroup() - .ifPresent( - batchUsersGroup -> initDefaultAclsForBatchUsers(capabilities, config, batchUsersGroup)); + config.upsertAccessSection( + AccessSection.GLOBAL_CAPABILITIES, + capabilities -> { + input + .batchUsersGroup() + .ifPresent( + batchUsersGroup -> + initDefaultAclsForBatchUsers(capabilities, config, batchUsersGroup)); + }); input .administratorsGroup() - .ifPresent( - adminsGroup -> - initDefaultAclsForAdmins( - capabilities, config, heads, codeReviewLabel, adminsGroup)); + .ifPresent(adminsGroup -> initDefaultAclsForAdmins(config, codeReviewLabel, adminsGroup)); } private void initDefaultAclsForRegisteredUsers( - AccessSection heads, LabelType codeReviewLabel, ProjectConfig config) { - AccessSection refsFor = config.getAccessSection("refs/for/*", true); - AccessSection magic = config.getAccessSection("refs/for/" + AccessSection.ALL, true); - AccessSection all = config.getAccessSection("refs/*", true); + AccessSection.Builder heads, LabelType codeReviewLabel, ProjectConfig config) { + config.upsertAccessSection( + "refs/for/*", + refsFor -> { + grant(config, refsFor, Permission.ADD_PATCH_SET, registered); + }); - grant(config, refsFor, Permission.ADD_PATCH_SET, registered); grant(config, heads, codeReviewLabel, -1, 1, registered); grant(config, heads, Permission.FORGE_AUTHOR, registered); - grant(config, all, Permission.REVERT, registered); - grant(config, magic, Permission.PUSH, registered); - grant(config, magic, Permission.PUSH_MERGE, registered); + + config.upsertAccessSection( + "refs/*", + all -> { + grant(config, all, Permission.REVERT, registered); + }); + + config.upsertAccessSection( + "refs/for/" + AccessSection.ALL, + magic -> { + grant(config, magic, Permission.PUSH, registered); + grant(config, magic, Permission.PUSH_MERGE, registered); + }); } private void initDefaultAclsForBatchUsers( - AccessSection capabilities, ProjectConfig config, GroupReference batchUsersGroup) { - Permission priority = capabilities.getPermission(GlobalCapability.PRIORITY, true); - priority.add(rule(config, batchUsersGroup).setAction(Action.BATCH).build()); + AccessSection.Builder capabilities, ProjectConfig config, GroupReference batchUsersGroup) { + Permission.Builder priority = capabilities.upsertPermission(GlobalCapability.PRIORITY); + priority.add(rule(config, batchUsersGroup).setAction(Action.BATCH)); - Permission stream = capabilities.getPermission(GlobalCapability.STREAM_EVENTS, true); - stream.add(rule(config, batchUsersGroup).build()); + Permission.Builder stream = capabilities.upsertPermission(GlobalCapability.STREAM_EVENTS); + stream.add(rule(config, batchUsersGroup)); } private void initDefaultAclsForAdmins( - AccessSection capabilities, - ProjectConfig config, - AccessSection heads, - LabelType codeReviewLabel, - GroupReference adminsGroup) { - AccessSection all = config.getAccessSection(AccessSection.ALL, true); - AccessSection tags = config.getAccessSection("refs/tags/*", true); - AccessSection meta = config.getAccessSection(RefNames.REFS_CONFIG, true); + ProjectConfig config, LabelType codeReviewLabel, GroupReference adminsGroup) { + config.upsertAccessSection( + AccessSection.GLOBAL_CAPABILITIES, + capabilities -> { + grant(config, capabilities, GlobalCapability.ADMINISTRATE_SERVER, adminsGroup); + }); - grant(config, capabilities, GlobalCapability.ADMINISTRATE_SERVER, adminsGroup); - grant(config, all, Permission.READ, adminsGroup, anonymous); - grant(config, heads, codeReviewLabel, -2, 2, adminsGroup, owners); - grant(config, heads, Permission.CREATE, adminsGroup, owners); - grant(config, heads, Permission.PUSH, adminsGroup, owners); - grant(config, heads, Permission.SUBMIT, adminsGroup, owners); - grant(config, heads, Permission.FORGE_COMMITTER, adminsGroup, owners); - grant(config, heads, Permission.EDIT_TOPIC_NAME, true, adminsGroup, owners); + config.upsertAccessSection( + AccessSection.ALL, + all -> { + grant(config, all, Permission.READ, adminsGroup, anonymous); + }); - grant(config, tags, Permission.CREATE, adminsGroup, owners); - grant(config, tags, Permission.CREATE_TAG, adminsGroup, owners); - grant(config, tags, Permission.CREATE_SIGNED_TAG, adminsGroup, owners); + config.upsertAccessSection( + AccessSection.HEADS, + heads -> { + grant(config, heads, codeReviewLabel, -2, 2, adminsGroup, owners); + grant(config, heads, Permission.CREATE, adminsGroup, owners); + grant(config, heads, Permission.PUSH, adminsGroup, owners); + grant(config, heads, Permission.SUBMIT, adminsGroup, owners); + grant(config, heads, Permission.FORGE_COMMITTER, adminsGroup, owners); + grant(config, heads, Permission.EDIT_TOPIC_NAME, true, adminsGroup, owners); + }); - meta.getPermission(Permission.READ, true).setExclusiveGroup(true); - grant(config, meta, Permission.READ, adminsGroup, owners); - grant(config, meta, codeReviewLabel, -2, 2, adminsGroup, owners); - grant(config, meta, Permission.CREATE, adminsGroup, owners); - grant(config, meta, Permission.PUSH, adminsGroup, owners); - grant(config, meta, Permission.SUBMIT, adminsGroup, owners); + config.upsertAccessSection( + "refs/tags/*", + tags -> { + grant(config, tags, Permission.CREATE, adminsGroup, owners); + grant(config, tags, Permission.CREATE_TAG, adminsGroup, owners); + grant(config, tags, Permission.CREATE_SIGNED_TAG, adminsGroup, owners); + }); + + config.upsertAccessSection( + RefNames.REFS_CONFIG, + meta -> { + meta.upsertPermission(Permission.READ).setExclusiveGroup(true); + grant(config, meta, Permission.READ, adminsGroup, owners); + grant(config, meta, codeReviewLabel, -2, 2, adminsGroup, owners); + grant(config, meta, Permission.CREATE, adminsGroup, owners); + grant(config, meta, Permission.PUSH, adminsGroup, owners); + grant(config, meta, Permission.SUBMIT, adminsGroup, owners); + }); } private void initSequences(Repository git, BatchRefUpdate bru, int firstChangeId) diff --git a/java/com/google/gerrit/server/schema/AllUsersCreator.java b/java/com/google/gerrit/server/schema/AllUsersCreator.java index 10d7070af0..1ac8e6987f 100644 --- a/java/com/google/gerrit/server/schema/AllUsersCreator.java +++ b/java/com/google/gerrit/server/schema/AllUsersCreator.java @@ -22,7 +22,6 @@ import static com.google.gerrit.server.schema.AllProjectsInput.getDefaultCodeRev import com.google.gerrit.common.Nullable; import com.google.gerrit.common.UsedAt; import com.google.gerrit.common.Version; -import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.Permission; @@ -113,33 +112,39 @@ public class AllUsersCreator { ProjectConfig config = projectConfigFactory.read(md); config.updateProject(p -> p.setDescription("Individual user settings and preferences.")); - AccessSection users = - config.getAccessSection( - RefNames.REFS_USERS + "${" + RefPattern.USERID_SHARDED + "}", true); + config.upsertAccessSection( + RefNames.REFS_USERS + "${" + RefPattern.USERID_SHARDED + "}", + users -> { + grant(config, users, Permission.READ, false, true, registered); + grant(config, users, Permission.PUSH, false, true, registered); + grant(config, users, Permission.SUBMIT, false, true, registered); + grant(config, users, codeReviewLabel, -2, 2, true, registered); + }); // Initialize "Code-Review" label. config.upsertLabelType(codeReviewLabel); - grant(config, users, Permission.READ, false, true, registered); - grant(config, users, Permission.PUSH, false, true, registered); - grant(config, users, Permission.SUBMIT, false, true, registered); - grant(config, users, codeReviewLabel, -2, 2, true, registered); - if (admin != null) { - AccessSection defaults = config.getAccessSection(RefNames.REFS_USERS_DEFAULT, true); - defaults.getPermission(Permission.READ, true).setExclusiveGroup(true); - grant(config, defaults, Permission.READ, admin); - defaults.getPermission(Permission.PUSH, true).setExclusiveGroup(true); - grant(config, defaults, Permission.PUSH, admin); - defaults.getPermission(Permission.CREATE, true).setExclusiveGroup(true); - grant(config, defaults, Permission.CREATE, admin); + config.upsertAccessSection( + RefNames.REFS_USERS_DEFAULT, + defaults -> { + defaults.upsertPermission(Permission.READ).setExclusiveGroup(true); + grant(config, defaults, Permission.READ, admin); + defaults.upsertPermission(Permission.PUSH).setExclusiveGroup(true); + grant(config, defaults, Permission.PUSH, admin); + defaults.upsertPermission(Permission.CREATE).setExclusiveGroup(true); + grant(config, defaults, Permission.CREATE, admin); + }); } // Grant read permissions on the group branches to all users. // This allows group owners to see the group refs. VisibleRefFilter ensures that read // permissions for non-group-owners are ignored. - AccessSection groups = config.getAccessSection(RefNames.REFS_GROUPS + "*", true); - grant(config, groups, Permission.READ, false, true, registered); + config.upsertAccessSection( + RefNames.REFS_GROUPS + "*", + groups -> { + grant(config, groups, Permission.READ, false, true, registered); + }); config.commit(md); } diff --git a/java/com/google/gerrit/server/schema/GrantRevertPermission.java b/java/com/google/gerrit/server/schema/GrantRevertPermission.java index 2f890d52c9..d4ba29b50c 100644 --- a/java/com/google/gerrit/server/schema/GrantRevertPermission.java +++ b/java/com/google/gerrit/server/schema/GrantRevertPermission.java @@ -30,6 +30,7 @@ import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.project.ProjectConfig; import com.google.inject.Inject; import java.io.IOException; +import java.util.concurrent.atomic.AtomicBoolean; import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.lib.PersonIdent; import org.eclipse.jgit.lib.Repository; @@ -63,28 +64,41 @@ public class GrantRevertPermission { try (Repository repo = repoManager.openRepository(projectName)) { MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, projectName, repo); ProjectConfig projectConfig = projectConfigFactory.read(md); - AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true); - Permission permissionOnRefsHeads = heads.getPermission(Permission.REVERT); + AtomicBoolean shouldExit = new AtomicBoolean(false); + projectConfig.upsertAccessSection( + AccessSection.HEADS, + heads -> { + Permission permissionOnRefsHeads = heads.build().getPermission(Permission.REVERT); - if (permissionOnRefsHeads != null) { - if (permissionOnRefsHeads.getRule(registeredUsers) == null - || permissionOnRefsHeads.getRules().size() > 1) { - // If admins already changed the permission, don't do anything. - return; - } - // permission already exists in refs/heads/*, delete it for Registered Users. - remove(projectConfig, heads, Permission.REVERT, registeredUsers); - } + if (permissionOnRefsHeads != null) { + if (permissionOnRefsHeads.getRule(registeredUsers) == null + || permissionOnRefsHeads.getRules().size() > 1) { + // If admins already changed the permission, don't do anything. + shouldExit.set(true); + return; + } + // permission already exists in refs/heads/*, delete it for Registered Users. + remove(projectConfig, heads, Permission.REVERT, registeredUsers); + } + }); - AccessSection all = projectConfig.getAccessSection(AccessSection.ALL, true); - Permission permissionOnRefsStar = all.getPermission(Permission.REVERT); - if (permissionOnRefsStar != null && permissionOnRefsStar.getRule(registeredUsers) != null) { - // permission already exists in refs/*, don't do anything. + if (shouldExit.get()) { return; } - // If the permission doesn't exist of refs/* for Registered Users, grant it. - grant(projectConfig, all, Permission.REVERT, registeredUsers); + + projectConfig.upsertAccessSection( + AccessSection.ALL, + all -> { + Permission permissionOnRefsStar = all.build().getPermission(Permission.REVERT); + if (permissionOnRefsStar != null + && permissionOnRefsStar.getRule(registeredUsers) != null) { + // permission already exists in refs/*, don't do anything. + return; + } + // If the permission doesn't exist of refs/* for Registered Users, grant it. + grant(projectConfig, all, Permission.REVERT, registeredUsers); + }); md.getCommitBuilder().setAuthor(serverUser); md.getCommitBuilder().setCommitter(serverUser); diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 3c1bc2f32b..43cc1f44c4 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -1653,8 +1653,11 @@ public class AccountIT extends AbstractDaemonTest { // remove default READ permissions try (ProjectConfigUpdate u = updateProject(allUsers)) { u.getConfig() - .getAccessSection(RefNames.REFS_USERS + "${" + RefPattern.USERID_SHARDED + "}", true) - .remove(new Permission(Permission.READ)); + .upsertAccessSection( + RefNames.REFS_USERS + "${" + RefPattern.USERID_SHARDED + "}", + as -> { + as.remove(Permission.builder(Permission.READ)); + }); u.save(); } diff --git a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java index 98b93a880f..b1c07ad957 100644 --- a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java +++ b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java @@ -362,22 +362,28 @@ public class PushPermissionsIT extends AbstractDaemonTest { } private static void removeAllBranchPermissions(ProjectConfig cfg, String... permissions) { - cfg.getAccessSections().stream() - .filter( - s -> - s.getName().startsWith("refs/heads/") - || s.getName().startsWith("refs/for/") - || s.getName().equals("refs/*")) - .forEach(s -> Arrays.stream(permissions).forEach(s::removePermission)); + for (AccessSection s : ImmutableList.copyOf(cfg.getAccessSections())) { + if (s.getName().startsWith("refs/heads/") + || s.getName().startsWith("refs/for/") + || s.getName().equals("refs/*")) { + cfg.upsertAccessSection( + s.getName(), + updatedSection -> { + Arrays.stream(permissions).forEach(p -> updatedSection.remove(Permission.builder(p))); + }); + } + } } private static void removeAllGlobalCapabilities(ProjectConfig cfg, String... capabilities) { Arrays.stream(capabilities) .forEach( c -> - cfg.getAccessSection(AccessSection.GLOBAL_CAPABILITIES, true) - .getPermission(c, true) - .clearRules()); + cfg.upsertAccessSection( + AccessSection.GLOBAL_CAPABILITIES, + as -> { + as.upsertPermission(c).clearRules(); + })); } private PushResult push(String... refSpecs) throws Exception { diff --git a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java index 14288b58e2..8b9d173d73 100644 --- a/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java +++ b/javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java @@ -127,8 +127,14 @@ public class RefAdvertisementIT extends AbstractDaemonTest { private void setUpPermissions() throws Exception { // Remove read permissions for all users besides admin. try (ProjectConfigUpdate u = updateProject(allProjects)) { - for (AccessSection sec : u.getConfig().getAccessSections()) { - sec.removePermission(Permission.READ); + + for (AccessSection sec : ImmutableList.copyOf(u.getConfig().getAccessSections())) { + u.getConfig() + .upsertAccessSection( + sec.getName(), + updatedSec -> { + updatedSec.removePermission(Permission.READ); + }); } u.save(); } @@ -139,8 +145,13 @@ public class RefAdvertisementIT extends AbstractDaemonTest { // Remove all read permissions on All-Users. try (ProjectConfigUpdate u = updateProject(allUsers)) { - for (AccessSection sec : u.getConfig().getAccessSections()) { - sec.removePermission(Permission.READ); + for (AccessSection sec : ImmutableList.copyOf(u.getConfig().getAccessSections())) { + u.getConfig() + .upsertAccessSection( + sec.getName(), + updatedSec -> { + updatedSec.removePermission(Permission.READ); + }); } u.save(); } diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java index 0514e03540..46054ecb37 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java @@ -120,8 +120,11 @@ public class AccessIT extends AbstractDaemonTest { try (Repository repo = repoManager.openRepository(newProjectName)) { MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, newProjectName, repo); ProjectConfig projectConfig = projectConfigFactory.read(md); - AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true); - grant(projectConfig, heads, Permission.REVERT, registeredUsers); + projectConfig.upsertAccessSection( + AccessSection.HEADS, + heads -> { + grant(projectConfig, heads, Permission.REVERT, registeredUsers); + }); md.getCommitBuilder().setAuthor(admin.newIdent()); md.getCommitBuilder().setCommitter(admin.newIdent()); md.setMessage("Add revert permission for all registered users\n"); @@ -155,15 +158,19 @@ public class AccessIT extends AbstractDaemonTest { try (Repository repo = repoManager.openRepository(newProjectName)) { MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, newProjectName, repo); ProjectConfig projectConfig = projectConfigFactory.read(md); - AccessSection heads = projectConfig.getAccessSection(AccessSection.HEADS, true); - grant(projectConfig, heads, Permission.REVERT, registeredUsers); - grant(projectConfig, heads, Permission.REVERT, otherGroup); + projectConfig.upsertAccessSection( + AccessSection.HEADS, + heads -> { + grant(projectConfig, heads, Permission.REVERT, registeredUsers); + grant(projectConfig, heads, Permission.REVERT, otherGroup); + }); md.getCommitBuilder().setAuthor(admin.newIdent()); md.getCommitBuilder().setCommitter(admin.newIdent()); md.setMessage("Add revert permission for all registered users\n"); projectConfig.commit(md); } + projectCache.evict(newProjectName); ProjectAccessInfo expected = pApi().access(); grantRevertPermission.execute(newProjectName); @@ -181,7 +188,7 @@ public class AccessIT extends AbstractDaemonTest { try (Repository repo = repoManager.openRepository(newProjectName)) { MetaDataUpdate md = new MetaDataUpdate(GitReferenceUpdated.DISABLED, newProjectName, repo); ProjectConfig projectConfig = projectConfigFactory.read(md); - AccessSection all = projectConfig.getAccessSection(AccessSection.ALL, true); + AccessSection all = projectConfig.getAccessSection(AccessSection.ALL); Permission permission = all.getPermission(Permission.REVERT); assertThat(permission.getRules()).hasSize(1); diff --git a/javatests/com/google/gerrit/acceptance/rest/project/GetCommitIT.java b/javatests/com/google/gerrit/acceptance/rest/project/GetCommitIT.java index b18db81256..1b1a36de36 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/GetCommitIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/GetCommitIT.java @@ -129,7 +129,12 @@ public class GetCommitIT extends AbstractDaemonTest { private void unblockRead() throws Exception { try (ProjectConfigUpdate u = updateProject(project)) { - u.getConfig().getAccessSection("refs/*").remove(new Permission(Permission.READ)); + u.getConfig() + .upsertAccessSection( + "refs/*", + as -> { + as.remove(Permission.builder(Permission.READ)); + }); u.save(); } } diff --git a/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java b/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java index f5d2db464e..3d3865a2c0 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java @@ -28,6 +28,7 @@ import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; +import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.projects.ProjectApi.ListRefsRequest; import com.google.gerrit.extensions.api.projects.TagApi; @@ -158,6 +159,12 @@ public class TagsIT extends AbstractDaemonTest { @Test public void listTagsOfNonVisibleBranch() throws Exception { grantTagPermissions(); + // Allow creating a new hidden branch + projectOperations + .project(project) + .forUpdate() + .add(allow(Permission.CREATE).group(REGISTERED_USERS).ref("refs/heads/hidden")) + .update(); PushOneCommit push1 = pushFactory.create(admin.newIdent(), testRepo); PushOneCommit.Result r1 = push1.to("refs/heads/master"); @@ -169,7 +176,7 @@ public class TagsIT extends AbstractDaemonTest { assertThat(result.ref).isEqualTo(R_TAGS + tag1.ref); assertThat(result.revision).isEqualTo(tag1.revision); - pushTo("refs/heads/hidden"); + pushTo("refs/heads/hidden").assertOkStatus(); PushOneCommit push2 = pushFactory.create(admin.newIdent(), testRepo); PushOneCommit.Result r2 = push2.to("refs/heads/hidden"); r2.assertOkStatus(); @@ -470,8 +477,12 @@ public class TagsIT extends AbstractDaemonTest { } private static void removeAllBranchPermissions(ProjectConfig cfg, String... permissions) { - cfg.getAccessSections().stream() - .filter(s -> s.getName().startsWith("refs/tags/")) - .forEach(s -> Arrays.stream(permissions).forEach(s::removePermission)); + for (AccessSection accessSection : ImmutableList.copyOf(cfg.getAccessSections())) { + cfg.upsertAccessSection( + accessSection.getName(), + updatedAccessSection -> { + Arrays.stream(permissions).forEach(updatedAccessSection::removePermission); + }); + } } } diff --git a/javatests/com/google/gerrit/common/data/AccessSectionTest.java b/javatests/com/google/gerrit/common/data/AccessSectionTest.java index e775cbcdb5..f60156c83e 100644 --- a/javatests/com/google/gerrit/common/data/AccessSectionTest.java +++ b/javatests/com/google/gerrit/common/data/AccessSectionTest.java @@ -17,9 +17,6 @@ package com.google.gerrit.common.data; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.testing.GerritJUnit.assertThrows; -import com.google.common.collect.ImmutableList; -import java.util.ArrayList; -import java.util.List; import java.util.Locale; import org.junit.Before; import org.junit.Test; @@ -27,11 +24,11 @@ import org.junit.Test; public class AccessSectionTest { private static final String REF_PATTERN = "refs/heads/master"; - private AccessSection accessSection; + private AccessSection.Builder accessSection; @Before public void setup() { - this.accessSection = new AccessSection(REF_PATTERN); + this.accessSection = AccessSection.builder(REF_PATTERN); } @Test @@ -41,22 +38,36 @@ public class AccessSectionTest { @Test public void getEmptyPermissions() { - assertThat(accessSection.getPermissions()).isNotNull(); - assertThat(accessSection.getPermissions()).isEmpty(); + AccessSection builtAccessSection = accessSection.build(); + assertThat(builtAccessSection.getPermissions()).isNotNull(); + assertThat(builtAccessSection.getPermissions()).isEmpty(); } @Test public void setAndGetPermissions() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); - accessSection.setPermissions(ImmutableList.of(abandonPermission, rebasePermission)); - assertThat(accessSection.getPermissions()) - .containsExactly(abandonPermission, rebasePermission) - .inOrder(); + Permission.Builder abandonPermission = Permission.builder(Permission.ABANDON); + Permission.Builder rebasePermission = Permission.builder(Permission.REBASE); + accessSection.modifyPermissions( + permissions -> { + permissions.clear(); + permissions.add(abandonPermission); + permissions.add(rebasePermission); + }); - Permission submitPermission = new Permission(Permission.SUBMIT); - accessSection.setPermissions(ImmutableList.of(submitPermission)); - assertThat(accessSection.getPermissions()).containsExactly(submitPermission); + AccessSection builtAccessSection = accessSection.build(); + assertThat(builtAccessSection.getPermissions()).hasSize(2); + assertThat(builtAccessSection.getPermission(abandonPermission.getName())).isNotNull(); + assertThat(builtAccessSection.getPermission(rebasePermission.getName())).isNotNull(); + + Permission.Builder submitPermission = Permission.builder(Permission.SUBMIT); + accessSection.modifyPermissions( + p -> { + p.clear(); + p.add(submitPermission); + }); + builtAccessSection = accessSection.build(); + assertThat(builtAccessSection.getPermissions()).hasSize(1); + assertThat(builtAccessSection.getPermission(submitPermission.getName())).isNotNull(); assertThrows(NullPointerException.class, () -> accessSection.setPermissions(null)); } @@ -65,122 +76,117 @@ public class AccessSectionTest { assertThrows( IllegalArgumentException.class, () -> - accessSection.setPermissions( - ImmutableList.of( - new Permission(Permission.ABANDON), new Permission(Permission.ABANDON)))); + accessSection + .addPermission(Permission.builder(Permission.ABANDON)) + .addPermission(Permission.builder(Permission.ABANDON)) + .build()); } @Test public void cannotSetPermissionsWithConflictingNames() { - Permission abandonPermissionLowerCase = - new Permission(Permission.ABANDON.toLowerCase(Locale.US)); - Permission abandonPermissionUpperCase = - new Permission(Permission.ABANDON.toUpperCase(Locale.US)); + Permission.Builder abandonPermissionLowerCase = + Permission.builder(Permission.ABANDON.toLowerCase(Locale.US)); + Permission.Builder abandonPermissionUpperCase = + Permission.builder(Permission.ABANDON.toUpperCase(Locale.US)); assertThrows( IllegalArgumentException.class, () -> - accessSection.setPermissions( - ImmutableList.of(abandonPermissionLowerCase, abandonPermissionUpperCase))); + accessSection + .addPermission(abandonPermissionLowerCase) + .addPermission(abandonPermissionUpperCase) + .build()); } @Test public void getNonExistingPermission() { - assertThat(accessSection.getPermission("non-existing")).isNull(); - assertThat(accessSection.getPermission("non-existing", false)).isNull(); + assertThat(accessSection.build().getPermission("non-existing")).isNull(); + assertThat(accessSection.build().getPermission("non-existing")).isNull(); } @Test public void getPermission() { - Permission submitPermission = new Permission(Permission.SUBMIT); - accessSection.setPermissions(ImmutableList.of(submitPermission)); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isEqualTo(submitPermission); - assertThrows(NullPointerException.class, () -> accessSection.getPermission(null)); + Permission.Builder submitPermission = Permission.builder(Permission.SUBMIT); + accessSection.addPermission(submitPermission); + assertThat(accessSection.upsertPermission(Permission.SUBMIT)).isEqualTo(submitPermission); + assertThrows(NullPointerException.class, () -> accessSection.upsertPermission(null)); } @Test public void getPermissionWithOtherCase() { - Permission submitPermissionLowerCase = new Permission(Permission.SUBMIT.toLowerCase(Locale.US)); - accessSection.setPermissions(ImmutableList.of(submitPermissionLowerCase)); - assertThat(accessSection.getPermission(Permission.SUBMIT.toUpperCase(Locale.US))) + Permission.Builder submitPermissionLowerCase = + Permission.builder(Permission.SUBMIT.toLowerCase(Locale.US)); + accessSection.addPermission(submitPermissionLowerCase); + assertThat(accessSection.upsertPermission(Permission.SUBMIT.toUpperCase(Locale.US))) .isEqualTo(submitPermissionLowerCase); } @Test public void createMissingPermissionOnGet() { - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull(); + assertThat(accessSection.build().getPermission(Permission.SUBMIT)).isNull(); - assertThat(accessSection.getPermission(Permission.SUBMIT, true)) - .isEqualTo(new Permission(Permission.SUBMIT)); + assertThat(accessSection.upsertPermission(Permission.SUBMIT).build()) + .isEqualTo(Permission.create(Permission.SUBMIT)); - assertThrows(NullPointerException.class, () -> accessSection.getPermission(null, true)); + assertThrows(NullPointerException.class, () -> accessSection.upsertPermission(null)); } @Test public void addPermission() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); + Permission.Builder abandonPermission = Permission.builder(Permission.ABANDON); + Permission.Builder rebasePermission = Permission.builder(Permission.REBASE); - accessSection.setPermissions(ImmutableList.of(abandonPermission, rebasePermission)); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull(); + accessSection.addPermission(abandonPermission); + accessSection.addPermission(rebasePermission); + assertThat(accessSection.build().getPermission(Permission.SUBMIT)).isNull(); - Permission submitPermission = new Permission(Permission.SUBMIT); + Permission.Builder submitPermission = Permission.builder(Permission.SUBMIT); accessSection.addPermission(submitPermission); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isEqualTo(submitPermission); - assertThat(accessSection.getPermissions()) - .containsExactly(abandonPermission, rebasePermission, submitPermission) + assertThat(accessSection.build().getPermission(Permission.SUBMIT)) + .isEqualTo(submitPermission.build()); + assertThat(accessSection.build().getPermissions()) + .containsExactly( + abandonPermission.build(), rebasePermission.build(), submitPermission.build()) .inOrder(); assertThrows(NullPointerException.class, () -> accessSection.addPermission(null)); } - @Test - public void cannotAddPermissionByModifyingListThatWasProvidedToAccessSection() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); - - List permissions = new ArrayList<>(); - permissions.add(abandonPermission); - permissions.add(rebasePermission); - accessSection.setPermissions(permissions); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull(); - - Permission submitPermission = new Permission(Permission.SUBMIT); - permissions.add(submitPermission); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull(); - } - @Test public void removePermission() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); - Permission submitPermission = new Permission(Permission.SUBMIT); + Permission.Builder abandonPermission = Permission.builder(Permission.ABANDON); + Permission.Builder rebasePermission = Permission.builder(Permission.REBASE); + Permission.Builder submitPermission = Permission.builder(Permission.SUBMIT); - accessSection.setPermissions( - ImmutableList.of(abandonPermission, rebasePermission, submitPermission)); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNotNull(); + accessSection.addPermission(abandonPermission); + accessSection.addPermission(rebasePermission); + accessSection.addPermission(submitPermission); + assertThat(accessSection.build().getPermission(Permission.SUBMIT)).isNotNull(); accessSection.remove(submitPermission); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull(); - assertThat(accessSection.getPermissions()) - .containsExactly(abandonPermission, rebasePermission) + assertThat(accessSection.build().getPermission(Permission.SUBMIT)).isNull(); + assertThat(accessSection.build().getPermissions()) + .containsExactly(abandonPermission.build(), rebasePermission.build()) .inOrder(); assertThrows(NullPointerException.class, () -> accessSection.remove(null)); } @Test public void removePermissionByName() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); - Permission submitPermission = new Permission(Permission.SUBMIT); + Permission.Builder abandonPermission = Permission.builder(Permission.ABANDON); + Permission.Builder rebasePermission = Permission.builder(Permission.REBASE); + Permission.Builder submitPermission = Permission.builder(Permission.SUBMIT); - accessSection.setPermissions( - ImmutableList.of(abandonPermission, rebasePermission, submitPermission)); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNotNull(); + accessSection.addPermission(abandonPermission); + accessSection.addPermission(rebasePermission); + accessSection.addPermission(submitPermission); + AccessSection builtAccessSection = accessSection.build(); + assertThat(builtAccessSection.getPermission(Permission.SUBMIT)).isNotNull(); accessSection.removePermission(Permission.SUBMIT); - assertThat(accessSection.getPermission(Permission.SUBMIT)).isNull(); - assertThat(accessSection.getPermissions()) - .containsExactly(abandonPermission, rebasePermission) + builtAccessSection = accessSection.build(); + assertThat(builtAccessSection.getPermission(Permission.SUBMIT)).isNull(); + assertThat(builtAccessSection.getPermissions()) + .containsExactly(abandonPermission.build(), rebasePermission.build()) .inOrder(); assertThrows(NullPointerException.class, () -> accessSection.removePermission(null)); @@ -188,62 +194,49 @@ public class AccessSectionTest { @Test public void removePermissionByNameOtherCase() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); + Permission.Builder abandonPermission = Permission.builder(Permission.ABANDON); + Permission.Builder rebasePermission = Permission.builder(Permission.REBASE); String submitLowerCase = Permission.SUBMIT.toLowerCase(Locale.US); String submitUpperCase = Permission.SUBMIT.toUpperCase(Locale.US); - Permission submitPermissionLowerCase = new Permission(submitLowerCase); + Permission.Builder submitPermissionLowerCase = Permission.builder(submitLowerCase); - accessSection.setPermissions( - ImmutableList.of(abandonPermission, rebasePermission, submitPermissionLowerCase)); - assertThat(accessSection.getPermission(submitLowerCase)).isNotNull(); - assertThat(accessSection.getPermission(submitUpperCase)).isNotNull(); + accessSection.addPermission(abandonPermission); + accessSection.addPermission(rebasePermission); + accessSection.addPermission(submitPermissionLowerCase); + AccessSection builtAccessSection = accessSection.build(); + assertThat(builtAccessSection.getPermission(submitLowerCase)).isNotNull(); + assertThat(builtAccessSection.getPermission(submitUpperCase)).isNotNull(); accessSection.removePermission(submitUpperCase); - assertThat(accessSection.getPermission(submitLowerCase)).isNull(); - assertThat(accessSection.getPermission(submitUpperCase)).isNull(); - assertThat(accessSection.getPermissions()) - .containsExactly(abandonPermission, rebasePermission) + builtAccessSection = accessSection.build(); + assertThat(builtAccessSection.getPermission(submitLowerCase)).isNull(); + assertThat(builtAccessSection.getPermission(submitUpperCase)).isNull(); + assertThat(builtAccessSection.getPermissions()) + .containsExactly(abandonPermission.build(), rebasePermission.build()) .inOrder(); } - @Test - public void mergeAccessSections() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); - Permission submitPermission = new Permission(Permission.SUBMIT); - - AccessSection accessSection1 = new AccessSection("refs/heads/foo"); - accessSection1.setPermissions(ImmutableList.of(abandonPermission, rebasePermission)); - - AccessSection accessSection2 = new AccessSection("refs/heads/bar"); - accessSection2.setPermissions(ImmutableList.of(rebasePermission, submitPermission)); - - accessSection1.mergeFrom(accessSection2); - assertThat(accessSection1.getPermissions()) - .containsExactly(abandonPermission, rebasePermission, submitPermission) - .inOrder(); - assertThrows(NullPointerException.class, () -> accessSection.mergeFrom(null)); - } - @Test public void testEquals() { - Permission abandonPermission = new Permission(Permission.ABANDON); - Permission rebasePermission = new Permission(Permission.REBASE); + Permission.Builder abandonPermission = Permission.builder(Permission.ABANDON); + Permission.Builder rebasePermission = Permission.builder(Permission.REBASE); - accessSection.setPermissions(ImmutableList.of(abandonPermission, rebasePermission)); + accessSection.addPermission(abandonPermission); + accessSection.addPermission(rebasePermission); - AccessSection accessSectionSamePermissionsOtherRef = new AccessSection("refs/heads/other"); - accessSectionSamePermissionsOtherRef.setPermissions( - ImmutableList.of(abandonPermission, rebasePermission)); - assertThat(accessSection.equals(accessSectionSamePermissionsOtherRef)).isFalse(); + AccessSection builtAccessSection = accessSection.build(); + AccessSection.Builder accessSectionSamePermissionsOtherRef = + AccessSection.builder("refs/heads/other"); + accessSectionSamePermissionsOtherRef.addPermission(abandonPermission); + accessSectionSamePermissionsOtherRef.addPermission(rebasePermission); + assertThat(builtAccessSection.equals(accessSectionSamePermissionsOtherRef.build())).isFalse(); - AccessSection accessSectionOther = new AccessSection(REF_PATTERN); - accessSectionOther.setPermissions(ImmutableList.of(abandonPermission)); - assertThat(accessSection.equals(accessSectionOther)).isFalse(); + AccessSection.Builder accessSectionOther = AccessSection.builder(REF_PATTERN); + accessSectionOther.addPermission(abandonPermission); + assertThat(builtAccessSection.equals(accessSectionOther.build())).isFalse(); accessSectionOther.addPermission(rebasePermission); - assertThat(accessSection.equals(accessSectionOther)).isTrue(); + assertThat(builtAccessSection.equals(accessSectionOther.build())).isTrue(); } } diff --git a/javatests/com/google/gerrit/common/data/PermissionTest.java b/javatests/com/google/gerrit/common/data/PermissionTest.java index a36312945a..9dd71cab10 100644 --- a/javatests/com/google/gerrit/common/data/PermissionTest.java +++ b/javatests/com/google/gerrit/common/data/PermissionTest.java @@ -16,21 +16,18 @@ package com.google.gerrit.common.data; import static com.google.common.truth.Truth.assertThat; -import com.google.common.collect.ImmutableList; import com.google.gerrit.entities.AccountGroup; -import java.util.ArrayList; -import java.util.List; import org.junit.Before; import org.junit.Test; public class PermissionTest { private static final String PERMISSION_NAME = "foo"; - private Permission permission; + private Permission.Builder permission; @Before public void setup() { - this.permission = new Permission(PERMISSION_NAME); + this.permission = Permission.builder(PERMISSION_NAME); } @Test @@ -117,209 +114,179 @@ public class PermissionTest { @Test public void getLabel() { - assertThat(new Permission(Permission.LABEL + "Code-Review").getLabel()) + assertThat(Permission.create(Permission.LABEL + "Code-Review").getLabel()) .isEqualTo("Code-Review"); - assertThat(new Permission(Permission.LABEL_AS + "Code-Review").getLabel()) + assertThat(Permission.create(Permission.LABEL_AS + "Code-Review").getLabel()) .isEqualTo("Code-Review"); - assertThat(new Permission("Code-Review").getLabel()).isNull(); - assertThat(new Permission(Permission.ABANDON).getLabel()).isNull(); + assertThat(Permission.create("Code-Review").getLabel()).isNull(); + assertThat(Permission.create(Permission.ABANDON).getLabel()).isNull(); } @Test public void exclusiveGroup() { - assertThat(permission.getExclusiveGroup()).isFalse(); + assertThat(permission.build().getExclusiveGroup()).isFalse(); permission.setExclusiveGroup(true); - assertThat(permission.getExclusiveGroup()).isTrue(); + assertThat(permission.build().getExclusiveGroup()).isTrue(); permission.setExclusiveGroup(false); - assertThat(permission.getExclusiveGroup()).isFalse(); + assertThat(permission.build().getExclusiveGroup()).isFalse(); } @Test public void noExclusiveGroupOnOwnerPermission() { - Permission permission = new Permission(Permission.OWNER); + Permission permission = Permission.create(Permission.OWNER); assertThat(permission.getExclusiveGroup()).isFalse(); - permission.setExclusiveGroup(true); + permission = permission.toBuilder().setExclusiveGroup(true).build(); assertThat(permission.getExclusiveGroup()).isFalse(); } @Test public void getEmptyRules() { - assertThat(permission.getRules()).isNotNull(); - assertThat(permission.getRules()).isEmpty(); + assertThat(permission.getRulesBuilders()).isNotNull(); + assertThat(permission.getRulesBuilders()).isEmpty(); } @Test public void setAndGetRules() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); - permission.setRules(ImmutableList.of(permissionRule1, permissionRule2)); - assertThat(permission.getRules()).containsExactly(permissionRule1, permissionRule2).inOrder(); + PermissionRule.Builder permissionRule1 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); + PermissionRule.Builder permissionRule2 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); + permission.add(permissionRule1); + permission.add(permissionRule2); + assertThat(permission.getRulesBuilders()) + .containsExactly(permissionRule1, permissionRule2) + .inOrder(); - PermissionRule permissionRule3 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-3"), "group3")); - permission.setRules(ImmutableList.of(permissionRule3)); - assertThat(permission.getRules()).containsExactly(permissionRule3); - } - - @Test - public void cannotAddPermissionByModifyingListThatWasProvidedToAccessSection() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); - GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"); - - List rules = new ArrayList<>(); - rules.add(permissionRule1); - rules.add(permissionRule2); - permission.setRules(rules); - assertThat(permission.getRule(groupReference3)).isNull(); - - PermissionRule permissionRule3 = PermissionRule.create(groupReference3); - rules.add(permissionRule3); - assertThat(permission.getRule(groupReference3)).isNull(); + PermissionRule.Builder permissionRule3 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-3"), "group3")); + permission.modifyRules( + rules -> { + rules.clear(); + rules.add(permissionRule3); + }); + assertThat(permission.getRulesBuilders()).containsExactly(permissionRule3); } @Test public void getNonExistingRule() { GroupReference groupReference = GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"); - assertThat(permission.getRule(groupReference)).isNull(); - assertThat(permission.getRule(groupReference, false)).isNull(); + assertThat(permission.build().getRule(groupReference)).isNull(); + assertThat(permission.build().getRule(groupReference)).isNull(); } @Test public void getRule() { GroupReference groupReference = GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"); - PermissionRule permissionRule = PermissionRule.create(groupReference); - permission.setRules(ImmutableList.of(permissionRule)); - assertThat(permission.getRule(groupReference)).isEqualTo(permissionRule); - } - - @Test - public void createMissingRuleOnGet() { - GroupReference groupReference = GroupReference.create(AccountGroup.uuid("uuid-1"), "group1"); - assertThat(permission.getRule(groupReference)).isNull(); - - assertThat(permission.getRule(groupReference, true)) - .isEqualTo(PermissionRule.create(groupReference)); + PermissionRule.Builder permissionRule = PermissionRule.builder(groupReference); + permission.add(permissionRule); + assertThat(permission.build().getRule(groupReference)).isEqualTo(permissionRule.build()); } @Test public void addRule() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); - permission.setRules(ImmutableList.of(permissionRule1, permissionRule2)); + PermissionRule.Builder permissionRule1 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); + PermissionRule.Builder permissionRule2 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); + permission.add(permissionRule1); + permission.add(permissionRule2); GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"); - assertThat(permission.getRule(groupReference3)).isNull(); + assertThat(permission.build().getRule(groupReference3)).isNull(); - PermissionRule permissionRule3 = PermissionRule.create(groupReference3); + PermissionRule.Builder permissionRule3 = PermissionRule.builder(groupReference3); permission.add(permissionRule3); - assertThat(permission.getRule(groupReference3)).isEqualTo(permissionRule3); - assertThat(permission.getRules()) - .containsExactly(permissionRule1, permissionRule2, permissionRule3) + assertThat(permission.build().getRule(groupReference3)).isEqualTo(permissionRule3.build()); + assertThat(permission.build().getRules()) + .containsExactly(permissionRule1.build(), permissionRule2.build(), permissionRule3.build()) .inOrder(); } @Test public void removeRule() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); + PermissionRule.Builder permissionRule1 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); + PermissionRule.Builder permissionRule2 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"); - PermissionRule permissionRule3 = PermissionRule.create(groupReference3); + PermissionRule.Builder permissionRule3 = PermissionRule.builder(groupReference3); - permission.setRules(ImmutableList.of(permissionRule1, permissionRule2, permissionRule3)); - assertThat(permission.getRule(groupReference3)).isNotNull(); + permission.add(permissionRule1); + permission.add(permissionRule2); + permission.add(permissionRule3); + assertThat(permission.build().getRule(groupReference3)).isNotNull(); - permission.remove(permissionRule3); - assertThat(permission.getRule(groupReference3)).isNull(); - assertThat(permission.getRules()).containsExactly(permissionRule1, permissionRule2).inOrder(); - } - - @Test - public void removeRuleByGroupReference() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); - GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"); - PermissionRule permissionRule3 = PermissionRule.create(groupReference3); - - permission.setRules(ImmutableList.of(permissionRule1, permissionRule2, permissionRule3)); - assertThat(permission.getRule(groupReference3)).isNotNull(); - - permission.removeRule(groupReference3); - assertThat(permission.getRule(groupReference3)).isNull(); - assertThat(permission.getRules()).containsExactly(permissionRule1, permissionRule2).inOrder(); - } - - @Test - public void clearRules() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); - - permission.setRules(ImmutableList.of(permissionRule1, permissionRule2)); - assertThat(permission.getRules()).isNotEmpty(); - - permission.clearRules(); - assertThat(permission.getRules()).isEmpty(); - } - - @Test - public void mergePermissions() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); - PermissionRule permissionRule3 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-3"), "group3")); - - Permission permission1 = new Permission("foo"); - permission1.setRules(ImmutableList.of(permissionRule1, permissionRule2)); - - Permission permission2 = new Permission("bar"); - permission2.setRules(ImmutableList.of(permissionRule2, permissionRule3)); - - permission1.mergeFrom(permission2); - assertThat(permission1.getRules()) - .containsExactly(permissionRule1, permissionRule2, permissionRule3) + permission.remove(permissionRule3.build()); + assertThat(permission.build().getRule(groupReference3)).isNull(); + assertThat(permission.build().getRules()) + .containsExactly(permissionRule1.build(), permissionRule2.build()) .inOrder(); } + @Test + public void removeRuleByGroupReference() { + PermissionRule.Builder permissionRule1 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); + PermissionRule.Builder permissionRule2 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); + GroupReference groupReference3 = GroupReference.create(AccountGroup.uuid("uuid-3"), "group3"); + PermissionRule.Builder permissionRule3 = PermissionRule.builder(groupReference3); + + permission.add(permissionRule1); + permission.add(permissionRule2); + permission.add(permissionRule3); + assertThat(permission.build().getRule(groupReference3)).isNotNull(); + + permission.removeRule(groupReference3); + assertThat(permission.build().getRule(groupReference3)).isNull(); + assertThat(permission.build().getRules()) + .containsExactly(permissionRule1.build(), permissionRule2.build()) + .inOrder(); + } + + @Test + public void clearRules() { + PermissionRule.Builder permissionRule1 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); + PermissionRule.Builder permissionRule2 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); + + permission.add(permissionRule1); + permission.add(permissionRule2); + assertThat(permission.build().getRules()).isNotEmpty(); + + permission.clearRules(); + assertThat(permission.build().getRules()).isEmpty(); + } + @Test public void testEquals() { - PermissionRule permissionRule1 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); - PermissionRule permissionRule2 = - PermissionRule.create(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); + PermissionRule.Builder permissionRule1 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-1"), "group1")); + PermissionRule.Builder permissionRule2 = + PermissionRule.builder(GroupReference.create(AccountGroup.uuid("uuid-2"), "group2")); - permission.setRules(ImmutableList.of(permissionRule1, permissionRule2)); + permission.add(permissionRule1); + permission.add(permissionRule2); - Permission permissionSameRulesOtherName = new Permission("bar"); - permissionSameRulesOtherName.setRules(ImmutableList.of(permissionRule1, permissionRule2)); + Permission.Builder permissionSameRulesOtherName = Permission.builder("bar"); + permissionSameRulesOtherName.add(permissionRule1); + permissionSameRulesOtherName.add(permissionRule2); assertThat(permission.equals(permissionSameRulesOtherName)).isFalse(); - Permission permissionSameRulesSameNameOtherExclusiveGroup = new Permission("foo"); - permissionSameRulesSameNameOtherExclusiveGroup.setRules( - ImmutableList.of(permissionRule1, permissionRule2)); + Permission.Builder permissionSameRulesSameNameOtherExclusiveGroup = Permission.builder("foo"); + permissionSameRulesSameNameOtherExclusiveGroup.add(permissionRule1); + permissionSameRulesSameNameOtherExclusiveGroup.add(permissionRule2); permissionSameRulesSameNameOtherExclusiveGroup.setExclusiveGroup(true); assertThat(permission.equals(permissionSameRulesSameNameOtherExclusiveGroup)).isFalse(); - Permission permissionOther = new Permission(PERMISSION_NAME); - permissionOther.setRules(ImmutableList.of(permissionRule1)); - assertThat(permission.equals(permissionOther)).isFalse(); + Permission.Builder permissionOther = Permission.builder(PERMISSION_NAME); + permissionOther.add(permissionRule1); + assertThat(permission.build().equals(permissionOther.build())).isFalse(); permissionOther.add(permissionRule2); - assertThat(permission.equals(permissionOther)).isTrue(); + assertThat(permission.build().equals(permissionOther.build())).isTrue(); } } diff --git a/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java b/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java index ba8485bd17..ea210ab618 100644 --- a/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java +++ b/javatests/com/google/gerrit/server/change/LabelNormalizerTest.java @@ -98,10 +98,15 @@ public class LabelNormalizerTest { private void configureProject() throws Exception { ProjectConfig pc = loadAllProjects(); - for (AccessSection sec : pc.getAccessSections()) { - for (String label : pc.getLabelSections().keySet()) { - sec.removePermission(forLabel(label)); - } + + for (AccessSection sec : ImmutableList.copyOf(pc.getAccessSections())) { + pc.upsertAccessSection( + sec.getName(), + updatedSection -> { + for (String label : pc.getLabelSections().keySet()) { + updatedSection.removePermission(forLabel(label)); + } + }); } LabelType lt = label("Verified", value(1, "Verified"), value(0, "No score"), value(-1, "Fails")); diff --git a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java index b7125c3db4..3686e4e008 100644 --- a/javatests/com/google/gerrit/server/project/ProjectConfigTest.java +++ b/javatests/com/google/gerrit/server/project/ProjectConfigTest.java @@ -303,12 +303,15 @@ public class ProjectConfigTest { update(rev); ProjectConfig cfg = read(rev); - AccessSection section = cfg.getAccessSection("refs/heads/*"); + cfg.upsertAccessSection( + "refs/heads/*", + section -> { + Permission.Builder submit = section.upsertPermission(Permission.SUBMIT); + submit.add(PermissionRule.builder(cfg.resolve(staff))); + }); cfg.getAccountsSection() .setSameGroupVisibility( Collections.singletonList(PermissionRule.create(cfg.resolve(staff)))); - Permission submit = section.getPermission(Permission.SUBMIT); - submit.add(PermissionRule.create(cfg.resolve(staff))); ContributorAgreement.Builder ca = cfg.getContributorAgreement("Individual").toBuilder(); ca.setAccepted(ImmutableList.of(PermissionRule.create(cfg.resolve(staff)))); ca.setAutoVerify(null); @@ -423,9 +426,12 @@ public class ProjectConfigTest { update(rev); ProjectConfig cfg = read(rev); - AccessSection section = cfg.getAccessSection("refs/heads/*"); - Permission submit = section.getPermission(Permission.SUBMIT); - submit.add(PermissionRule.create(cfg.resolve(staff))); + cfg.upsertAccessSection( + "refs/heads/*", + section -> { + Permission.Builder submit = section.upsertPermission(Permission.SUBMIT); + submit.add(PermissionRule.builder(cfg.resolve(staff))); + }); rev = commit(cfg); assertThat(text(rev, "project.config")) .isEqualTo( diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 968d4f70e2..2eb25da36c 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -44,7 +44,6 @@ import com.google.common.truth.ThrowableSubject; import com.google.gerrit.acceptance.config.GerritConfig; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.common.Nullable; -import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.Permission; @@ -1859,13 +1858,16 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) { md.setMessage(String.format("Grant %s on %s", permission, ref)); ProjectConfig config = projectConfigFactory.read(md); - AccessSection s = config.getAccessSection(ref, true); - Permission p = s.getPermission(permission, true); - PermissionRule rule = - PermissionRule.builder(GroupReference.create(groupUUID, groupUUID.get())) - .setForce(force) - .build(); - p.add(rule); + config.upsertAccessSection( + ref, + s -> { + Permission.Builder p = s.upsertPermission(permission); + PermissionRule.Builder rule = + PermissionRule.builder(GroupReference.create(groupUUID, groupUUID.get())) + .setForce(force); + p.add(rule); + }); + config.commit(md); projectCache.evict(config.getProject()); }