allow detour auth

[iceljc]

No description provided.

[qodo-code-review]

PR Summary by Qodo

Allow anonymous connector auth in Teams plugin (DEBUG) and ensure per-turn HttpContext

🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

AI Description

• Add DEBUG-only channel client factory that forces anonymous connector/user token clients.
• Ensure Teams turns have an HttpContext so scoped services can resolve user identity.
• Use ClaimsPrincipal for HttpContext.User to match ASP.NET Core expectations.

Diagram

graph TD
  Teams["Microsoft Teams"] --> Adapter["TeamsAdapter"] --> Bot["TeamsActivityBot"] --> Ctx["IHttpContextAccessor"] --> Downstream["Downstream services"]
  Adapter --> Factory["IChannelServiceClientFactory (DEBUG)"] --> ConnectorAPI["Bot Connector API"]
  Bot --> Repo[("BotSharp user DB")]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Config-driven anonymous mode (non-DEBUG)

• ➕ Allows controlled enablement in staging environments without rebuilding
• ➕ Can be toggled per deployment via configuration
• ➖ Higher risk of accidental anonymous auth in production if misconfigured
• ➖ Adds config surface area and validation needs

2. Upstream fix: make RestChannelServiceClientFactory honor useAnonymous consistently

• ➕ Eliminates need for an obsolete-overload workaround
• ➕ Centralizes behavior in the SDK component for all consumers
• ➖ Requires contributing upstream or maintaining a fork/patch
• ➖ Longer lead time; may block immediate developer productivity

3. Custom minimal IChannelServiceClientFactory implementation

• ➕ Avoids calling obsolete overloads
• ➕ Can precisely control branching behavior and logging
• ➖ More code to maintain vs. thin wrapper
• ➖ Potential to drift from SDK behavior across upgrades

Recommendation: The current DEBUG-only wrapper is a pragmatic, low-risk way to unblock local/dev scenarios while keeping production behavior unchanged. Longer-term, prefer addressing the inconsistent useAnonymous handling upstream (or via a targeted custom factory) to remove reliance on the obsolete overload and implicit Activity.Recipient.Role branching.

Files changed (2) +37 / -2

Enhancement (1) +32 / -0

MicrosoftTeamsPlugin.csInject DEBUG anonymous channel service client factory wrapper +32/-0

Inject DEBUG anonymous channel service client factory wrapper

• Adds a DEBUG-only DI registration for IChannelServiceClientFactory that wraps RestChannelServiceClientFactory and forces anonymous connector and user-token client creation. The wrapper routes the ITurnContext overload through the ClaimsIdentity overload (marked obsolete) to bypass an internal branch that otherwise ignores useAnonymous.

src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs

Bug fix (1) +5 / -2

TeamsActivityBot.csGuarantee per-turn HttpContext and set ClaimsPrincipal identity +5/-2

Guarantee per-turn HttpContext and set ClaimsPrincipal identity

• Ensures an HttpContext exists for each Teams turn by initializing DefaultHttpContext when IHttpContextAccessor.HttpContext is null. Updates SetHttpContextUser to set HttpContext.User to a ClaimsPrincipal (instead of GenericPrincipal) and clarifies that the HttpContext is turn-owned because the SDK does not provide one.

src/Plugins/BotSharp.Plugin.MicrosoftTeams/Services/TeamsActivityBot.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📜 Skill insights (0)

1. Unvalidated ServiceUrl usage 📘 Rule violation ☼ Reliability

Description

AnonymousDebugChannelServiceClientFactory.CreateConnectorClientAsync(ITurnContext ...) passes
turnContext.Activity.ServiceUrl (and turnContext.Identity) to connector creation without
null/empty/state checks. This can cause exception-driven control flow at an integration boundary
instead of a safe, deterministic failure.

Code

src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[R94-97]

+    public Task<IConnectorClient> CreateConnectorClientAsync(ITurnContext turnContext, string? audience = null, IList<string>? scopes = null, bool useAnonymous = false, CancellationToken cancellationToken = default)
+#pragma warning disable CS0618 // intentionally using the legacy overload to bypass the agentic branch
+        => inner.CreateConnectorClientAsync(turnContext.Identity, turnContext.Activity.ServiceUrl, audience, cancellationToken, scopes, true);
+#pragma warning restore CS0618

Evidence

PR Compliance ID 2 requires null/empty/state validation at API/integration boundaries rather than
relying on exceptions. The new factory method directly dereferences
turnContext.Activity.ServiceUrl and forwards it without any guard, so missing/empty values will
fail via exceptions inside connector client creation.

src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[94-97]
Best Practice: Learned patterns

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The debug `IChannelServiceClientFactory` implementation forwards `turnContext.Identity` and `turnContext.Activity.ServiceUrl` to `RestChannelServiceClientFactory` without validating they are present/valid, which can lead to exceptions at an integration boundary.

## Issue Context
Even though this is `#if DEBUG`, the compliance requirement expects boundary inputs (like URLs and request-derived fields) to be validated and to fail safely/deterministically.

## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[94-97]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Nullable audience forwarded 🐞 Bug ≡ Correctness

Description

AnonymousDebugChannelServiceClientFactory.CreateConnectorClientAsync(ITurnContext, ...) accepts
string? audience = null but forwards that value into a legacy overload that declares `string
audience, relying on the inner factory to tolerate null and risking failures when audience` is
omitted. This is a new nullability/contract mismatch introduced in the DEBUG detour path.

Code

src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[R94-97]

+    public Task<IConnectorClient> CreateConnectorClientAsync(ITurnContext turnContext, string? audience = null, IList<string>? scopes = null, bool useAnonymous = false, CancellationToken cancellationToken = default)
+#pragma warning disable CS0618 // intentionally using the legacy overload to bypass the agentic branch
+        => inner.CreateConnectorClientAsync(turnContext.Identity, turnContext.Activity.ServiceUrl, audience, cancellationToken, scopes, true);
+#pragma warning restore CS0618

Evidence

The wrapper’s ITurnContext overload explicitly allows audience to be null and then passes it to
the inner call that corresponds to the wrapper’s own legacy overload signature where audience is
declared non-nullable (string audience). This introduces a nullable-to-non-nullable handoff in the
new DEBUG-only detour implementation.

src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[85-101]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`AnonymousDebugChannelServiceClientFactory.CreateConnectorClientAsync(ITurnContext ...)` takes `string? audience = null` and passes it to `RestChannelServiceClientFactory.CreateConnectorClientAsync(ClaimsIdentity, string, string audience, ...)`.

This violates the nullability contract of the legacy overload and makes connector creation depend on the inner implementation’s handling of `null` (which may fail or produce incorrect audience selection).

### Issue Context
This only executes under `#if DEBUG`, but it directly affects local/debug runs and test environments where the detour factory is registered.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[94-97]

### Suggested fix
Update the ITurnContext overload to ensure a non-null audience is passed to the legacy overload. Options:
1) Coalesce to a safe default (preferably the same default the SDK would use when `audience` is null), e.g. derive it from configuration/settings, and only fall back to `string.Empty` if that is known to be accepted by the SDK.
2) Refactor the wrapper/constructor to accept whatever value you consider the default audience (e.g., AppId or configured audience) and apply `audience ??= <defaultAudience>` before calling the legacy overload.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

1. Unvalidated serviceurl usage 📘 Rule violation ☼ Reliability

AnonymousDebugChannelServiceClientFactory.CreateConnectorClientAsync(ITurnContext ...) passes
turnContext.Activity.ServiceUrl (and turnContext.Identity) to connector creation without
null/empty/state checks. This can cause exception-driven control flow at an integration boundary
instead of a safe, deterministic failure.

Agent Prompt

## Issue description
The debug `IChannelServiceClientFactory` implementation forwards `turnContext.Identity` and `turnContext.Activity.ServiceUrl` to `RestChannelServiceClientFactory` without validating they are present/valid, which can lead to exceptions at an integration boundary.

## Issue Context
Even though this is `#if DEBUG`, the compliance requirement expects boundary inputs (like URLs and request-derived fields) to be validated and to fail safely/deterministically.

## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[94-97]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

[qodo-code-review]

2. Nullable audience forwarded 🐞 Bug ≡ Correctness

AnonymousDebugChannelServiceClientFactory.CreateConnectorClientAsync(ITurnContext, ...) accepts
string? audience = null but forwards that value into a legacy overload that declares `string
audience, relying on the inner factory to tolerate null and risking failures when audience` is
omitted. This is a new nullability/contract mismatch introduced in the DEBUG detour path.

Agent Prompt

### Issue description
`AnonymousDebugChannelServiceClientFactory.CreateConnectorClientAsync(ITurnContext ...)` takes `string? audience = null` and passes it to `RestChannelServiceClientFactory.CreateConnectorClientAsync(ClaimsIdentity, string, string audience, ...)`.

This violates the nullability contract of the legacy overload and makes connector creation depend on the inner implementation’s handling of `null` (which may fail or produce incorrect audience selection).

### Issue Context
This only executes under `#if DEBUG`, but it directly affects local/debug runs and test environments where the detour factory is registered.

### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.MicrosoftTeams/MicrosoftTeamsPlugin.cs[94-97]

### Suggested fix
Update the ITurnContext overload to ensure a non-null audience is passed to the legacy overload. Options:
1) Coalesce to a safe default (preferably the same default the SDK would use when `audience` is null), e.g. derive it from configuration/settings, and only fall back to `string.Empty` if that is known to be accepted by the SDK.
2) Refactor the wrapper/constructor to accept whatever value you consider the default audience (e.g., AppId or configured audience) and apply `audience ??= <defaultAudience>` before calling the legacy overload.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools