Commit @0a1f95981869c1dc79a0e18528a46edbb0b4c91d

[Notice] Announcing the End of Demo Server [Read me]

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

a555b42

0a1f959

app/models/Attachment.java

--- app/models/Attachment.java

+++ app/models/Attachment.java


 
 import javax.persistence.*;
 
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import models.resource.ResourceConvertible;
 

      * @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();

     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();
 

      */
     @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;
+                        }
+                    };
+                }
+            };
+        }
     }
 }

a555b42

0a1f959

app/models/Label.java

--- app/models/Label.java

+++ app/models/Label.java


 package models;
 
 import models.enumeration.ResourceType;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import models.resource.ResourceConvertible;
 import play.data.validation.Constraints.Required;

      */
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override

a555b42

0a1f959

app/models/NotificationEvent.java

--- app/models/NotificationEvent.java

+++ app/models/NotificationEvent.java


 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;

         default:
             Resource resource = getResource();
             if (resource != null) {
-                return resource.getProject();
+                if (resource instanceof GlobalResource) {
+                    return null;
+                } else {
+                    return resource.getProject();
+                }
             } else {
                 return null;
             }

a555b42

0a1f959

app/models/NullUser.java

--- app/models/NullUser.java

+++ app/models/NullUser.java


 
 import controllers.UserApp;
 import models.enumeration.ResourceType;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import play.i18n.Messages;
 

 
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override

a555b42

0a1f959

app/models/Project.java

--- app/models/Project.java

+++ app/models/Project.java


 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;

      */
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
 
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override

a555b42

0a1f959

app/models/User.java

--- app/models/User.java

+++ app/models/User.java


 
 import controllers.UserApp;
 import models.enumeration.*;
+import models.resource.GlobalResource;
 import models.resource.Resource;
 import models.resource.ResourceConvertible;
 import models.support.FinderTemplate;

      */
     @Override
     public Resource asResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override

     }
 
     public Resource avatarAsResource() {
-        return new Resource() {
+        return new GlobalResource() {
             @Override
             public String getId() {
                 return id.toString();
-            }
-
-            @Override
-            public Project getProject() {
-                return null;
             }
 
             @Override

0a1f959

app/models/resource/GlobalResource.java (added)

+++ app/models/resource/GlobalResource.java

...	...	@@ -0,0 +1,12 @@
	1	+package models.resource;
	2	+
	3	+import models.Project;
	4	+import models.enumeration.ResourceType;
	5	+import models.resource.Resource;
	6	+
	7	+abstract public class GlobalResource extends Resource {
	8	+ @Override
	9	+ public Project getProject() {
	10	+ throw new UnsupportedOperationException();
	11	+ }
	12	+}

a555b42

0a1f959

app/utils/AccessControl.java

--- app/utils/AccessControl.java

+++ app/utils/AccessControl.java


 import models.enumeration.Operation;
 import models.enumeration.ResourceType;
 
+import models.resource.GlobalResource;
 import models.resource.Resource;
 
 public class AccessControl {

      * @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) {

      * @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);
         }
     }

a555b42

0a1f959

test/utils/AccessControlTest.java

--- test/utils/AccessControlTest.java

+++ test/utils/AccessControlTest.java


 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

         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

Open 0
Closed 0

List

...	...	@@ -12,6 +12,7 @@
12	12
13	13	import javax.persistence.*;
14	14
	15	+import models.resource.GlobalResource;
15	16	import models.resource.Resource;
16	17	import models.resource.ResourceConvertible;
17	18
...	...	@@ -143,10 +144,10 @@
143	144	* @return
144	145	*/
145	146	public void moveTo(Resource to) {
146		- if(to.getProject() != null) {
147		- projectId = to.getProject().id;
148		- } else {
	147	+ if (to instanceof GlobalResource) {
149	148	projectId = null;
	149	+ } else {
	150	+ projectId = to.getProject().id;
150	151	}
151	152	containerType = to.getType();
152	153	containerId = to.getId();
...	...	@@ -223,9 +224,9 @@
223	224	public boolean store(File file, String name, Resource container) throws IOException, NoSuchAlgorithmException {
224	225	// Store the file as its SHA1 hash in filesystem, and record its
225	226	// metadata - projectId, containerType, containerId, size and hash - in Database.
226		-
227		- Project project = container.getProject();
228		- this.projectId = project == null ? 0L : project.id;
	227	+ if (!(container instanceof GlobalResource)) {
	228	+ this.projectId = container.getProject().id;
	229	+ }
229	230	this.containerType = container.getType();
230	231	this.containerId = container.getId();
231	232
...	...	@@ -325,50 +326,70 @@
325	326	*/
326	327	@Override
327	328	public Resource asResource() {
328		- return new Resource() {
329		- @Override
330		- public String getId() {
331		- return id.toString();
332		- }
333		-
334		- @Override
335		- public Project getProject() {
336		- if (projectId != null) {
337		- return Project.find.byId(projectId);
338		- } else {
339		- return null;
	329	+ if (projectId == null) {
	330	+ return new GlobalResource() {
	331	+ @Override
	332	+ public String getId() {
	333	+ return id.toString();
340	334	}
341		- }
342	335
343		- @Override
344		- public ResourceType getType() {
345		- return ResourceType.ATTACHMENT;
346		- }
	336	+ @Override
	337	+ public ResourceType getType() {
	338	+ return ResourceType.ATTACHMENT;
	339	+ }
347	340
348		- @Override
349		- public Resource getContainer() {
350		- return new Resource() {
351		-
352		- @Override
353		- public String getId() {
354		- return containerId;
355		- }
356		-
357		- @Override
358		- public Project getProject() {
359		- if (projectId != null) {
360		- return Project.find.byId(projectId);
361		- } else {
362		- return null;
	341	+ @Override
	342	+ public Resource getContainer() {
	343	+ return new GlobalResource() {
	344	+ @Override
	345	+ public String getId() {
	346	+ return containerId;
363	347	}
364		- }
365	348
366		- @Override
367		- public ResourceType getType() {
368		- return containerType;
369		- }
370		- };
371		- }
372		- };
	349	+ @Override
	350	+ public ResourceType getType() {
	351	+ return containerType;
	352	+ }
	353	+ };
	354	+ }
	355	+ };
	356	+ } else {
	357	+ return new Resource() {
	358	+ @Override
	359	+ public String getId() {
	360	+ return id.toString();
	361	+ }
	362	+
	363	+ @Override
	364	+ public Project getProject() {
	365	+ return Project.find.byId(projectId);
	366	+ }
	367	+
	368	+ @Override
	369	+ public ResourceType getType() {
	370	+ return ResourceType.ATTACHMENT;
	371	+ }
	372	+
	373	+ @Override
	374	+ public Resource getContainer() {
	375	+ return new Resource() {
	376	+ @Override
	377	+ public String getId() {
	378	+ return containerId;
	379	+ }
	380	+
	381	+ @Override
	382	+ public Project getProject() {
	383	+ return Project.find.byId(projectId);
	384	+ }
	385	+
	386	+ @Override
	387	+ public ResourceType getType() {
	388	+ return containerType;
	389	+ }
	390	+ };
	391	+ }
	392	+ };
	393	+ }
373	394	}
374	395	}

...	...	@@ -1,6 +1,7 @@
1	1	package models;
2	2
3	3	import models.enumeration.ResourceType;
	4	+import models.resource.GlobalResource;
4	5	import models.resource.Resource;
5	6	import models.resource.ResourceConvertible;
6	7	import play.data.validation.Constraints.Required;
...	...	@@ -98,15 +99,10 @@
98	99	*/
99	100	@Override
100	101	public Resource asResource() {
101		- return new Resource() {
	102	+ return new GlobalResource() {
102	103	@Override
103	104	public String getId() {
104	105	return id.toString();
105		- }
106		-
107		- @Override
108		- public Project getProject() {
109		- return null;
110	106	}
111	107
112	108	@Override

...	...	@@ -2,6 +2,7 @@
2	2
3	3	import controllers.UserApp;
4	4	import models.enumeration.ResourceType;
	5	+import models.resource.GlobalResource;
5	6	import models.resource.Resource;
6	7	import play.i18n.Messages;
7	8
...	...	@@ -31,15 +32,10 @@
31	32
32	33	@Override
33	34	public Resource asResource() {
34		- return new Resource() {
	35	+ return new GlobalResource() {
35	36	@Override
36	37	public String getId() {
37	38	return id.toString();
38		- }
39		-
40		- @Override
41		- public Project getProject() {
42		- return null;
43	39	}
44	40
45	41	@Override

...	...	@@ -9,6 +9,7 @@
9	9	import models.enumeration.RequestState;
10	10	import models.enumeration.ResourceType;
11	11	import models.enumeration.RoleType;
	12	+import models.resource.GlobalResource;
12	13	import models.resource.Resource;
13	14
14	15	import org.apache.commons.lang3.StringUtils;
...	...	@@ -485,16 +486,11 @@
485	486	*/
486	487	@Override
487	488	public Resource asResource() {
488		- return new Resource() {
	489	+ return new GlobalResource() {
489	490
490	491	@Override
491	492	public String getId() {
492	493	return id.toString();
493		- }
494		-
495		- @Override
496		- public Project getProject() {
497		- return null;
498	494	}
499	495
500	496	@Override

...	...	@@ -7,6 +7,7 @@
7	7
8	8	import controllers.UserApp;
9	9	import models.enumeration.*;
	10	+import models.resource.GlobalResource;
10	11	import models.resource.Resource;
11	12	import models.resource.ResourceConvertible;
12	13	import models.support.FinderTemplate;
...	...	@@ -319,15 +320,10 @@
319	320	*/
320	321	@Override
321	322	public Resource asResource() {
322		- return new Resource() {
	323	+ return new GlobalResource() {
323	324	@Override
324	325	public String getId() {
325	326	return id.toString();
326		- }
327		-
328		- @Override
329		- public Project getProject() {
330		- return null;
331	327	}
332	328
333	329	@Override
...	...	@@ -338,15 +334,10 @@
338	334	}
339	335
340	336	public Resource avatarAsResource() {
341		- return new Resource() {
	337	+ return new GlobalResource() {
342	338	@Override
343	339	public String getId() {
344	340	return id.toString();
345		- }
346		-
347		- @Override
348		- public Project getProject() {
349		- return null;
350	341	}
351	342
352	343	@Override

...	...	@@ -4,6 +4,7 @@
4	4	import models.enumeration.RequestState;
5	5	import models.enumeration.ResourceType;
6	6	import models.enumeration.State;
	7	+import models.resource.GlobalResource;
7	8	import models.resource.Resource;
8	9	import org.apache.commons.lang3.StringUtils;
9	10	import org.joda.time.DateTime;
...	...	@@ -186,7 +187,11 @@
186	187	default:
187	188	Resource resource = getResource();
188	189	if (resource != null) {
189		- return resource.getProject();
	190	+ if (resource instanceof GlobalResource) {
	191	+ return null;
	192	+ } else {
	193	+ return resource.getProject();
	194	+ }
190	195	} else {
191	196	return null;
192	197	}

...	...	@@ -4,6 +4,7 @@
4	4	import models.enumeration.Operation;
5	5	import models.enumeration.ResourceType;
6	6
	7	+import models.resource.GlobalResource;
7	8	import models.resource.Resource;
8	9
9	10	public class AccessControl {
...	...	@@ -84,7 +85,8 @@
84	85	* @param operation
85	86	* @return
86	87	*/
87		- private static boolean isGlobalResourceAllowed(User user, Resource resource, Operation operation) {
	88	+ private static boolean isGlobalResourceAllowed(User user, GlobalResource resource,
	89	+ Operation operation) {
88	90	// Temporary attachments are allowed only for the user who uploads them.
89	91	if (resource.getType() == ResourceType.ATTACHMENT
90	92	&& resource.getContainer().getType() == ResourceType.USER) {
...	...	@@ -195,16 +197,21 @@
195	197	* @return {@code user}가 {@code resource}에 {@code operation}을
196	198	* 하는 것이 허용되는지의 여부
197	199	*/
198		- public static boolean isAllowed(User user, Resource resource, Operation operation) {
	200	+ public static boolean isAllowed(User user, Resource resource, Operation operation)
	201	+ throws IllegalStateException {
199	202	if (user.isSiteManager()) {
200	203	return true;
201	204	}
202	205
203		- Project project = resource.getProject();
204		-
205		- if (project == null) {
206		- return isGlobalResourceAllowed(user, resource, operation);
	206	+ if (resource instanceof GlobalResource) {
	207	+ return isGlobalResourceAllowed(user, (GlobalResource) resource, operation);
207	208	} else {
	209	+ Project project = resource.getProject();
	210	+
	211	+ if (project == null) {
	212	+ throw new IllegalStateException("A project resource lost its project");
	213	+ }
	214	+
208	215	return isProjectResourceAllowed(user, project, resource, operation);
209	216	}
210	217	}

...	...	@@ -5,12 +5,14 @@
5	5	import org.junit.After;
6	6	import org.junit.Before;
7	7	import org.junit.Test;
	8	+import org.junit.Assert;
8	9
9	10	import play.test.Helpers;
10	11
11	12	import static org.fest.assertions.Assertions.assertThat;
12	13
13	14	import models.enumeration.Operation;
	15	+import models.enumeration.State;
14	16
15	17	public class AccessControlTest extends ModelTest<Role>{
16	18	@Before
...	...	@@ -74,4 +76,30 @@
74	76	assertThat(canUpdate).isEqualTo(false);
75	77	assertThat(canDelete).isEqualTo(false);
76	78	}
	79	+
	80	+ // AccessControl.isAllowed throws IllegalStateException if the resource
	81	+ // belongs to a project but the project is missing.
	82	+ @Test
	83	+ public void isAllowed_lostProject() {
	84	+ // Given
	85	+ User author = User.findByLoginId("nori");
	86	+ Project projectYobi = Project.findByOwnerAndProjectName("yobi", "projectYobi");
	87	+ Issue issue = new Issue();
	88	+ issue.setProject(projectYobi);
	89	+ issue.setTitle("hello");
	90	+ issue.setBody("world");
	91	+ issue.setAuthor(author);
	92	+ issue.state = State.OPEN;
	93	+ issue.save();
	94	+
	95	+ // When
	96	+ issue.project = null;
	97	+
	98	+ // Then
	99	+ try {
	100	+ AccessControl.isAllowed(author, issue.asResource(), Operation.READ);
	101	+ Assert.fail();
	102	+ } catch (IllegalStateException e) {
	103	+ }
	104	+ }
77	105	}

Delete comment