[pull] dev from KelvinTegelaar:dev#1100
Merged
Merged
Conversation
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>
…ching (#6347) The drift report failed to detect Intune configuration deviations because Get-CIPPDrift.ps1 compared template and tenant policy names using `.displayName -eq .displayName` alone. When both sides had no `.displayName` property (common for Graph policy types that only expose `.name`), this evaluated `$null -eq $null` as true, causing every such tenant policy to be incorrectly matched against the first Intune template in the loop and suppressing real drift. The fix restores all four original name/displayName pairings (displayName<->displayName, name<->name, displayName<->name, name<->displayName), with each pairing individually null-guarded so a match only counts when both sides have a real, non-null value. This is a strict superset of the prior matching capability - it only excludes the null-eq-null case that caused the false positive - so Settings Catalog and other name-only policy types deployed from a template continue to match correctly on name<->name. Adds Tests/Reports/Get-CIPPDrift.Tests.ps1, a new Pester suite (16 tests) covering: - the original null-eq-null false-match bug - each of the four name/displayName pairing combinations - the Settings Catalog name<->name regression path - multi-template alignment loops (matching a later template when earlier ones don't match) - Conditional Access template matching - standards deviation display-name/description resolution - stale drift-entity pruning Fixes KelvinTegelaar/CIPP#6347
Remove the scheduled SharePointSharingLinks cache task from Push-CIPPDBCacheData and leave it as an ad-hoc-only operation, since full sharing-link enumeration can run too long on large tenants.
…ching (#6347) (#2143) ## What Fixes drift report failing to detect Intune configuration deviations (#6347). `Get-CIPPDrift.ps1` matched an Intune template to a tenant policy using `.displayName -eq .displayName` only. Graph policy types that expose only a `.name` property (no `.displayName`) evaluated `$null -eq $null` as `true`, so every such tenant policy silently matched the first template in the loop regardless of actual content - suppressing real drift detection entirely for those policy types. ## Fix Restores all four original name/displayName pairings used before the regression was introduced, each individually null-guarded so a match only counts when both sides have a real, non-null value: - `displayName <-> displayName` - `name <-> name` - `displayName <-> name` - `name <-> displayName` This is a strict superset of the prior matching capability - the only behavior removed is the null-eq-null false match. Settings Catalog and other name-only policy types deployed from a template still match correctly via `name <-> name`. ## Tests Adds `Tests/Reports/Get-CIPPDrift.Tests.ps1`, a new Pester suite (16 tests) covering: - the original null-eq-null false-match bug - each of the four name/displayName pairing combinations - the Settings Catalog name<->name matching path - multi-template alignment loops (matching a later template when earlier ones don't match) - Conditional Access template matching - standards deviation display-name/description resolution - stale drift-entity pruning All 16 tests pass locally via `Invoke-Pester -Path Tests\Reports\Get-CIPPDrift.Tests.ps1`. Fixes KelvinTegelaar/CIPP#6347
…#2141) Resolves KelvinTegelaar/CIPP#6349, "[Feature Request]: Auto-create NinjaOne vulnerability scan group during CVE sync". ## Summary 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, 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) 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 #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).
Refactor Set-CIPPUser to use a shared helper that decides whether group membership changes should use Exchange cmdlets based on `addedFields.calculatedGroupType`, with fallback to legacy `groupType` values. This fixes inconsistent add/remove behavior caused by the previous inline condition and variable-case mismatch, ensuring distribution lists and mail-enabled security groups are handled consistently.
Signed-off-by: KelvinTegelaar <49186168+KelvinTegelaar@users.noreply.github.com>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )