diff --git a/core/src/main/java/hudson/security/Permission.java b/core/src/main/java/hudson/security/Permission.java index 20c43b0980..7c758072da 100644 --- a/core/src/main/java/hudson/security/Permission.java +++ b/core/src/main/java/hudson/security/Permission.java @@ -144,10 +144,11 @@ public final class Permission { * See {@link #description}. * @param impliedBy * See {@link #impliedBy}. + * @throws IllegalStateException if this permission was already defined */ public Permission(@Nonnull PermissionGroup group, @Nonnull String name, @CheckForNull Localizable description, @CheckForNull Permission impliedBy, boolean enable, - @Nonnull PermissionScope[] scopes) { + @Nonnull PermissionScope[] scopes) throws IllegalStateException { if(!JSONUtils.isJavaIdentifier(name)) throw new IllegalArgumentException(name+" is not a Java identifier"); this.owner = group.owner; @@ -221,6 +222,14 @@ public final class Permission { return owner.getName()+'.'+name; } + @Override public boolean equals(Object o) { + return o instanceof Permission && getId().equals(((Permission) o).getId()); + } + + @Override public int hashCode() { + return getId().hashCode(); + } + /** * Convert the ID representation into {@link Permission} object. * diff --git a/core/src/main/java/hudson/security/PermissionGroup.java b/core/src/main/java/hudson/security/PermissionGroup.java index dc7bdaad25..724e359462 100644 --- a/core/src/main/java/hudson/security/PermissionGroup.java +++ b/core/src/main/java/hudson/security/PermissionGroup.java @@ -23,17 +23,13 @@ */ package hudson.security; -import hudson.CopyOnWrite; import hudson.model.Hudson; - -import java.util.List; -import java.util.Collections; import java.util.ArrayList; import java.util.Iterator; -import java.util.Map; -import java.util.concurrent.CopyOnWriteArrayList; -import java.util.concurrent.ConcurrentHashMap; - +import java.util.List; +import java.util.SortedSet; +import java.util.TreeSet; +import javax.annotation.CheckForNull; import org.jvnet.localizer.Localizable; /** @@ -42,8 +38,7 @@ import org.jvnet.localizer.Localizable; * Sortable by the owner class name. */ public final class PermissionGroup implements Iterable, Comparable { - private final List permisisons = new CopyOnWriteArrayList(); - private final List permisisonsView = Collections.unmodifiableList(permisisons); + private final SortedSet permissions = new TreeSet(Permission.ID_COMPARATOR); public final Class owner; /** @@ -52,37 +47,41 @@ public final class PermissionGroup implements Iterable, Comparable

allGroups = new ArrayList(ALL); - allGroups.add(this); - Collections.sort(allGroups); - ALL = Collections.unmodifiableList(allGroups); - } - - PERMISSIONS.put(owner,this); + private String id() { + return owner.getName(); } public Iterator iterator() { - return permisisons.iterator(); + return getPermissions().iterator(); } - /*package*/ void add(Permission p) { - permisisons.add(p); + /*package*/ synchronized void add(Permission p) { + if (!permissions.add(p)) { + throw new IllegalStateException("attempt to register a second Permission for " + p.getId()); + } } /** * Lists up all the permissions in this group. */ - public List getPermissions() { - return permisisonsView; + public synchronized List getPermissions() { + return new ArrayList(permissions); } - public boolean hasPermissionContainedBy(PermissionScope scope) { - for (Permission p : permisisons) + public synchronized boolean hasPermissionContainedBy(PermissionScope scope) { + for (Permission p : permissions) if (p.isContainedBy(scope)) return true; return false; @@ -91,8 +90,8 @@ public final class PermissionGroup implements Iterable, Comparable

, Comparable

, Comparable

ALL = Collections.emptyList(); + @Override public int hashCode() { + return id().hashCode(); + } + + public synchronized int size() { + return permissions.size(); + } + + @Override public String toString() { + return "PermissionGroup[" + id() + "]"; + } + + private static synchronized void register(PermissionGroup g) { + if (!PERMISSIONS.add(g)) { + throw new IllegalStateException("attempt to register a second PermissionGroup for " + g.id()); + } + } /** * Returns all the {@link PermissionGroup}s available in the system. * @return * always non-null. Read-only. */ - public static List getAll() { - return ALL; + public static synchronized List getAll() { + return new ArrayList(PERMISSIONS); } /** @@ -139,12 +150,17 @@ public final class PermissionGroup implements Iterable, Comparable

PERMISSIONS = new ConcurrentHashMap(); + private static final SortedSet PERMISSIONS = new TreeSet(); } diff --git a/test/src/test/java/hudson/security/PermissionGroupTest.java b/test/src/test/java/hudson/security/PermissionGroupTest.java index b06d4e02a4..ebcae065ea 100644 --- a/test/src/test/java/hudson/security/PermissionGroupTest.java +++ b/test/src/test/java/hudson/security/PermissionGroupTest.java @@ -23,19 +23,35 @@ */ package hudson.security; +import hudson.model.Hudson; +import hudson.model.Messages; import jenkins.model.Jenkins; -import org.jvnet.hudson.test.HudsonTestCase; +import static org.junit.Assert.*; +import org.junit.Rule; +import org.junit.Test; import org.jvnet.hudson.test.Email; +import org.jvnet.hudson.test.JenkinsRule; + +public class PermissionGroupTest { + + @Rule public JenkinsRule r = new JenkinsRule(); -/** - * @author Kohsuke Kawaguchi - */ -public class PermissionGroupTest extends HudsonTestCase { /** - * "Overall" persmission group should be always the first. + * "Overall" permission group should be always the first. */ - @Email("http://www.nabble.com/Master-slave-refactor-td21361880.html") - public void testOrder() { + @Email("http://jenkins-ci.361315.n4.nabble.com/Master-slave-refactor-tp391495.html") + @Test public void order() { assertSame(PermissionGroup.getAll().get(0), Jenkins.PERMISSIONS); } + + @SuppressWarnings("ResultOfObjectAllocationIgnored") + @Test(expected=IllegalStateException.class) public void duplicatedGroups() { + new PermissionGroup(Hudson.class, Messages._Hudson_Permissions_Title()); + } + + @SuppressWarnings("ResultOfObjectAllocationIgnored") + @Test(expected=IllegalStateException.class) public void duplicatedPermissions() { + new Permission(Jenkins.PERMISSIONS, "Read", Messages._Hudson_ReadPermission_Description(), Permission.READ, PermissionScope.JENKINS); + } + }