Fix error when tenant has no licenses

Jeff-Jerousek commented 3 months ago

🗣 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

[ ] This PR has an informative and human-readable title.
[ ] PR targets the correct parent branch (e.g., main or release-name) for merge.
[ ] Changes are limited to a single goal - eschew scope creep!
[ ] Changes are sized such that they do not touch excessive number of files.
[ ] All future TODOs are captured in issues, which are referenced in code comments.
[ ] These code changes follow the ScubaGear content style guide.
[ ] Related issues these changes resolve are linked preferably via closing keywords.
[ ] All relevant type-of-change labels added.
[ ] All relevant project fields are set.
[ ] All relevant repo and/or project documentation updated to reflect these changes.
[ ] Unit tests added/updated to cover PowerShell and Rego changes.
[ ] Functional tests added/updated to cover PowerShell and Rego changes.
[ ] All relevant functional tests passed.
[ ] All automated checks (e.g., linting, static analysis, unit/smoke tests) passed.

✅ 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

[ ] Feature branch deleted after merge to clean up repository.
[ ] Verified that all checks pass on parent branch (e.g., main or release-name) after merge.

schrolla commented 3 months ago

May conflict with other MS Graph related updates and needs to be reviewed against them before merging.

tkol2022 commented 1 month ago

@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.

tkol2022 commented 1 month ago

@Jeff-Jerousek @gdasher @schrolla I prototyped the solution provided in Grant's comment which was as follows:

$LicenseInfo = ConvertTo-Json -Depth 3 @($SubscribedSku | Select-Object -Property Sku*, ConsumedUnits, PrepaidUnits)

I simulated the condition where $SubscribedSku could be in either of the states below: State 1: $SubscribedSku = @()

State 2: $SubscribedSku = $null

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.

$LicenseInfo = ConvertTo-Json -Depth 3 @($SubscribedSku | Select-Object -Property Sku*, ConsumedUnits, PrepaidUnits)

# I commented the outer "if/else" statement below that was examining $ServicePlans.

# if ($ServicePlans) {
    # ********** The next three lines are NEW code.
    $RequiredServicePlan = $null
    if ($ServicePlans) {
        $RequiredServicePlan = $ServicePlans | Where-Object -Property ServicePlanName -eq -Value "AAD_PREMIUM_P2"
    }

    if ($RequiredServicePlan) {
        # If the tenant has the premium license then we want to also include PIM Eligible role assignments - otherwise we don't to avoid an API error
        $PrivilegedUsers = $Tracker.TryCommand("Get-PrivilegedUser", @{"TenantHasPremiumLicense"=$true; "M365Environment"=$M365Environment})
    }
    else{
        $PrivilegedUsers = $Tracker.TryCommand("Get-PrivilegedUser", @{"TenantHasPremiumLicense"=$false; "M365Environment"=$M365Environment})
    }
    $PrivilegedUsers = $PrivilegedUsers | ConvertTo-Json
    $PrivilegedUsers = if ($null -eq $PrivilegedUsers) {"{}"} else {$PrivilegedUsers}

    if ($RequiredServicePlan){
        # If the tenant has the premium license then we want to also include PIM Eligible role assignments - otherwise we don't to avoid an API error
        $PrivilegedRoles = $Tracker.TryCommand("Get-PrivilegedRole", @{"TenantHasPremiumLicense"=$true; "M365Environment"=$M365Environment})
    }
    else {
        $PrivilegedRoles = $Tracker.TryCommand("Get-PrivilegedRole", @{"TenantHasPremiumLicense"=$false; "M365Environment"=$M365Environment})
    }
    $PrivilegedRoles = ConvertTo-Json -Depth 10 @($PrivilegedRoles) # Depth required to get policy rule object details
# }
# else {
#     Write-Warning "Omitting calls to Get-PrivilegedRole and Get-PrivilegedUser."
#     $PrivilegedUsers = ConvertTo-Json @()
#     $PrivilegedRoles = ConvertTo-Json @()
#     $Tracker.AddUnSuccessfulCommand("Get-PrivilegedRole")
#     $Tracker.AddUnSuccessfulCommand("Get-PrivilegedUser")
# }

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".

cisagov / ScubaGear