Add a warning when resources with explicit permissions defined as app resources#4876
Merged
andrewnester merged 5 commits intomainfrom Apr 9, 2026
Merged
Add a warning when resources with explicit permissions defined as app resources#4876andrewnester merged 5 commits intomainfrom
andrewnester merged 5 commits intomainfrom
Conversation
andrewnester
requested review from
anton-107,
denik,
pietern,
shreyas-goenka and
simonfaltum
as code owners
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
andrewnester
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: c5c8514
13 interesting tests: 12 SKIP, 1 RECOVERED
Top 6 slowest tests (at least 2 minutes):
|
Collaborator
|
Commit: c5c8514
19 interesting tests: 10 SKIP, 7 KNOWN, 2 FAIL
Top 19 slowest tests (at least 2 minutes):
|
andrewnester
had a problem deploying
to
test-trigger-is
GitHub Actions
Failure
andrewnester
had a problem deploying
to
test-trigger-is
GitHub Actions
Failure
janniklasrose
approved these changes
Apr 9, 2026
| // hasAppSPInPermissions checks if any permission entry for the given resource | ||
| // references the app's service principal via variable interpolation. | ||
| func hasAppSPInPermissions(b *bundle.Bundle, resourcePath, appKey string) bool { | ||
| appSPRef := fmt.Sprintf("${resources.apps.%s.service_principal_client_id}", appKey) |
Contributor
There was a problem hiding this comment.
maybe an unlikely case, but if someone specifies the client id directly instead of by reference we'd still show the warning right?
Contributor
Author
There was a problem hiding this comment.
We will indeed, but to be honest this is not recommended way, as the app can be recretaed and SP ID will change, so better to use the reference
Approved (maintainer-authored PR)See OWNERS for ownership rules. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Add a warning when resources with explicit permissions defined as app resources
Why
When an app has resources (jobs, SQL warehouses, serving endpoints, experiments, Postgres projects, or UC securables) defined in its resources section that reference bundle-configured resources with explicit permissions, the app's service principal gets implicitly granted access on first deploy. However, on subsequent deploys the bundle overwrites the resource's permissions without the app's SP, breaking the app's access.
This PR adds a warning during bundle validate when a referenced resource has permissions set but doesn't include the app's service principal. The warning recommends explicitly adding the SP to the resource's permissions to prevent this silent permission override.
See #4309 for details.
Fixes #4309
Tests
Added an acceptance test