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.

Sign up for GitHub

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

[Bug Fix] GR1 & Gr3 - list the UPNs having non-enabled MFA #89

Closed
wants to merge 4 commits into from
Closed

[Bug Fix] GR1 & Gr3 - list the UPNs having non-enabled MFA #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cea06f8
adding more authentication methods
dutt0 Jan 31, 2024
3388b4d
fixed typo in comments
dutt0 Jan 31, 2024
8f7ca43
adding updated module
dutt0 Jan 31, 2024
57788a9
adding/typo fix comments
dutt0 Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified psmodules/GR-Common.zip
View file
Open in desktop
Binary file not shown.
98 changes: 68 additions & 30 deletions src/Guardrails-Common/GR-Common.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,30 +396,61 @@ function Check-GAAuthenticationMethods {
$data = $response.Content
if ($null -ne $data -and $null -ne $data.value) {
$authenticationmethods = $data.value

# To check if MFA is setup for a user, we're looking for either :
# #microsoft.graph.microsoftAuthenticatorAuthenticationMethod or
# #microsoft.graph.phoneAuthenticationMethod
foreach ($authmeth in $authenticationmethods) {
if (($($authmeth.'@odata.type') -eq "#microsoft.graph.phoneAuthenticationMethod") -or `
($($authmeth.'@odata.type') -eq "#microsoft.graph.microsoftAuthenticatorAuthenticationMethod")) {
# need to keep track of each GA mfa in counter and compare it to count
$mfaCounter += 1
# found atleast one auth method so we move to the next UPN
break
}

# To check if MFA is setup for a user, we're looking for 4 types of authentication methods :
# 1. #microsoft.graph.microsoftAuthenticatorAuthenticationMethod
# 2. #microsoft.graph.phoneAuthenticationMethod
# 3. #microsoft.graph.passwordAuthenticationMethod
# 4. #microsoft.graph.emailAuthenticationMethod

# Condition: when there is only 1 authentication method found, usually MFA is not setup - it needs to be checked separately
if ($authenticationmethods.Count -eq 1){
if(($($authmeth.'@odata.type') -eq "#microsoft.graph.passwordAuthenticationMethod") ) {

# need to keep track of each GA mfa in counter and compare it to count
$mfaCounter += 1

}
else {
# This message will be used for debugging
Write-Host "Found only 1 authentication method with MFA not enabled"
# create an instance of inner list object
$GAUPNtemplate = [PSCustomObject]@{
UPN = $globalAdminAccount
MFAStatus = $false
MFAComments = $hiddenUPN

}
#add the list to GA MFA list
# add the list to GA MFA list
$GAUPNsMFA += $GAUPNtemplate
}
}
# Condition: when user's authentication methods > 1
Copy link
Contributor

@singhgss singhgss Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dutt0

We can make this code a little bit cleaner

$targetAuthMethodTypes = @(
    "#microsoft.graph.phoneAuthenticationMethod",
    "#microsoft.graph.microsoftAuthenticatorAuthenticationMethod",
    "#microsoft.graph.passwordAuthenticationMethod",
    "#microsoft.graph.emailAuthenticationMethod"
)

$GAUPNtemplate = [PSCustomObject]@{
    UPN        = $globalAdminAccount
    MFAStatus  = $false
    MFAComments = $hiddenUPN
}

foreach ($authmeth in $authenticationmethods) {
    if ($targetAuthMethodTypes -contains $($authmeth.'@odata.type')) {
        $mfaCounter += 1
        break
    } else {
        # Clone the template and add to the list
        $GAUPNsMFA += $GAUPNtemplate.psobject.Copy()
    }
}

I think this introduces a bug what happens if first authMeth that comes in the loop iteration is none of the 4 auth methods, wouldn't this go to else block and create a GAUPNTemplate list and then in the next iteration it finds the one of the 4 auth methods and it also increases mfacounter

So we end up posting message of mfa not enabled as well as showing message that it is enabled for that UPN

else{
foreach ($authmeth in $authenticationmethods) {
if (($($authmeth.'@odata.type') -eq "#microsoft.graph.phoneAuthenticationMethod") -or `
($($authmeth.'@odata.type') -eq "#microsoft.graph.microsoftAuthenticatorAuthenticationMethod") -or `
($($authmeth.'@odata.type') -eq "#microsoft.graph.passwordAuthenticationMethod") -or `
($($authmeth.'@odata.type') -eq "#microsoft.graph.emailAuthenticationMethod") ) {

# need to keep track of each GA mfa in counter and compare it to count
$mfaCounter += 1
# atleast one auth method is true - so we move to the next UPN
break
}
else {
# create an instance of inner list object
$GAUPNtemplate = [PSCustomObject]@{
UPN = $globalAdminAccount
MFAStatus = $false
MFAComments = $hiddenUPN

}
# add the list to GA MFA list
$GAUPNsMFA += $GAUPNtemplate
}
}
}
}
else {
$errorMsg = "No authentication methods data found for $globalAdminAccount"
Expand All @@ -436,34 +467,41 @@ function Check-GAAuthenticationMethods {

}

# GA UPN list has less than 2 UPN
# Condition: GA UPN list has less than 2 UPN
if ($globalAdminUPNs.Count -lt 2) {
$commentsArray += $msgTable.globalAdminMinAccnts
}
# GA UPN list has > 2 UPNs and all are MFA enabled
# Condition: GA UPN list has > 2 UPNs and all are MFA enabled
elseif($globalAdminUPNs.Count -ge 2 -and $mfaCounter -eq $globalAdminUPNs.Count) {
$commentsArray += $msgTable.globalAdminMFAPassAndMin2Accnts
$IsCompliant = $true
}
# GA UPN list has > 2 UPNs and not all UPNs are MFA enabled
# Condition: GA UPN list has > 2 UPNs and not all UPNs are MFA enabled
else{
# only one UPN is not MFA enable
if ( $GAUPNsMFA.Count -eq 1 ) {
$commentsArray += $msgTable.globalAdminAccntsMFADisabled1 -f $GAUPNsMFA.MFAComments
# This will be used for debugging
if($GAUPNsMFA.Count -lt 1){
Write-Host "Something is wrong as GAUPNsMFA Count equals 0. This output should only execute if there is an error populating GAUPNsMFA"
}
# None are MFA enabled
elseif ( $GAUPNsMFA.Count -eq $globalAdminUPNs.Count) {
$commentsArray += $msgTable.globalAdminAccntsMFADisabled3
}
# 2 or more UPNs in the list are not MFA enabled
else {
$hiddenUPNsString = ""
for ($i =0; $i -lt $GAUPNsMFA.Count; $i++) {
$hiddenUPNsString += $GAUPNsMFA[$i].MFAComments + ", "
else{
# only one UPN is not MFA enable
if ( $GAUPNsMFA.Count -eq 1 ) {
$commentsArray += $msgTable.globalAdminAccntsMFADisabled1 -f $GAUPNsMFA.MFAComments
}
# None are MFA enabled
elseif ( $GAUPNsMFA.Count -eq $globalAdminUPNs.Count) {
$commentsArray += $msgTable.globalAdminAccntsMFADisabled3
}
# 2 or more UPNs in the list are not MFA enabled
else {
$hiddenUPNsString = ""
for ($i =0; $i -lt $GAUPNsMFA.Count; $i++) {
$hiddenUPNsString += $GAUPNsMFA[$i].MFAComments + ", "
}
$hiddenUPNsString = $hiddenUPNsString.TrimEnd(', ')
$commentsArray += $msgTable.globalAdminAccntsMFADisabled2 -f $hiddenUPNsString
}
$hiddenUPNsString = $hiddenUPNsString.TrimEnd(', ')
$commentsArray += $msgTable.globalAdminAccntsMFADisabled2 -f $hiddenUPNsString
}


}

Expand Down
Loading