Skip to content

Fix template enabled-state assignment#273

Open
SamErde wants to merge 1 commit intomainfrom
samerde/fix-template-enabled-state
Open

Fix template enabled-state assignment#273
SamErde wants to merge 1 commit intomainfrom
samerde/fix-template-enabled-state

Conversation

@SamErde
Copy link
Copy Markdown
Collaborator

@SamErde SamErde commented May 6, 2026

Summary

Move Add-Member calls for Enabled and EnabledOn outside the inner foreach ($ca ...) loop in Set-AdditionalTemplateProperty, so both properties are computed across all enrollment services before being assigned to the template object.

Problem

In the original code, Add-Member was called on every iteration of the CA loop — once per enrollment service per template — writing intermediate state to the template object before all CAs had been checked:

foreach ($ca in ($ADCSObjects | Where-Object objectClass -EQ 'pKIEnrollmentService')) {
    if ($ca.certificateTemplates -contains $template.Name) {
        $Enabled = $true
        $EnabledOn += $ca.Name
    }

    # BUG: called inside loop — fires on every CA, not just at the end
    $template | Add-Member -NotePropertyName Enabled -NotePropertyValue $Enabled -Force
    $template | Add-Member -NotePropertyName EnabledOn -NotePropertyValue $EnabledOn -Force
}

This causes two issues:

  1. Performance: Add-Member is invoked O(n_cas) times per template instead of once.
  2. Correctness: The template object receives partial/intermediate values during the loop. Any code that reads Enabled/EnabledOn concurrently (or after an early-exit scenario) could see incorrect state.

Fix

Move both Add-Member calls to after the loop completes:

foreach ($ca in ($ADCSObjects | Where-Object objectClass -EQ 'pKIEnrollmentService')) {
    if ($ca.certificateTemplates -contains $template.Name) {
        $Enabled = $true
        $EnabledOn += $ca.Name
    }
}
# Assigned once, after all CAs have been checked
$template | Add-Member -NotePropertyName Enabled -NotePropertyValue $Enabled -Force
$template | Add-Member -NotePropertyName EnabledOn -NotePropertyValue $EnabledOn -Force

Files Changed

  • Private/Set-AdditionalTemplateProperty.ps1 — source fix
  • Invoke-Locksmith.ps1 — generated artifact updated in parity with the build convention (copied from Artefacts/Script/ by Build/Build-Module.ps1)

Validation

  • PSScriptAnalyzer: no warnings or errors on the changed file
  • No existing behavior changed — object mutation and output are preserved; only the assignment timing and call count change

Move Add-Member calls for Enabled and EnabledOn outside the inner
foreach-ca loop so that both properties are computed across all
enrollment services before being assigned to the template object.
Previously, Add-Member was called on every CA iteration, writing
intermediate (potentially incomplete) state to the template object
and performing O(n_cas) unnecessary property assignments per template.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SamErde SamErde marked this pull request as ready for review May 6, 2026 09:24
Copilot AI review requested due to automatic review settings May 6, 2026 09:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes how the Enabled / EnabledOn properties are assigned in Set-AdditionalTemplateProperty by ensuring they’re computed across all enrollment services first, then attached to the template object once per template (instead of once per CA iteration). This improves both correctness (no intermediate/partial state written mid-loop) and performance (fewer Add-Member calls).

Changes:

  • Move Add-Member for Enabled and EnabledOn outside the inner CA foreach loop so assignment happens once per template.
  • Update the generated Invoke-Locksmith.ps1 artifact to mirror the source change.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
Private/Set-AdditionalTemplateProperty.ps1 Assigns Enabled/EnabledOn after evaluating all CAs for a template (avoids per-CA intermediate writes).
Invoke-Locksmith.ps1 Updates the generated script artifact to match the fixed assignment timing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@SamErde SamErde marked this pull request as draft May 6, 2026 09:27
@SamErde
Copy link
Copy Markdown
Collaborator Author

SamErde commented May 6, 2026

Copilot review analysis (automated):

The GitHub Copilot automatic review (4234840282) reviewed all 2 changed files and generated no inline comments or suggestions. The review correctly characterizes the fix: \Add-Member\ calls moved outside the inner CA loop so \Enabled/\EnabledOn\ are assigned once per template after all enrollment services are evaluated.

Classification: ✅ No actionable items — review confirms the implementation is correct as-is.

Nothing to implement or disregard. No follow-up changes needed from this review cycle.

@SamErde SamErde marked this pull request as ready for review May 6, 2026 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants