Migrate MicrosoftTeams plugin from Bot Framework SDK to M365 Agents SDK#1372
Merged
Merged
Conversation
…DK v1.6.150 Replace Microsoft.Bot.Builder.Integration.AspNet.Core (v4.22.7) with the Microsoft 365 Agents SDK (Microsoft.Agents.* v1.6.150). Package changes: - Add Microsoft.Agents.Hosting.AspNetCore, Microsoft.Agents.Builder, Microsoft.Agents.Authentication.Msal - Remove Microsoft.Bot.Builder.Integration.AspNet.Core Key changes: - All Microsoft.Bot.* namespaces replaced with Microsoft.Agents.* equivalents - Auth registration: ConfigurationBotFrameworkAuthentication replaced with AddDefaultMsalAuth + AddAgent<TeamsActivityBot, TeamsAdapter> - CloudAdapter constructor updated to new signature (IChannelServiceClientFactory, IActivityTaskQueue, ...) - ActivityHandler moved to Microsoft.Agents.Builder.Compat namespace - Activity types and models moved to Microsoft.Agents.Core.Models - Proactive messaging: ContinueConversationAsync now takes ClaimsIdentity via AgentClaims.CreateIdentity instead of raw appId string - Inbound endpoint registered via MapAgentApplicationEndpoints() in IBotSharpAppPlugin.Configure (route: /teams/api/messages) instead of a controller action, avoiding MVC application-part discovery issues - Scoped BotSharp services resolved via CreateScope() inside turn handlers since IAgent is resolved from the root DI provider by the SDK - TeamsRequestState removed; agentId now read from MicrosoftTeamsSetting - MicrosoftTeamsSetting.AllowUnauthenticated added for local debugging with Agents Playground (set true in appsettings.Development.json) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
PR Summary by QodoMigrate MicrosoftTeams plugin from Bot Framework SDK to M365 Agents SDK
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Contributor
Code Review by Qodo
1. Caching mutable User instance
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+229
to
+248
| private async Task<User?> ResolveUserAsync(string aadId, ITurnContext turnContext, CancellationToken cancellationToken) | ||
| { | ||
| var key = CacheKey(aadId); | ||
|
|
||
| // IMemoryCache.TryGetValue returns false for missing keys AND for cached nulls stored | ||
| // as object, so we wrap with a sentinel to distinguish "not cached" from "cached null". | ||
| if (_cache.TryGetValue(key, out User? cached)) | ||
| { | ||
| return cached; | ||
| } | ||
|
|
||
| var email = await GetSenderEmailAsync(turnContext, cancellationToken); | ||
| var user = string.IsNullOrEmpty(email) ? null : await FindUserByEmailAsync(email, cancellationToken); | ||
|
|
||
| _cache.Set(key, user, new MemoryCacheEntryOptions | ||
| { | ||
| SlidingExpiration = CacheTtl | ||
| }); | ||
|
|
||
| return user; |
Contributor
There was a problem hiding this comment.
1. Caching mutable user instance 📘 Rule violation ⛨ Security
ResolveUserAsync stores and returns a mutable User domain model directly from IMemoryCache, allowing downstream mutation to leak across turns/requests. This can cause cross-request state corruption (and the cached object includes sensitive fields like Password/Salt).
Agent Prompt
## Issue description
`TeamsActivityBot.ResolveUserAsync` caches and returns a mutable `User` object directly. This violates the requirement to avoid exposing shared/cached mutable domain models by reference.
## Issue Context
`User` is mutable (many settable properties) and includes sensitive fields (e.g., `Password`, `Salt`). Returning a cached instance can leak mutations across requests/turns.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsActivityBot.cs[229-249]
- src/Infrastructure/BotSharp.Abstraction/Users/Models/User.cs[5-37]
## Implementation notes
- Prefer caching a purpose-built immutable DTO (e.g., `CachedUserIdentity` with only `Id`, `UserName`, `Email`, `FirstName`, `LastName`, `Role`).
- Alternatively, if you must cache `User`, store and return a defensive copy (and copy nested collections like `Permissions`), ensuring sensitive fields are not retained if not needed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+110
to
114
| var agentId = _setting.AgentId; | ||
| if (string.IsNullOrEmpty(agentId)) | ||
| { | ||
| _logger.LogWarning("Teams: no agentId on the request route, dropping message."); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
2. Missing agentid fails silently 📘 Rule violation ☼ Reliability
When MicrosoftTeamsSetting.AgentId is empty, message handling returns without a predictable error/response, effectively dropping requests. This silent fallback makes misconfiguration hard to detect and violates the fail-fast boundary validation requirement.
Agent Prompt
## Issue description
`TeamsActivityBot` silently returns when `_setting.AgentId` is empty, dropping inbound messages without a predictable error path.
## Issue Context
`AgentId` is a required boundary/config input for routing messages. Silent no-op behavior masks configuration issues.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsActivityBot.cs[110-114]
## Implementation notes
- Prefer validating `MicrosoftTeams:AgentId` at startup (e.g., in plugin DI registration) and throw a clear exception / log error so misconfiguration is detected early.
- If runtime validation is required, log at `Error`/`Warning` and send a clear message back to the user (or set an appropriate response) instead of returning silently.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
34
to
39
| [HttpPost("/teams/messages/{agentId}")] | ||
| public async Task PostAsync([FromRoute] string agentId) | ||
| public async Task PostAsync([FromRoute] string agentId, CancellationToken cancellationToken) | ||
| { | ||
| _requestState.AgentId = agentId; | ||
| await _adapter.ProcessAsync(Request, Response, _bot); | ||
| Request.Headers.Remove("Authorization"); | ||
| await _adapter.ProcessAsync(Request, Response, _bot, cancellationToken); | ||
| } |
Contributor
There was a problem hiding this comment.
3. Route agentid ignored 📘 Rule violation ≡ Correctness
The inbound Teams message endpoint accepts an {agentId} route value but neither validates nor uses
it, so requests silently default to MicrosoftTeamsSetting.AgentId regardless of the URL. This
makes the route parameter misleading, breaks per-route agent routing, and violates boundary input
validation requirements by masking client/configuration errors.
Agent Prompt
## Issue description
`TeamsMessageController.PostAsync` accepts a route parameter `{agentId}` but does not validate or use it, and inbound bot processing instead always uses `MicrosoftTeamsSetting.AgentId`, causing silent default behavior and making the route misleading.
## Issue Context
At system boundaries (routes), required inputs should be validated and handled predictably (e.g., `BadRequest`, `NotFound`, or an explicit documented fallback) rather than being ignored. The repository still contains `TeamsRequestState`, which reflects a prior/expected design of propagating the routed `agentId` into the bot’s turn scope for request-scoped routing.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Controllers/TeamsMessageController.cs[33-39]
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsActivityBot.cs[105-116]
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Models/TeamsRequestState.cs[3-10]
## Implementation notes
Choose one coherent approach and make the behavior explicit:
- **Option A (restore route-based routing + validation):**
1. Re-introduce/register a scoped `TeamsRequestState` in DI.
2. In `TeamsMessageController.PostAsync`, validate `agentId` (e.g., non-empty) and set `requestState.AgentId = agentId`.
3. In `TeamsActivityBot`, prefer `requestState.AgentId` for agent selection (optionally fallback to `_setting.AgentId` only if empty), and consider rejecting unknown/mismatched agentIds with `BadRequest`/`NotFound` depending on desired semantics.
- **Option B (single-agent endpoint):**
1. Remove `{agentId}` from the route and method signature (change to `/teams/messages`).
2. Keep using `_setting.AgentId` and update any related documentation/configuration so there is no misleading, unused route input.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+35
to
+38
| public async Task PostAsync([FromRoute] string agentId, CancellationToken cancellationToken) | ||
| { | ||
| _requestState.AgentId = agentId; | ||
| await _adapter.ProcessAsync(Request, Response, _bot); | ||
| Request.Headers.Remove("Authorization"); | ||
| await _adapter.ProcessAsync(Request, Response, _bot, cancellationToken); |
Contributor
There was a problem hiding this comment.
4. Authorization header stripped 🐞 Bug ⛨ Security
TeamsMessageController.PostAsync removes the Authorization header before calling the adapter, which can disable/break the Bot Service JWT validation and make the inbound /teams/messages endpoint accept unauthenticated/forged activities or reject legitimate ones. The code also documents a MicrosoftTeams:AllowUnauthenticated switch, but no such setting exists, so this behavior is always-on.
Agent Prompt
### Issue description
`TeamsMessageController.PostAsync` unconditionally removes the inbound `Authorization` header and then calls `ProcessAsync`. This prevents the adapter from seeing the Bot Service JWT, breaking or bypassing authentication.
### Issue Context
The controller comment suggests using `MicrosoftTeams:AllowUnauthenticated=true` for local dev, but `MicrosoftTeamsSetting` has no such property, and the code does not conditionally apply this behavior.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Controllers/TeamsMessageController.cs[28-39]
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Settings/MicrosoftTeamsSetting.cs[7-35]
### Proposed fix
1. Remove `Request.Headers.Remove("Authorization")`.
2. If local-dev unauth is desired, add `bool AllowUnauthenticated { get; set; }` to `MicrosoftTeamsSetting` and only bypass auth when explicitly enabled (and ideally only in Development environment).
3. Keep the adapter as the single place that validates Bot Service JWTs.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
277
to
279
| private static string GetUserId(IActivity activity) | ||
| => activity.From?.AadObjectId | ||
| ?? activity.From?.Id | ||
| ?? activity.Conversation?.Id | ||
| ?? string.Empty; |
Contributor
There was a problem hiding this comment.
5. Empty userid possible 🐞 Bug ≡ Correctness
TeamsActivityBot.GetUserId now returns only From.AadObjectId (or empty string), so if AadObjectId is missing the bot will treat senders as unknown and/or pass an empty conversation id into the BotSharp conversation engine. Using an empty conversation id risks conversation collisions and incorrect message routing.
Agent Prompt
### Issue description
`GetUserId` returns `activity.From?.AadObjectId ?? string.Empty`. If AadObjectId is absent for any activity type/environment, downstream logic receives an empty `userId`, which is used as the BotSharp conversation id.
### Issue Context
`TeamsMessageHandler` uses `userId` as both routing key and conversation id.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsActivityBot.cs[277-279]
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsMessageHandler.cs[35-39]
### Proposed fix
1. Restore a stable fallback chain, e.g. `AadObjectId ?? From.Id ?? Conversation.Id`, and ensure the final result is non-empty.
2. If no stable identifier is available, explicitly log and return a user-friendly error instead of proceeding with an empty id.
3. Keep `TeamsNotifyRequest.UserId` semantics aligned with whatever identifier you persist in the conversation reference store.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+27
to
39
| // Build sub-config with Connections section required by Microsoft.Agents.Authentication.Msal. | ||
| // Maps our MicrosoftTeamsSetting keys to the format ConfigurationConnections expects. | ||
| var authConfig = new ConfigurationBuilder() | ||
| .AddInMemoryCollection(new Dictionary<string, string?> | ||
| { | ||
| ["MicrosoftAppType"] = settings.AppType, | ||
| ["MicrosoftAppId"] = settings.AppId, | ||
| ["MicrosoftAppPassword"] = settings.AppPassword, | ||
| ["MicrosoftAppTenantId"] = settings.TenantId | ||
| ["Connections:BotServiceConnection:Assembly"] = "Microsoft.Agents.Authentication.Msal", | ||
| ["Connections:BotServiceConnection:Type"] = "MsalAuth", | ||
| ["Connections:BotServiceConnection:Settings:AuthType"] = "ClientSecret", | ||
| ["Connections:BotServiceConnection:Settings:ClientId"] = settings.AppId, | ||
| ["Connections:BotServiceConnection:Settings:ClientSecret"] = settings.AppPassword, | ||
| ["Connections:BotServiceConnection:Settings:TenantId"] = settings.TenantId, | ||
| ["Connections:BotServiceConnection:Settings:Scopes:0"] = "https://api.botframework.com/.default", | ||
| }) |
Contributor
There was a problem hiding this comment.
6. Apptype/msi support dropped 🐞 Bug ≡ Correctness
MicrosoftTeamsPlugin hardcodes MSAL auth to ClientSecret using settings.AppPassword, while MicrosoftTeamsSetting still documents UserAssignedMSI and an empty secret. Deployments using MSI (or any non-client-secret auth mode) will stop working or misconfigure authentication.
Agent Prompt
### Issue description
The new Agents SDK auth configuration is always created with `AuthType=ClientSecret` and always sets `ClientSecret=settings.AppPassword`, ignoring `MicrosoftTeamsSetting.AppType`.
### Issue Context
`MicrosoftTeamsSetting` still exposes `AppType` with values like `UserAssignedMSI`, and its comment explicitly says the secret can be empty in that mode.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[27-39]
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Settings/MicrosoftTeamsSetting.cs[9-28]
### Proposed fix
1. Either implement `AppType`-based configuration mapping for the Agents SDK (ClientSecret vs Managed Identity / SingleTenant), or
2. If MSI is not supported in this plugin anymore, remove/rename `AppType` and update documentation, and fail fast at startup when `AppType != ClientSecret` to avoid silent misconfiguration.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+255
to
+268
| private void SetHttpContextUser(User user) | ||
| { | ||
| var claims = new[] | ||
| { | ||
| new Claim(ClaimTypes.NameIdentifier, user.Id), | ||
| new Claim(ClaimTypes.Name, user.UserName ?? string.Empty), | ||
| new Claim(ClaimTypes.Email, user.Email ?? string.Empty), | ||
| new Claim(ClaimTypes.GivenName, user.FirstName ?? string.Empty), | ||
| new Claim(ClaimTypes.Surname, user.LastName ?? string.Empty), | ||
| new Claim(ClaimTypes.Role, user.Role ?? string.Empty), | ||
| }; | ||
| var identity = new ClaimsIdentity(claims, authenticationType: "Teams"); | ||
| _httpContextAccessor.HttpContext!.User = new System.Security.Principal.GenericPrincipal(identity, roles: null); | ||
| } |
Contributor
There was a problem hiding this comment.
7. Httpcontext null dereference 🐞 Bug ☼ Reliability
TeamsActivityBot.SetHttpContextUser force-dereferences _httpContextAccessor.HttpContext, which will crash if a turn executes without an active HttpContext (e.g., queued/background turn execution). This makes message processing fragile and can cause intermittent 500s.
Agent Prompt
### Issue description
`SetHttpContextUser` assigns `_httpContextAccessor.HttpContext!.User = ...` with no null check. If `HttpContext` is absent, this throws and fails the turn.
### Issue Context
The adapter is constructed with an `IActivityTaskQueue` (background queue), which commonly implies some work may execute outside the request thread/context.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsActivityBot.cs[255-268]
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsAdapter.cs[15-21]
### Proposed fix
1. Change to:
- `var http = _httpContextAccessor.HttpContext; if (http == null) { _logger.LogDebug(...); return; }`
- then set `http.User = new ClaimsPrincipal(identity)` (or keep GenericPrincipal if desired).
2. Keep downstream code resilient when no HttpContext is present (avoid assuming IUserIdentity is available in such execution paths).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.