AWS auth manager: implement all is_authorized_* methods (but is_authorized_dag)
#35928
Conversation
boring-cyborg
bot
added
area:providers
provider:amazon
| @@ -91,7 +91,13 @@ def is_authorized_configuration( | |||
| details: ConfigurationDetails | None = None, | |||
| user: BaseUser | None = None, | |||
| ) -> bool: | |||
| return self.is_logged_in() | |||
| config_section = details.section if details else None | |||
There was a problem hiding this comment.
If the section is None then is this basically checking if the user is authorized for all/any config?
There was a problem hiding this comment.
It is checking if the user is authorized for all config. This is a really good question and this has been bugging me for a while. Does auth_manager.is_authorized_variable("GET") means:
- Does the user has permission to read all variable
- Does the user has permission to read any variable
I decided to go with the former because this is actually how Amazon Verified Permissions work. You cannot ask AVP to check whether the user is authorized to access ANY variable (or any type of resource type).
But this has some implications. Example: As a user I am allowed to access only variables starting with "tmp_" (thus not all variables), thus I wont be allowed to see the list of variables (or even the menu). The reason why the user would not be able to see the menu "Variables" or the list of variables is because to display those, we check whether the user has permissions to read all variables. So this is not perfect either (and we might change it) but I guess we can leave that interrogation for later/another PR
There was a problem hiding this comment.
But I dont know if this is something we should thing about because Airflow is not ready for such use cases. The views (this is the first time that comes to my mind) are displaying all resources from database without filtering (we could change that of course)
There was a problem hiding this comment.
@shubham22. Related to the discussion we had
There was a problem hiding this comment.
Good discussion, it sounds like we'll likely merge this either way so I'll approve since the code looks good otherwise. But please keep the discussion going to see where we net out on this topic.
| method="GET", | ||
| entity_type=AvpEntities.VIEW, | ||
| user=user or self.get_user(), | ||
| entity_id=access_view.value, |
There was a problem hiding this comment.
Why don't we have a check for this value and default to None like the others?
There was a problem hiding this comment.
access_view is a required field
There was a problem hiding this comment.
Why is this one required but not the others? I think maybe the answer to my other comment might answer this as well.
There was a problem hiding this comment.
The other are optional because when you do authorization request, you might not need to specify a resource ID. Example: "Does the user have permission to create a Variable". There is no resource ID here. Here this is not really a resource ID but more a specific of which view the user is trying to access. We must specify which view the user is trying to access
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.