You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vincbeck (via GitHub)" <gi...@apache.org> on 2023/11/28 15:11:25 UTC
[PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
vincbeck opened a new pull request, #35928:
URL: https://github.com/apache/airflow/pull/35928
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1408263195
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -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
Review Comment:
If the section is None then is this basically checking if the user is authorized for all/any config?
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -142,7 +166,12 @@ def is_authorized_view(
access_view: AccessView,
user: BaseUser | None = None,
) -> bool:
- return self.is_logged_in()
+ return self.avp_facade.is_authorized(
+ method="GET",
+ entity_type=AvpEntities.VIEW,
+ user=user or self.get_user(),
+ entity_id=access_view.value,
Review Comment:
Why don't we have a check for this value and default to None like the others?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1408520363
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -142,7 +166,12 @@ def is_authorized_view(
access_view: AccessView,
user: BaseUser | None = None,
) -> bool:
- return self.is_logged_in()
+ return self.avp_facade.is_authorized(
+ method="GET",
+ entity_type=AvpEntities.VIEW,
+ user=user or self.get_user(),
+ entity_id=access_view.value,
Review Comment:
`access_view` is a required field
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1409690314
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -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
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1409505876
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -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
Review 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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #35928:
URL: https://github.com/apache/airflow/pull/35928
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1409492309
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -142,7 +166,12 @@ def is_authorized_view(
access_view: AccessView,
user: BaseUser | None = None,
) -> bool:
- return self.is_logged_in()
+ return self.avp_facade.is_authorized(
+ method="GET",
+ entity_type=AvpEntities.VIEW,
+ user=user or self.get_user(),
+ entity_id=access_view.value,
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "o-nikolas (via GitHub)" <gi...@apache.org>.
o-nikolas commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1408550555
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -142,7 +166,12 @@ def is_authorized_view(
access_view: AccessView,
user: BaseUser | None = None,
) -> bool:
- return self.is_logged_in()
+ return self.avp_facade.is_authorized(
+ method="GET",
+ entity_type=AvpEntities.VIEW,
+ user=user or self.get_user(),
+ entity_id=access_view.value,
Review Comment:
Why is this one required but not the others? I think maybe the answer to my other comment might answer this as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1409501281
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -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
Review 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AWS auth manager: implement all `is_authorized_*` methods (but `is_authorized_dag`) [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #35928:
URL: https://github.com/apache/airflow/pull/35928#discussion_r1409510501
##########
airflow/providers/amazon/aws/auth_manager/aws_auth_manager.py:
##########
@@ -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
Review Comment:
@shubham22. Related to the discussion we had
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org