feat(ninjaone): auto-create CVE vulnerability scan group when missing#5
Open
Logan Cook (MWG-Logan) wants to merge 1 commit into
Open
feat(ninjaone): auto-create CVE vulnerability scan group when missing#5Logan Cook (MWG-Logan) wants to merge 1 commit into
Logan Cook (MWG-Logan) wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the NinjaOne tenant sync CVE upload flow by extracting scan-group resolution into a dedicated helper that can auto-create the configured CVE vulnerability scan group when it doesn’t already exist, and adds Pester coverage around the new behavior and the overall sync flow.
Changes:
- Added
Resolve-NinjaOneCveScanGrouphelper to lookup (and when missing, create) the CVE scan group for a tenant. - Updated
Invoke-NinjaOneTenantSyncto use the helper instead of inline scan-group lookup logic. - Added Pester suites covering scan-group resolution and a broad set of tenant-sync scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Tests/NinjaOne/Resolve-NinjaOneCveScanGroup.Tests.ps1 | Adds focused unit coverage for scan-group lookup/create behavior and headers. |
| Tests/NinjaOne/Invoke-NinjaOneTenantSync.Tests.ps1 | Adds broader end-to-end + edge-case coverage for the tenant sync function, including CVE sync integration. |
| Modules/CippExtensions/Public/NinjaOne/Invoke-NinjaOneTenantSync.ps1 | Switches CVE scan-group lookup to the new helper. |
| Modules/CippExtensions/Private/NinjaOne/Resolve-NinjaOneCveScanGroup.ps1 | Implements scan-group resolution and auto-creation logic. |
Logan Cook (MWG-Logan)
force-pushed
the
feat/ninjaone-cve-autocreate-scan-group
branch
3 times, most recently
from
July 14, 2026 18:07
456d8fb to
838f17c
Compare
Resolves KelvinTegelaar/CIPP#6349, "[Feature Request]: Auto-create NinjaOne vulnerability scan group during CVE sync". Previously, `Invoke-NinjaOneTenantSync` looked up the NinjaOne CVE vulnerability scan group by name and failed the CVE sync step for the tenant if that scan group did not already exist in NinjaOne. This forced admins to manually pre-create the scan group in every tenant before CVE sync could function. - Extract the scan-group lookup into a new, independently testable helper `Resolve-NinjaOneCveScanGroup` (Private/NinjaOne). It now: - Looks up the scan group by configured name. - Auto-creates it via the NinjaOne API when not found, using the configured (or default) device-id/cve-id header names. - Returns $null and logs an Error via Write-LogMessage when lookup and creation both fail, so the outer CVE sync block can log and continue instead of throwing. - Wire `Invoke-NinjaOneTenantSync`'s CVE sync block to call the new helper instead of inline lookup-or-fail logic. - Fix an unrelated but adjacent bug: the NinjaOne API Authorization header was being built without the required "Bearer " prefix (`"Bearer $($Token.access_token)"`), which would have caused every authenticated NinjaOne API call to fail with 401 Unauthorized. - Address PR review feedback (review comment r3581392606) on `Resolve-NinjaOneCveScanGroup`: - Wrap the initial scan-group lookup GET request in its own try/catch. Previously an uncaught exception here (e.g. 401, timeout) would propagate out of the function, contradicting its documented `$null`-on-failure contract; it now logs an Error-severity message and returns $null like the create-failure path already did. - Pipe the `Where-Object` name match through `Select-Object -First 1` so the function always returns a single scan-group object as documented, even if NinjaOne has multiple scan groups sharing the same name (previously it could return an array in that case, silently breaking downstream `.id`/`.deviceIdHeader`/`.cveIdHeader` access and upload-URI construction in the caller). Tests: - Add `Tests/NinjaOne/Resolve-NinjaOneCveScanGroup.Tests.ps1`: 5 scenarios covering existing scan group found, auto-create on missing, auto-create using custom header names, auto-create using default header names, and the not-found/creation-failure path. Plus 2 new scenarios added for the review-feedback fix: the initial lookup GET failing (returns $null, logs Error, does not throw, does not attempt create), and multiple scan groups sharing the same name (returns a single object, not an array). - Add `Tests/NinjaOne/Invoke-NinjaOneTenantSync.Tests.ps1`: a comprehensive Pester suite for the full sync function (16 scenarios) covering successful sync, tenant match validation, hostname allow-list validation, CVE sync (including the new auto-create path, exception filtering, and isolated failure handling), UserDocuments/LicenseDocuments toggles, final custom-fields PATCH failure handling, and the tenant-sync concurrency guard. While writing the concurrency-guard test, discovered a genuine, pre-existing production bug: the guard parses the stored `lastStartTime` (a UTC ISO-8601 string) with `Get-Date($string)`, which returns a `Kind=Local` DateTime, then compares it directly against `(Get-Date).ToUniversalTime()` (`Kind=Utc`). .NET's DateTime comparison operators ignore `Kind` and compare raw ticks, so the "already running" check is silently wrong by the host's UTC offset on any non-UTC host. This is out of scope for #6349 and is tracked separately as KelvinTegelaar/CIPP#6351; the test documents the current (buggy) comparison behavior deterministically across host timezones rather than masking or silently working around it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Logan Cook (MWG-Logan)
force-pushed
the
feat/ninjaone-cve-autocreate-scan-group
branch
from
July 14, 2026 18:11
838f17c to
24db2c5
Compare
Logan Cook (MWG-Logan)
changed the title
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.
Resolves KelvinTegelaar/CIPP#6349, "[Feature Request]: Auto-create NinjaOne vulnerability scan group during CVE sync".
Summary
Previously,
Invoke-NinjaOneTenantSynclooked up the NinjaOne CVE vulnerability scan group by name and failed the CVE sync step for the tenant if that scan group did not already exist in NinjaOne. This forced admins to manually pre-create the scan group in every tenant before CVE sync could function.Resolve-NinjaOneCveScanGroup(Private/NinjaOne). It now:$nulland logs an Error viaWrite-LogMessagewhen lookup and creation both fail, so the outer CVE sync block can log and continue instead of throwing.Invoke-NinjaOneTenantSync's CVE sync block to call the new helper instead of inline lookup-or-fail logic.Bearerprefix, which would have caused every authenticated NinjaOne API call to fail with 401 Unauthorized.Tests
Tests/NinjaOne/Resolve-NinjaOneCveScanGroup.Tests.ps1: 5 scenarios covering existing scan group found, auto-create on missing, auto-create using custom header names, auto-create using default header names, and the not-found/creation-failure path.Tests/NinjaOne/Invoke-NinjaOneTenantSync.Tests.ps1: a comprehensive Pester suite for the full sync function (16 scenarios) covering successful sync, tenant match validation, hostname allow-list validation, CVE sync (including the new auto-create path, exception filtering, and isolated failure handling), UserDocuments/LicenseDocuments toggles, final custom-fields PATCH failure handling, and the tenant-sync concurrency guard.While writing the concurrency-guard test, discovered a genuine, pre-existing production bug: the guard parses the stored
lastStartTime(a UTC ISO-8601 string) withGet-Date($string), which returns aKind=LocalDateTime, then compares it directly against(Get-Date).ToUniversalTime()(Kind=Utc). .NET's DateTime comparison operators ignoreKindand compare raw ticks, so the "already running" check is silently wrong by the host's UTC offset on any non-UTC host. This is out of scope for #6349 and is tracked separately as Invoke-NinjaOneTenantSync: concurrency "already running" guard fails to detect running sync on non-UTC hosts #4; the test documents the current (buggy) comparison behavior deterministically across host timezones rather than masking or silently working around it.Both suites pass 100% (5/5 and 16/16). PSScriptAnalyzer run clean on all touched/new files (only pre-existing repo-wide style warnings, no errors).