Scope asset API responses to the assets a user may read - #72682
Conversation
a40b290 to
3d4e0b6
Compare
Multi-team deployments isolate Dags, connections, variables and pools per team, but any caller could still list every asset. Asset names and URIs commonly encode bucket, table or dataset names, so a team could read the whole data catalog of every other team. Auth managers also had no way to express such a rule: asset authorization only ever received the numeric id, never the name or uri the rule needs to decide on.
Unspecced mocks accept any attribute and any call signature, so a rename or signature change in the auth manager leaves these tests passing while the code under test is broken. The surrounding tests already spec their mocks; these new ones were the outliers.
The 403 path belongs to _requires_access, which predates this change and is already covered elsewhere. Keeping a copy of it here spends CI time on logic this branch does not touch, and would keep passing if every line this branch adds were reverted.
Scoping the asset endpoints makes the auth manager decide on every asset rather than once per request. Both of these managers answer each decision with a remote call, so inheriting the per-asset default would turn a single asset listing into one round trip per asset. They already batch the same way for connections, pools and variables.
FAB grants asset access at the resource level, so its is_authorized_asset ignores the asset it is handed and returns the same answer every time. Inheriting the per-asset default would spend a permission-list scan per asset to recompute one boolean. It already short-circuits the same way for connections, pools and variables.
The provider tests build AssetDetails with name and uri, which only exist from Airflow 3.4, so they broke the older versions amazon and keycloak still support. Two others proved less than they claimed: the AWS one stubbed out the response matching it was meant to exercise, and the Keycloak cache key carried a resource prefix whose stated reason -- collision with another resource's entry -- cannot happen, since every sibling key is a longer tuple.
3d4e0b6 to
8b1d48e
Compare
|
This scopes the asset collection endpoints to the assets the caller may read,
It does not extend to endpoints that return an asset inside a response which is not an asset collection. Those still authorize on the coarse
Each needs its own call on where the filter belongs, and I would rather settle the shared filter and the auth manager interface first. Happy to fold any of them in if reviewers would prefer this land as one change. |
vincbeck
left a comment
There was a problem hiding this comment.
Assets are global to the environment. They do not belong to any team.
Thanks for taking a look, and that's a fair point you're right that assets are global and don't belong to any team. I should clarify that this PR doesn't try to change that; it doesn't introduce any team-to-asset ownership. What it's really after is authorization granularity. Right now the asset list,events endpoints run a single coarse My thinking was to mirror what connections, pools and variables already do: those are global too, yet their list endpoints are still scoped to what the caller may read, and assets were the one exception with no way to express that. That said, I may well be missing context here would you prefer that we not scope global assets at all, or is it more about how the filtering is done? Happy to adjust either way. |
Oh I see now! Alright, let me take a look :) |
|
Should this go into 3.4 or a 3.3.x patch release? |
|
Definitely 3.4 |
Multi-team deployments isolate Dags, connections, variables and pools per team, but any caller could still list every asset. Asset names and URIs commonly encode bucket, table or dataset names, so a team could read the whole data catalog of every other team. Auth managers also had no way to express such a rule: asset authorization only ever received the numeric id, never the name or uri the rule needs to decide on.
Multi-team deployments isolate Dags, connections, variables and pools per team, but any caller could still list every asset. Asset names and URIs commonly encode bucket, table or dataset names, so a team could read the whole data catalog of every other team. Auth managers also had no way to express such a rule: asset authorization only ever received the numeric id, never the name or uri the rule needs to decide on.
Why
GET /api/v2/assets,GET /ui/assetsandGET /api/v2/assets/eventsrun one coarseis_authorized_assetcheck and then return every row. In a multi-team deployment every caller can therefore see every other team's asset names and URIs, which commonly encode bucket, table or dataset names. Dags, connections, pools and variables already scope their list endpoints to what the caller may read; assets had no equivalent.Auth managers also had no way to express such a rule:
AssetDetailsonly carried the numericid, so a manager that wants to authorize by URI prefix or name had nothing to decide on.What
Core:
AssetDetailsgainsnameanduri.BaseAuthManagergainsget_authorized_assetsandfilter_authorized_assets, following the existing connection pattern. The default callsis_authorized_assetper asset, so a manager that ignores asset details keeps today's behaviour.security.pyaddsPermittedAssetFilter,PermittedAssetEventByAssetFilterandpermitted_asset_filter_factory. The two asset list endpoints and the asset events endpoint are scoped with them, so both the rows andtotal_entriesreflect only readable assets. Events whose asset row no longer exists stay visible, since there is no name or uri left to authorize on.requires_access_assetresolves the asset's name and uri (via a newAssetModel.get_name_and_uri) before callingis_authorized_asset, so single-asset routes can be authorized by URI or name too.filter_authorized_assetsamong the methods recommended to override.Providers, so the per-asset default does not turn one listing into one remote call per asset:
AwsAuthManager.filter_authorized_assetssends a single batched AVP request.KeycloakAuthManager.filter_authorized_assetsparallelises the checks over its request pool behindsingle_flight, keyed on user, method and asset ids.FabAuthManager.get_authorized_assetsreturns every asset id from one query, since FAB grants asset access at the resource level.Tests cover the new auth manager methods, the filters and factory, the enriched
requires_access_asset, the three endpoints, and the provider overrides. Provider tests that buildAssetDetailswithname/uriare gated on Airflow 3.4+. Query-count assertions on the touched endpoints go up by one.closes: #72333
Was generative AI tooling used to co-author this PR?