Add support JdbcOneTimeTokenService #15842
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
CrazyParanoid
force-pushed
the
gh-15735
branch
4 times, most recently
from
f3917b6
to
300b965
Compare
There was a problem hiding this comment.
Thank you for the PR @CrazyParanoid!
I've provided feedback inline.
|
|
||
| private final JdbcOperations jdbcOperations; | ||
|
|
||
| private Function<OneTimeToken, List<SqlParameterValue>> oneTimeTokenParametersMapper = new OneTimeTokenParametersMapper(); |
There was a problem hiding this comment.
Unless we have a need for it, let's not make this a pluggable strategy or expose it publicly. We can always do it in another iteration.
|
|
||
| private Function<OneTimeToken, List<SqlParameterValue>> oneTimeTokenParametersMapper = new OneTimeTokenParametersMapper(); | ||
|
|
||
| private RowMapper<OneTimeToken> oneTimeTokenRowMapper = new OneTimeTokenRowMapper(); |
There was a problem hiding this comment.
Unless we have a need for it, let's not make this a pluggable strategy or expose it publicly. We can always do it in another iteration.
| * @author Max Batischev | ||
| * @since 6.4 | ||
| */ | ||
| public final class OneTimeTokenUtils { |
There was a problem hiding this comment.
Let's keep this package scope, alternatively it might be worth just having some duplicated logic in the Repository instances at least for now. It isn't a lot and we can always extract it out later. It also will allow us to avoid a static Clock (see my comment on the clock.
|
|
||
| public static long DEFAULT_ONE_TIME_TOKEN_TIME_TO_LIVE = 300; | ||
|
|
||
| private static Clock clock = Clock.systemUTC(); |
There was a problem hiding this comment.
I don't view it as ideal to have a static Clock for a few reasons.
It makes it easier to have errors when setting it.
- For example, the tests right now only set this before a test is ran. However, some tests override the value. All future tests that use the Clock must also set it. Alternatively, we could also set after the tests run
- The tests that are setting the value must know what to set the Clock to. Any changes to the Clock would likely need to be made in all of the tests.
Ensuring we are using an instance variable (perhaps on the Repository itself) would avoid all of this. It would also allow us to potentially expose the Clock on the Repository to be set in the future.
There was a problem hiding this comment.
Yes, you are right. Not the best solution. I will return the logic for generating and checking the token from the InMemoryOneTimeTokenService, I will need to think about how to extract it in the next iterations.
| * @author Max Batischev | ||
| * @since 6.4 | ||
| */ | ||
| public final class JdbcOneTimeTokenService implements OneTimeTokenService { |
There was a problem hiding this comment.
If a token is never consumed, then it is never deleted even after it expires. We probably need a way to ensure that expired tokens are cleaned up. Likely this would need to be done using something like a TaskScheduler. For an example of what it might look like, you can refer to JdbcIndexedSessionRepository in Spring Session.
| @@ -0,0 +1,6 @@ | |||
| create table one_time_tokens | |||
There was a problem hiding this comment.
Can you please name this one-time-tokens-schema.sql to align with the current conventions in Spring Security?
CrazyParanoid
force-pushed
the
gh-15735
branch
from
300b965
to
50cc36d
Compare
|
Hi @rwinch! All your comments have been resolved. |
Closes gh-15735