Add an expiry time on server-to-server tokens #11262
Conversation
Changed Packages
|
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
freben
force-pushed
the
freben/server-token-exp
branch
from
ebb6d91 to
4874ebc
Compare
| return this.currentTokenPromise; | ||
| } | ||
|
|
||
| const result = Promise.resolve().then(async () => { |
There was a problem hiding this comment.
Why is this Promise.resolve wrapping needed? Can we do it with new Promise()? Or is it because of the timing?
There was a problem hiding this comment.
Yeah not actually strictly needed - just makes sure that the actual "hot" function in the then gets called on the next tick, out of band, after this method has returned. This was to ensure that it would never accidentally overwrite the promise member with undefined before it was even written in the first place. But then I refractored to have the then/catch separate at the bottom anyway, rendering the initial exercise a bit moot. But still, it ensures a potentially quicker return to the caller. 🤷
There was a problem hiding this comment.
Just skipping the await at this point would be enough right?
There was a problem hiding this comment.
Not quite the same thing. I still prefer this way of doing it because
- it reads well to me at least (VERY clearly defers the action) since it does the assignment to a member below and i wanted it to be crystal clear that it'll then for sure also be set to undefined in the right order upon failure
- it makes sure that any work that might be sync in
sign()will be happening inside the promise chain, so that exceptions are caught and handled for sure down at the bottom, not bubbling up and out already here - and for the same reason as above, any heavy work for sure happens out of band
but i think you are right though that a skipped await on sign() might have basically the same effect in practice in this instance
| return this.currentTokenPromise; | ||
| } | ||
|
|
||
| const result = Promise.resolve().then(async () => { |
There was a problem hiding this comment.
Just skipping the await at this point would be enough right?
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
Signed-off-by: Fredrik Adelöw <freben@gmail.com>
😁
ServerTokenManagerOptionsmay seem excessive - it's there because I started out adding like issuer and expiry in there, but backtracked. I don't mind having it though for consistency and when we maybe expand on it later.