You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Simplifies bulk onboarding authentication so every advertised runner authentication method is supported consistently across create, update, remove, orchestration, and bulk execution flows.
The supported runner authentication methods are now:
Client secret
Certificate by thumbprint, X509Certificate2 object, or PFX file
System-assigned managed identity
Interactive delegated sign-in
Partially implemented runner authentication paths—including user-assigned managed identity selection, raw access tokens, federated/client assertions, GitHub OIDC, Azure CLI/Az PowerShell tokens, existing Graph sessions, and removal-only device code—have been removed.
Changes
Normalizes authentication parameters and validation across the bulk wrapper, orchestrator, and entity scripts.
Adds certificate object and PFX support to removal operations.
Supports interactive delegated authentication for AgentUser operations when a caller-controlled client ID is supplied.
Updates automation-app setup to declare the selected application permissions and delegated scopes even when -SkipGrant is used, allowing an administrator to grant consent later through Microsoft Entra admin center.
Enables public-client flows when delegated scopes are selected.
Retries immediate post-creation permission declaration reads and writes to handle Microsoft Entra replication delays.
Fixes interactive automation-app setup when no client secret or authentication certificate is supplied.
Updates bulk onboarding documentation and test fixtures to match the supported authentication contract.
Declare application roles and delegated scopes with -SkipGrant so
administrators can grant consent in Entra without running the script.
- Preserve existing API permissions and avoid duplicate declarations
- Report declared permissions, consent failures, and portal links
- Add regression coverage for permission merging and idempotency
The reason will be displayed to describe this comment to others. Learn more.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR streamlines the authentication surface across bulk onboarding/orchestration and update/remove wrappers by removing deprecated token/tool-based auth paths and standardizing on a smaller set of supported modes, while extending interactive delegated support and adding automated coverage around permission declaration behavior.
Changes:
Removed -AccessToken and other deprecated auth options from wrappers and step scripts; standardized on client secret, certificate, system-assigned managed identity, and interactive delegated sign-in.
Added/updated tests and fixtures to validate secure auth forwarding and new permission declaration/report semantics.
Enhanced New-A365AutomationApp.ps1 to declare permissions (and reconcile duplicates) separately from granting consent, with richer reporting and documentation updates.
Prevent certificate authentication from persisting imported private keys to user or machine profile stores. Add coverage for password-protected and passwordless PFX loaders and include the registration fixture formatting update.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 744dc687-79b1-4181-a626-57d46d06260f
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Interactive scenario configuration, cascade authentication, environment-secret forwarding, and the generated admin-consent link have unresolved functional defects.
Environment-based client-secret auth is detected above, but this splat forwards only explicitly bound parameters. When A365_CLIENT_SECRET is the selected credential, create scripts recover it themselves, whereas the removal scripts do not, so an orchestrated removal reaches them with no authentication method and fails. Materialize the selected environment secret into the shared splat.
Delegated permission setup remains incomplete for two advertised interactive scenarios. Blueprint adds no delegated scopes at all, while AgentIdentity adds only custom-security-attribute scopes, but the corresponding runners request the core AgentIdentityBlueprint.*, AgentIdentity.*, User.Read, and User.ReadBasic.All scopes. Apps created for those scenarios therefore are not enabled/prepared for the interactive flow. Add scenario-specific delegated scope sets matching each runner before computing isFallbackPublicClient.
This expectation codifies an unusable Entra admin-consent link: /adminconsent needs a registered, URL-encoded redirect_uri, and omitting it can produce AADSTS500113. Either configure/include a matching redirect URI or stop emitting this link and direct users to portalPermissionsUrl; then make the test assert that protocol requirement explicitly.
Device-code authentication lacks behavioral test coverage
The new device-code implementation has no behavioral tests. Current additions test bulk forwarding and a separate Connect-MgGraph helper, but do not verify the device-code request, authorization_pending/slow_down polling, terminal errors, or timeout behavior. Add extracted-function tests with mocked Invoke-RestMethod and Start-Sleep so regressions in this authentication path are caught without network calls.
Oversized test file combines unrelated test concerns
This new test file is 809 lines and combines merge-helper unit tests, full-script source rewriting, permission-report assertions, retry behavior, and authentication configuration. Split it into focused test files (for example merge logic versus end-to-end declaration scenarios) so fixtures and failures remain maintainable.
Forward the resolved tenant through blueprint removal cascades and require a caller-controlled client ID before deleting AgentUsers interactively.
Limit routine AgentUser interactive tokens to provisioning scopes, request app-role assignment permissions only for permission configuration, and remove the unused delegated-grant scope from automation app declarations.
Add regression coverage for cascade authentication and least-privilege interactive scope selection, and document the supported bulk authentication contract in the release notes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 744dc687-79b1-4181-a626-57d46d06260f
This warning is false when delegated scopes were selected: the code enables public-client flows, so the application can authenticate interactively without a secret or certificate. Distinguish “interactive-only” from “cannot authenticate” to avoid telling users that a valid public client is unusable.
Add wrapper test coverage for forwarding the Interactive switch
The wrapper now exposes -Interactive, but its wrapper tests still exercise only client-secret/password forwarding, and the AgentUser fixture does not even declare -Interactive. Add an invocation test that passes this switch and asserts the fixture receives it; otherwise this newly supported update path can regress while the suite remains green.
The reason will be displayed to describe this comment to others. Learn more.
The authentication normalization is directionally sound and thoughtfully documented, but cross-script invariants are not yet enforced consistently enough for this head to merge safely.
Four things to address before this merges; details inline.
The reason will be displayed to describe this comment to others. Learn more.
Minor: Do not codify an incomplete bootstrap permission set
The AgentUser fixture requires AppRoleAssignment.ReadWrite.All for every generated application but omits Application.ReadWrite.All. That is neither the least-privilege contract for routine AgentUser provisioning nor the complete contract for -ConfigurePermissions, so this test now protects an over-privileged but still unusable declaration.
Evidence
PermissionDeclaration.Tests.ps1:87-102 includes AppRoleAssignment.ReadWrite.All, and lines 592-601 require every listed scope to be declared. In contrast, AgentUserInteractiveScopes.Tests.ps1:46-57 proves routine provisioning must exclude both Application.ReadWrite.All and AppRoleAssignment.ReadWrite.All, while lines 60-69 prove -ConfigurePermissions requires both. Production follows those runtime expectations in New-A365AgentUser.ps1:1151-1170, but New-A365AutomationApp.ps1:1068-1082 declares only AppRoleAssignment.ReadWrite.All and lines 1275-1290 persist that declaration even for portal-consent workflows. An administrator who portal-consents the generated AgentUser app therefore grants a broad permission ordinary runs do not use, yet a later interactive -ConfigurePermissions run still requests undeclared Application.ReadWrite.All and requires additional consent or fails for an operator unable to grant it.
Suggested fix: Keep both permission-management scopes out of the default AgentUser declaration. If generated apps are intended to support permission bootstrapping, gate that capability explicitly and require both Application.ReadWrite.All and AppRoleAssignment.ReadWrite.All together in a separate bootstrap test case.
The reason will be displayed to describe this comment to others. Learn more.
Should fix: Generated public client has no redirect URI, so the newly required -Interactive -ClientId runs fail at Connect-MgGraph
This PR makes -ClientId mandatory for every interactive run that touches AgentUsers: the orchestrator, the bulk runner, Remove-A365AgentUser and the Remove-A365Blueprint cascade. It also sets up the generated automation app as that client: the AgentUser delegated scopes are declared on it, and the README says it 'can be used for interactive device-code authentication'.
However, the app only gets isFallbackPublicClient = true, and no publicClient.redirectUris are ever registered. That flag only enables flows without a redirect (device code, ROPC, IWA). Only New-A365AgentUser.ps1 uses device code. Every other step in the same run calls Connect-MgGraph -TenantId -ClientId -Scopes, which is the browser/WAM authorization-code flow: New-A365AgentBlueprint, New-A365AgentIdentity, New-A365AgentRegistration, Remove-A365AgentUser, Remove-A365AgentIdentity and Remove-A365Blueprint. That flow needs http://localhost registered on the client, plus the WAM broker URI on Windows. With the generated app as -ClientId, those steps fail at sign-in (no reply address registered) before any work is done.
Before this change there were two ways around this: interactive runs could omit -ClientId and use the Graph SDK's first-party client, and Remove-A365AgentUser had -UseDeviceCode. This PR removes both.
Evidence
What the app gets
New-A365AutomationApp.ps1:1253-1258 creates the app with only displayName, signInAudience, notes and isFallbackPublicClient.
:1280-1286 PATCHes only @{ isFallbackPublicClient = $true } and prints 'Enabled public-client flows for interactive delegated authentication.'
Searching scripts/bulk-agent-registration for localhost|redirect uri|reply address|brokerplugin finds nothing, so no redirect URI is ever registered.
Where users are pointed at this app
readme.md:255: 'AgentUser operations also require -ClientId for a caller-controlled public client'.
readme.md:259: '...so the generated application can be used for interactive device-code authentication'.
New-A365AutomationApp.ps1:1068-1069: the AgentUser scopes are declared on the automation app.
:1650-1653: the next-step hint is New-A365AgentRegistration.ps1 -Interactive -ClientId $applicationAppId.
Where ClientId is now required
A365-AutomationOrchestrator.ps1:2126-2128
A365-BulkOnboarding.ps1:472-475
Remove-A365AgentUser.ps1:719-721
Remove-A365Blueprint.ps1:984-986
Where it reaches Connect-MgGraph
A365-AutomationOrchestrator.ps1:2133-2137 forwards ClientId to every phase via $authSplat.
New-A365AgentBlueprint.ps1:1923-1930 sets $connect.ClientId = $ClientId plus Scopes, then calls Connect-MgGraph @connect (interactive auth-code flow).
Remove-A365AgentUser.ps1:734 and :778 do the same.
Repo evidence for the requirement
src/Microsoft.Agents.A365.DevTools.Cli/Constants/AuthenticationConstants.cs:42-43 says http://localhost is 'Required by the Microsoft Graph PowerShell SDK (Connect-MgGraph -ClientId)'.
Line 62 gives the WAM broker URI format.
Line 147 notes that AADSTS500113 is returned when an app has no redirect URI.
Failing scenarios
Run New-A365AutomationApp.ps1 -Interactive with the default -Scenario All, then A365-AutomationOrchestrator.ps1 -Interactive -ClientId with an AgentUser phase. The phase-1 Connect-MgGraph fails before any provisioning.
Standalone Remove-A365AgentUser.ps1 -Interactive -ClientId fails the same way, and its -UseDeviceCode alternative was removed in this PR.
Suggested fix: When delegated scopes are requested, also merge publicClient.redirectUris into the same PATCH that sets isFallbackPublicClient: add http://localhost and ms-appx-web://microsoft.aad.brokerplugin/<appId>, keeping any existing URIs. At minimum, document the manual redirect-URI step in readme section 5 and in the script's next-step output.
The reason will be displayed to describe this comment to others. Learn more.
Minor: Auth PFX loader passes an unresolved relative path and a $null password to X509Certificate2
The new -AuthCertificatePath support differs from every sibling PFX loader in two ways:
Relative paths: the path goes straight into X509Certificate2::new after Test-Path. Test-Path resolves relative paths against PowerShell's location, but .NET resolves them against the process working directory, which Set-Location doesn't change. So -AuthCertificatePath ./auth.pfx passes the existence check, then fails with file-not-found once the user has changed directory. The siblings, and this file's own credential upload at line 1390, call Resolve-Path ... .ProviderPath first.
No password: for a PFX without a password (-AuthCertificatePassword is documented as optional), ConvertTo-SecureStringValue returns $null. $null matches the (string, string, flags) and (string, SecureString, flags) overloads equally, so PowerShell can't choose between them. This PR changed the siblings to pass [string]::Empty explicitly in that branch for exactly this reason.
:906-910 calls [X509Certificate2]::new($CertificatePath, $password, EphemeralKeySet) with the raw parameter string.
:905 sets $password = ConvertTo-SecureStringValue -Value $CertificatePassword -Name 'AuthCertificatePassword', which returns $null when the value is absent (:466 if ($null -eq $Value) { return $null }).
Siblings
They resolve the path first: New-A365AgentBlueprint.ps1:1904 $pfx = (Resolve-Path -LiteralPath $CertificatePath).ProviderPath, and New-A365AutomationApp.ps1:1390 does the same.
They branch on the password and use [string]::Empty when there is none: New-A365AgentBlueprint.ps1:1906-1911, New-A365AgentIdentity.ps1:2050-2055, New-A365AgentRegistration.ps1:1190-1195, Remove-A365AgentUser.ps1:752-757.
The diff changed the siblings' passwordless call from ::new($pfx) to this explicit form.
Documentation
readme.md:259 documents '-AuthCertificatePath plus optional -AuthCertificatePassword'.
Failing scenario
In a pwsh session started in the home directory, run cd C:/a365 and then ./New-A365AutomationApp.ps1 -TenantId t -ClientId c -AuthCertificatePath ./auth.pfx -AuthCertificatePassword p.
Test-Path succeeds, then the constructor looks for ~/auth.pfx and fails.
Without -AuthCertificatePassword, the constructor call is ambiguous between the two overloads.
Suggested fix: Make this loader match its siblings: resolve the path with Resolve-Path, and branch on the password, passing [string]::Empty when there is none.
The reason will be displayed to describe this comment to others. Learn more.
Minor: Interactive bulk runs start a new device-code sign-in for every AgentUser row
Get-InteractiveToken runs a fresh device-code flow every time New-A365AgentUser.ps1 is invoked. It keeps only the access_token in $script:CachedToken (no refresh token, no offline_access), and the script clears that cache at startup.
Bulk onboarding calls the orchestrator once per CSV row, and the orchestrator calls New-A365AgentUser.ps1 once per AgentUser. An interactive CSV with N AgentUser rows therefore needs N separate device-code completions, each with code entry and MFA. The Connect-MgGraph-based rows reuse the SDK token cache and stay silent after the first sign-in.
If the operator steps away, each AgentUser row blocks for the device-code lifetime (about 15 minutes) and then fails. This PR is what newly allows interactive AgentUser rows in bulk mode: the previous up-front refusal was replaced.
Evidence
Token acquisition
New-A365AgentUser.ps1:1178-1181 POSTs to /oauth2/v2.0/devicecode on every call.
:1190-1216 polls until expires_in.
:1198 returns only $tokenResponse.access_token.
:1218 then throws 'Interactive delegated sign-in timed out before authorization completed.'
Cache
:1264-1265 is the only cache.
:2467-2468 resets it with $script:CachedToken = $null at the start of every run.
Call chain
A365-AutomationOrchestrator.ps1:2623 and :2634 invoke & $agentUserScript @auArgs once per orchestrator run.
A365-BulkOnboarding.ps1:495-525 calls & $orchestratorPath @rowArgs once per plan node, with the same -Interactive -ClientId on every row.
What changed
The base version of A365-BulkOnboarding.ps1 refused interactive runs that had AgentUser rows ('...which require app-only authentication').
This PR replaced that with only a ClientId check at :472-475.
Failing scenario
A CSV with 20 AgentUser rows run with -Interactive -ClientId X gives 20 separate device-code prompts.
Any prompt that isn't completed stalls its row for about 15 minutes, then the row fails with the timeout error.
Suggested fix: Reuse the delegated token across invocations within the process. For example, keep a session- or module-scoped cache keyed by tenant + client + scope set that honours expiry, or request offline_access and redeem the refresh token on later invocations. Alternatively, acquire the token once in the orchestrator or bulk runner and pass it down.
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
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.
Krishnadheeraj (DheerajPannala)
left a comment
Summary
Simplifies bulk onboarding authentication so every advertised runner authentication method is supported consistently across create, update, remove, orchestration, and bulk execution flows.
The supported runner authentication methods are now:
Partially implemented runner authentication paths—including user-assigned managed identity selection, raw access tokens, federated/client assertions, GitHub OIDC, Azure CLI/Az PowerShell tokens, existing Graph sessions, and removal-only device code—have been removed.
Changes