Merge branch 'unique-Permission' (#1444).

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.
      *

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<Permission>, Comparable<PermissionGroup> {
-    private final List<Permission> permisisons = new CopyOnWriteArrayList<Permission>();
-    private final List<Permission> permisisonsView = Collections.unmodifiableList(permisisons);
+    private final SortedSet<Permission> permissions = new TreeSet<Permission>(Permission.ID_COMPARATOR);
     public final Class owner;
 
     /**
@@ -52,37 +47,41 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
      */
     public final Localizable title;
 
-    public PermissionGroup(Class owner, Localizable title) {
+    /**
+     * Both creates a registers a new permission group.
+     * @param owner sets {@link #owner}
+     * @param title sets {@link #title}
+     * @throws IllegalStateException if this group was already registered
+     */
+    public PermissionGroup(Class owner, Localizable title) throws IllegalStateException {
         this.owner = owner;
         this.title = title;
+        register(this);
+    }
 
-        synchronized(PermissionGroup.class) {
-            List<PermissionGroup> allGroups = new ArrayList<PermissionGroup>(ALL);
-            allGroups.add(this);
-            Collections.sort(allGroups);
-            ALL = Collections.unmodifiableList(allGroups);
-        }
-
-        PERMISSIONS.put(owner,this);
+    private String id() {
+        return owner.getName();
     }
 
     public Iterator<Permission> 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<Permission> getPermissions() {
-        return permisisonsView;
+    public synchronized List<Permission> getPermissions() {
+        return new ArrayList<Permission>(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<Permission>, Comparable<P
     /**
      * Finds a permission that has the given name.
      */
-    public Permission find(String name) {
-        for (Permission p : permisisons) {
+    public synchronized Permission find(String name) {
+        for (Permission p : permissions) {
             if(p.name.equals(name))
                 return p;
         }
@@ -107,7 +106,7 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
 
         // among the permissions of the same group, just sort by their names
         // so that the sort order is consistent regardless of classloading order.
-        return this.owner.getName().compareTo(that.owner.getName());
+        return id().compareTo(that.id());
     }
 
     private int compareOrder() {
@@ -115,23 +114,35 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
         return 1;
     }
 
-    public int size() {
-        return permisisons.size();
+    @Override public boolean equals(Object o) {
+        return o instanceof PermissionGroup && id().equals(((PermissionGroup) o).id());
     }
 
-    /**
-     * All groups. Sorted.
-     */
-    @CopyOnWrite
-    private static List<PermissionGroup> 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<PermissionGroup> getAll() {
-        return ALL;
+    public static synchronized List<PermissionGroup> getAll() {
+        return new ArrayList<PermissionGroup>(PERMISSIONS);
     }
 
     /**
@@ -139,12 +150,17 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
      *
      * @return  null if not found.
      */
-    public static PermissionGroup get(Class owner) {
-        return PERMISSIONS.get(owner);
+    public static synchronized @CheckForNull PermissionGroup get(Class owner) {
+        for (PermissionGroup g : PERMISSIONS) {
+            if (g.owner == owner) {
+                return g;
+            }
+        }
+        return null;
     }
 
     /**
      * All the permissions in the system, keyed by their owners.
      */
-    private static final Map<Class, PermissionGroup> PERMISSIONS = new ConcurrentHashMap<Class, PermissionGroup>();
+    private static final SortedSet<PermissionGroup> PERMISSIONS = new TreeSet<PermissionGroup>();
 }

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);
+    }
+
 }