> On Nov. 11, 2013, 12:36 p.m., Stanton Sievers wrote: > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthenticationHandler.java, > > line 90 > > <https://reviews.apache.org/r/15417/diff/1/?file=381931#file381931line90> > > > > You've introduced some new functionality by allowing the token to be > > null. Is there a use case for this? If you want to bypass the request, I > > would rather see the getSecurityTokenFromRequest method get overridden > > instead of allowing this new method to affect how the AuthenticationHandler > > flow works.
Fair enough. null was a possible return for #getSecurityTokenFromRequest(), and the newly added #create() might want to do additional validation (see below) and skip the request. Fine with changing this though. > On Nov. 11, 2013, 12:36 p.m., Stanton Sievers wrote: > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthenticationHandler.java, > > line 91 > > <https://reviews.apache.org/r/15417/diff/1/?file=381931#file381931line91> > > > > What is the use case for throwing this exception? The request itself might be valid, but not sufficient for the application to hand out a token. In our case we're overriding this with roughly this code: try { ... User user = getUserInformationFromDatabase(); if (user matches token information) { // good return new ShiroSecurityToken(...); } else { // something bad throw new InvalidAuthenticationException("token is invalid"); } } catch (database exceptions) { // cannot let in the user. return null; } In terms of _which_ type of exception to use: I simply wasn't sure, so went with what the existing code already had. It might make sense to introduce a different exception, or handle exceptions generically in the #get() method. In that case the 'null' there might also be translated into that exception. In any case: I think the method should have a way to indicate an error, so either 'null' or 'exception' cases should be provided. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15417/#review28647 ----------------------------------------------------------- On Nov. 11, 2013, 11:32 a.m., Andreas Kohn wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15417/ > ----------------------------------------------------------- > > (Updated Nov. 11, 2013, 11:32 a.m.) > > > Review request for shindig. > > > Bugs: SHINDIG-1950 > https://issues.apache.org/jira/browse/SHINDIG-1950 > > > Repository: shindig > > > Description > ------- > > Factor out creation of the actual SecurityToken in > OAuth2AuthenticationHandler into a separate method that can be overridden in > a child class. > > > Diffs > ----- > > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthenticationHandler.java > 1540645 > > Diff: https://reviews.apache.org/r/15417/diff/ > > > Testing > ------- > > Patch used in our application to create a different type of token. > > > Thanks, > > Andreas Kohn > >
