Hi Jesse I have uploaded a new patch to address the suggestions.
Here are the list of changes: Removed '-DISABLED' from title as suggested Added Dutch language to messages_nl.properties file (using Google translate) Created new JIRA ticket to address the broader Javascript security issues: https://issues.apache.org/jira/browse/RAVE-385 -Venkat -----Original Message----- From: Jesse Ciancetta [mailto:[email protected]] Sent: Friday, December 09, 2011 2:09 PM To: Mahadevan, Venkat; Ciancetta, Jesse E.; rave Subject: Re: Review Request: RAVE-210 Administrators should be able to disable widgets > On 2011-12-09 15:36:47, Jesse Ciancetta wrote: > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/admin/widgetdetail.jsp, > > line 108 > > <https://reviews.apache.org/r/3078/diff/2/?file=63362#file63362line108> > > > > I think we need to be careful about scripting attacks here -- if > > administrators are the only ones who can ever disable a widget and set the > > message its probably fine, but if end users can ever do this we need to > > watch out for the potential for them to add malicious script to the message. > > > > I'm not sure if this is an issue currently or not (I'm hoping others > > more familiar with this part of the codebase can say one way or another), > > but if it is I wouldn't hold up the patch to fix it -- I'd just make sure a > > JIRA ticket is created to address it. > > Venkat Mahadevan wrote: > Jesse, good point. There is an issue with the existing 'add new widget' > page as well where the end users are allowed to create widgets because > potentially all the fields like title, description etc can be compromised. I > think it is a good idea to have a new JIRA ticket created to address such > issues. Ok -- creating a JIRA ticket to follow up on this stuff sounds good to me. > On 2011-12-09 15:36:47, Jesse Ciancetta wrote: > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/script/rave.js, > > line 652 > > <https://reviews.apache.org/r/3078/diff/2/?file=63363#file63363line652> > > > > If we append something to the title then I think we need to > > internationalize it. > > > > I think I'd prefer to leave the title alone though and just rely on the > > message that the administrator can set to convey to the user that the > > widget is disabled. > > > > So I guess I'd either pull this out or find some way to > > internationalize the appended DISABLED string. > > > > My vote is to pull it out. > > Venkat Mahadevan wrote: > Initially I thought of putting an image or a * at the end of the title > indicating the disabled state. Maybe we can follow this approach. I think an image of some kind would be fine. Although I'm still not convinced we need to change the title at all -- I think the message that shows in the space where the gadget normally appears is probably sufficient. I'd be fine with an image though too -- I don't feel too strongly about it one way or the other. - Jesse ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3078/#review3782 ----------------------------------------------------------- On 2011-12-08 21:10:12, Venkat Mahadevan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3078/ > ----------------------------------------------------------- > > (Updated 2011-12-08 21:10:12) > > > Review request for rave. > > > Summary > ------- > > RAVE-210 Administrators should be able to disable widgets > > To disable a gadget, login as Admin and go to "Admin Interface" and to the > Widgets tab and click on a widget to get its metadata. Here you will find two > new columns, "Disable Gadget" and "Disable Gadget Message". One can disable > the gadget from rendering if you check he disable gadget checkbox and > whatever you put in the message will be displayed on the gadget when the > gadget is rendered. If you want to enable it, uncheck the checkbox. The > gadget will be displayed as usual (even if the disable message is still there > in the metadata, since the checkbox overrides it). > > > Diffs > ----- > > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/WEB-INF/jsp/views/admin/widgetdetail.jsp > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/resources/messages.properties > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/tag/RegionWidgetTag.java > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-core/src/main/java/org/apache/rave/portal/model/Widget.java > 1211932 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-components/rave-web/src/main/java/org/apache/rave/portal/web/tag/AbstractContextAwareSingletonBeanDependentTag.java > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/main/webapp/script/rave.js > 1211897 > > https://svn.apache.org/repos/asf/incubator/rave/trunk/rave-portal-resources/src/test/javascript/raveSpec.js > 1211897 > > Diff: https://reviews.apache.org/r/3078/diff > > > Testing > ------- > > > Thanks, > > Venkat > >
