-
Notifications
You must be signed in to change notification settings - Fork 1.2k
add support for token provider #1587
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
base: master
Are you sure you want to change the base?
Conversation
b4dd476
to
f514473
Compare
src/client.ts
Outdated
@@ -438,6 +457,31 @@ export class OpenAI { | |||
return Errors.APIError.generate(status, error, message, headers); | |||
} | |||
|
|||
async _setToken(): Promise<boolean> { |
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: I think "refresh" is a better verb than "set" here?
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 decision to refresh is taken by the input function and not by this method, i.e. it is not guaranteed that the token will be refreshed if this method is called. However, let me know if you feel strongly about it and I can update the name.
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 do dislike _set
but don't feel strongly about it being called _refresh
- I find it weird that a method called _setX()
doesn't actually take any arguments.
fwiw I was also framing "refresh" as in, refresh the apiKey
property - not necessarily do a full token refresh.
wdyt about _callApiKey()
? I don't think that's great either... my main fud with _setApiKey()
is that the return value is not intuitive imo and it'd be great if we could find a name that makes that work
f514473
to
a5ce6ac
Compare
1453757
to
b623760
Compare
b623760
to
0307ed5
Compare
export interface ClientOptions { | ||
/** | ||
* Defaults to process.env['OPENAI_API_KEY']. | ||
*/ | ||
apiKey?: string | undefined; | ||
|
||
apiKey?: string | ApiKeySetter | undefined; |
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 docstring should mention the new function behaviour
@@ -385,7 +386,7 @@ export class OpenAI { | |||
|
|||
this._options = options; | |||
|
|||
this.apiKey = apiKey; | |||
this.apiKey = typeof apiKey === 'string' ? apiKey : 'Missing Key'; |
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: should probably be something like <not set yet>
?
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.
actually, we should move API_KEY_SENTINEL
our of the azure specific file and use that
let fail = true; | ||
const testFetch = async (url: RequestInfo, req: RequestInit | undefined): Promise<Response> => { | ||
const testFetch = async (url: any, { headers }: RequestInit = {}): Promise<Response> => { |
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.
q: why did you have to use any
?
src/client.ts
Outdated
@@ -438,6 +457,31 @@ export class OpenAI { | |||
return Errors.APIError.generate(status, error, message, headers); | |||
} | |||
|
|||
async _setToken(): Promise<boolean> { |
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 do dislike _set
but don't feel strongly about it being called _refresh
- I find it weird that a method called _setX()
doesn't actually take any arguments.
fwiw I was also framing "refresh" as in, refresh the apiKey
property - not necessarily do a full token refresh.
wdyt about _callApiKey()
? I don't think that's great either... my main fud with _setApiKey()
is that the return value is not intuitive imo and it'd be great if we could find a name that makes that work
@@ -49,6 +50,10 @@ export class OpenAIRealtimeWebSocket extends OpenAIRealtimeEmitter { | |||
|
|||
client ??= new OpenAI({ dangerouslyAllowBrowser }); | |||
|
|||
if (typeof (client as any)?._options?.apiKey !== 'string') { | |||
throw new Error('Call the create method instead to construct the client'); |
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: this error message could be more helpful
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.
also, wait does this work? because _setApiKey()
doesn't mutate options, won't this check always return true
if you pass in a function?
should we instead define the client.apiKey
fallback string as a constant and check that? e.g. if (client.apiKey === API_KEY_MISSING) { ...
try { | ||
const token = await apiKey(); | ||
if (!token || typeof token !== 'string') { | ||
throw new Errors.OpenAIError( | ||
`Expected 'apiKey' function argument to return a string but it returned ${token}`, | ||
); | ||
} | ||
this.apiKey = token; | ||
return true; | ||
} catch (err: any) { | ||
if (err instanceof Errors.OpenAIError) { | ||
throw err; | ||
} | ||
throw new Errors.OpenAIError( | ||
`Failed to get token from 'apiKey' function: ${err.message}`, | ||
// @ts-ignore | ||
{ cause: 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.
having a try catch around the whole block and then checking inside the catch if we threw isn't great, could we instead just either put the try catch just around the await apiKey()
? or await apiKey().catch(...)
?
Changes being requested
This PR introduces a new
tokenProvider
option to the OpenAI client, enabling dynamic token retrieval for authentication scenarios where API keys need to be refreshed or obtained at runtime.Additional context & links
Changes
New Feature:
tokenProvider
OptionTokenProvider
type definition:() => Promise<AccessToken>
AccessToken
interface withtoken: string
propertytokenProvider
option toClientOptions
interface_setToken()
methodKey Benefits
Implementation Details
Client Construction:
tokenProvider
andapiKey
are mutually exclusive - only one can be providedtokenProvider
(setsdangerouslyAllowBrowser: true
)Token Management:
_setToken()
method now returns a boolean indicating if a token was successfully retrievedprepareOptions()
Usage Example
Backward Compatibility
This change is fully backward compatible. Existing code using
apiKey
continues to work unchanged. The newtokenProvider
option is purely additive.