> On Aug. 16, 2012, 8:12 p.m., Dan Dumont wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java, > > line 152 > > <https://reviews.apache.org/r/6436/diff/2/?file=138689#file138689line152> > > > > Could you please overload this method so that you don't need to specify > > junk params?
Done. > On Aug. 16, 2012, 8:12 p.m., Dan Dumont wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java, > > line 1063 > > <https://reviews.apache.org/r/6436/diff/2/?file=138689#file138689line1063> > > > > Can you use Joiner.on(':') here? I prefer not to use the Google Joiner. > On Aug. 16, 2012, 8:12 p.m., Dan Dumont wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Store.java, > > line 333 > > <https://reviews.apache.org/r/6436/diff/2/?file=138690#file138690line333> > > > > Can you put a line here documenting what this is doing? I'm not sure > > what a persister is and why it's different than a cache, or why we need to > > clean up both and one doesn't do the other. I think some of your confusion is over what the BasicOAuth2Store is intended for. It's purpose is to manage a persistence setup with a cache and another "hard" persistence layer. So asking the OAuth2Store to remove something is telling the BasicOAuth2Store to remove it from the cache and the persistence. I added some fluff comments at that particular line. > On Aug. 16, 2012, 8:12 p.m., Dan Dumont wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/OAuth2HandlerError.java, > > line 91 > > <https://reviews.apache.org/r/6436/diff/2/?file=138700#file138700line91> > > > > Please use Joiner.on(':') I prefer not to use Google Joiner. > On Aug. 16, 2012, 8:12 p.m., Dan Dumont wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/OAuth2CallbackServlet.java, > > line 129 > > <https://reviews.apache.org/r/6436/diff/2/?file=138705#file138705line129> > > > > Can you please collapse this code and use a ternary operator to pass in > > your message if accessor.isValid() or not? Done. - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6436/#review10425 ----------------------------------------------------------- On Aug. 24, 2012, 1:04 p.m., Adam Clarke wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6436/ > ----------------------------------------------------------- > > (Updated Aug. 24, 2012, 1:04 p.m.) > > > Review request for shindig, Stanton Sievers, Brian Lillie, and Marshall Shi. > > > Description > ------- > > Latest (and probably last) iteration of OAuth2 Consumer fixes rolled up into > a single patch. > > Applied formatting and Checkstyle fixes to all changed files. > MapCache inconsistent null checks for storing Collections. > update OAuth2Persister javadoc to clarify what findClient() should return. > BasicOAuth2Store improve removeToken() logic, from Brian Lillie. > Refreshing Token does not include originator info. > Improve information propagated through OAuth2RequestException. > More complete filtering of server information when sendTraceToClient is > disabled. > Log when an invalid or error OAuth2Accessor is used for redirect. > Limit concurrent refreshes to 1, syncrhonizing on interned accessor string. > Refresh token is not properly removed on provider error. > Allow clearing of accessor cache on OAuth2Store. > > > This addresses bug SHINDIG-1839. > https://issues.apache.org/jira/browse/SHINDIG-1839 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Request.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/BasicOAuth2Store.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Module.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2RequestException.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/OAuth2Store.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/BasicAuthenticationHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/BearerTokenHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/ClientAuthenticationHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/ClientCredentialsGrantTypeHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/CodeAuthorizationResponseHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/MacTokenHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/OAuth2HandlerError.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/StandardAuthenticationHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/handler/TokenAuthorizationResponseHandler.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/MapCache.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth2/persistence/OAuth2Persister.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/OAuth2CallbackServlet.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/handler/BasicAuthenticationHandlerTest.java > 1376889 > > http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth2/handler/OAuth2HandlerErrorTest.java > 1376889 > > Diff: https://reviews.apache.org/r/6436/diff/ > > > Testing > ------- > > All existing JUnits pass > > > Thanks, > > Adam Clarke > >
