> On 2011-09-29 17:51:35, Ryan Baxter wrote: > > This is my quick review. I am not expert on OAuth2 so this was purely from > > a code perspective. It would be great is people who already have oauth2 > > implementations could review this code. Most of my comments are below, but > > I have some general ones as well. > > > > 1. Throughout the code there are TODOs with actions items, which seem to > > indicate there may be some gaps in the code. How complete is this code? > > > > 2. I see tests for the different flows but I don't see unit tests for many > > of new classes.
The remaining TODOs are for future considerations, not incorrect functionality or gaps in the supported flows. For example, scoping is not currently supported. Neither are client registration services. These are candidates to implement in the future, but are not currently being developed. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthenticationHandler.java, > > line 52 > > <https://reviews.apache.org/r/1940/diff/2/?file=45163#file45163line52> > > > > Log the exception Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthorizationHandler.java, > > line 31 > > <https://reviews.apache.org/r/1940/diff/2/?file=45164#file45164line31> > > > > Add javadoc Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthorizationHandler.java, > > line 93 > > <https://reviews.apache.org/r/1940/diff/2/?file=45164#file45164line93> > > > > if we should never get here should we throw and exception that the > > calling code can handle instead of returning null Changed to correctly throw OAuth2Exception for receiving unsupported response type. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Code.java, > > line 29 > > <https://reviews.apache.org/r/1940/diff/2/?file=45166#file45166line29> > > > > Add javadoc Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataService.java, > > line 28 > > <https://reviews.apache.org/r/1940/diff/2/?file=45167#file45167line28> > > > > make sure you add @param and @returns Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 56 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line56> > > > > use the Lists API Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 57 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line57> > > > > Use the Maps API Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 58 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line58> > > > > Use the Maps API Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 64 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line64> > > > > Could getId return null? Under no circumstances should a client not have an ID. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 87 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line87> > > > > Use the Lists API Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 97 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line97> > > > > Could getValue be null? An OAuth2Code should always have a value; it's not optional. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 110 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line110> > > > > Could getValue be null? An OAuth2Code should always have a value; it's not optional. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 122 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line122> > > > > Use the Lists API Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Exception.java, > > line 23 > > <https://reviews.apache.org/r/1940/diff/2/?file=45169#file45169line23> > > > > Add javadoc Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Filter.java, > > line 48 > > <https://reviews.apache.org/r/1940/diff/2/?file=45170#file45170line48> > > > > remove the @author Silly Matt - Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedRequest.java, > > line 173 > > <https://reviews.apache.org/r/1940/diff/2/?file=45171#file45171line173> > > > > Should the startsWith be case sensitive? Great catch. HTTP header names are case-insensitive. Fixed. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedRequest.java, > > line 186 > > <https://reviews.apache.org/r/1940/diff/2/?file=45171#file45171line186> > > > > Same thing should this be case sensitive? Fixed. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java, > > line 132 > > <https://reviews.apache.org/r/1940/diff/2/?file=45168#file45168line132> > > > > Could getValue be null? An OAuth2Code should always have a value; it's not optional. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedRequest.java, > > line 211 > > <https://reviews.apache.org/r/1940/diff/2/?file=45171#file45171line211> > > > > Log the exception Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedRequest.java, > > line 224 > > <https://reviews.apache.org/r/1940/diff/2/?file=45171#file45171line224> > > > > Should we be hardcoding localhost:8080 here? The localhost:8080 is only used to build a valid URL from which parameters may be extracted. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedRequest.java, > > line 252 > > <https://reviews.apache.org/r/1940/diff/2/?file=45171#file45171line252> > > > > Log the exception Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedRequest.java, > > line 253 > > <https://reviews.apache.org/r/1940/diff/2/?file=45171#file45171line253> > > > > I think you need a finally here to make sure the InputStream is closed Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedResponse.java, > > line 36 > > <https://reviews.apache.org/r/1940/diff/2/?file=45172#file45172line36> > > > > Use the Maps API Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedResponse.java, > > line 37 > > <https://reviews.apache.org/r/1940/diff/2/?file=45172#file45172line37> > > > > Use the Maps API Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedResponse.java, > > line 80 > > <https://reviews.apache.org/r/1940/diff/2/?file=45172#file45172line80> > > > > Might make sense to make these strings private static final since you > > are using them more than once in the code I'm fine with that. Added private static final strings. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2ServiceImpl.java, > > line 46 > > <https://reviews.apache.org/r/1940/diff/2/?file=45174#file45174line46> > > > > Should these be configurable? I can imagine people wanting the > > configure the expiration times. Added 3 properties to shindig.properties for configuration: shindig.oauth2.authCodeExpiration=300000 shindig.oauth2.accessTokenExpiration=18000000 shindig.oauth2.refreshTokenExpiration=432000000 > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Servlet.java, > > line 96 > > <https://reviews.apache.org/r/1940/diff/2/?file=45175#file45175line96> > > > > Log the exception and you probably want a finally clause here to make > > sure the PrintWriter is cleaned up properly Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2TokenHandler.java, > > line 32 > > <https://reviews.apache.org/r/1940/diff/2/?file=45176#file45176line32> > > > > Add Javadoc Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2TokenHandler.java, > > line 62 > > <https://reviews.apache.org/r/1940/diff/2/?file=45176#file45176line62> > > > > Log the exception Catching the exception is an expected flow, no different from the normal flow. If an exception is thrown, the NormalizedResponse is returned to the user as it would a valid and successful request. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Utils.java, > > line 37 > > <https://reviews.apache.org/r/1940/diff/2/?file=45178#file45178line37> > > > > Add javadoc to all methods Fixed. Well actually, the undocumented methods were carry-overs and were removed. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Utils.java, > > line 67 > > <https://reviews.apache.org/r/1940/diff/2/?file=45178#file45178line67> > > > > Log the exception Method removed. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/OAuth2ProtectedResourceValidator.java, > > line 23 > > <https://reviews.apache.org/r/1940/diff/2/?file=45185#file45185line23> > > > > Add javadoc Done. > On 2011-09-29 17:51:35, Ryan Baxter wrote: > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/OAuth2RequestValidator.java, > > line 23 > > <https://reviews.apache.org/r/1940/diff/2/?file=45186#file45186line23> > > > > Add Javadoc Done. - Eric ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1940/#review2157 ----------------------------------------------------------- On 2011-10-10 20:41:02, Eric Woods wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1940/ > ----------------------------------------------------------- > > (Updated 2011-10-10 20:41:02) > > > Review request for shindig. > > > Summary > ------- > > OAuth 2.0 service provider implementation in Apache Shindig. > > Documentation wiki: > http://docs.opensocial.org/display/OSD/OAuth+2.0+Service+Provider+Implementation+in+Apache+Shindig > > JIRA issue: https://issues.apache.org/jira/browse/SHINDIG-1623 > > > Diffs > ----- > > > http://svn.apache.org/repos/asf/shindig/trunk/content/sampledata/canonicaldb.json > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/common/conf/shindig.properties > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/server/src/main/webapp/WEB-INF/web.xml > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth/AuthenticationHandlerProvider.java > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthenticationHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthorizationHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Client.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Code.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataService.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2DataServiceImpl.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Exception.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedRequest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2NormalizedResponse.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Service.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2ServiceImpl.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Servlet.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2TokenHandler.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Types.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2Utils.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/AccessTokenRequestValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/AuthCodeGrantValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/AuthorizationCodeRequestValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/ClientCredentialsGrantValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/DefaultResourceRequestValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/OAuth2GrantValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/OAuth2ProtectedResourceValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/validators/OAuth2RequestValidator.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/main/java/org/apache/shindig/social/sample/SampleModule.java > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/SocialApiTestsGuiceModule.java > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/config/SocialApiGuiceModuleTest.java > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/AuthenticationProviderHandlerTest.java > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/MockServletOutputStream.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuth2AuthCodeFlowTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuth2AuthenticationHandlerTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuth2ClientCredentialFlowTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuth2ImplicitFlowTest.java > PRE-CREATION > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuthAuthenticationHanderTest.java > 1181203 > > http://svn.apache.org/repos/asf/shindig/trunk/java/social-api/src/test/java/org/apache/shindig/social/dataservice/integration/AbstractLargeRestfulTests.java > 1181203 > > Diff: https://reviews.apache.org/r/1940/diff > > > Testing > ------- > > Yes, JUnits executed with maven build. > > > Thanks, > > Eric > >
