Yi EungJun 2013-10-18
AccessControl: Do not allow to access orphan resource.
When trying to check whether someone can access a resource, if the
resource belongs to a project but the project is missing,
an IllegalStateException is thrown.
@0a1f95981869c1dc79a0e18528a46edbb0b4c91d
app/models/Attachment.java
--- app/models/Attachment.java
+++ app/models/Attachment.java
@@ -12,6 +12,7 @@
 
 import javax.persistence.*;
 
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import models.resource.ResourceConvertible;
 
@@ -143,10 +144,10 @@
      * @return
      */
     public void moveTo(Resource to) {
-        if(to.getProject() != null) {
-            projectId = to.getProject().id;
-        } else {
+        if (to instanceof GlobalResource) {
             projectId = null;
+        } else {
+            projectId = to.getProject().id;
         }
         containerType = to.getType();
         containerId = to.getId();
@@ -223,9 +224,9 @@
     public boolean store(File file, String name, Resource container) throws IOException, NoSuchAlgorithmException {
         // Store the file as its SHA1 hash in filesystem, and record its
         // metadata - projectId, containerType, containerId, size and hash - in Database.
-
-        Project project = container.getProject();
-        this.projectId = project == null ? 0L : project.id;
+        if (!(container instanceof GlobalResource)) {
+            this.projectId = container.getProject().id;
+        }
         this.containerType = container.getType();
         this.containerId = container.getId();
 
@@ -325,50 +326,70 @@
      */
     @Override
     public Resource asResource() {
-        return new Resource() {
-            @Override
-            public String getId() {
-                return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                if (projectId != null) {
-                    return Project.find.byId(projectId);
-                } else {
-                    return null;
+        if (projectId == null) {
+            return new GlobalResource() {
+                @Override
+                public String getId() {
+                    return id.toString();
                 }
-            }
 
-            @Override
-            public ResourceType getType() {
-                return ResourceType.ATTACHMENT;
-            }
+                @Override
+                public ResourceType getType() {
+                    return ResourceType.ATTACHMENT;
+                }
 
-            @Override
-            public Resource getContainer() {
-                return new Resource() {
-
-                    @Override
-                    public String getId() {
-                        return containerId;
-                    }
-
-                    @Override
-                    public Project getProject() {
-                        if (projectId != null) {
-                            return Project.find.byId(projectId);
-                        } else {
-                            return null;
+                @Override
+                public Resource getContainer() {
+                    return  new GlobalResource() {
+                        @Override
+                        public String getId() {
+                            return containerId;
                         }
-                    }
 
-                    @Override
-                    public ResourceType getType() {
-                        return containerType;
-                    }
-                };
-            }
-        };
+                        @Override
+                        public ResourceType getType() {
+                            return containerType;
+                        }
+                    };
+                }
+            };
+        } else {
+            return new Resource() {
+                @Override
+                public String getId() {
+                    return id.toString();
+                }
+
+                @Override
+                public Project getProject() {
+                    return Project.find.byId(projectId);
+                }
+
+                @Override
+                public ResourceType getType() {
+                    return ResourceType.ATTACHMENT;
+                }
+
+                @Override
+                public Resource getContainer() {
+                    return new Resource() {
+                        @Override
+                        public String getId() {
+                            return containerId;
+                        }
+
+                        @Override
+                        public Project getProject() {
+                            return Project.find.byId(projectId);
+                        }
+
+                        @Override
+                        public ResourceType getType() {
+                            return containerType;
+                        }
+                    };
+                }
+            };
+        }
     }
 }
app/models/Label.java
--- app/models/Label.java
+++ app/models/Label.java
@@ -1,6 +1,7 @@
 package models;
 
 import models.enumeration.ResourceType;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import models.resource.ResourceConvertible;
 import play.data.validation.Constraints.Required;
@@ -98,15 +99,10 @@
      */
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override
app/models/NotificationEvent.java
--- app/models/NotificationEvent.java
+++ app/models/NotificationEvent.java
@@ -4,6 +4,7 @@
 import models.enumeration.RequestState;
 import models.enumeration.ResourceType;
 import models.enumeration.State;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import org.apache.commons.lang3.StringUtils;
 import org.joda.time.DateTime;
@@ -186,7 +187,11 @@
         default:
             Resource resource = getResource();
             if (resource != null) {
-                return resource.getProject();
+                if (resource instanceof GlobalResource) {
+                    return null;
+                } else {
+                    return resource.getProject();
+                }
             } else {
                 return null;
             }
app/models/NullUser.java
--- app/models/NullUser.java
+++ app/models/NullUser.java
@@ -2,6 +2,7 @@
 
 import controllers.UserApp;
 import models.enumeration.ResourceType;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import play.i18n.Messages;
 
@@ -31,15 +32,10 @@
 
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override
app/models/Project.java
--- app/models/Project.java
+++ app/models/Project.java
@@ -9,6 +9,7 @@
 import models.enumeration.RequestState;
 import models.enumeration.ResourceType;
 import models.enumeration.RoleType;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 
 import org.apache.commons.lang3.StringUtils;
@@ -485,16 +486,11 @@
      */
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
 
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override
app/models/User.java
--- app/models/User.java
+++ app/models/User.java
@@ -7,6 +7,7 @@
 
 import controllers.UserApp;
 import models.enumeration.*;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import models.resource.ResourceConvertible;
 import models.support.FinderTemplate;
@@ -319,15 +320,10 @@
      */
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override
@@ -338,15 +334,10 @@
     }
 
     public Resource avatarAsResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override
 
app/models/resource/GlobalResource.java (added)
+++ app/models/resource/GlobalResource.java
@@ -0,0 +1,12 @@
+package models.resource;
+
+import models.Project;
+import models.enumeration.ResourceType;
+import models.resource.Resource;
+
+abstract public class GlobalResource extends Resource {
+    @Override
+    public Project getProject() {
+        throw new UnsupportedOperationException();
+    }
+}
app/utils/AccessControl.java
--- app/utils/AccessControl.java
+++ app/utils/AccessControl.java
@@ -4,6 +4,7 @@
 import models.enumeration.Operation;
 import models.enumeration.ResourceType;
 
+import models.resource.GlobalResource;
 import models.resource.Resource;
 
 public class AccessControl {
@@ -84,7 +85,8 @@
      * @param operation
      * @return
      */
-    private static boolean isGlobalResourceAllowed(User user, Resource resource, Operation operation) {
+    private static boolean isGlobalResourceAllowed(User user, GlobalResource resource,
+                                                   Operation operation) {
         // Temporary attachments are allowed only for the user who uploads them.
         if (resource.getType() == ResourceType.ATTACHMENT
                 && resource.getContainer().getType() == ResourceType.USER) {
@@ -195,16 +197,21 @@
      * @return {@code user}가 {@code resource}에 {@code operation}을
      *         하는 것이 허용되는지의 여부
      */
-    public static boolean isAllowed(User user, Resource resource, Operation operation) {
+    public static boolean isAllowed(User user, Resource resource, Operation operation)
+            throws IllegalStateException {
         if (user.isSiteManager()) {
             return true;
         }
 
-        Project project = resource.getProject();
-
-        if (project == null) {
-            return isGlobalResourceAllowed(user, resource, operation);
+        if (resource instanceof GlobalResource) {
+            return isGlobalResourceAllowed(user, (GlobalResource) resource, operation);
         } else {
+            Project project = resource.getProject();
+
+            if (project == null) {
+                throw new IllegalStateException("A project resource lost its project");
+            }
+
             return isProjectResourceAllowed(user, project, resource, operation);
         }
     }
test/utils/AccessControlTest.java
--- test/utils/AccessControlTest.java
+++ test/utils/AccessControlTest.java
@@ -5,12 +5,14 @@
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.Assert;
 
 import play.test.Helpers;
 
 import static org.fest.assertions.Assertions.assertThat;
 
 import models.enumeration.Operation;
+import models.enumeration.State;
 
 public class AccessControlTest extends ModelTest<Role>{
     @Before
@@ -74,4 +76,30 @@
         assertThat(canUpdate).isEqualTo(false);
         assertThat(canDelete).isEqualTo(false);
     }
+
+    // AccessControl.isAllowed throws IllegalStateException if the resource
+    // belongs to a project but the project is missing.
+    @Test
+    public void isAllowed_lostProject() {
+        // Given
+        User author = User.findByLoginId("nori");
+        Project projectYobi = Project.findByOwnerAndProjectName("yobi", "projectYobi");
+        Issue issue = new Issue();
+        issue.setProject(projectYobi);
+        issue.setTitle("hello");
+        issue.setBody("world");
+        issue.setAuthor(author);
+        issue.state = State.OPEN;
+        issue.save();
+
+        // When
+        issue.project = null;
+
+        // Then
+        try {
+            AccessControl.isAllowed(author, issue.asResource(), Operation.READ);
+            Assert.fail();
+        } catch (IllegalStateException e) {
+        }
+    }
 }
Add a comment
List