portals-jetspeed-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Jencks (JIRA)" <jetspeed-...@portals.apache.org>
Subject [jira] Commented: (JS2-473) Many uses of Fragment.getFragments() assume access to the underlying list, not a copy: this is invalid
Date Fri, 20 Jan 2006 08:54:43 GMT
    [ http://issues.apache.org/jira/browse/JS2-473?page=comments#action_12363377 ] 

David Jencks commented on JS2-473:
----------------------------------

On further thought, all or almost all of these problems can be solved by adding add and remove
methods to Fragment that add and remove to the private fragment list, before security filtering.

> Many uses of Fragment.getFragments() assume access to the underlying list, not a copy:
this is invalid
> ------------------------------------------------------------------------------------------------------
>
>          Key: JS2-473
>          URL: http://issues.apache.org/jira/browse/JS2-473
>      Project: Jetspeed 2
>         Type: Bug
>     Versions: 2.1-dev
>     Reporter: David Jencks

>
> While trying to see if my proposed solution to JS2-472 would break anything, I noticed
that many uses of Fragment.getFragments() assume they are getting the original mutable list,
and proceed to modify it.  Since they may get a copy, any modifications would be discarded.
  Here's a usage search from intellij with some annotations: I've marked obviously dangerous
uses with >>>>.  There is no reason to suppose the other uses are safe without
looking into them further.  I don't think I'm qualified to fix this without at least some
guidance.
> Method
>     getFragments():List of interface org.apache.jetspeed.om.page.Fragment
> Found usages  (70 usages)
>     Unclassified usage  (70 usages)
>         jetspeed-layouts  (5 usages)
>             org.apache.jetspeed.portlets.layout  (5 usages)
>                 LayoutPortlet  (2 usages)
>                     addPortletToPage(String, String)  (1 usage)
> >>>>                        (279, 18) root.getFragments().add(fragment);
>                     removeFragment(String, String)  (1 usage)
> >>>>                        (257, 18) root.getFragments().remove(f);
>                 MultiColumnPortlet  (2 usages)
>                     doView(RenderRequest, RenderResponse)  (1 usage)
>                         (94, 65) layout = new ColumnLayout(numColumns, layoutType, f.getFragments(),
this.colSizes.split("\\,") );
>                     processAction(ActionRequest, ActionResponse)  (1 usage)
>                         (242, 80) layout = new ColumnLayout(numColumns, layoutType, rootFragment.getFragments(),
this.colSizes.split("\\,") );
>                 PageManagerLayoutEventListener  (1 usage)
>                     handleEvent(LayoutEvent)  (1 usage)
> >>>>                        (28, 40) page.getRootFragment().getFragments().add(event.getFragment());
>         jetspeed-page-manager  (60 usages)
>             org.apache.jetspeed.om.page  (15 usages)
>                 ContentFragmentImpl.ContentFragmentList  (1 usage)
>                     (441, 42) private List baseList = fragment.getFragments();
>                 TestPageObjectModel  (14 usages)
>                     testFragmentManipulation()  (14 usages)
>                         (108, 28) assertNotNull(root.getFragments());
> >>>>                        (114, 14) root.getFragments().add(frag1);
> >>>>                        (128, 15) frag2.getFragments().add(frag3);
> >>>>                        (129, 14) root.getFragments().add(frag2);
>                         (132, 25) assertTrue(root.getFragments().size()==2);
>                         (133, 27) Iterator i = root.getFragments().iterator();
>                         (142, 22) assertTrue(f.getFragments().size()==0);
>                         (147, 22) assertTrue(f.getFragments().size()==1);
>                         (149, 22) assertTrue(f.getFragments().size()==1);
>                         (150, 15) i = f.getFragments().iterator();
> >>>>                        (164, 11) f.getFragments().remove(frag3);
> >>>>                        (167, 11) f.getFragments().add(frag2);
>                         (168, 22) assertTrue(f.getFragments().size()==1);
>                         (169, 29) f = (FragmentImpl)f.getFragments().get(0);
>             org.apache.jetspeed.om.page.psml  (4 usages)
>                 PageImpl  (4 usages)
>                     getFragmentById(String)  (1 usage)
>                         (177, 28) Iterator i = f.getFragments().iterator();
>                     getFragmentsByName(String)  (1 usage)
>                         (276, 28) Iterator i = f.getFragments().iterator();
>                     removeFragmentById(String)  (2 usages)
>                         (209, 28) Iterator i = f.getFragments().iterator();
> >>>>                        (234, 28) if (parent.getFragments().remove(f))
>             org.apache.jetspeed.page  (41 usages)
>                 AbstractPageManager  (2 usages)
>                     copyFragment(Fragment, String)  (2 usages)
>                         (889, 37) Iterator fragments = source.getFragments().iterator();
> >>>>                        (894, 18) copy.getFragments().add(copiedFragment);
>                 PageManagerTestShared.Shared  (15 usages)
>                     testSecurePageManager(TestCase, PageManager)  (15 usages)
> >>>>                        (281, 34) root.getFragments().add(portlet);
> >>>>                        (287, 34) root.getFragments().add(portlet);
>                         (290, 71) test.assertNotNull(page.getRootFragment().getFragments());
>                         (291, 73) test.assertEquals(2, page.getRootFragment().getFragments().size());
>                         (292, 106) test.assertEquals("some-app::SomePortlet", ((Fragment)page.getRootFragment().getFragments().get(1)).getName());
>                         (293, 91) test.assertFalse("0".equals(((Fragment)page.getRootFragment().getFragments().get(1)).getId()));
>                         (294, 82) somePortletId[0] = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>                         (348, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (349, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (389, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (390, 74) test.assertEquals(2, page0.getRootFragment().getFragments().size());
>                         (458, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (459, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                         (518, 72) test.assertNotNull(page0.getRootFragment().getFragments());
>                         (519, 74) test.assertEquals(1, page0.getRootFragment().getFragments().size());
>                 TestCastorXmlPageManager  (15 usages)
>                     testClonePage()  (4 usages)
>                         (861, 30) List children = root.getFragments();
>                         (862, 40) List cloneChildren = cloneRoot.getFragments();
>                         (905, 26) assertNotNull(cf.getFragments());
>                         (906, 23) assertTrue(cf.getFragments().size() == 2);
>                     testCreatePage()  (3 usages)
> >>>>                        (266, 14) root.getFragments().add(f);
>                         (302, 43) assertTrue(page.getRootFragment().getFragments().size()
== 1);
>                         (304, 47) f = (Fragment) page.getRootFragment().getFragments().get(0);
>                     testGetPage()  (3 usages)
>                         (190, 30) List children = root.getFragments();
>                         (237, 25) assertNotNull(f.getFragments());
>                         (238, 22) assertTrue(f.getFragments().size() == 2);
>                     testUpdatePage()  (5 usages)
>                         (373, 28) assertNotNull(root.getFragments());
>                         (374, 30) assertEquals(1, root.getFragments().size());
>                         (375, 41) String testId = ((Fragment)root.getFragments().get(0)).getId();
>                         (394, 28) assertNotNull(root.getFragments());
>                         (395, 25) assertTrue(root.getFragments().isEmpty());
>                 TestDatabasePageManager  (9 usages)
>                     testCreates()  (2 usages)
> >>>>                        (317, 14) root.getFragments().add(portlet);
> >>>>                        (328, 14) root.getFragments().add(portlet);
>                     testGets()  (4 usages)
>                         (620, 51) assertNotNull(check.getRootFragment().getFragments());
>                         (621, 53) assertEquals(2, check.getRootFragment().getFragments().size());
>                         (622, 65) Fragment check0 = (Fragment)check.getRootFragment().getFragments().get(0);
>                         (643, 65) Fragment check1 = (Fragment)check.getRootFragment().getFragments().get(1);
>                     testUpdates()  (3 usages)
>                         (814, 46) assertNotNull(page.getRootFragment().getFragments());
>                         (815, 48) assertEquals(2, page.getRootFragment().getFragments().size());
>                         (816, 61) String removeId = ((Fragment)page.getRootFragment().getFragments().get(1)).getId();
>         jetspeed-portal  (4 usages)
>             org.apache.jetspeed.aggregator.impl  (1 usage)
>                 BasicAggregator  (1 usage)
>                     build(RequestContext)  (1 usage)
>                         (97, 34) for (Iterator fit = root.getFragments().iterator();
fit.hasNext();)
>             org.apache.jetspeed.decoration  (1 usage)
>                 PageTheme  (1 usage)
>                     PageTheme(Page, DecorationFactory, RequestContext)  (1 usage)
>                         (57, 43) Iterator fragments = rootFragment.getFragments().iterator();
>             org.apache.jetspeed.layout.impl  (2 usages)
>                 AddPortletAction  (1 usage)
>                     run(RequestContext, Map)  (1 usage)
> >>>>                        (126, 18) root.getFragments().add(fragment);
           
>                 PortletPlacementContextImpl  (1 usage)
>                     processFragment(Fragment)  (1 usage)
>                         (145, 29) List children = fragment.getFragments();
>         jetspeed-registry  (1 usage)
>             org.apache.jetspeed.components.portletentity  (1 usage)
>                 TestPortletEntityDAO.ContentFragmentImpl  (1 usage)
>                     getFragments()  (1 usage)
>                         (145, 22) return f.getFragments();

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


---------------------------------------------------------------------
To unsubscribe, e-mail: jetspeed-dev-unsubscribe@portals.apache.org
For additional commands, e-mail: jetspeed-dev-help@portals.apache.org


Mime
View raw message