AIP 84 - Add auth for asset alias

[rawwar]

related to #42360

[rawwar]

@vincbeck , Since we are now exposing AssetAlias, both AWS auth manager and FAB need to implement is_authorized_asset_alias. Can I add them separately in a different PR or should I covert them here?

[rawwar]

"Get alias" and "Get aliases" just show Id, name and group. No details regarding an asset. So, I think there's no need to check if the user has access to the asset referred by these aliases.

wdyt, @Lee-W , @uranusjr

[vincbeck]

@vincbeck , Since we are now exposing AssetAlias, both AWS auth manager and FAB need to implement is_authorized_asset_alias. Can I add them separately in a different PR or should I covert them here?

You have to, otherwise mypy will not be happy

[vincbeck]

It looks good. Could you please add unit tests?

[rawwar]

failed tests do not seem to be related to the changes in this PR.

@vincbeck , can you please retrigger just the failed boto test?

[vincbeck]

failed tests do not seem to be related to the changes in this PR.

@vincbeck , can you please retrigger just the failed boto test?

#47512 is the fix

[uranusjr]

"Get alias" and "Get aliases" just show Id, name and group. No details regarding an asset. So, I think there's no need to check if the user has access to the asset referred by these aliases.

I agree with Get Alias(es) do not need to check asset perm. However, we probably should check alias perms in Get Asset(s) since it shows a list of aliases an asset is associated to. The list should only contain aliases the user has access to.

(This brings up the question if the alias responses should also include a list of associated assets, but that’s a question for another day, and perm check should be added to it if that’s implemented.)

[Lee-W]

"Get alias" and "Get aliases" just show Id, name and group. No details regarding an asset. So, I think there's no need to check if the user has access to the asset referred by these aliases.

I agree with Get Alias(es) do not need to check asset perm. However, we probably should check alias perms in Get Asset(s) since it shows a list of aliases an asset is associated to. The list should only contain aliases the user has access to.

(This brings up the question if the alias responses should also include a list of associated assets, but that’s a question for another day, and perm check should be added to it if that’s implemented.)

Agree. I think the idea is to ensure that users only access the objects they have permissions for. For the asset endpoint, if we return its alias, we should check the alias permission and vice versa.

[pierrejeambrun]

If I understand correctly that is good from an asset_alias endpoints point of views (TP + Wei) as well as from the AuthManager permissions (vincent).

The only thing missing is to add AssetAlias permission check on Asset endpoints ? (I think we can do that in a followup PR, this one focus on asset aliases and it seems to be completed ?)

[Lee-W]

Yep, this PR only get asset alias itself without resolving it into assets.