Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Group Policy enforced include/exclude lists not being imported corrected #580

Merged
merged 7 commits into from
Apr 15, 2024

Conversation

nickjisc
Copy link
Contributor

@nickjisc nickjisc commented Mar 6, 2024

Proposed Changes

Fix for Group Policy enforced include/exclude lists not being imported corrected.

Previously the GPO allow/block list wasn't getting imported correctly, (empty array), this fixes that by using native powershell registry calls

Fix Get-ExcludedApps where GPO is in use to properly generate the array
Updated Get-IncludedApps to work correctly with GPO source.
add log for excluded / included apps
$AppIDs = [Microsoft.Win32.Registry]::GetValue($Key, $ValueName, $false)
[PSCustomObject]@{
Value = $ValueName
Data = $AppIDs.Trim()
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't we keep the Trim control?

$AppIDs = [Microsoft.Win32.Registry]::GetValue($Key, $ValueName, $false)
[PSCustomObject]@{
Value = $ValueName
Data = $AppIDs.Trim()
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't we keep the Trim control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add that back in and updated the PR

Copy link
Owner

@Romanitho Romanitho left a comment

Choose a reason for hiding this comment

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

Another thing 😇

@@ -245,7 +245,8 @@ if (Test-Network) {
New-Item "$WorkingDir\logs\error.txt" -Value "Whitelist doesn't exist in GPO" -Force
Exit 1
}
$toUpdate = $toUpdate.Data
$toUpdate = $toUpdate
Copy link
Owner

Choose a reason for hiding this comment

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

This could be removed ?

@@ -254,7 +255,8 @@ if (Test-Network) {
New-Item "$WorkingDir\logs\error.txt" -Value "Blacklist doesn't exist in GPO" -Force
Exit 1
}
$toSkip = $toSkip.Data
$toSkip = $toSkip
Copy link
Owner

Choose a reason for hiding this comment

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

Same ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep that'd be the same, I've removed that whole line now

@AndrewDemski-ad-gmail-com
Copy link
Contributor

I went through different path..
Maybe it s worth a look
#586

@nickjisc
Copy link
Contributor Author

Added the uri fetching. It does seem odd you are creating custom objects (key value pair) then ignoring those in the main code base, question if that is needed or you just need an array of the items, which is what I'm doing.

I'm mainly focused on the excluded apps section, as the original code wasn't fully working. New code does seem happy though it can always be slightly optimized

@AndrewDemski-ad-gmail-com
Copy link
Contributor

@Romanitho Romanitho merged commit eb52fe4 into Romanitho:main Apr 15, 2024
3 checks passed
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.

3 participants