> 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? > > Andreas Kohn wrote: > 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. >
I agree with the need for show error path. I'd prefer throwing an exception over returning null. I think an InvalidAuthenticationException would be fine, per your example. - Stanton ----------------------------------------------------------- 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 > >
