> On July 21, 2015, 3:35 p.m., Ryan Baxter wrote: > > The changes look fine, but I imagine this could break some clients right? > > Andreas Kohn wrote: > Interesting question. > > If they were coded by following what shindig does, then yes: these > clients would now treat the tokens with a 3-orders-of-magnitude smaller > expiration, and then in the worst case these clients would try to re-login > (they can't use refresh tokens, because shindig doesn't support those :D). > How many of this type exist: I really have no idea, and don't dare to guess. > :) > One could argue that this is easy to handle in configuration though: bump > the expiration setting by a factor 1000 to counteract. > > Clients written according to the spec would now obviously DTRT, but I'm > somehow feeling that there aren't that many - otherwise the issue should have > popped up earlier. The good news here is probably that there are also no > clients having specific workarounds for shindig. > > > Given how long it took us to find this problem there might be a > silver-lining here as well: the field is optional, and it could well be that > no one cared at all about it so far.
I think it is important we get the fix in because we want to follow the spec, but I am weary of breaking things even if it is a small number of clients. Could we perhaps introduce a config parameter to turn this on (leaving it off by default) and then in a later release when we are free to make more disruptive changes, switch the default value? - Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36642/#review92412 ----------------------------------------------------------- On July 21, 2015, 1:46 p.m., Andreas Kohn wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36642/ > ----------------------------------------------------------- > > (Updated July 21, 2015, 1:46 p.m.) > > > Review request for shindig. > > > Bugs: SHINDIG-1996 > https://issues.apache.org/jira/browse/SHINDIG-1996 > > > Repository: shindig > > > Description > ------- > > Change the 'expires_in' field in the token response to be in seconds > > > Diffs > ----- > > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2AuthorizationHandler.java > 1692108 > > trunk/java/social-api/src/main/java/org/apache/shindig/social/core/oauth2/OAuth2TokenHandler.java > 1692108 > > trunk/java/social-api/src/test/java/org/apache/shindig/social/core/oauth/OAuth2AuthCodeFlowTest.java > 1692108 > > Diff: https://reviews.apache.org/r/36642/diff/ > > > Testing > ------- > > * unit tests adjusted to verify the value (previously they just checked for > > 0) > > > Thanks, > > Andreas Kohn > >
