-
Notifications
You must be signed in to change notification settings - Fork 22
[apps/api] feat: allow oauth for regional bots #386
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: main
Are you sure you want to change the base?
Conversation
|
Loving it 😍 Thanks a lot @lilyydu for your work on this. I have several comments after going through the changes. Have you considered using OAuthUrl as the setting's name instead of tokenUrl? I understand the choice for tokenUrl as it is the URL to get the token but the bot service doc talks about OAuth URL in OAuth URL support in Azure AI Bot Service for example so it might be good to be aligned. Regarding the instructions for setting up:
Regarding the CLI templates:
|
… instructions to test sample README
| export type ClientSettings = { | ||
| /** | ||
| * the URL to use for managing user oauth tokens. | ||
| * Specify this value if you are using a regional bot. | ||
| * For e.g., https://europe.token.botframework.com | ||
| * Default is https://token.botframework.com | ||
| */ | ||
| readonly OAuthUrl?: 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.
camelCase?
| readonly OAuthUrl?: string; | ||
| }; | ||
|
|
||
| export const DEFAULT_CLIENT_SETTINGS: Required<ClientSettings> = { |
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: Instead of making all keys optional in ClientSettings, and making DEFAULT_CLIENT_SETTINGS Required, you could make all keys required in ClientSettings, and make all parameters Partial. This way, in places you need ClientSettings, the value of the keys isn't string | undefined.
For eg, in sign-in.ts, you have:
`${this._clientSettings.OAuthUrl}/${BOT_SIGNIN_ENDPOINTS.URL}?${q}`here OauthUrl is actually string | undefined, which is less understandable than if it was just string.
| * For e.g., https://europe.token.botframework.com | ||
| * Default is https://token.botframework.com | ||
| */ | ||
| readonly OAuthUrl?: 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.
Also oauthUrl, or oauthBaseEndpoint? It's not really the full url. Thoughts?
|
|
||
| this.bots = new BotClient(this.http); | ||
| this.users = new UserClient(this.http); | ||
| this._clientSettings = {...DEFAULT_CLIENT_SETTINGS, ...(clientSettings ?? {})}; |
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 is copied in many places. Consider a helper method that takes in Partial<ClientSettings> and returns ClientSettings
| * authentication. This is important to | ||
| * configure for regional bots. | ||
| */ | ||
| readonly clientSettings?: ClientSettings |
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.
why is this in "OauthSettings". this seems more general than just Oauth, right?
| * For e.g., https://europe.token.botframework.com | ||
| * Default is https://token.botframework.com | ||
| */ | ||
| readonly OAuthUrl?: 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.
Also, each of these should have an associated environment variable that populates them through a config, if not provided
resolves: #367
ClientSettingsthats passed into our clients from the app layertokenUrlspecifies the regional token endpoint to use (e.g., europe.token.botframework)I didn't reuse
OauthSettingsbecausea) its associated w/ the Apps ecosystem vs
ClientSettingsis concerned w the API layerb) eventually may diverge with additional params
b) would cause circular dependencies
INSTRUCTIONS FOR SETTING UP
(documenting here for now, should add to docs, partial courtesy to @Benjiiim):
resources: