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

Fix error when tenant has no licenses #1145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jeff-Jerousek
Copy link

@Jeff-Jerousek Jeff-Jerousek commented Jun 5, 2024

🗣 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 schrolla added the bug This issue or pull request addresses broken functionality label Jun 5, 2024
@buidav buidav added the public-reported This issue is reported by the public users of the tool. label Jun 5, 2024
Copy link
Collaborator

@gdasher gdasher left a 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
}
Copy link
Collaborator

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.

Copy link
Author

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.

@schrolla
Copy link
Collaborator

schrolla commented Jun 24, 2024

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

@schrolla schrolla added this to the Backlog milestone Jun 24, 2024
@tkol2022
Copy link
Collaborator

@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
Copy link
Collaborator

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

image

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.

image

image

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

image

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":  ,
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality public-reported This issue is reported by the public users of the tool.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants