Repository navigation
feat/HCK-18239 Align OAuth login to use provided functions in studio - #149
massintha-azamoum wants to merge 14 commits into
Conversation
…uth module is exposed. Studio builds that do not expose the module keep the authorization-code exchange. Co-authored-by: Cursor <cursoragent@cursor.com>
… a user-to-machine token or fall back to the code exchange. Share the external-browser decision in the MFA helper and call the token helper directly from the connection. Co-authored-by: Cursor <cursoragent@cursor.com>
Sylvie-P
changed the title
| tokenEndpoint, | ||
| clientId: productionClientId, | ||
| port: redirectPort, | ||
| failOnPortConflict: true, |
There was a problem hiding this comment.
Port 8080 is often taken by a local web server, and with failOnPortConflict: true the sign-in then fails right away. If the app registration accepts any port on http://localhost (Entra ignores the port on loopback redirect URIs), failOnPortConflict: false would let the sign-in use the next free port.
| tenantId: resolveAuthorizeTenant({ connectionInfo }), | ||
| }); | ||
| const redirectPort = resolveExternalBrowserPort({ connectionInfo }); | ||
| const { accessToken } = await oauth.browserLoginFlow({ |
There was a problem hiding this comment.
browserLoginFlow is the name Studio exposes today; @hackolade/oauth calls it signInWithBrowser. If Studio aligns the name before release, this call needs to follow.
|
|
||
| const exchangeAuthorizationCode = async ({ app, connectionInfo, logger } = {}) => { | ||
| try { | ||
| const { productionClientId, tokenEndpoint, desktopTokenRequestHeaders } = await getEntraOAuthConfig({ app }); |
There was a problem hiding this comment.
This only runs when @hackolade/oauth isn't exposed, so getEntraOAuthConfig always returns the legacy values here, and in practice the same goes for getExternalBrowserUrl in api.js. Using the legacy config directly in these two places would make it clear they're the older-Studio path, and getEntraOAuthConfig would only serve the Username / Password client id. Also, resolveAuthorizeTenant always returns a tenant, so FALLBACK_AUTHORIZE_ENDPOINT in entraOAuthConfig.js is never used.
|
|
||
| const isEntraMfa = ({ connectionInfo } = {}) => connectionInfo?.authMethod === 'Azure Active Directory (MFA)'; | ||
|
|
||
| const canRequestUserToMachineAccessToken = ({ app, connectionInfo } = {}) => { |
There was a problem hiding this comment.
"UserToMachine" is gone from the package's public names. Something like canSignInWithStudio would say what this checks: an MFA connection and a Studio that exposes @hackolade/oauth.
| const shouldUseExternalBrowser = ({ app, connectionInfo } = {}) => | ||
| isEntraMfa({ connectionInfo }) && !canRequestUserToMachineAccessToken({ app, connectionInfo }); |
There was a problem hiding this comment.
canRequestUserToMachineAccessToken checks isEntraMfa again.
| const shouldUseExternalBrowser = ({ app, connectionInfo } = {}) => | |
| isEntraMfa({ connectionInfo }) && !canRequestUserToMachineAccessToken({ app, connectionInfo }); | |
| const shouldUseExternalBrowser = ({ app, connectionInfo } = {}) => | |
| isEntraMfa({ connectionInfo }) && !loadOauthModule({ app }); |
| throw new Error('hckFetch should not be called'); | ||
| }; | ||
|
|
||
| registerHooks({ |
There was a problem hiding this comment.
Nothing runs this spec: the repo has no test script or runner. Either add one (node --test; module.registerHooks needs Node 22.15 or later) or leave the spec out.
| connectionInfo.authMethod === 'Azure Active Directory (Username / Password)' | ||
| ? (await getEntraOAuthConfig({ app })).productionClientId | ||
| : undefined; | ||
| const redirectUri = 'http://localhost:8080'; |
There was a problem hiding this comment.
The MFA connection no longer takes redirectUri, so it can go, here, below and in the getConnection doc comment in connection.js.
| const redirectUri = 'http://localhost:8080'; |
| @@ -48,6 +52,7 @@ const getConnectionClient = async (connectionInfo, logger) => { | |||
| clientId, | |||
| redirectUri, | |||
There was a problem hiding this comment.
Unused, see above.
| redirectUri, |
|
Studio now provides functions for the OAuth authorization flow and this PR makes use of that.
This also makes it backward compatible if Studio isn't update to date, the plugin revert to the old approach
Jira sub-task: HCK-18239