[Fixed] Parse GPO ignoring when registry policy name is not Registry.pol

[dangerousplay]

Hello, I hope you are good.

Currently, if the registry policy name file is not exactly Registry.pol the AD parse GPO does not find it, ignoring it.
However, there are cases where the registry file name is registry.pol for a GPO as reported in issue #1098.
In this pull request the implementation lists the files in the policy class directory and looks for the registry.pol file ignoring the case. If the file is found it's parsed, otherwise the policy is ignored.

Solves the issue:
Policy files in all lower case are not applied. #1098

Have a good day ^^

[didrocks]

Thanks a lot for this other excellent PR! I’m quite impressed that you were able to dive into the code and get there quite easily, well done!

I have some suggestions that I think will enhance the code from a maintainability perspectice. Some parts of my reviews may be wrong (because the github diff due to the identation changed as you rightly refactor some things into its own function) made it hard to read. I had to rely then on git diff directly to be able to have a better sense, ignoring those, but I may still have made some mistakes in my review!

Also, we do have most of the code under tests, and I think this needs a testcase at least in the ad package. Maybe even two (one to parse the a registry.pol and one for a mixed case like regiSTRY.pol).
I don’t think this necessitate integration tests though as this is more a property to handle some edge cases on internal/ad. I would suggest then to only add it here (you can easily run the tests with go test . inside that directory).

If the requested changes and/or the tests are too much for you, we can take it from there and finish the work you started, just tell us! Thanks again for your contribution! :)

[dangerousplay]

Thank you for the review.
The code was simplified to fail fast when an error occurs during the class directory list or Registry.pol read.

I would appreciate a help to create an automated test for this scenario. I'm trying to figure out how to change the test data to have different cases for the Registry.pol.

[didrocks]

Excellent! As you can see, I only have 2 very small nitpicks, but the code itself is completely what I was envisionning for. It’s so much more readabale with the early returns, thanks for tackling this!

On the tests, now, as you can see, existing tests still pass, so it means that there is no regression :)
I think we should add some new cases with a case of registry.pol and testing that it’s parsed.

In internal/ad/ad_test.go, as you can see, we have multiple tests doing parametric (table) testing. Look at TestGetPolicies and see what we have a section for uppercase/lowercase in the class (USER/Machine…): // Policy class directory spelling cases

This is the section where you can add new cases (probably changing the comment above), like the following:

"Lowercase registry file is parsed":  {
			gpoListArgs: []string{"gpoonly.com", "bob:lowercase-registry"},
			want:        policies.Policies{GPOs: []policies.GPO{standardUserGPO("standard")}},
			},
		},
"Mixed case registry file is parsed":  {
			gpoListArgs: []string{"gpoonly.com", "bob:mixedcase-registry"},
			want:        policies.Policies{GPOs: []policies.GPO{standardUserGPO("standard")}},
			},
		},

And the finale missing part, will be to create those 2 lowercase-registry and mixedcase-registry policies for the gpoonly.com domain!

We mock an AD file directory under internal/ad/testdata/AD/SYSVOL/gpoonly.com/Policies/. You should copy standard to both lowercase-registry and mixedcase-registry. That way, you have your GPO fixture used in tests with some policies inside.
Now, rename In the first our User/Registry.pol to User/registry.pol, and on the second one to something like User/reGIStry.pol. I would suggest remove in both the Machine/ directory itself as it’s of no use.

I know this is a little bit involved, but that should be all what is needed to implement those 2 additional tests and ensuring the code is then covered! How does that sound? If you need any help, I’m happy again to take it over to the finish line, your work and code is already excellent, Thanks again!

[dangerousplay]

I highly appreciate your detailed explanation. Thank you for the awesome support ^^

I removed the blank lines and added new tests covering the lower case and mixed case for both computer and user policies.

[didrocks]

This looks excellent! Thanks a lot for adding the tests and fixing the remaining comments. The new code is really aligned with the current style and code of adsys, well done to figure this out!

I’m going to merge your excellent contribution. Please note that we are already backporting one version of authd currently to 22.04 and 24.04 LTS. This will be in the next one and we will probably publish this in a ppa in between.

Thanks again, approving and merging now!

Note for myself: for some reasons, even on approved workflow, the environments variables (like the runners we are running the tests against) aren’t populated and this is why I had to run the tests manually. But I’m happy to confirm they all pass (and I did run code quality check manually too).