Customize OneTimeToken expire time

[R4N]

Expected Behavior

To be able to utilize the default JdbcOneTimeTokenService and set a custom expire time for the OneTImeToken within the generate method.

Current Behavior

OneTimeToken expire time is hard coded to 5 minutes in the JdbcOneTimeTokenService and InMemoryOneTimeTokenService.

Context

We've started implementing OneTimeTokenLogin after its recent inclusion in Spring Security and appreciate this great feature addition.

During testing, the default expiration time (5 minutes) seems to be sufficient. As we move towards production usage we've started considering more scenarios which we think may warrant increasing it: delayed mail delivery, user doesn't check the email right away, etc. Because of this, we're planning on increasing the expiration time slightly (to 10 or 15 minutes).

We've switched over to using JdbcOneTimeTokenService for production, but when looking for a spot to modify the expiration time, we saw that there wasn't an option present to do so.

After consulting the documentation, there is mention of modifying the one-time token expire time by creating a Custom OneTimeTokenService.

A full custom implementation to only override the expire time is potentially risky as it requires implementing/duplicating the majority of the logic (in JdbcOneTimeTokenService) which doesn't need to change in order to fulfill this type of behavior.

Implementation Ideas

Overloaded Constructors for OneTimeTokenService(s)

PR details here: #16260

1. Add OneTimeTokenSettings class which has a property for OneTimeToken timeToLive Duration (default to 5 minutes)
2. Pass OneTimeTokenSettings into overloaded JdbcOneTimeTokenService/InMemoryTokenService constructors
3. Utilize clock.now() + timeToLive Duration for expire time in generate methods of OneTimeTokenServices.

Set in project's application.properties

Property for OneTimeToken timeToLive Duration fetched from application.properties and utilized in OneTimeTokenServices generate method (defaults to 5m if not set).

Switch JdbcOneTimeTokenService's insertOneTimeToken(OneTimeToken) to protected

This would allow subclassing of JdbcOneTimeTokenService and specifying the CustomOneTImeTokenService as a bean and overriding the generate method within CustomOneTimeTokenService to specify the expire time then calling super.insertOneTimeToken(OneTimeToken)

Specify OneTimeTokenSettings in OneTimeTokenLoginConfigurer

OneTimeTokenService would need some way of fetching the setting for timeToLive duration from OneTimeTokenSettings specified to the OneTimeTokenLoginConfigurer.

[jzheaux]

Thanks for the suggestion, @R4N, and for the earlier PR.

One thing we might consider is adding expiresIn to GenerateOneTimeTokenRequest and adding a request resolver like so:

@Bean 
GenerateOneTimeTokenRequestResolver resolve() {
    return (request) -> new GenerateOneTimeTokenRequest(...);
}

Then, all services could be updated to look for an expiry in GenerateOneTimeTokenRequest. The value of this is then the expiry can be determined dynamically.

This follows a similar pattern when generating other request objects in Spring Security like OAuth2AuthorizeRequestResolver and Saml2AuthenticationRequestResolver and to an extent AuthenticationConverter.

This also makes it easier to delegate; something that is a little tricky with a service that persists:

@Bean 
GenerateOneTimeTokenRequestResolver resolve() {
    DefaultGenerateOneTimeTokenRequestResolver delegate = new DefaultOneTimeTokenRequestResolver();
    return (request) -> {
        GenerateOneTimeTokenRequest generate = delegate.resolve(request);
        return new GenerateOneTimeTokenRequest(generate.getUsername(), myExpiresIn());
    }
}

It also gives one place for setters, should we want to go that route to further simplify:

DefaultGeneratorOneTimeTokenRequestResolver#setExpiresIn(Duration)

So that every implementation of OneTimeTokenService doesn't need to add the same setter.

@franticticktick, I saw your comment on the PR. Please feel free to weigh in with your thoughts here.

[franticticktick]

We have already discussed this possibility here. @marcusdacoregio suggested:

public interface GenerateOneTimeTokenRequestResolver {

    GenerateOneTimeTokenRequest resolve(HttpServletRequest request);

}

I think this is a good idea.

[franticticktick]

@jzheaux this solution will not work. Don’t forget that we use an instance clock to generate expiresAt. And the same instance will be used for checking expiration, i.e. in the isExpired function. At the same time, we can change the instance clock using the setClock setter. This is true for all oneTimeTokenService implementations. So we can't do this:

@Bean 
GenerateOneTimeTokenRequestResolver resolve() {
    DefaultGenerateOneTimeTokenRequestResolver delegate = new DefaultOneTimeTokenRequestResolver();
    return (request) -> {
        GenerateOneTimeTokenRequest generate = delegate.resolve(request);
        return new GenerateOneTimeTokenRequest(generate.getUsername(), myExpiresIn());
    }
}

This will lead to the fact that the time can be generated on one clock and checked on another.
At the same time, I admit that GenerateOneTimeTokenRequestResolver is needed and will open a separate ticket, but the problem of expiresAt customization does not need to be solved through it.
The simplest way I can suggest is a simple setTokenTimeToLive setter in each of the oneTimeTokenService implementations. This simple API will be enough in 90% of cases. For example:

OneTimeTokenService service = new InMemoryOneTimeTokenService();
service.setTokenTimeToLive(10);

[jzheaux]

Can you clarify for me why the following doesn't work? (pseudocode follows)

resolver.resolve(http.request) --> GenerateOneTimeTokenRequest("username", 10m);
service.generate(generate.request) -->
    Instant expiration = Instant.now(this.clock).plus(request.getExpiresIn())
    OneTimeToken token = OneTimeToken(token, username, expiration);

It seems that the service's clock would be used for both. What am I missing?

[franticticktick]

I understand what you mean and as it stands, yes, this is a good solution. I considered the solution where the generation of expiresAt occurs in the resolver and it does not work. I apologize, I was wrong) I can prepare a PR.