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

SA-3176: New/Set-JCPolicy Import Reg Files #501

Merged
merged 76 commits into from
Jul 31, 2023

Conversation

gweinjc
Copy link
Contributor

@gweinjc gweinjc commented Jun 21, 2023

Issues

  • SA-3176 - New/Set-JCPolicy Import Reg Files

What does this solve?

This feature will allow admins to upload .reg files into new or existing Windows - Advanced: Custom Registry Keys Policy JumpCloud Policies to deploy to their Windows machines.

Is there anything particularly tricky?

Currently, Windows - Advanced: Custom Registry Keys Policy only accepts registry keys that belong in the HKEY_LOCAL_MACHINE (HKLM) hive. If a .reg file is attempted to be uploaded that contains a non-HKLM key, it will not be valid.

How should this be tested?

  1. Download the sample .reg file and unzip
    i. SampleRegFile.reg.zip

  2. Run the following function:
    ii. New-JCPolicy -TemplateName windows_Advanced:_Custom_Registry_Keys -Name "Test - Registry File Upload" -registryFile "/path/to/SampleRegFile.reg"

  3. Validate that the "Test - Registry File Upload" Policy was successfully created and contains the expected registry values

  4. Run the following function:
    iv. Set-JCPolicy -PolicyName "Test - Registry File Upload" -registryFile "/path/to/SampleRegFile.reg"

  5. Validate that the "Test - Registry File Upload" Policy now has 10 values instead of the original 5

  6. Run the following function to replace the existing values of the policy with just the values from the SampleRegFile:
    iv. Set-JCPolicy -PolicyName "Test - Registry File Upload" -registryFile "/path/to/SampleRegFile.reg" -registryOverwrite

  7. Validate that the "Test - Registry File Upload" Policy now has the 5 values from the file

Copy link
Contributor

@jworkmanjc jworkmanjc left a comment

Choose a reason for hiding this comment

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

There's a few comments I've left we at least need to fix the dynamic parameter regression. We should add tests to validate that each type of accepted registry data is returned correctly and double check that the dword values are returned as expected

@@ -12,7 +12,7 @@
RootModule = 'JumpCloud.psm1'

# Version number of this module.
ModuleVersion = '2.5.1'
ModuleVersion = '2.6.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Module version should be 2.7.0 when we release this new functionality

if ([String]::IsNullOrEmpty($templateObject.defaultName)) {
throw "Could not find policy template by ID"
}
if ($PSCmdlet.ParameterSetName -eq "Standard") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are breaking our previous functionality here. I know it's not something we have a test for because it's only behavior you'd get out of manually using the module.

On the published version of the module if you type

new-JCPolicy -TemplateName darwin_MDM_Custom_Configuration_Profile -name "soemthing" -*TAB_KEY*

where you press the tab key, the list of dynamic parameters are generated, in the current version, this only happens once we drop into the standard set of parameters which would mean suppling a fake value for the values param. That parameter set won't work with the current set of parameters.

if ($valueObject[1].StartsWith("dword:")) {
#DWORD
$customRegType = "DWORD"
$customData = ($valueObject[1]).Substring(5)
Copy link
Contributor

Choose a reason for hiding this comment

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

[Int]"0x($valueObject[1]).Substring(6)"

@@ -122,6 +124,21 @@ Accept pipeline input: False
Accept wildcard characters: False
```

### -registryFile
Copy link
Contributor

Choose a reason for hiding this comment

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

Coming back to this, we did update this param to be dynamic in New-JCPolicy.ps1, we should do the same for Set-JCPolicy.ps1.

@@ -20,6 +20,10 @@ function New-JCPolicy {
HelpMessage = 'The values object either built manually or passed in through Get-JCPolicy')]
[System.object[]]
$Values
# [Parameter(Mandatory = $false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we do the work to update Set-JCPolicy, let's remove this set of comments

@@ -22,7 +22,10 @@ function Set-JCPolicy {
[Parameter(ValueFromPipelineByPropertyName = $true,
HelpMessage = 'The values object either built manually or passed in through Get-JCPolicy')]
[System.object[]]
$Values
$Values,
Copy link
Contributor

Choose a reason for hiding this comment

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

specifically this param should be dynamic not available for all policies.

Copy link
Contributor

@jworkmanjc jworkmanjc left a comment

Choose a reason for hiding this comment

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

Let's plan to make Set-JCPolicy like New-JCPolicy in the sense that the reg file param should only be available for registry policies. Since I've requested that change and I helped do the work for New-JCPolicy I can do that in the morning.

Copy link
Contributor

@kmaranionjc kmaranionjc left a comment

Choose a reason for hiding this comment

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

Tests and code looks good. Great work

Copy link
Contributor

@jworkmanjc jworkmanjc left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for all the changes, this was a big one, let's change our merge target to a release branch instead of main branch and we'll merge the changes to fix the AWS EC2 tests.

@jworkmanjc jworkmanjc changed the base branch from master to JumpCloudModule_2.7.0 July 20, 2023 20:33
@jworkmanjc jworkmanjc merged commit b403314 into JumpCloudModule_2.7.0 Jul 31, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants