-
Notifications
You must be signed in to change notification settings - Fork 0
feat: jwt client #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
.fernignore
Outdated
| # Specify files that shouldn't be modified by Fern | ||
|
|
||
| src/wrapper | ||
| src/index.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we ignore index.ts does this mean in the future if we for e.g. create a new error type it won't automatically be included by fern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point lemme discuss with Fern side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
confirmed with Fern that src/index.ts won't be touched by codegen. I changed to use wildcard for error types.
|
|
||
| public async getToken(): Promise<string> { | ||
| const nowSeconds = Math.floor(Date.now() / 1000); | ||
| if (this.cachedToken != null && nowSeconds < this.cachedToken.expiresAtEpochSeconds - 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can turn this into a constant, maybe TOKEN_CACHE_BUFFER_SECONDS or something
Wonder if the performance benefit is worth the possibility of returning expired tokens. When we call SmartCMS now we don't cache the JWTs im pretty sure. ofc we call them less often than for eg HAS calls RefX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation signs a new token if the last token valid time is less than 1s, hence the client itself should not return expired token. However, this means the returned token TTL can vary from 1s ~ 15s. In case of the network, if the request spend more than 1s reaching RefX server, the token with 1s TTL might be expired.
To mitigate that, we might change the refresh buffer to be 5s, meaning returned token TTL can vary from 5s - 15s, giving more buffer to tolerate bad network.
Lemme know your thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was worried about 1s of network latency. 5s is safer for sure, but at some point I started wondering what the sweet spot is or if we should even do this optimisation HAHAH. But yeah honestly I don’t think network is likely to take 5s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it yeah I double checked data dog and HAS -> RefX is roughly <100 request per min, i.e. <1.6 QPS. Even with Bright it's still very small scale hence performance impact should be very minor.
I will make the cached token configurable so we can start with no cache and fine tune it if we really hit a certain scale in NRP.
254c8b0 to
1b0b8b1
Compare
1b0b8b1 to
4573ca9
Compare
Problem
After refactoring RefX auth model from a static API key to dynamic JWT token, we need to implement this JWT signing logic in SDK so less effort needed at the caller side.
Closes REFX-590
Solution
Reference: https://buildwithfern.com/learn/sdks/generators/typescript/dynamic-authentication
By referencing Fern documentation, this PR extended the default client to implement a
ReferralExchangeJwtClientwith customisedfetcher. The customisedfetchercan help sign a JWT token and then utilised the default fetcher to finish up the call. This extended client usesjwtwebtokento sign the JWT hence added this new dependency.why do we need a
src/browser/jsonwebtoken-stub.tsjsonwebtokenuses Node-only crypto module and can failyarn & yarn testbecause it checks browser compatibility, as by design(Fern) the SDK should be able to be used in web env as well. However, this JWT client should only be used in server hence added a stub so browser bundles stay lean and predictable. The stub throws a descriptive error at runtime to make it clear the JWT client isn’t meant for browsers, while Node builds continue to use the real library.why add .fernignore
To avoid the extended client being overridden by auto generation
Tests
Published an alpha version in NPM and tested the SDK in HAS locally. Tested the new client calls RefX successfully. In RefX server we can see it passed the new auth guard.
Deploy Notes
NA