-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: renewal & better errors #4
Conversation
src/custom/auth.ts
Outdated
}); | ||
|
||
return res.data.accessToken; | ||
} catch (err) { |
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 pattern seems very redundant. Maybe you can catch things at the instance level? That way, you catch everything in one spot. Worth doing a bit of research into
|
||
while (true) { | ||
try { | ||
await new Promise(resolve => setTimeout(resolve, 10_000)); |
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.
question: why wait 10 seconds before each loop?
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.
To slow down the loop. There's no need to check it more frequently. We could reduce it to 5 seconds if we wanted though, but I don't see a specific reason to so
headers: { | ||
Authorization: `Bearer ${accessToken}` | ||
// If there's less than 15 seconds until the token expires, we reauthenticate early | ||
if (firstFetchTimeInSeconds >= this.#tokenDetails.accessTokenMaxTTL - 15) { |
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.
So this would mean that if the ttl was 15 or less, it would keep renewing indefinitely? How about we do a percent, like 2/3rds of the way to end of life and then we renew
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.
It might also be worth putting a warning if the max access token/access token ttl is shorter than how much we are subtracting because in those cases it will keep re logging in and firing tons of API calls
[AuthMethod.UniversalAuth]: async () => | ||
await this.#authClient.universalAuth.login(this.#authCredentials?.credentials as UniversalAuthCredentials), | ||
[AuthMethod.AWSIam]: async () => await this.#authClient.awsIamAuth.login(this.#authCredentials?.credentials as AWSIamCredentials), | ||
[AuthMethod.AccessToken]: async () => |
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.
It doesn't seem like you are accessing this one, are you?
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.
Yes it's being accessed with await authMethodMap[this.#authCredentials.type]();
this.#secretsClient = new SecretsClient(this.#apiInstance, this.#requestOptions); | ||
this.#dynamicSecretsClient = new DynamicSecretsClient(this.#apiInstance, this.#requestOptions); | ||
this.#authClient = new AuthClient(this.authenticate.bind(this), this.#apiInstance, this.#basePath); | ||
#authenticator = { |
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.
question: Is there a reason why these methods are in a object like this instead of having them be standalone methods?
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.
No specific reason, but I wanted to consolidate the auth-related helper functions
await authMethodMap[this.#authCredentials.type](); | ||
this.#tokenDetails.firstFetchTime = new Date(); | ||
// If there's less than 10 seconds until the token expires, we renew it early | ||
} else if (timeSinceFetchInSeconds >= this.#tokenDetails.expiresIn - 10) { |
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.
It might make more sense to see if we can renew the token first before we resort to the last hope which is re login
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.
This makes sense though. If the max TTL has been reached, there's no point trying to renew the token because it will always fail if max TTL has been reached
await this.#authClient.accessToken((this.#authCredentials?.credentials as AccessTokenCredentials).accessToken) | ||
}; | ||
|
||
while (true) { |
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.
I'm worried about how memory safe this is. i know in go we have go routine but unsure how this compares to a ever running while loop
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.
- Excessive CPU usage is limited by the sleep function
- The loop will never break due to the try-catch statement
- while(true) is necessary in this case, because intended behavior is to have this run in the background. Javascript isn't multi-threaded, and we don't want to spawn a sub-process for handling token lifecycle
I don't see any reason for this being unsafe
this.#authClient = new AuthClient(this.authenticate.bind(this), this.#apiInstance, this.#basePath); | ||
#authenticator = { | ||
authenticate: async (tokenDetails: Omit<TTokenDetails, "fetchedTime" | "firstFetchTime">) => { | ||
this.#apiInstance = new InfisicalApi( |
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.
instead of a new instance of the client here, maybe you can do it in the renew logic to keep all related code in one spot?
|
||
type AwsAuthLoginOptions = { | ||
identityId?: string; | ||
}; | ||
|
||
export const renewToken = async (apiClient: InfisicalApi, token?: string) => { |
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.
Suggestion: Add JSDoc comment to explain the purpose, parameters, return type, and possible errors of the function. This is helpful for maintainability and for other developers who might use this function.
'''
/**
- Renews the access token using the provided Infisical API client.
- @param apiClient - An instance of InfisicalApi to make the request.
- @param token - The current access token to be renewed.
- @returns The renewed token data.
- @throws {InfisicalError} If the token is missing or the renewal request fails.
*/
'''
Handled in a separate PR. |
This PR introduces renewal for all currently supported authentication methods. Additionally it also improves our error handling. Previously we were returning raw Axios errors, which isn't ideal. Now we are parsing the errors and throwing either
InfisicalError
orInfisicalRequestError
. TheInfisicalRequestError
is only thrown when we're certain that the error is an API error, as it contains special formatting specifically for API errors.Minor
Updated the Dynamic Secrets types.