From 16b46ce27d43c5a53e2f277dbda5760a290b62cc Mon Sep 17 00:00:00 2001
From: Jesse Glick <jglick@cloudbees.com>
Date: Tue, 28 Oct 2014 11:57:30 -0700
Subject: [PATCH 1/5] Typos.

---
 .../java/hudson/security/PermissionGroup.java    | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/core/src/main/java/hudson/security/PermissionGroup.java b/core/src/main/java/hudson/security/PermissionGroup.java
index dc7bdaad25..47cf38acaa 100644
--- a/core/src/main/java/hudson/security/PermissionGroup.java
+++ b/core/src/main/java/hudson/security/PermissionGroup.java
@@ -42,8 +42,8 @@ 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 List<Permission> permissions = new CopyOnWriteArrayList<Permission>();
+    private final List<Permission> permissionsView = Collections.unmodifiableList(permissions);
     public final Class owner;
 
     /**
@@ -67,22 +67,22 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
     }
 
     public Iterator<Permission> iterator() {
-        return permisisons.iterator();
+        return permissions.iterator();
     }
 
     /*package*/ void add(Permission p) {
-        permisisons.add(p);
+        permissions.add(p);
     }
 
     /**
      * Lists up all the permissions in this group.
      */
     public List<Permission> getPermissions() {
-        return permisisonsView;
+        return permissionsView;
     }
 
     public boolean hasPermissionContainedBy(PermissionScope scope) {
-        for (Permission p : permisisons)
+        for (Permission p : permissions)
             if (p.isContainedBy(scope))
                 return true;
         return false;
@@ -92,7 +92,7 @@ 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) {
+        for (Permission p : permissions) {
             if(p.name.equals(name))
                 return p;
         }
@@ -116,7 +116,7 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
     }
 
     public int size() {
-        return permisisons.size();
+        return permissions.size();
     }
 
     /**

From d0cdcb5798299d68d3565f6980f88ce618ba0f56 Mon Sep 17 00:00:00 2001
From: Jesse Glick <jglick@cloudbees.com>
Date: Tue, 28 Oct 2014 15:42:17 -0400
Subject: [PATCH 2/5] JUnit 4.

---
 .../hudson/security/PermissionGroupTest.java     | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/test/src/test/java/hudson/security/PermissionGroupTest.java b/test/src/test/java/hudson/security/PermissionGroupTest.java
index b06d4e02a4..4d2cda7204 100644
--- a/test/src/test/java/hudson/security/PermissionGroupTest.java
+++ b/test/src/test/java/hudson/security/PermissionGroupTest.java
@@ -24,18 +24,24 @@
 package hudson.security;
 
 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;
 
 /**
  * @author Kohsuke Kawaguchi
  */
-public class PermissionGroupTest extends HudsonTestCase {
+public class PermissionGroupTest {
+
+    @Rule public JenkinsRule r = new JenkinsRule();
+
     /**
-     * "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);
     }
 }

From 8878c7fa5b2bc129b6185ed44d8c160a4c4bd651 Mon Sep 17 00:00:00 2001
From: Jesse Glick <jglick@cloudbees.com>
Date: Tue, 28 Oct 2014 16:18:20 -0400
Subject: [PATCH 3/5] Prevent duplicate Permission or PermissionGroup objects
 from being created.

---
 .../main/java/hudson/security/Permission.java |  8 ++
 .../java/hudson/security/PermissionGroup.java | 82 +++++++++++--------
 .../hudson/security/PermissionGroupTest.java  | 16 +++-
 3 files changed, 67 insertions(+), 39 deletions(-)

diff --git a/core/src/main/java/hudson/security/Permission.java b/core/src/main/java/hudson/security/Permission.java
index 20c43b0980..785b3070c9 100644
--- a/core/src/main/java/hudson/security/Permission.java
+++ b/core/src/main/java/hudson/security/Permission.java
@@ -221,6 +221,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 47cf38acaa..648c5786e4 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<Permission>, Comparable<PermissionGroup> {
-    private final List<Permission> permissions = new CopyOnWriteArrayList<Permission>();
-    private final List<Permission> permissionsView = Collections.unmodifiableList(permissions);
+    private final SortedSet<Permission> permissions = new TreeSet<Permission>(Permission.ID_COMPARATOR);
     public final Class owner;
 
     /**
@@ -55,33 +50,31 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
     public PermissionGroup(Class owner, Localizable title) {
         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 permissions.iterator();
+        return getPermissions().iterator();
     }
 
-    /*package*/ void add(Permission p) {
-        permissions.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 permissionsView;
+    public synchronized List<Permission> getPermissions() {
+        return new ArrayList<Permission>(permissions);
     }
 
-    public boolean hasPermissionContainedBy(PermissionScope scope) {
+    public synchronized boolean hasPermissionContainedBy(PermissionScope scope) {
         for (Permission p : permissions)
             if (p.isContainedBy(scope))
                 return true;
@@ -91,7 +84,7 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
     /**
      * Finds a permission that has the given name.
      */
-    public Permission find(String name) {
+    public synchronized Permission find(String name) {
         for (Permission p : permissions) {
             if(p.name.equals(name))
                 return p;
@@ -107,7 +100,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 +108,35 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
         return 1;
     }
 
+    @Override public boolean equals(Object o) {
+        return o instanceof PermissionGroup && id().equals(((PermissionGroup) o).id());
+    }
+
+    @Override public int hashCode() {
+        return id().hashCode();
+    }
+
     public int size() {
         return permissions.size();
     }
 
-    /**
-     * All groups. Sorted.
-     */
-    @CopyOnWrite
-    private static List<PermissionGroup> ALL = Collections.emptyList();
+    @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 +144,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>();
 }
diff --git a/test/src/test/java/hudson/security/PermissionGroupTest.java b/test/src/test/java/hudson/security/PermissionGroupTest.java
index 4d2cda7204..ebcae065ea 100644
--- a/test/src/test/java/hudson/security/PermissionGroupTest.java
+++ b/test/src/test/java/hudson/security/PermissionGroupTest.java
@@ -23,6 +23,8 @@
  */
 package hudson.security;
 
+import hudson.model.Hudson;
+import hudson.model.Messages;
 import jenkins.model.Jenkins;
 import static org.junit.Assert.*;
 import org.junit.Rule;
@@ -30,9 +32,6 @@ import org.junit.Test;
 import org.jvnet.hudson.test.Email;
 import org.jvnet.hudson.test.JenkinsRule;
 
-/**
- * @author Kohsuke Kawaguchi
- */
 public class PermissionGroupTest {
 
     @Rule public JenkinsRule r = new JenkinsRule();
@@ -44,4 +43,15 @@ public class PermissionGroupTest {
     @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);
+    }
+
 }

From 2e778e49383800e8cccd17b64b409364d76424e1 Mon Sep 17 00:00:00 2001
From: Jesse Glick <jglick@cloudbees.com>
Date: Mon, 10 Nov 2014 15:44:20 -0500
Subject: [PATCH 4/5] Document that IllegalStateException will be thrown for a
 duplicate Permission or PermissionGroup.

---
 core/src/main/java/hudson/security/Permission.java      | 3 ++-
 core/src/main/java/hudson/security/PermissionGroup.java | 8 +++++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/core/src/main/java/hudson/security/Permission.java b/core/src/main/java/hudson/security/Permission.java
index 785b3070c9..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;
diff --git a/core/src/main/java/hudson/security/PermissionGroup.java b/core/src/main/java/hudson/security/PermissionGroup.java
index 648c5786e4..9ffb5bfcef 100644
--- a/core/src/main/java/hudson/security/PermissionGroup.java
+++ b/core/src/main/java/hudson/security/PermissionGroup.java
@@ -47,7 +47,13 @@ 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);

From a946c126229f27029ed3c32c3ef25e685a98f658 Mon Sep 17 00:00:00 2001
From: Jesse Glick <jglick@cloudbees.com>
Date: Mon, 10 Nov 2014 15:45:24 -0500
Subject: [PATCH 5/5] Enforcing consistent synchronization of
 PermissionGroup.permissions.

---
 core/src/main/java/hudson/security/PermissionGroup.java | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/core/src/main/java/hudson/security/PermissionGroup.java b/core/src/main/java/hudson/security/PermissionGroup.java
index 9ffb5bfcef..724e359462 100644
--- a/core/src/main/java/hudson/security/PermissionGroup.java
+++ b/core/src/main/java/hudson/security/PermissionGroup.java
@@ -122,7 +122,7 @@ public final class PermissionGroup implements Iterable<Permission>, Comparable<P
         return id().hashCode();
     }
 
-    public int size() {
+    public synchronized int size() {
         return permissions.size();
     }