Add security token to the iframe if it's defined in the context

[Paul Lindner, 2010-05-21 20:12:13]

here's a first step towards allowing security tokens to be added to the iframe
factory.

[johnfargo, 2010-05-21 20:28:44]

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.

[johnfargo, 2010-05-22 02:40:03]

LGTM beyond this comment.

http://codereview.appspot.com/1240043/diff/9001/10001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java
(right):

http://codereview.appspot.com/1240043/diff/9001/10001#newcode172
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:172:
protected String generateSecurityToken(Gadget gadget) {
didn't enjoy the wantsSecurityToken(gadget) idea?

[Paul Lindner, 2010-05-22 14:00:39]

http://codereview.appspot.com/1240043/diff/9001/10001
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java
(right):

http://codereview.appspot.com/1240043/diff/9001/10001#newcode172
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:172:
protected String generateSecurityToken(Gadget gadget) {
On 2010/05/22 02:40:03, johnfargo wrote:
> didn't enjoy the wantsSecurityToken(gadget) idea?

Sure I'll modify the patch to do that; however it seems like you could just
return null here to signal the same thing..

[fargo, 2010-05-24 18:24:05]

True, it's really just a relatively minor structuring nit.

--j

On Sat, May 22, 2010 at 7:00 AM, <lindner@inuus.com> wrote:

>
> http://codereview.appspot.com/1240043/diff/9001/10001
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java
> (right):
>
> http://codereview.appspot.com/1240043/diff/9001/10001#newcode172
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/DefaultIframeUriManager.java:172:
> protected String generateSecurityToken(Gadget gadget) {
> On 2010/05/22 02:40:03, johnfargo wrote:
>
>> didn't enjoy the wantsSecurityToken(gadget) idea?
>>
>
> Sure I'll modify the patch to do that; however it seems like you could
> just return null here to signal the same thing..
>
>
> http://codereview.appspot.com/1240043/show
>

[johnfargo, 2010-05-25 16:27:06]

lgtm