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

Update-M365DSCAzureAdApplication with AppOwnerTenantID Filter #2566

Closed
wants to merge 2 commits into from

Conversation

andikrueger
Copy link
Collaborator

Pull Request (PR) description

Added addtional filter for AppOwernTenantId in Update-M365DSCAzureAdApplication

This Pull Request (PR) fixes the following issues

$graphSvcprincipal = Get-AzADServicePrincipal | Where-Object -FilterScript { $_.DisplayName -eq "Microsoft Graph" }
$spSvcprincipal = Get-AzADServicePrincipal | Where-Object -FilterScript { $_.DisplayName -eq "Office 365 SharePoint Online" }
$exSvcprincipal = Get-AzADServicePrincipal | Where-Object -FilterScript { $_.DisplayName -eq "Office 365 Exchange Online" }
$graphSvcprincipal = Get-AzADServicePrincipal | Where-Object -FilterScript { $_.DisplayName -eq 'Microsoft Graph' -and $_.AppOwnerTenantId -eq 'f8cdef31-a31e-4b4a-93e4-5f571e91255a' }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we doing this, and also, it looks like the property is appOwnerOrganizationId and not appOwnerTenantId
image
Will this owner id be the same in foreign clouds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point.

The display name is not unique across all tenants. At the moment, any other application with the same name could be the first and we would try to add permissions to this app.

Do you have any insight on other clouds, if the IDs are the same?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not at the moment. I'll need to ask around and investigate

Copy link
Contributor

Choose a reason for hiding this comment

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

Get-AzureADServicePrincipal -> appOwnerTenantId
Get-AzADServicePrincipal/Get-MgServicePrincipal -> AppOwnerOrganizationId

f8cdef31-a31e-4b4a-93e4-5f571e91255a is the 1st party tenant for commercial and usgov, can't speak to other dedicated clouds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

…what a mess. I’ll look into moving to MG methods tomorrow. Maybe there is another criteria available to make sure the apps are published by Microsoft.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. For the sake of efficiency, I'd provide the displayname as part of the query rather than filter on the complete results set - the difference in query time can be significant when there are a large number of SPs in the tenant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just had another thought: How does PnP PowerShell do it?

This is the answer: https://github.com/pnp/powershell/blob/ed883559841bb89e8bb08515d0fdfed4f69faf17/src/Commands/Model/PermissionScopes.cs#L11-L13

        public static string ResourceAppId_Graph = "00000003-0000-0000-c000-000000000000";
        public static string ResourceAppId_SPO = "00000003-0000-0ff1-ce00-000000000000";
        public static string ResourceAppID_O365Management = "c5393580-f805-4401-95e8-94b7a6ef2fc2";

Maybe we can go with fixed IDs within our code as well and just warn, if we do not find a thing. As soon as we do not find any app, we could switch back to the old "query".

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking about that as well, create a table of well-known GUIDs and call it a day.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That works

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will update accordingly.

@andikrueger
Copy link
Collaborator Author

Will create a new PR to add the points that were discussed above.

andikrueger added a commit to andikrueger/Microsoft365DSC that referenced this pull request Feb 14, 2023
NikCharlebois added a commit that referenced this pull request Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update-M365DSCAzureAdApplication could grant permissions to non-MS service application
3 participants