[Bug Fix] GR1 & Gr3 - list the UPNs having non-enabled MFA #89
Conversation
There was a problem hiding this comment.
The methods that you are checking for in your new code
#microsoft.graph.passwordAuthenticationMethod - This is users password and doesnt qualify as a second auth method for MFA
#microsoft.graph.emailAuthenticationMethod - This is users email used to reset user password and doesnt qualify as second auth method for MFA
The two methods above should not be checked for MFA as they don't represent the multi-factor that we are looking for hence why the old code didn't include them. We should check for authenticator app or phone authentication since theyre used as second auth method
If we use the new logic in the pr, then any user without authenticator app and phone number will be able to pass the compliancy test if they have a password and email used to reset
|
@alalvi00 As you mentioned yesterday, you have #microsoft.graph.passwordAuthenticationMethod and #microsoft.graph.AuthenticatorAuthenticationMethod and both are enabled for MFA. This logic goes by that perspective. |
Thats fine but a user will also pass if they only have password and email and I have tested this in the test tenant |
|
Just for the reference in localexecution this is what I see for $comments and the ComplianceStatus is also false. Thanks for your testing and giving reviews. I will discuss the logic in tomorrow's meeting and thereafter finalize the PR Testcase 1 : Testcase 2 :
|
| $GAUPNsMFA += $GAUPNtemplate | ||
| } | ||
| } | ||
| # Condition: when user's authentication methods > 1 |
There was a problem hiding this comment.
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
Overview/Summary
This pull request fixes the reporting bug in 163gccspm for GR1 & Gr3 GA MFA UPN list feature.
This PR fixes/adds/changes/removes
In total, there should be 4 types of authentication methods to consider:
This PR has the fix for the MFA authentication by including two more authentication methods that users may have. Since dev tenant users did not have password and email authentication methods, this issue was not prominent in that tenant. However, the users in Test tenant carries these MFA. This pull request has the fix for this use case.
Testing Evidence
As part of this Pull Request I have
mainbranch