[
https://issues.apache.org/jira/browse/SHINDIG-1601?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13113708#comment-13113708
]
[email protected] commented on SHINDIG-1601:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1666/#review2051
-----------------------------------------------------------
Feedback inline.
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js
<https://reviews.apache.org/r/1666/#comment4614>
Can we take this a step further and display an error in an "else" to this
"if"?
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js
<https://reviews.apache.org/r/1666/#comment4615>
Display an error in an "else"
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
<https://reviews.apache.org/r/1666/#comment4616>
What happens if no "container" config is provided? I believe
gadgets.config.get will return an empty object, but it would be worth verifying
so we don't blow up here.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4618>
Probably want to throw this. :)
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4619>
Throw this guy
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4620>
Is it feasible to think that in the future other features will always be
allowed? For instance, is RPC always allowed in practice?
Maybe this should be extensible if that seems likely.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4621>
These overrides are going to blow up in a Java5 build because you're
implementing an interface.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4622>
Can context ever be null?
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4623>
Can spec or url ever be null?
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4625>
Style only: I'd rather see if(!isFeatureAdminEnabled(container)) { return
true
}
It removes one level of indentation and makes the code easier to read.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4626>
This Set logic gets complicated when trying to think of all of the use
cases. I hope there are Unit tests around all of this.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4624>
Just return the result of the areAllFeaturesAllowed() call
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4627>
Context can be null?
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4628>
Spec and url can be null?
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
<https://reviews.apache.org/r/1666/#comment4629>
Style only: I'd like to see the return true case happen first. It removes
a level of indentation and makes this easier to read.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/ContainerAdminData.java
<https://reviews.apache.org/r/1666/#comment4630>
Picking nits: You could just call this(null)
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/ContainerAdminData.java
<https://reviews.apache.org/r/1666/#comment4631>
In keeping with the Map APIs, could you return the result of remove()?
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/FeatureAdminData.java
<https://reviews.apache.org/r/1666/#comment4632>
Picking nits: You could just call this(null, null, null)
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/FeatureAdminData.java
<https://reviews.apache.org/r/1666/#comment4633>
I don't think the contains call is necessary here. By definition a Set
doesn't allow duplicates.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/FeatureAdminData.java
<https://reviews.apache.org/r/1666/#comment4634>
Style only: qualify with "this"
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/FeatureAdminData.java
<https://reviews.apache.org/r/1666/#comment4635>
Style only: qualify with "this"
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/FeatureAdminData.java
<https://reviews.apache.org/r/1666/#comment4636>
What are you trying to accomplish here? What if the set has a list of
features and a "*" in it? This would return false. Shouldn't it be true in
that case?
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/ServerAdminData.java
<https://reviews.apache.org/r/1666/#comment4637>
Picking nits: You could call this(null)
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/admin/FeatureAdminDataTest.java
<https://reviews.apache.org/r/1666/#comment4638>
Should probably be nullData and defaultData in two of these cases.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
<https://reviews.apache.org/r/1666/#comment4639>
Was this annotation just dangling before or did you accidentally remove it
from testGetMetadata()?
- Stanton
On 2011-09-23 14:29:37, Ryan Baxter wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1666/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-09-23 14:29:37)
bq.
bq.
bq. Review request for shindig, Paul Lindner, Henry Saputra, johnfargo, Dan
Dumont, and Stanton Sievers.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. We want to add some administration features to Shindig. This patch
adds/changes 3 things in Shindig.
bq.
bq. 1. Remove the existing blacklist functionality in Shindig which currently
is enabled by pointing Shindig to a text file with a list of gadgets to
blacklist. The new functionality uses a whitelist instead of a blacklist and
is indexed on a per container basis. Meaning an admin could whitelist some
gadgets in one container but not another. This functionality is enabled
through a Guice module allowing implementors to use something else besides a
text file.
bq.
bq. 2. Add the ability for administrators to specify which features are
allowed and denied for a specific gadget in a specific container. This
information is checked in two places, when the metadata request is made and
when the gadget is rendered.
bq.
bq. 3. Add the ability for containers to secure RPC requests made by gadgets.
The RPC code now has the ability to arbitrate all RPC calls made. Containers
can specify their own arbitrator. The common container has its own default
arbitrator available.
bq.
bq.
bq. This addresses bug SHINDIG-1601.
bq. https://issues.apache.org/jira/browse/SHINDIG-1601
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/container.js
1174132
bq. http://svn.apache.org/repos/asf/shindig/trunk/config/gadget-admin.json
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/assembler.js
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/content/samplecontainer/examples/commoncontainer/viewController.js
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container.util/util.js
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/container/container.js
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/features/src/main/javascript/features/rpc/rpc.js
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/conf/shindig.properties
1174132
bq. http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/pom.xml
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/BasicGadgetBlacklist.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultGuiceModule.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetBlacklist.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetException.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/RenderingContext.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStore.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/ContainerAdminData.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/FeatureAdminData.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/GadgetAdminData.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/GadgetAdminModule.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/GadgetAdminStore.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/admin/ServerAdminData.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/config/CoreUtilConfigContributor.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/process/Processor.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/MakeRequestHandler.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/BasicGadgetBlacklistTest.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/admin/BasicGadgetAdminStoreTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/admin/ContainerAdminDataTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/admin/FeatureAdminDataTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/admin/GadgetAdminDataTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/admin/ServerAdminDataTest.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/process/ProcessorTest.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeProcessor.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestHandlerTest.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/MakeRequestServletTest.java
1174132
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
1174132
bq.
bq. Diff: https://reviews.apache.org/r/1666/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Created/updated unit tests.
bq.
bq.
bq. Thanks,
bq.
bq. Ryan
bq.
bq.
> Enhance Gadget Administration
> -----------------------------
>
> Key: SHINDIG-1601
> URL: https://issues.apache.org/jira/browse/SHINDIG-1601
> Project: Shindig
> Issue Type: New Feature
> Affects Versions: 3.0.0
> Reporter: Ryan Baxter
> Attachments: issue-1601.patch
>
> Original Estimate: 672h
> Remaining Estimate: 672h
>
> We want to add some administration features to Shindig. This patch
> adds/changes 3 things in Shindig.
> 1. Remove the existing blacklist functionality in Shindig which currently is
> enabled by pointing Shindig to a text file with a list of gadgets to
> blacklist. The new functionality uses a whitelist instead of a blacklist and
> is indexed on a per container basis. Meaning an admin could whitelist some
> gadgets in one container but not another. This functionality is enabled
> through a Guice module allowing implementors to use something else besides a
> text file.
> 2. Add the ability for administrators to specify which features are allowed
> and denied for a specific gadget in a specific container. This information
> is checked in two places, when the metadata request is made and when the
> gadget is rendered.
> 3. Add the ability for containers to secure RPC requests made by gadgets.
> The RPC code now has the ability to arbitrate all RPC calls made. Containers
> can specify their own arbitrator. The common container has its own default
> arbitrator available.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
