-
Notifications
You must be signed in to change notification settings - Fork 228
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 error when tenant has no licenses #1145
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the submission!
{$LicenseInfo = "[]"} | ||
else{ | ||
$LicenseInfo = $SubscribedSku | Select-Object -Property Sku*, ConsumedUnits, PrepaidUnits | ConvertTo-Json -Depth 3 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test this, but I think a cleaner solution would be something like:
$LicenseInfo = ConvertTo-Json -Depth 3 @($SubscribedSku | Select-Object -Property Sku*, ConsumedUnits, PrepaidUnits)
In the case where $SubscribedSku is null, this should reduce to an empty list. In the case where its not we should extract what we want.
Can you add a comment asking us to add a test case here: https://github.com/cisagov/ScubaGear/blob/main/PowerShell/ScubaGear/Testing/Unit/PowerShell/Providers/DefenderProvider/Export-DefenderProvider.Tests.ps1
It looks like the tests need some refactoring to support testing this situation with how we're mocking stuff up, so I won't ask for that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should work. I was just copying the original logic.
May conflict with other MS Graph related updates and needs to be reviewed against them before merging. |
@Jeff-Jerousek I wanted to thank you for this submission and your code. It is very helpful and we have been aware of the problem internally but haven't gotten the chance to address it yet. There is a previous issue #979 related to this although that issue doesn't really contain all the important information about the underlying topic, which is, what should ScubaGear do when it cannot locate any subscribed SKUs and/or service plans? Look at my newer comments for more info. |
@Jeff-Jerousek @gdasher @schrolla I prototyped the solution provided in Grant's comment which was as follows:
I simulated the condition where $SubscribedSku could be in either of the states below: State 2: This solves the problem of the licensing report crashing. Instead the AAD provider executes and the user sees an empty licensing table the report. No errors are observed at the command line. However if $SubscribedSku is either null or empty, the AAD provider will produce error messages in section 7 of the report. This is because of the logic directly following the initialization of the $LicenseInfo variable and specifically the else statement. I think this means that we need to address the broader question of how do we handle the scenario when ScubaGear cannot locate any subscribed SKUs and/or service plans instead of just producing errors in section 7 of the report? At a high level, I think we can still execute the AAD provider for the policies in section 7 (there are a few of them) that do not depend on the AAD_PREMIUM_P2 license. Below are some suggested code changes that I prototyped which will do that. My code includes your update to the way the $LicenseInfo is created. Take a look and if you think this is feasible I will talk to Addam on the best approach to get this change incorporated.
So if these changes are made, the AAD provider will still run the Get-PrivilegedRole and Get-PrivilegedUser and ensure that the first three policies in section 7 are evaluated instead of producing an error message in the report. All policies in the baseline (including the ones in section 2) that depend on the AAD_PREMIUM_P2 license will behave in the same manner that they behave today when we don't find that license, which is to produce a Fail in the report along with the message "NOTE: Your tenant does not have a Microsoft Entra ID P2 license, which is required for this feature". |
Running on a new tenant with no licenses in use Get-MgBetaSubscribedSku will return with a body of []. $LicenseInfo was not verifying $SubscribedSku had data before creating the variable that will be used in the report, resulting in a JSON primitive error because the JSON is "license_information": ,
76510d7
to
a22477f
Compare
🗣 Description
Running on a new tenant with no licenses in use Get-MgBetaSubscribedSku will return with a body of [].
💭 Motivation and context
$LicenseInfo was not verifying $SubscribedSku had data before creating the variable that will be used in the report, resulting in a JSON primitive error because the JSON is "license_information": ,
🧪 Testing
✅ Pre-approval checklist
✅ Pre-merge checklist
PR passed smoke test check.
Feature branch has been rebased against changes from parent branch, as needed
Use
Rebase branch
button below or use this reference to rebase from the command line.Resolved all merge conflicts on branch
Notified merge coordinator that PR is ready for merge via comment mention
✅ Post-merge checklist