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/08/02 19:48:23 UTC
[GitHub] [airflow] vincbeck opened a new pull request, #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
vincbeck opened a new pull request, #33044:
URL: https://github.com/apache/airflow/pull/33044
This PR is built on top of #[32987](https://github.com/apache/airflow/pull/32987). Please review only the last commit.
The intent of this PR is to move FAB related methods from security managers ([AirflowSecurityManager](https://github.com/apache/airflow/blob/main/airflow/www/security.py), [BaseSecurityManager](https://github.com/apache/airflow/blob/main/airflow/www/fab_security/manager.py) and [SecurityManager](https://github.com/apache/airflow/blob/main/airflow/www/fab_security/sqla/manager.py)) to `FabAirflowSecurityManagerOverride`. Since the effort is quite big, this PR is just a first step and more methods will be moved in separate PRs. This PR moves the most "obvious" methods to be moved. This will help to review the PR as well.
Also, in order to avoid having one gigantic `FabAirflowSecurityManagerOverride`, I decided to split it in different modules which `FabAirflowSecurityManagerOverride` inherits from. So far I created two modules: `DB` and `oauth`, each one defining methods related to either the database or Oauth.
There is no new code in this PR, the changes are only code being moved around.
<!--
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
[GitHub] [airflow] vincbeck commented on pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #33044:
URL: https://github.com/apache/airflow/pull/33044#issuecomment-1677406937
Totally understandable 👌 No worries @potiuk. I just resolved the conflicts. If you can review it, that would be awesome 🥳
--
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
[GitHub] [airflow] vincbeck merged pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #33044:
URL: https://github.com/apache/airflow/pull/33044
--
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
[GitHub] [airflow] vincbeck closed pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck closed pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
URL: https://github.com/apache/airflow/pull/33044
--
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
[GitHub] [airflow] vincbeck commented on a diff in pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33044:
URL: https://github.com/apache/airflow/pull/33044#discussion_r1282412931
##########
.pre-commit-config.yaml:
##########
@@ -491,6 +491,7 @@ repos:
exclude: >
(?x)
^airflow/api_connexion/openapi/v1.yaml$|
+ ^airflow/auth/managers/fab/security_manager/|
Review Comment:
It was: `^airflow/www/fab_security/manager.py$|`
--
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
[GitHub] [airflow] vincbeck commented on pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #33044:
URL: https://github.com/apache/airflow/pull/33044#issuecomment-1674945322
Can I have another review please? I am aware it is not the most interesting PR but this is needed. FYI: this is only moving code, there is no new code here, that should make the review easier :)
--
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
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33044:
URL: https://github.com/apache/airflow/pull/33044#discussion_r1282238697
##########
.pre-commit-config.yaml:
##########
@@ -491,6 +491,7 @@ repos:
exclude: >
(?x)
^airflow/api_connexion/openapi/v1.yaml$|
+ ^airflow/auth/managers/fab/security_manager/|
Review Comment:
why do we need this when the previous location was not in this list ?
--
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
[GitHub] [airflow] vandonr-amz commented on a diff in pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33044:
URL: https://github.com/apache/airflow/pull/33044#discussion_r1282414195
##########
.pre-commit-config.yaml:
##########
@@ -491,6 +491,7 @@ repos:
exclude: >
(?x)
^airflow/api_connexion/openapi/v1.yaml$|
+ ^airflow/auth/managers/fab/security_manager/|
Review Comment:
ah yes, I thought it was in alphabetical order and I missed it -_-
--
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
[GitHub] [airflow] potiuk commented on pull request #33044: Move methods from security managers to `FabAirflowSecurityManagerOverride`
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33044:
URL: https://github.com/apache/airflow/pull/33044#issuecomment-1676483664
I am sorry - I have just merged https://github.com/apache/airflow/pull/33347 which has been important to be out for 2.7.0, and this one conflicts with it so if you please rebase witht the chnage of mine and ping me, I will review it quickly (also now when I am more familiar with recent changes you've done). Sorry - but 2.7. has been priority over the last few days
--
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