Hidden dependency on Organization.Read.All when using cert thumbprint or app secret

[ricmestre]

When performing an export with certificate thumbprint, cert path or app secret then function Start-M365DSCConfigurationExtract calls Get-M365DSCTenantDomain, this function then if not using cert path, so in this case cert thumbprint or app secret, it calls Get-MgOrganization to determine the onmicrosoft.com of the tenant, which is already known at that point but might not be the default domain of the organization.

That being said, what is the rationale behind this when for instance all other authentication methods just have an if condition to check if TenantId contains 'onmicrosoft' (M365DSCUtil.psm1:1506)?

The reason I'm asking this is because calling Get-MgOrganization creates an hidden dependency on needing Organization.Read.All, for all apps/workloads, which won't be added automatically to Get-M365DSCCompilerPermissionList and even though the export works anyway it will create a log file which reports exactly that Organization.Read.All is missing in the app(s).

Maybe there is a reason for doing it this way but if that's a case then it should at least be documented so feedback on this is much appreciated, I might send a diff depending on the reply.

[andikrueger]

Just reviewed the code and we are already making sure, the TenantId does match the .onmicrosoft.com format. This might be a relic of old times. We introduced the validation lately. I think, we could change the part in M365DSCReverse

Microsoft365DSC/Modules/Microsoft365DSC/Modules/M365DSCReverse.psm1

Lines 245 to 261 in e2073d1

	if ($AuthMethods -contains 'CertificateThumbprint' -or `
	$AuthMethods -contains 'CertificatePath' -or `
	$AuthMethods -contains 'ApplicationWithSecret')
	{
	$AppSecretAsPSCredential = $null
	if (-not [System.String]::IsNullOrEmpty($ApplicationSecret))
	{
	[SecureString]$secStringPassword = ConvertTo-SecureString $ApplicationSecret -AsPlainText -Force
	[PSCredential]$AppSecretAsPSCredential = New-Object System.Management.Automation.PSCredential ('ApplicationSecret', $secStringPassword)
	}
	$organization = Get-M365DSCTenantDomain -ApplicationId $ApplicationId `
	-TenantId $TenantId `
	-CertificateThumbprint $CertificateThumbprint `
	-ApplicationSecret $AppSecretAsPSCredential `
	-CertificatePath $CertificatePath
	}

to just call any further if it is not the .onmicrosoft.XX domain.

[ricmestre]

Something like this?

diff --git a/Modules/Microsoft365DSC/Modules/M365DSCReverse.psm1 b/Modules/Microsoft365DSC/Modules/M365DSCReverse.psm1
index 8fb4a49f5..026972a16 100644
--- a/Modules/Microsoft365DSC/Modules/M365DSCReverse.psm1
+++ b/Modules/Microsoft365DSC/Modules/M365DSCReverse.psm1
@@ -246,18 +246,25 @@ function Start-M365DSCConfigurationExtract
                 $AuthMethods -contains 'CertificatePath' -or `
                 $AuthMethods -contains 'ApplicationWithSecret')
         {
-            $AppSecretAsPSCredential = $null
-            if (-not [System.String]::IsNullOrEmpty($ApplicationSecret))
+            if ($TenantId.Contains('onmicrosoft'))
             {
-                [SecureString]$secStringPassword = ConvertTo-SecureString $ApplicationSecret -AsPlainText -Force
-                [PSCredential]$AppSecretAsPSCredential = New-Object System.Management.Automation.PSCredential ('ApplicationSecret', $secStringPassword)
+                $organization = $TenantId
             }
+            else
+            {
+	            $AppSecretAsPSCredential = $null
+	            if (-not [System.String]::IsNullOrEmpty($ApplicationSecret))
+	            {
+	                [SecureString]$secStringPassword = ConvertTo-SecureString $ApplicationSecret -AsPlainText -Force
+	                [PSCredential]$AppSecretAsPSCredential = New-Object System.Management.Automation.PSCredential ('ApplicationSecret', $secStringPassword)
+	            }
 
-            $organization = Get-M365DSCTenantDomain -ApplicationId $ApplicationId `
-                -TenantId $TenantId `
-                -CertificateThumbprint $CertificateThumbprint `
-                -ApplicationSecret $AppSecretAsPSCredential `
-                -CertificatePath $CertificatePath
+	            $organization = Get-M365DSCTenantDomain -ApplicationId $ApplicationId `
+	                -TenantId $TenantId `
+	                -CertificateThumbprint $CertificateThumbprint `
+	                -ApplicationSecret $AppSecretAsPSCredential `
+	                -CertificatePath $CertificatePath
+            }
         }
         elseif ($AuthMethods -Contains 'Credentials' -or `
                 $AuthMethods -Contains 'CredentialsWithApplicationId')

[andikrueger]

Did we already have this kind of code??? With which PR got the code changed?

[ricmestre]

It seems to have been changed with this commit 798fdb7 by @ykuijs

[andikrueger]

@ykuijs could you have a look at this one? Do you remember what caused this change?

[ricmestre]

Any updates on this one?

[NikCharlebois]

Sorry folks, I am having a hard time following the discussion here. Even if the user passes TenantId as *.onmicrosoft.com, we still need to call into Get-MgOrganization to make sure the tenant is the default one ($_.IsInitial). I believe the logic flow is correct in that sence.

[andikrueger]

At the moment, not every resource has the scope “ Organization.Read.All ” within the settings file - as it it not needed for the resource. It is only a requirement for authentication in certain scenarios. With a change of the code flow, we could mitigate the need to have the Organization.Read.All scope for all resources and only if the users passes the *.on Microsoft.com TenantId. In any other case, we need to have the scope for all resources as authentication requires it.

[ricmestre]

So what I'm trying to say here is that the permission is hidden because it's not in the settings of the resources because as Andi says is not required, but in some auth scenarios it is but it's not mentioned in the docs therefore either the code needs to be changed or it should be in the docs.

[NikCharlebois]

Would adding the permission by default to the Get-M365DSCCompiledPermissionList cmdlet address this issue?

[ricmestre]

If it's really necessary to call up Get-MgOrganization when using certificate, with regular credentials it's not required, then yes adding the permission to the list would solve this.

[andikrueger]

Yes, it would. Some how like a static return value. We should check if it is already part of the retuned values and only add it, if the user tries to get scopes for workloads that would require the information.