Add guided CYOT external phone provider setup - #14
Open
Rohit Gulati (rogulati) wants to merge 2 commits into
Open
Rohit Gulati (rogulati) wants to merge 2 commits into
Rohit Gulati (rogulati) wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate issues affect activation, deployment, provider configuration, and security.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a guided, resumable CYOT external phone provider setup workflow with validation, diagnostics, Bicep provisioning, and approval-gated policy activation.
Changes:
- Adds staged PowerShell orchestration, state persistence, logging, diagnostics, and smoke tests.
- Adds Entra registration, provider endpoint configuration, and Azure infrastructure deployment.
- Adds policy validation, documentation, examples, package metadata, and artifact ignores.
File summaries
| File | Reviewed changes |
|---|---|
README.md |
Links to guided setup documentation. |
CYOT-Setup/tests/Setup-Cyot.SmokeTests.ps1 |
Adds orchestration smoke tests. |
CYOT-Setup/stages/Step3-Set-CyotPolicy.ps1 |
Validates and activates CYOT policy. |
CYOT-Setup/stages/Step2-Setup-ExternalPhoneProvider.ps1 |
Configures provider endpoints, resources, identity, security, and deployment. |
CYOT-Setup/stages/Step1-Register-CyotApplication.ps1 |
Registers or reuses Entra applications. |
CYOT-Setup/stages/Deploy-CyotInfrastructure.ps1 |
Runs Bicep deployment preflight and deployment. |
CYOT-Setup/Setup-Cyot.ps1 |
Orchestrates stages, state, logging, diagnostics, and approvals. |
CYOT-Setup/infra/resources.bicep |
Defines endpoint infrastructure and role assignments. |
CYOT-Setup/infra/main.parameters.json |
Provides deployment parameter examples. |
CYOT-Setup/infra/main.bicep |
Defines subscription-scoped deployment. |
CYOT-Setup/examples/customer-config.example.json |
Provides sample customer configuration. |
CYOT-Setup/docs/Troubleshooting.md |
Documents diagnostics and recovery. |
CYOT-Setup/docs/README.md |
Documents prerequisites and the setup runbook. |
CYOT-Setup/CYOT-Setup.psd1 |
Describes package metadata and contents. |
CYOT-Setup/.gitignore |
Excludes generated logs, state, and backups. |
Review details
Suppressed comments (6)
CYOT-Setup/Setup-Cyot.ps1:296
- When Bicep mode is selected, these assignments overwrite any configured
FunctionAppName,StorageAccountName, andKeyVaultNamewith template outputs. The Bicep template has no name parameters and generates deterministic names, while the example configuration asks the customer forfunctionAppName; that documented input is silently ignored. Pass the names through the template or remove the misleading configuration fields.
if ($null -ne $infrastructureResult) {
foreach ($name in @('FunctionAppName', 'StorageAccountName', 'KeyVaultName', 'ResourceGroup', 'Location', 'PlanType')) {
$arguments[$name] = $infrastructureResult.$name
}
CYOT-Setup/Setup-Cyot.ps1:254
- When the orchestrator runs Register without
tenantIdin the configuration, Step 1 prompts for a tenant but intentionally returns only the application ID. This lookup therefore returns null and the prompted tenant is never persisted, leaving a completed register state withouttenantIdfor later guided/resume flows. Persist the resolved tenant ID or require it before orchestrating the stage.
$tenantId = Get-CyotValue $Config $State TenantId tenantId
if ($tenantId) { $State.tenantId = $tenantId }
CYOT-Setup/Setup-Cyot.ps1:295
Deploy-CyotInfrastructurereturns the subscription it resolved interactively, but this transfer dropsSubscriptionIdbefore invoking Step 2 and saving state. If it was omitted from configuration, deployment can succeed whilestate.subscriptionIdremains null, so resumed runs lose the selected subscription. Include the returned subscription in this transfer.
foreach ($name in @('FunctionAppName', 'StorageAccountName', 'KeyVaultName', 'ResourceGroup', 'Location', 'PlanType')) {
$arguments[$name] = $infrastructureResult.$name
CYOT-Setup/stages/Deploy-CyotInfrastructure.ps1:18
- This validation allows leading, trailing, and consecutive hyphens, but the Bicep template embeds
environmentNameinto the Key Vault name (...-${environmentName}-kv-...), where those inputs create invalid names such ascyot-prod--kv-.... The guided Bicep path therefore accepts the value and then fails during deployment; constrain the discriminator to start/end with an alphanumeric and avoid consecutive separators.
[ValidatePattern('^[a-z0-9-]{2,12}$')]
[string] $EnvironmentName = 'prod',
CYOT-Setup/stages/Deploy-CyotInfrastructure.ps1:110
- In Bicep mode,
-NonInteractiveskips the only deployment confirmation and immediately creates or updates the resource group, Function, Key Vault, role assignments, and related resources. This contradicts the setup's documented resource-creation approval behavior and the claim that every new resource requires explicit approval. Add an explicit infrastructure approval switch or fail noninteractive mode instead of bypassing the prompt.
if (-not $NonInteractive) {
$confirmation = Read-Host "Deploy or update CYOT infrastructure in '$ResourceGroup' ($Location)? [y/N]"
if ($confirmation -notmatch '^(?i)y(?:es)?$') { throw 'Infrastructure deployment was cancelled.' }
}
CYOT-Setup/tests/Setup-Cyot.SmokeTests.ps1:52
- Stage 3 ignores
StatePathand always fetches live Graph metadata before it reachesConfirm-SetupAction. The stage's own comments say the current beta metadata does not exposecyot, so this invocation will fail with the schema error rather thanrequires -ApprovePolicyActivation; the assertion is currently false and depends on the live service. Isolate the approval check with a local stub or mocked supported schema instead of invoking live metadata.
$activation = Invoke-TestProcess -Arguments @('-Stage', 'Activate', '-NonInteractive', '-StatePath', $activationStatePath)
Test-Condition 'Noninteractive activation requires explicit approval' `
($activation.ExitCode -ne 0 -and $activation.Output -match 'requires -ApprovePolicyActivation') $activation.Output
- Files reviewed: 14/15 changed files
- Comments generated: 11
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+362
to
+365
| if ($State.Contains('graphSchemaSupported') -and -not $State.graphSchemaSupported) { | ||
| Write-CyotEvent -Level WARN -Message 'Activation skipped because the validated public Graph schema does not expose CYOT.' | ||
| return | ||
| } |
| StorageAccountName = $outputs.storageAccountName.value | ||
| KeyVaultName = $outputs.keyVaultName.value | ||
| } | ||
| *** End Patch No newline at end of file |
Comment on lines
+1645
to
+1648
| Invoke-EndpointGraph { | ||
| Update-MgApplication -ApplicationId $application.Id ` | ||
| -KeyCredentials (@($currentKeys) + $keyCredential) ` | ||
| -TokenEncryptionKeyId $keyId -ErrorAction Stop |
Comment on lines
+1731
to
+1733
| $secretArguments = @('keyvault', 'secret', 'set', | ||
| '--vault-name', $KeyVaultName, '--name', $secretName, '--value', $secretValue, | ||
| '--query', 'id', '--output', 'tsv', '--only-show-errors') |
| -Role 'Monitoring Metrics Publisher' -Scope $insightsId | ||
|
|
||
| Invoke-Az rest --method patch --url "${insightsId}?api-version=2020-02-02" ` | ||
| --body '{\"properties\":{\"DisableLocalAuth\":true}}' ` |
| $Name = Read-SetupValue -Name ProviderName -DefaultValue $Name -Required -ValueType Choice -Choices $SupportedProviders | ||
| } | ||
| else { $Name = Read-SetupValue -Name ProviderName -DefaultValue $Name -Required } | ||
| $Endpoint = Read-SetupValue -Name ProviderEndpoint -DefaultValue $Endpoint -Required -ValueType Url |
Comment on lines
+1083
to
+1088
| $ProviderTenantId = Read-SetupValue -Name ProviderTenantId -DefaultValue $ProviderTenantId -Required -ValueType Guid | ||
| $ProviderScope = Read-SetupValue -Name ProviderScope -DefaultValue $ProviderScope -Required -ValueType Scope | ||
| return @{ | ||
| EPP_PROVIDER_AUTH_MODE = 'ests' | ||
| EPP_PROVIDER_TENANT_ID = $ProviderTenantId | ||
| EPP_PROVIDER_SCOPE = $ProviderScope |
Comment on lines
+1495
to
+1499
| $uri = [System.Uri]::new($EndpointUrl) | ||
|
|
||
| if ($uri.Scheme -ne 'https') { | ||
| throw "The endpoint must use HTTPS. Got '$($uri.Scheme)'." | ||
| } |
Comment on lines
+1532
to
+1533
| $certificate = [System.Security.Cryptography.X509Certificates.X509Certificate2]::new($CertificatePath) | ||
| Write-Host " Using : $CertificatePath" |
Comment on lines
+1941
to
+1949
| $sourceUrl = if ($ZipUrl) { | ||
| Read-SetupValue -Name ZipUrl -DefaultValue $ZipUrl -ValueType HttpsUrl -Secret | ||
| } | ||
| elseif ($ReferencePackageUrl -notmatch '[<>]') { | ||
| $ReferencePackageUrl | ||
| } | ||
| else { | ||
| $null | ||
| } |
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
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.
Adds a guided, resumable CYOT external phone provider setup package with validation, diagnostics, Bicep provisioning, and approval-gated policy activation.