http://codereview.appspot.com/23041/diff/1/11
File
java/common/src/main/java/org/apache/shindig/auth/AuthenticationMode.java
(right):
http://codereview.appspot.com/23041/diff/1/11#newcode24
Line 24:
On 2009/02/27 22:40:51, awiner wrote:
some docs for each? Esp. OAUTH vs. OAUTH_CONSUMER_REQUEST
Done.
http://codereview.appspot.com/23041/diff/1/11#newcode29
Line 29: COOKIE;
On 2009/02/27 22:40:51, awiner wrote:
should have static method for String -> AuthenticationMode?
valueOf works fine here
http://codereview.appspot.com/23041/diff/1/8
File
java/common/src/main/java/org/apache/shindig/auth/BasicSecurityToken.java
(right):
http://codereview.appspot.com/23041/diff/1/8#newcode145
Line 145: return AuthenticationMode.SECURITY_TOKEN_URL_PARAMETER.name();
On 2009/02/27 22:40:51, awiner wrote:
shouldn't this be passed into the constructor, not assumed?
This is correct, this class is only used for gadget render tokens. The
name is somewhat misleading though, we use different token
implementations for other cases.
http://codereview.appspot.com/23041/diff/1/7
File
java/common/src/main/java/org/apache/shindig/auth/SecurityToken.java
(right):
http://codereview.appspot.com/23041/diff/1/7#newcode70
Line 70: * @see{AuthenticationMode}
On 2009/02/27 22:40:51, awiner wrote:
@see AuthenticationMode - no {}
Done.
http://codereview.appspot.com/23041/diff/1/14
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultInvalidationService.java
(right):
http://codereview.appspot.com/23041/diff/1/14#newcode40
Line 40: private HttpCache httpCache;
On 2009/02/27 22:40:51, awiner wrote:
can all be final
Done.
http://codereview.appspot.com/23041/diff/1/14#newcode57
Line 57: httpCache.removeResponse(new HttpRequest(uri));
On 2009/02/27 22:40:51, awiner wrote:
This will only remove the unsigned requests for those URIs. Don't we
need to
remove signed and unsigned? Or is this a spec clarification - signed
requests
can only be invalidated by invalidating the whole user, unsigned
requests only
by invalidating the URL?
Yup, it needs to be clearer in the spec. Fetches of gadget specs,
message bundles etc are all unsigned.
http://codereview.appspot.com/23041/diff/1/14#newcode62
Line 62: Long mark = marker.incrementAndGet();
On 2009/02/27 22:40:51, awiner wrote:
this strategy could use some local explanation. It's non-obvious.
Added a long discussion to class
http://codereview.appspot.com/23041/diff/1/14#newcode69
Line 69: if (request.getOAuthArguments() == null) {
On 2009/02/27 22:40:51, awiner wrote:
should just be request.getAuthType() == AuthType.NONE
Done.
http://codereview.appspot.com/23041/diff/1/14#newcode81
Line 81: if (request.getSecurityToken() == null ||
request.getOAuthArguments() == null) {
On 2009/02/27 22:40:51, awiner wrote:
ditto
Done.
http://codereview.appspot.com/23041/diff/1/14#newcode101
Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" +
userId;
On 2009/02/27 22:40:51, awiner wrote:
what purpose does the long prefix serve? It's going over the net a
lot, so
would rather see it kept small (if it's needed at all)
It doesnt go over the net for this implementation. Other implementations
should implement appropriately. Aim here is to be as readable as
possible. This is fine for in-mem and is fairly consistent with what we
do elsewhere (see Http keys)
http://codereview.appspot.com/23041/diff/1/14#newcode101
Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" +
userId;
On 2009/02/27 22:40:51, awiner wrote:
invalidationhandler code allows both appUrl and appId. This requires
appId;
should be consistent.
Done.
http://codereview.appspot.com/23041/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/DefaultRequestPipeline.java
(right):
http://codereview.appspot.com/23041/diff/1/21#newcode58
Line 58: // Note that we dont remove invalidated entried from the cache
as we want them to be
On 2009/02/27 22:40:51, awiner wrote:
entried -> entries
Done.
http://codereview.appspot.com/23041/diff/1/21#newcode61
Line 61: invalidationService.isValid(request, cachedResponse)) {
On 2009/02/27 22:40:51, awiner wrote:
would rewrite this a bit:
if (cachedResponse != null) {
if (cachedResponse.isStale()) {
cachedResponse = null;
} else if (invalidationService.isValid(request, cachedResponse)) {
return cachedResponse;
}
}
Then remove the check of isStale() below. This clarifies that stale
responses
just never get used.
Done.
http://codereview.appspot.com/23041/diff/1/21#newcode80
Line 80: // Use the invalidated cache response if it is not stale. We
dont update its
On 2009/02/27 22:40:51, awiner wrote:
dont -> don't
Done.
http://codereview.appspot.com/23041/diff/1/9
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationHandler.java
(right):
http://codereview.appspot.com/23041/diff/1/9#newcode46
Line 46: private InvalidationService invalidation;
On 2009/02/27 22:40:51, awiner wrote:
final
Done.
http://codereview.appspot.com/23041/diff/1/9#newcode54
Line 54: @Operation(httpMethods = {"POST","GET"}, path = "/invalidate")
On 2009/02/27 22:40:51, awiner wrote:
why POST and GET? Spec only mentions POST.
Allows for convenience flushing for the viewerid by hitting a URL in the
browser with the appropriate security token. I can remove later if it
gets confusing but seemed useful
http://codereview.appspot.com/23041/diff/1/9#newcode60
Line 60: "Cannot invalidate content wihtout specifying application");
On 2009/02/27 22:40:51, awiner wrote:
wihtout -> without
Done.
http://codereview.appspot.com/23041/diff/1/9#newcode63
Line 63: // Is the invalidation call from the application backend. If
not we dont allow
On 2009/02/27 22:40:51, awiner wrote:
dont -> don't
Done.
http://codereview.appspot.com/23041/diff/1/9#newcode66
Line 66: AuthenticationMode.OAUTH_CONSUMER_REQUEST.name());
On 2009/02/27 22:40:51, awiner wrote:
might as well flip the .equals() order to prevent NPEs
Done.
http://codereview.appspot.com/23041/diff/1/9#newcode69
Line 69: Set<String> userIds = Sets.newLinkedHashSet();
On 2009/02/27 22:40:51, awiner wrote:
why linked hash sets? More expensive, and don't see order as relevant
here.
For service implementations that have partial failures they may wan to
report last successful flush. On reflection I think thats
over-engineered. Switching back to Set
http://codereview.appspot.com/23041/diff/1/9#newcode101
Line 101: if (!resources.isEmpty()) {
On 2009/02/27 22:40:51, awiner wrote:
check of isEmpty() not really needed
Agreed. I should get out of the business of protecting service
implementors.
http://codereview.appspot.com/23041/diff/1/6
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/InvalidationService.java
(right):
http://codereview.appspot.com/23041/diff/1/6#newcode50
Line 50: * @param
On 2009/02/27 22:40:51, awiner wrote:
missing variable name
Done.
http://codereview.appspot.com/23041
