Interested in your thoughts.
http://codereview.appspot.com/1240043/diff/1/2 File java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java (right): http://codereview.appspot.com/1240043/diff/1/2#newcode59 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:59: private final SecurityTokenDecoder securityTokenDecoder; SecurityTokenCodec does seem like the better name now... http://codereview.appspot.com/1240043/diff/1/2#newcode71 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:71: // TODO this doesn't scale to large numbers of containers sure it does, if you dedupe and don't require a unique subdomain per container ;) http://codereview.appspot.com/1240043/diff/1/2#newcode154 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:154: // Inject a security token if set in the context A. Let's put this in a separate method: injectSecurityToken(...) B. Let's also wrap the logic in an if-block: if (wantsSecurityToken(gadget)) { ... } ...where wantsSecurityToken is defined by default as "return true" This allows, for those who choose to opt into it, the ability to append a security token only when the gadget affirmatively asks for one - notably, when gadget.getAllFeatures().contains("security-token"); This accommodates all cases except signed requests by gadgets that have no <Requires> that transitively depend on "security-token". http://codereview.appspot.com/1240043/diff/1/2#newcode159 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:159: uri.addQueryParameter("st", tokenVal); s/"st"/UriCommon.Param.SECURITY_TOKEN.getKey()/ http://codereview.appspot.com/1240043/diff/1/2#newcode163 java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:163: // ignore -- no security token really? here, getToken() != null so presumably we should succeed in returning a token. I recognize this is as much an audit of the SecurityTokenDecoder interface, but IMO we should throw an Exception in exceptional circumstances; perhaps null in "normal" ones, however defined. http://codereview.appspot.com/1240043/show
