feat(msteams): handle signin/tokenExchange and signin/verifyState for SSO (#60956)#63964
feat(msteams): handle signin/tokenExchange and signin/verifyState for SSO (#60956)#63964sudie-codes wants to merge 1 commit intoopenclaw:mainfrom
Conversation
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Greptile SummaryAdds opt-in Microsoft Teams SSO support by handling Confidence Score: 5/5Safe to merge; SSO is opt-in and all pre-SSO paths are unaffected. Both findings are P2: the connectionName inconsistency is harmless in practice (Teams echoes back the configured name), and the missing expiry check affects only future consumers of the get API. No data loss, no correctness breakage on existing paths. extensions/msteams/src/sso.ts (connectionName storage key consistency) and extensions/msteams/src/sso-token-store.ts (expiry check on get) Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/sso.ts
Line: 200-242
Comment:
**Inconsistent `connectionName` key between the two handlers**
`handleSigninTokenExchangeInvoke` stores the token under `value.connectionName?.trim() || deps.connectionName`, so if Teams sends an activity `connectionName` that differs from the configured one, the token is persisted under a different key. `handleSigninVerifyStateInvoke` (line 291) always saves under `deps.connectionName`. Downstream callers that look up the token by `deps.connectionName` will never find a token stored via the exchange path when the two names diverge.
In practice Teams echoes back the same `connectionName` the bot sent in the OAuth card, so this is harmless today. But aligning both save sites to use `deps.connectionName` (or explicitly normalise the activity value against the configured name) makes the behaviour less surprising and removes a latent mismatch.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/msteams/src/sso-token-store.ts
Line: 81-83
Comment:
**`get` returns tokens without checking `expiresAt`**
The store returns whatever is on disk regardless of whether `expiresAt` has passed. A consumer that calls `get` after the Bot Framework token has expired will receive an already-expired token and may only discover the problem after a downstream API call fails. Adding an optional expiry guard (e.g. return `null` when `expiresAt` is set and is in the past) would surface stale tokens early and make the caller re-trigger the SSO flow.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(msteams): handle signin/tokenExchan..." | Re-trigger Greptile |
| } | ||
| const connectionName = value.connectionName?.trim() || deps.connectionName; | ||
| if (!connectionName) { | ||
| return { ok: false, code: "missing_connection", message: "no OAuth connection name" }; | ||
| } | ||
| if (!value.token) { | ||
| return { ok: false, code: "missing_token", message: "no exchangeable token on invoke" }; | ||
| } | ||
|
|
||
| const bearer = await deps.tokenProvider.getAccessToken(BOT_FRAMEWORK_TOKEN_SCOPE); | ||
| const fetchImpl = deps.fetchImpl ?? (globalThis.fetch as unknown as MSTeamsSsoFetch); | ||
| const result = await callUserTokenService({ | ||
| baseUrl: deps.userTokenBaseUrl ?? BOT_FRAMEWORK_USER_TOKEN_BASE_URL, | ||
| path: "/api/usertoken/exchange", | ||
| query: { | ||
| userId: user.userId, | ||
| connectionName, | ||
| channelId: user.channelId ?? "msteams", | ||
| }, | ||
| method: "POST", | ||
| body: { token: value.token }, | ||
| bearerToken: bearer, | ||
| fetchImpl, | ||
| }); | ||
|
|
||
| if ("error" in result) { | ||
| return { | ||
| ok: false, | ||
| code: result.status >= 500 ? "service_error" : "unexpected_response", | ||
| message: result.error, | ||
| status: result.status, | ||
| }; | ||
| } | ||
|
|
||
| await deps.tokenStore.save({ | ||
| connectionName, | ||
| userId: user.userId, | ||
| token: result.token, | ||
| expiresAt: result.expiration, | ||
| updatedAt: new Date().toISOString(), | ||
| }); | ||
|
|
||
| return { ok: true, token: result.token, expiresAt: result.expiration }; |
There was a problem hiding this comment.
connectionName key between the two handlers
handleSigninTokenExchangeInvoke stores the token under value.connectionName?.trim() || deps.connectionName, so if Teams sends an activity connectionName that differs from the configured one, the token is persisted under a different key. handleSigninVerifyStateInvoke (line 291) always saves under deps.connectionName. Downstream callers that look up the token by deps.connectionName will never find a token stored via the exchange path when the two names diverge.
In practice Teams echoes back the same connectionName the bot sent in the OAuth card, so this is harmless today. But aligning both save sites to use deps.connectionName (or explicitly normalise the activity value against the configured name) makes the behaviour less surprising and removes a latent mismatch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/sso.ts
Line: 200-242
Comment:
**Inconsistent `connectionName` key between the two handlers**
`handleSigninTokenExchangeInvoke` stores the token under `value.connectionName?.trim() || deps.connectionName`, so if Teams sends an activity `connectionName` that differs from the configured one, the token is persisted under a different key. `handleSigninVerifyStateInvoke` (line 291) always saves under `deps.connectionName`. Downstream callers that look up the token by `deps.connectionName` will never find a token stored via the exchange path when the two names diverge.
In practice Teams echoes back the same `connectionName` the bot sent in the OAuth card, so this is harmless today. But aligning both save sites to use `deps.connectionName` (or explicitly normalise the activity value against the configured name) makes the behaviour less surprising and removes a latent mismatch.
How can I resolve this? If you propose a fix, please make it concise.| async get({ connectionName, userId }) { | ||
| const store = await readStore(); | ||
| return store.tokens[makeKey(connectionName, userId)] ?? null; |
There was a problem hiding this comment.
get returns tokens without checking expiresAt
The store returns whatever is on disk regardless of whether expiresAt has passed. A consumer that calls get after the Bot Framework token has expired will receive an already-expired token and may only discover the problem after a downstream API call fails. Adding an optional expiry guard (e.g. return null when expiresAt is set and is in the past) would surface stale tokens early and make the caller re-trigger the SSO flow.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/sso-token-store.ts
Line: 81-83
Comment:
**`get` returns tokens without checking `expiresAt`**
The store returns whatever is on disk regardless of whether `expiresAt` has passed. A consumer that calls `get` after the Bot Framework token has expired will receive an already-expired token and may only discover the problem after a downstream API call fails. Adding an optional expiry guard (e.g. return `null` when `expiresAt` is set and is in the past) would surface stale tokens early and make the caller re-trigger the SSO flow.
How can I resolve this? If you propose a fix, please make it concise.
Summary
Adds Microsoft Teams SSO support by handling the
signin/tokenExchangeandsignin/verifyStateinvoke activities. Enables seamless single-sign-on for users in tenants where the bot's AAD app is configured for SSO.What changed
channels.msteams.sso.enabledandchannels.msteams.sso.connectionName(additive, opt-in).signin/tokenExchangeandsignin/verifyStatein the existing monitor-handler chain. Both always ack withinvokeResponsestatus 200 so the Teams card UI stops reporting "Something went wrong".https://token.botframework.com/api/usertoken/exchange/GetToken) and persists the delegated user token.sso.ts(User Token service client + handlers) andsso-token-store.ts(file-backed store keyed by connection name + AAD object ID, with an in-memory store for tests).monitor.tsconstructsMSTeamsSsoDepswhensso.enabled && sso.connectionNameand passes it through toregisterMSTeamsHandlers; default off preserves pre-SSO behavior.AAD app requirements
access_as_user).knownClientApplicationslists the Teams client IDs (5e3ce6c0-2b1f-4285-8d4b-75ee78787346desktop,1fec8e78-bce4-4aaf-ab1b-5451cc387264mobile,4765445b-32c6-49b0-83e6-1d93765276caTeams web).channels.msteams.sso.connectionName, pointing at the same AAD app.Test plan
ssodeps are present or absentextensions/msteamssuite green (627 tests)Closes #60956