Dedupe $ProductNames

[twneale]

🗣 Description

Remove duplicate items from the $ProductNames list to avoid running checks repeatedly.

Environment: Powershell 7 on Ubuntu 22.04.3 LTS

💭 Motivation and context

Resolves #171

Including the same product twice in $ProductNames runs the checks multiple times unecessarily.

🧪 Testing

I ran Invoke-SCuBA -ProductNames aad,aad,aad,aad,aad,aad ... before and after the change to observe whether it authenticated multiple times.

✅ Pre-approval checklist

• This PR has an informative and human-readable title.
• Changes are limited to a single goal - eschew scope creep!
• All future TODOs are captured in issues, which are referenced
  in code comments.
• All relevant type-of-change labels have been added.
• I have read the CONTRIBUTING document.
• These code changes follow cisagov code standards.
• All relevant repo and/or project documentation has been updated
  to reflect the changes in this PR.
• Tests have been added and/or modified to cover the changes in this PR.
• All new and existing tests pass.

✅ Pre-merge checklist

• Revert dependencies to default branches.
• Finalize version.

✅ Post-merge checklist

• Create a release.

[schrolla]

@twneale Successfully migrated from flipper to main branch with rebase. If you have a local checkout be sure to do a git pull --rebase to get the latest version in your local repo.

[twneale]

@schrolla I'm trying to figure out "Tests have been added and/or modified to cover the changes in this PR." Since I didn't add any new lines of code, there's no need for additional code coverage, but I could modify a file like ./Testing/Functional/Auto/MinimumTest.txt to add a duplicate entry to one of the command lists. But I don't think there is an economical way to actually check whether the orchestrator repeats the provider export multiple times, at least not without adding a lot of additional code to keep track of that info. I ran it locally and confirmed it works as expected, just not sure how to proceed with this particular checkbox.

[schrolla]

@schrolla I'm trying to figure out "Tests have been added and/or modified to cover the changes in this PR." Since I didn't add any new lines of code, there's no need for additional code coverage, but I could modify a file like ./Testing/Functional/Auto/MinimumTest.txt to add a duplicate entry to one of the command lists. But I don't think there is an economical way to actually check whether the orchestrator repeats the provider export multiple times, at least not without adding a lot of additional code to keep track of that info. I ran it locally and confirmed it works as expected, just not sure how to proceed with this particular checkbox.

How about a unit test that tries to run with duplicate products specified with an expectation that it will fail? Could be done with a new PS unit pester test. That way, the new test validates the result of the code change works as intended and we have code coverage for the change to prevent a regression.

[crutchfield]

@schrolla I'm trying to figure out "Tests have been added and/or modified to cover the changes in this PR." Since I didn't add any new lines of code, there's no need for additional code coverage, but I could modify a file like ./Testing/Functional/Auto/MinimumTest.txt to add a duplicate entry to one of the command lists. But I don't think there is an economical way to actually check whether the orchestrator repeats the provider export multiple times, at least not without adding a lot of additional code to keep track of that info. I ran it locally and confirmed it works as expected, just not sure how to proceed with this particular checkbox.

How about a unit test that tries to run with duplicate products specified with an expectation that it will fail? Could be done with a new PS unit pester test. That way, the new test validates the result of the code change works as intended and we have code coverage for the change to prevent a regression.

I'd suggest adding to Invoke-Scuba.Tests.ps1. Mock out Invoke-ReportCreation and see that a unique list is passed even if dupes are originally passed into InvokeScuba.

[twneale]

I added some tests to verify that it works. I tried the approach @crutchfield suggested and added once for the case of ScubaConfig as well.

[Dylan-MITRE]

Ran Invoke-SCuBA -ProductNames in different variation and order, such as:

• aad, aad
• aad, aad, teams
• aad, aad, teams, teams
• teams, exo, defender, aad, sharepoint, teams
  all works as expected.

Invoke-Scuba.Tests.ps1 works as well

ScubaConfigJsonDuplicateProviderTests.ps1 however give me an error when i run it

[twneale]

Thank you for running it @Dylan-MITRE. My test is messed up. I will take another swing at it.

[crutchfield]

Maybe we should fix at least ResetInstance/ClearConfiguration as part of this PR. Otherwise I believe testing is more difficult.

[twneale]

This shouldn't be merged yet. I discovered the in powershell, sort-object -unique returns an array only if the sorted result contains more than two elements. If it only contains one element, it returns a string 🤦‍♂️. So despite passing tests, my changes actually return ProductNames as a string which I'm not sure is correct. I should probably confirm with @crutchfield whether this is what's expected when there's only one product name.

[schrolla]

@twneale Is this actually blocked, or did you add blocked to prevent merge? If the latter, recommend setting the PR to draft state until updates can be made and removing the blocked label instead. Draft PRs can't be merged, blocked ones, technically speaking, can.

[twneale]

@schrolla I think this is ready for review. Overall, I don't think the actual code changes truly justify the volume of tests and general controversy this PR created, so I am equally fine just closing it without merging.

[nanda-katikaneni]

Tested with duplicate product names - ScubaGear now runs for each product once even if its duplicated in the productname parameter. Looks good to me.

[ahuynhMITRE]

tested with a couple iterations of duplicate products with no issues. Nothing further from me.

[nanda-katikaneni]

@twneale can you please rebase the branch and also resolve some of earlier comments (not sure if they are addressed), thanks.