Didn't look much at the tests.
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: some docs for each? Esp. OAUTH vs. OAUTH_CONSUMER_REQUEST http://codereview.appspot.com/23041/diff/1/11#newcode29 Line 29: COOKIE; should have static method for String -> AuthenticationMode? 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(); shouldn't this be passed into the constructor, not assumed? 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} @see AuthenticationMode - no {} http://codereview.appspot.com/23041/diff/1/23 File java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java (right): http://codereview.appspot.com/23041/diff/1/23#newcode96 Line 96: fillInStackTrace() is automatically called by the Throwable constructor - you don't need to call it manually. 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; can all be final http://codereview.appspot.com/23041/diff/1/14#newcode57 Line 57: httpCache.removeResponse(new HttpRequest(uri)); 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? http://codereview.appspot.com/23041/diff/1/14#newcode62 Line 62: Long mark = marker.incrementAndGet(); this strategy could use some local explanation. It's non-obvious. http://codereview.appspot.com/23041/diff/1/14#newcode69 Line 69: if (request.getOAuthArguments() == null) { should just be request.getAuthType() == AuthType.NONE http://codereview.appspot.com/23041/diff/1/14#newcode81 Line 81: if (request.getSecurityToken() == null || request.getOAuthArguments() == null) { ditto http://codereview.appspot.com/23041/diff/1/14#newcode101 Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" + userId; 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) http://codereview.appspot.com/23041/diff/1/14#newcode101 Line 101: return "INVALIDATION_TOKEN:" + token.getAppId() + ":" + userId; invalidationhandler code allows both appUrl and appId. This requires appId; should be consistent. 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 entried -> entries http://codereview.appspot.com/23041/diff/1/21#newcode61 Line 61: invalidationService.isValid(request, cachedResponse)) { 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. 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 dont -> don't 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; final http://codereview.appspot.com/23041/diff/1/9#newcode54 Line 54: @Operation(httpMethods = {"POST","GET"}, path = "/invalidate") why POST and GET? Spec only mentions POST. http://codereview.appspot.com/23041/diff/1/9#newcode60 Line 60: "Cannot invalidate content wihtout specifying application"); wihtout -> without http://codereview.appspot.com/23041/diff/1/9#newcode63 Line 63: // Is the invalidation call from the application backend. If not we dont allow dont -> don't http://codereview.appspot.com/23041/diff/1/9#newcode66 Line 66: AuthenticationMode.OAUTH_CONSUMER_REQUEST.name()); might as well flip the .equals() order to prevent NPEs http://codereview.appspot.com/23041/diff/1/9#newcode69 Line 69: Set<String> userIds = Sets.newLinkedHashSet(); why linked hash sets? More expensive, and don't see order as relevant here. http://codereview.appspot.com/23041/diff/1/9#newcode101 Line 101: if (!resources.isEmpty()) { check of isEmpty() not really needed 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 missing variable name http://codereview.appspot.com/23041
