feat(msteams): add DefaultAzureCredential auth type for passwordless Teams auth#48014
feat(msteams): add DefaultAzureCredential auth type for passwordless Teams auth#48014krrrke wants to merge 11 commits intoopenclaw:mainfrom
Conversation
…enclaw#40855) Add support for alternative authentication methods in the MS Teams channel, enabling passwordless bot authentication for enterprise tenants that block client secrets via tenant-wide policy. New config fields under channels.msteams: - authType: 'clientSecret' (default) | 'certificate' | 'federatedCredential' - certPemFile / certKeyFile / sendX5C: for certificate-based auth - ficClientId: FIC client ID for federated credential auth - widAssertionFile: workload identity assertion file path Fully backward compatible: existing appId + appPassword configs continue to work unchanged.
Test coverage for resolveMSTeamsCredentials and hasConfiguredMSTeamsCredentials across all three auth types introduced in the preceding commit. Fixed existing test expectation to include authType field in resolved credentials result. Verifies certificate auth requires both cert files, federated auth requires ficClientId or widAssertionFile, and clientSecret is the default authType.
Variable appPassword in MSTeamsCredentials (token.ts) was declared as required but set to empty string for certificate and federated credential auth paths. Changed to optional to accurately represent the type contract. Guarded clientSecret assignment in sdk.ts to handle undefined appPassword. Added test assertions verifying appPassword is undefined for certificate and federated credential auth.
Type MSTeamsAuthType in token.ts was a standalone union duplicating the inline type in MSTeamsConfig.authType (types.msteams.ts). Replaced with NonNullable<MSTeamsConfig["authType"]> to maintain a single source of truth. Adding a new auth method now only requires updating types.msteams.ts.
Fields authType, certPemFile, certKeyFile, sendX5C, ficClientId, and widAssertionFile were added to MSTeamsConfig (types.msteams.ts) but not to MSTeamsConfigSchema (zod-schema.providers-core.ts). The strict Zod schema rejected configs using these fields as unrecognized properties. Added cross-field validation in superRefine: authType "certificate" requires certPemFile and certKeyFile, authType "federatedCredential" requires ficClientId or widAssertionFile.
Error messages in errors.ts and probe.ts dropped references to environment variables MSTEAMS_APP_ID and MSTEAMS_TENANT_ID when the new auth types were added. Restored env var hints alongside the new auth type guidance so users configuring via environment variables still get actionable error messages. Added test assertions verifying env var names appear in auth error hint.
Added presence helpers that check certificate and federated credential auth material across typed config, SDK plain env vars (certPemFile, certKeyFile, FICClientId, WIDAssertionFile), and SDK connection env vars (connections__*__settings__*). Credential resolution and hasConfiguredMSTeamsCredentials now reject empty configs that have no auth material in any source, preventing silent fallthrough to managed identity auth in the SDK. Configs backed by SDK env vars are accepted.
Aligned certificate case in buildMSTeamsAuthConfig with federated case — only assign certPemFile/certKeyFile to base when present, so undefined values do not shadow the SDK's env var resolution. Added explicit test assertions documenting that env-only cert paths are not forwarded in credentials (SDK merges them at runtime).
New authType "defaultAzureCredential" defers credential selection to @azure/identity's DefaultAzureCredential, which tries environment variables, workload identity, managed identity, az cli, and other sources automatically. Enables passwordless Teams auth in AKS via workload identity without explicit certificate or secret management. Added DefaultAzureCredentialTokenProvider in sdk.ts as a drop-in replacement for MsalTokenProvider. Added createTokenProvider() factory that returns the appropriate provider based on authType. Updated all 4 call sites (probe, monitor, send-context, graph) to use the factory. Added @azure/identity as a dependency of the msteams extension.
Greptile SummaryThis PR adds
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/token.ts
Line: 23-33
Comment:
**Misplaced JSDoc block**
The multi-line JSDoc comment for `hasCertificateAuthMaterial` (lines 23–32) is placed immediately above `trimPresence`, not above the function it describes. As written, documentation generators and IDE tooling will attach this docblock to `trimPresence` and leave `hasCertificateAuthMaterial` undocumented.
Move the block directly above `hasCertificateAuthMaterial` and the single-line comment directly above `trimPresence`:
```suggestion
/** Return trimmed value if non-blank, otherwise undefined. */
function trimPresence(value: string | undefined | null): string | undefined {
```
Then place the multi-line block just before `function hasCertificateAuthMaterial(`.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: fc79604 |
| /** | ||
| * Check whether certificate auth material is present in typed config | ||
| * or the SDK's plain env vars. Each field is checked independently so | ||
| * mixed-source configs (e.g. certPemFile in config, certKeyFile in env) | ||
| * are accepted — the SDK merges them field-by-field at runtime. | ||
| * | ||
| * Does not check connections__*__settings__* env vars because those may | ||
| * belong to an unrelated Agents SDK connection and cannot be reliably | ||
| * attributed to the Teams channel. | ||
| */ | ||
| /** Return trimmed value if non-blank, otherwise undefined. */ |
There was a problem hiding this comment.
Misplaced JSDoc block
The multi-line JSDoc comment for hasCertificateAuthMaterial (lines 23–32) is placed immediately above trimPresence, not above the function it describes. As written, documentation generators and IDE tooling will attach this docblock to trimPresence and leave hasCertificateAuthMaterial undocumented.
Move the block directly above hasCertificateAuthMaterial and the single-line comment directly above trimPresence:
| /** | |
| * Check whether certificate auth material is present in typed config | |
| * or the SDK's plain env vars. Each field is checked independently so | |
| * mixed-source configs (e.g. certPemFile in config, certKeyFile in env) | |
| * are accepted — the SDK merges them field-by-field at runtime. | |
| * | |
| * Does not check connections__*__settings__* env vars because those may | |
| * belong to an unrelated Agents SDK connection and cannot be reliably | |
| * attributed to the Teams channel. | |
| */ | |
| /** Return trimmed value if non-blank, otherwise undefined. */ | |
| /** Return trimmed value if non-blank, otherwise undefined. */ | |
| function trimPresence(value: string | undefined | null): string | undefined { |
Then place the multi-line block just before function hasCertificateAuthMaterial(.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/token.ts
Line: 23-33
Comment:
**Misplaced JSDoc block**
The multi-line JSDoc comment for `hasCertificateAuthMaterial` (lines 23–32) is placed immediately above `trimPresence`, not above the function it describes. As written, documentation generators and IDE tooling will attach this docblock to `trimPresence` and leave `hasCertificateAuthMaterial` undocumented.
Move the block directly above `hasCertificateAuthMaterial` and the single-line comment directly above `trimPresence`:
```suggestion
/** Return trimmed value if non-blank, otherwise undefined. */
function trimPresence(value: string | undefined | null): string | undefined {
```
Then place the multi-line block just before `function hasCertificateAuthMaterial(`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc7960465d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "description": "OpenClaw Microsoft Teams channel plugin", | ||
| "type": "module", | ||
| "dependencies": { | ||
| "@azure/identity": "^4.9.1", |
There was a problem hiding this comment.
Regenerate lockfile after adding msteams dependency
Adding @azure/identity here without a matching pnpm-lock.yaml update makes workspace installs fail under frozen lockfile mode; running pnpm install --frozen-lockfile --lockfile-only --ignore-scripts already errors with ERR_PNPM_OUTDATED_LOCKFILE for extensions/msteams/package.json, and CI uses pnpm install --frozen-lockfile in .github/workflows/ci.yml (line 515), so this change will block reproducible installs and CI for the commit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05a9a0f57b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| managedIdentityClientId: this.clientId, | ||
| workloadIdentityClientId: this.clientId, |
There was a problem hiding this comment.
Stop pinning DAC identity client IDs to appId
This hard-codes both managedIdentityClientId and workloadIdentityClientId to the Teams bot appId, which breaks defaultAzureCredential when the bot app registration ID and the user-assigned managed identity ID differ (a supported scenario in this plugin via separate FIC identity fields). In AKS/workload-identity deployments, overriding these options forces token acquisition to the wrong principal and can make auth fail even when the environment is correctly configured. Leave these options unset by default or allow a separate identity client ID input.
Useful? React with 👍 / 👎.
| const appId = normalizeSecretInputString(cfg?.appId); | ||
| const tenantId = normalizeSecretInputString(cfg?.tenantId); | ||
| if (!appId || !tenantId) return false; |
There was a problem hiding this comment.
Recognize env-backed appId/tenantId as configured
hasConfiguredMSTeamsCredentials now only reads cfg.appId/cfg.tenantId, while resolveMSTeamsCredentials still accepts MSTEAMS_APP_ID and MSTEAMS_TENANT_ID. In the setup finalize flow, this mismatch makes passwordless auth (defaultAzureCredential) appear unconfigured when IDs are env-backed, so the wizard can drop into the client-secret prompt path and overwrite the existing auth mode. The configured check should mirror resolver fallback for appId/tenantId.
Useful? React with 👍 / 👎.
CloudAdapter.process() acquires outbound tokens via MsalTokenProvider, which has its own credential chain (WID → FIC → secret → cert → MI). With defaultAzureCredential, none of these fields were set, causing fallthrough to ManagedIdentityApplication — which does not support AKS workload identity federation. Detect AZURE_FEDERATED_TOKEN_FILE (injected by the workload identity webhook) and pass it as WIDAssertionFile so the SDK uses its native WID path. Falls back to @azure/identity DefaultAzureCredential for environments without workload identity.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b85ec0e0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (authConfig.WIDAssertionFile) { | ||
| return new sdk.MsalTokenProvider(authConfig); |
There was a problem hiding this comment.
Keep defaultAzureCredential on DAC path
When authType is defaultAzureCredential, this branch switches to MsalTokenProvider whenever authConfig.WIDAssertionFile is set, which is common in AKS because AZURE_FEDERATED_TOKEN_FILE is injected automatically. That bypasses DefaultAzureCredential's multi-source fallback chain (CLI, managed identity, env credentials), so any partial or mismatched workload-identity setup turns into a hard auth failure instead of falling back to other available credentials in the same environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is intentional and required. The issue isn't about the explicit token provider we create — it's about CloudAdapter.process().
CloudAdapter.process() internally creates its own MsalTokenProvider from the authConfig to acquire outbound tokens (connector client + user token client). We don't control that code path. The SDK's MsalTokenProvider has a credential chain: WIDAssertionFile → FICClientId → clientSecret → certificate → ManagedIdentityApplication. Without WIDAssertionFile in the authConfig, it falls through to ManagedIdentityApplication, which does not support AKS workload identity federation — that was the root cause of the inbound webhook failure.
Setting WIDAssertionFile puts the SDK's internal token acquisition onto the correct WID code path. Our DefaultAzureCredentialTokenProvider is only used for the explicit token provider — it has no effect on what the adapter does internally.
Additionally, DAC's multi-source fallback is a liability in this context: silently falling back to a different credential (az cli, managed identity, env vars) when workload identity is misconfigured would authenticate as the wrong identity. Hard failure is the correct behavior.
|
Hi, reviewing this from enterprise deployment perspective. This is exactly the auth pattern we need. In our environment client secrets and uploaded certificates are not allowed so managed identity is the only path. The DefaultAzureCredential with managedIdentityClientId pinned to appId. Does this work when the bot app registration and the managed identity are different? The Codex comment on this point seems worth addressing. For Azure Container Apps (not AKS) the workload identity setup is a bit different. Would be good to call this out in the docs. |
|
Codex review: found issues before merge. Summary Reproducibility: yes. Source inspection on current main shows Next step before merge Security Review findings
Review detailsBest possible solution: Rework DAC as an additive Teams auth option on the current Teams SDK App path, preserving the shipped Do we have a high-confidence way to reproduce the issue? Yes. Source inspection on current main shows Is this the best way to solve the issue? No. The requested direction is useful, but this branch is not currently the best solution because it changes shipped config values, pins DAC identity selection to the bot app ID, and is stale against the current Teams SDK App token path. Full review comments:
Overall correctness: patch is incorrect Security concerns:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 5f373ae4d3db. |
Context
Issue #40855 requested passwordless authentication for the MS Teams channel. PR #40884 added certificate and federated credential support. PR #47934 addressed review feedback on that work — fixing schema validation, type design, env var hints, and SDK env fallback handling.
This PR builds on both to add
defaultAzureCredentialas a fourth auth type, completing the passwordless auth story for enterprise deployments.Problem
Certificate and federated credential auth modes require operators to explicitly configure credential material (cert paths, FIC client IDs, workload identity assertion files). In Kubernetes deployments with Azure Workload Identity, and in local development with
az login, these details are already available in the environment — but the operator still has to know which mode to select and which fields to set.DefaultAzureCredentialfrom@azure/identitysolves this by automatically discovering credentials from the environment: workload identity, managed identity, Azure CLI, environment variables, and other sources. The operator only needs to provideappIdandtenantId.Changes
New auth type:
defaultAzureCredential{ "channels": { "msteams": { "enabled": true, "appId": "<app-registration-id>", "tenantId": "<tenant-id>", "authType": "defaultAzureCredential" } } }DefaultAzureCredentialTokenProvider(extensions/msteams/src/sdk.ts)@azure/identity'sDefaultAzureCredentialwith the samegetAccessToken(scope)interface asMsalTokenProviderappIdasmanagedIdentityClientIdandworkloadIdentityClientIdso the credential targets the correct bot identityhttps://graph.microsoft.com→https://graph.microsoft.com/.default) sinceDefaultAzureCredential.getToken()requires scope format, not bare resource URIscreateTokenProvider()factory (extensions/msteams/src/sdk.ts)authTypeprobe.ts,monitor.ts,send-context.ts,graph.tsSetup surface fix (
extensions/msteams/src/setup-surface.ts)authTypeand related fields (certPemFile,certKeyFile,ficClientId,widAssertionFile) are now reset. Previously, re-running setup on a non-client-secret config would preserve the oldauthType, causingresolveMSTeamsCredentials()to ignore the newly entered secret.Release checks (
extensions/msteams/package.json)@azure/identitytorootDependencyMirrorAllowlistsoscripts/release-check.tsdoes not report bundled extension dependency drift.Config schema + types
authTypeunion extended with"defaultAzureCredential"in bothtypes.msteams.tsandzod-schema.providers-core.tsUse cases
AKS with Workload Identity — the pod's service account has a federated credential.
DefaultAzureCredentialpicks it up automatically. No secrets, no cert files, no Vault.Local development —
az loginand go. Same config works everywhere.Azure VMs / App Service — managed identity. Same config.
Dependencies
@azure/identity^4.9.1 added as a dependency of the msteams extension (not rootpackage.json, per plugin guidelines)AI Disclosure
pnpm build && pnpm check && pnpm testgreen (pre-existingnormalizeAgentIdtsgo error on main excluded)Fixes #40855
Related: #40884, #47934