Bugfix/issue 1176: Filter application ownership check to servicePrincipalType 'Application' only#1191
Bugfix/issue 1176: Filter application ownership check to servicePrincipalType 'Application' only#1191sandeepjha000 wants to merge 2 commits intodevfrom
Conversation
…p Check Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #1176 by excluding managed identities (and any non-Application service principals) from the “insufficient owners” checks used by the enterprise application ownership assessments.
Changes:
- Extend the
Get-ApplicationsWithPermissionsDB query to includeservicePrincipalType. - Filter
Get-ApplicationsWithInsufficientOwnersresults toservicePrincipalType = 'Application'before reporting insufficient owners.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/powershell/private/tests-shared/Get-ApplicationsWithPermissions.ps1 | Adds servicePrincipalType to the ServicePrincipal query output so downstream filters can distinguish application vs managed identity. |
| src/powershell/private/tests-shared/Get-ApplicationsWithInsufficientOwners.ps1 | Filters the ownership check to only include service principals of type Application. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…mance Co-authored-by: Copilot <copilot@github.com>
alexandair
left a comment
There was a problem hiding this comment.
@sandeepjha000 Please, address my feedback.
Out-of-scope for this PR but worth noting: Test-21770 also targets "applications"
Test-Assessment.21770.ps1 calls Get-ApplicationsWithPermissions -Database $Database with no type filter. The 21770 spec also targets enterprise applications via the /applications endpoint, so managed identities are likely false positives there too. Consider opening a follow-up issue to apply the same filter to 21770 (out of scope for this PR per the issue title).
| sp.owners, sp.signInAudience, sp.servicePrincipalType | ||
| from main.ServicePrincipal sp | ||
| left join main.ServicePrincipalSignIn spsi on spsi.appId = sp.appId | ||
| where sp.id in |
There was a problem hiding this comment.
Pre-filter is post-DB, not in SQL (minor — already raised by Copilot reviewer)
The filter runs in PowerShell after the full result set is materialized. The DB query still returns all ServicePrincipal rows including managed identities. On large tenants, pushing this into SQL avoids loading rows that will be discarded:
where sp.servicePrincipalType in ('Application', ...)
and (sp.id in (...) or sp.id in (...))This is the change the existing comment ("Filter the retrieved applications…") would actually justify. Not blocking, but worth doing while the file is open. The two prior Copilot review threads on this file are already resolved, so I'd raise it as a follow-up rather than block.
| $Database, | ||
|
|
||
| [Parameter()] | ||
| [string[]]$ServicePrincipalType |
There was a problem hiding this comment.
Missing ValidateSet on the new parameter
[Parameter()]
[string[]]$ServicePrincipalTypeGraph's servicePrincipalType is a closed enum (Application, ManagedIdentity, Legacy, SocialIdp, LegacyUpdatedApp). Adding [ValidateSet(...)] makes typos (e.g. 'application' lowercase, which would silently match nothing because the comparison is case-sensitive on the DB column value) fail fast.
Filter application ownership check to servicePrincipalType 'Application' only
Fixes #1176