feat: Clock interface as a time source #263
Conversation
| @@ -448,6 +448,32 @@ type ClientConfig struct { | |||
| BackendConfig | |||
| } | |||
|
|
|||
| // Clock is an interface that can be used with the library instead | |||
| // of the [time] package. | |||
There was a problem hiding this comment.
To the reviewer: The Clock interface is not a drop-in replacement for the time package.
I've only defined one method on Clock, Now(), because this is the only function of the time package that our library currently needs.
I'm not certain whether we should keep the interface small, or require that it implements all of the time package's available functions, even if we don't currently need them.
The alternative would be to have this interface for clerk.Clock:
type Clock interface {
After(d time.Duration) <-chan time.Time
Sleep(d time.Duration)
Now() time.Time
Since(t time.Time) time.Duration
NewTicker(d time.Duration) Ticker
NewTimer(d time.Duration) Timer
AfterFunc(d time.Duration, f func()) Timer
}Let me know what you think.
There was a problem hiding this comment.
I feel like the Now() method is enough for what we are trying to achieve here, expose a way to DI the clock dependency for testing purposes and not only
| @@ -9,6 +9,7 @@ require ( | |||
|
|
|||
| require ( | |||
| github.com/davecgh/go-spew v1.1.1 // indirect | |||
| github.com/jonboulle/clockwork v0.4.0 // indirect | |||
There was a problem hiding this comment.
Added this external dependency, but it's used only in tests. Would be nice to implement our own fake clock. At least there are no other indirect dependencies, the clockwork library is small.
| @@ -280,6 +295,15 @@ type cacheEntry struct { | |||
| expiresAt time.Time | |||
| } | |||
|
|
|||
| // IsValid returns true if a non-expired entry exists in the cache | |||
| // for the provided key, false otherwise. | |||
| func (c *jwkCache) IsValid(key string, t time.Time) bool { | |||
There was a problem hiding this comment.
🔧 Minor but perhaps currentTime or now makes a bit of sense as an argument name?
| func (c *jwkCache) IsValid(key string, t time.Time) bool { | |
| func (c *jwkCache) IsValid(key string, now time.Time) bool { |
| if kid == "" { | ||
| return nil, fmt.Errorf("missing jwt kid header claim") | ||
| } | ||
|
|
||
| jwk := getCache().Get(kid) | ||
| if jwk == nil { | ||
| if jwk == nil || !getCache().IsValid(kid, clock.Now().UTC()) { |
There was a problem hiding this comment.
💡 Not sure if that's difficult but perhaps the clock should be a field of jwkCache ? That would solve the issue of checking for expiration inside Get if we want to retain this approach.
Actually, maybe ignore that I see that getCache is a singleton.
http/middleware.go
Outdated
| var err error | ||
| jwk, err = forceGetJWK(ctx, jwksClient, kid) | ||
| if err != nil { | ||
| fmt.Println(err) |
There was a problem hiding this comment.
🔧
| fmt.Println(err) |
| if kid == "" { | ||
| return nil, fmt.Errorf("missing jwt kid header claim") | ||
| } | ||
|
|
||
| jwk := getCache().Get(kid) | ||
| if jwk == nil { | ||
| if jwk == nil || !getCache().IsValid(kid, clock.Now().UTC()) { |
There was a problem hiding this comment.
💡 You can maybe return a cacheEntry object here, something like:
| if jwk == nil || !getCache().IsValid(kid, clock.Now().UTC()) { | |
| var cachedJwk cacheEntry | |
| if cachedJwk == nil || !cachedJwk.IsValid(clock.Now().UTC()) { | |
| } | |
| jwk = cachedJwk.Value() |
Verifying a session JWT includes time sensitive validations, like
checking token expiration, or that the token can be used now ('nbf'
claim).
The HTTP middleware caches JSON web keys for one hour. After that, the
cache is considered invalid and a new request to fetch the JSON web key
set will be issued.
These are both flows that depend on the current time and time
comparisons.
Various system architectures with a distributed nature might not be
synchronized. A clock skew is expected.
In such cases it's common to share a "time source" so that all nodes can
coordinate on what the perceived current time is.
This commit adds a new clerk.Clock interface which can be passed as a
dependency in the http middleware functions and the jwt.Verify function.
We also add a dependency to the clockwork project, in order to use a
fake clock for our tests.
gkats
force-pushed
the
core-1719-clock
branch
from
b9ad230 to
7c70811
Compare
| @@ -448,6 +448,32 @@ type ClientConfig struct { | |||
| BackendConfig | |||
| } | |||
|
|
|||
| // Clock is an interface that can be used with the library instead | |||
| // of the [time] package. | |||
There was a problem hiding this comment.
I feel like the Now() method is enough for what we are trying to achieve here, expose a way to DI the clock dependency for testing purposes and not only
|
|
||
| // Now returns the current time. | ||
| func (c *defaultClock) Now() time.Time { | ||
| return time.Now() |
There was a problem hiding this comment.
🔧 I believe we should use UTC by default
| return time.Now() | |
| return time.Now().UTC() |
There was a problem hiding this comment.
Thanks. I'll update this in a follow up PR.
Verifying a session JWT includes time sensitive validations, like checking token expiration, or that the token can be used now ('nbf' claim).
The HTTP middleware caches JSON web keys for one hour. After that, the cache is considered invalid and a new request to fetch the JSON web key set will be issued.
These are both flows that depend on the current time and time comparisons.
Various system architectures with a distributed nature might not be synchronized. A clock skew is expected.
In such cases it's common to share a "time source" so that all nodes can coordinate on what the perceived current time is.
This commit adds a new clerk.Clock interface which can be passed as a dependency in the http middleware functions and the jwt.Verify function. We also add a dependency to the clockwork project, in order to use a fake clock for our tests.