token_expires_in method of oauthlib.Server is ignored by DOT

[petrdanecek]

My project has a requirement to set the token expiration per some auth cookie validity (means the token expiration is not static, but variable per token been issued. The token expiration is result of some method instead of static oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS).

The oauthlib Server init function accepts both static value or callable function as an input:

class Server(AuthorizationEndpoint, TokenEndpoint, ResourceEndpoint,
             RevocationEndpoint):

  def __init__(self, request_validator, token_expires_in=None,
                 token_generator=None, refresh_token_generator=None,
                 *args, **kwargs):

The oauthlib tokens.py BearerToken class has its own default expiration value set: self.expires_in = expires_in or 3600

class BearerToken(TokenBase):
 ....
    def __init__(self, request_validator=None, token_generator=None,
                 expires_in=None, refresh_token_generator=None):
        ....
        self.expires_in = expires_in or 3600

Unfortunately token_expires_in is ignored by DOT. The token expiration seems to be hardcoded into the save_bearer_token in oauth2_validators.py.

@transaction.atomic
    def save_bearer_token(self, token, request, *args, **kwargs):
    ....
    expires = timezone.now() + timedelta(seconds=oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS)

and

# TODO: check out a more reliable way to communicate expire time to oauthlib
        token['expires_in'] = oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS

I think that better implementation would be to explicitly pass the oauth2_settings.ACCESS_TOKEN_EXPIRE_SECONDS as token_expires_in value and respect token['expires_in'] in save_bearer_token().

[IvanAnishchuk]

Yeah, if you asked my opinion, that whole method is reimplementing too much of the logic that's already in oauthlib's create_token. I'd rewrite it if I had some free time.

[IvanAnishchuk]

It should probably start with passing token_expires_in parameter from settings to the Server during initialization, that would pass it to BearerToken constructor (or whatever token handler is used in custom server) -- that way when token handler's create_token is called it would use the correct expires_in. Note that token_expires_in here could be a callable, that's already supported by oauthlib (takes one argument, request), but if it's not enough it's also possible to register "token modifier" hooks for grant handlers. (It's probably the most correct way to add extra values to the tokens, maybe it would be a good idea to expose some configuration interface so it's not required to subclass stuff just for adding some hooks.)

I'm not sure about the best way of allowing EXPIRE_SECONDS setting to be either number or callable. I suspect it would either have to be two separate variables or require some updates in OAuth2ProviderSettings. But should definitely be possible.

Now, if we are hacking the places where we call that Server constructor (namely oauth2_backends.get_oauthlib_core and views.mixins.OAuthLibMixin, possibly some others too) it makes sense to allow passing other stuff there as well -- the default implementation accepts three optional arguments: token_expires_in, token_generator, refresh_token_generator and also arbitrary **kwargs which don't seem to be used but could be useful for custom classes (if fact, it might be a good idea to use such a subclass by default, just so we could pass kwargs to grant types and endpoints). It would be better if those token generator callables were configurable in settings (defaulting to oauthlib's defaults, possibly with some way of additionally configuring them if they are class-based like client id/secret generators, configurable length for example sounds like a good idea) and for kwargs I'd add yet another settings variable, something like OAUTH2_SERVER_KWARGS.

Now, after doing these preliminary changes we could try to simply the save_bearer_token method to set expires value for token as now + token['expires_in'] (as backend knows enough to set the correct value in token dict).

The rest of the method looks way too long and complicated for something so simple in essence: all it does is saving passed token dictionary to the db. I'm also not entirely sure about all the logic there: when refresh token rotation is disabled it seems to overwrite current access token instance rather than create a new on (which I'd think is more natural or at least is much closer to rotated flow), but more importantly I don't like that rotated vs. non-rotated flows have different side effects and make revocation logic even less clear. I guess part of the reason for weird stuff could be one to one fields being used for refresh/access token relations but sure there are other ways.

It would be additionally nice if the save method started returning token instances (and possibly some additional values signalling whether either instance was just created, reused, or simply retrieved) -- those are the method's actual results and subclasses could do more things easier if they could access it directly. The return value is ignored by oauthlib so it wouldn't be a problem.

[dopry]

@petrdanecek are you open to creating a PR?