You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "vandonr-amz (via GitHub)" <gi...@apache.org> on 2023/08/29 22:48:23 UTC
[GitHub] [airflow] vandonr-amz opened a new pull request, #33901: Fix inheritance chain in security manager
vandonr-amz opened a new pull request, #33901:
URL: https://github.com/apache/airflow/pull/33901
There is currently a weird situation with the security managers, where the `AirflowSecurityManager`, which is supposed to be the more generic construct, inherits from `FabAirflowSecurityManagerOverride`, supposed to be the more specific one. Because of this, `FabAirflowSecurityManagerOverride` doesn't bear well its name, because it cannot override anything from `AirflowSecurityManager` since it sits lower on the inheritance tree.
This was done initially to be able to _extend_ functionalities of the Security Manager, while retaining an existing feature which was that users could plug their own security manager inheriting from `AirflowSecurityManager` (so we couldn't squeeze the FAB one in between because that'd be a breaking change).
The solution I used here is to have an empty shell stay where the `AirflowSecurityManager` was, just to maintain back-compatibility. That one inherits from FAB Security Manager, which can now sit below the actual `AirflowSecurityManager`.
This means that users can only override the FAB security manager and not other auth provider's SM but that's ok because this feature is pretty much rendered obsolete by AIP-56. See https://github.com/apache/airflow/pull/33690#issuecomment-1696444150
Joining some class diagrams to help understanding.
Arrows read as "is inherited/implemented by".
current situation:
<img width="880" alt="image" src="https://github.com/apache/airflow/assets/114772123/86011ed5-2b24-49b7-8309-ac17ab923c51">
after:
<img width="1035" alt="image" src="https://github.com/apache/airflow/assets/114772123/7664887b-6c32-4059-95a9-cce92895a248">
I had to make the `ApplessAirflowSecurityManager` inherit from the "db" one instead of the generic airflow SM because it had some dependencies there. It's not ideal, and not what we want in a final state, but for now I tried to do the minimal change that'd land back on its feet in a working state, so that's that.
We'll sort that dependency later on.
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #33901:
URL: https://github.com/apache/airflow/pull/33901#issuecomment-1699663179
> > The idea of modules I introduced in `FabAirflowSecurityManagerOverride` was to avoid to build a big monster class very hard to maintain, so I split it in multiple modules.
>
> BTW, I'm wondering if this isn't causing more harm than good. A big file is not necessarily hard to maintain, and I'd argue it's actually harder to maintain code that's spread over 3 different files. Inheritance also comes with extra features that we don't want here. For instance, defining the same method/property/field in 2 files would result in one overriding the other rather than an error.
If you think this is better without these modules, let's go then
--
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] uranusjr commented on a diff in pull request #33901: Fix inheritance chain in security manager
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1315461451
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -36,8 +38,9 @@ class BaseAuthManager(LoggingMixin):
Auth managers are responsible for any user management related operation such as login, logout, authz, ...
"""
- def __init__(self):
+ def __init__(self, app: Flask):
Review Comment:
```suggestion
def __init__(self, app: Flask) -> None:
```
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #33901:
URL: https://github.com/apache/airflow/pull/33901
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1333196608
##########
airflow/www/security.py:
##########
@@ -16,785 +16,25 @@
# under the License.
from __future__ import annotations
-import warnings
-from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+from typing import TYPE_CHECKING
-from flask import g
-from sqlalchemy import or_, select
-from sqlalchemy.orm import joinedload
+from deprecated import deprecated
-from airflow.auth.managers.fab.models import Permission, Resource, Role, User
-from airflow.auth.managers.fab.views.permissions import (
- ActionModelView,
- PermissionPairModelView,
- ResourceModelView,
-)
-from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
-from airflow.auth.managers.fab.views.user import (
- CustomUserDBModelView,
- CustomUserLDAPModelView,
- CustomUserOAuthModelView,
- CustomUserOIDModelView,
- CustomUserRemoteUserModelView,
-)
-from airflow.auth.managers.fab.views.user_edit import (
- CustomResetMyPasswordView,
- CustomResetPasswordView,
- CustomUserInfoEditView,
-)
-from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
-from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
-from airflow.models import DagBag, DagModel
-from airflow.security import permissions
-from airflow.utils.log.logging_mixin import LoggingMixin
-from airflow.utils.session import NEW_SESSION, provide_session
-from airflow.www.extensions.init_auth_manager import get_auth_manager
-from airflow.www.fab_security.sqla.manager import SecurityManager
-from airflow.www.utils import CustomSQLAInterface
-
-EXISTING_ROLES = {
Review Comment:
Why would that be a breaking change? In which case user code would use this constant? The only use case I can think of is, if the user provide its own security manager and inside this security manager, they use this constant. But if we want to avoid such things ... every change will be breaking change and doing any change in security managers will be almost impossible.
Or maybe you had something else in mind?
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310613614
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
I was talking about the obsolete one :)
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310656075
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
I am good with that too. My understanding is `ApplessAirflowSecurityManager` is used mostly for testing purposes. We might want to make it inherit from `ApplessAirflowSecurityManager` for now and later remove this dependency somehow.
What we are trying to achieve with our work in security managers is to move everything that is FAB-auth related to FAB auth manager. What's left in security manager should be global to the application regardless of the auth manager used. The end goal is to have a working (or close to working) security manager with no override. The override should be an optional piece if you want to customize the default behavior. When we are there, then I guess we could make `ApplessAirflowSecurityManager` inherit from this security manager
--
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] pankajastro commented on a diff in pull request #33901: Fix inheritance chain in security manager
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1333246523
##########
airflow/www/security.py:
##########
@@ -16,785 +16,25 @@
# under the License.
from __future__ import annotations
-import warnings
-from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+from typing import TYPE_CHECKING
-from flask import g
-from sqlalchemy import or_, select
-from sqlalchemy.orm import joinedload
+from deprecated import deprecated
-from airflow.auth.managers.fab.models import Permission, Resource, Role, User
-from airflow.auth.managers.fab.views.permissions import (
- ActionModelView,
- PermissionPairModelView,
- ResourceModelView,
-)
-from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
-from airflow.auth.managers.fab.views.user import (
- CustomUserDBModelView,
- CustomUserLDAPModelView,
- CustomUserOAuthModelView,
- CustomUserOIDModelView,
- CustomUserRemoteUserModelView,
-)
-from airflow.auth.managers.fab.views.user_edit import (
- CustomResetMyPasswordView,
- CustomResetPasswordView,
- CustomUserInfoEditView,
-)
-from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
-from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
-from airflow.models import DagBag, DagModel
-from airflow.security import permissions
-from airflow.utils.log.logging_mixin import LoggingMixin
-from airflow.utils.session import NEW_SESSION, provide_session
-from airflow.www.extensions.init_auth_manager import get_auth_manager
-from airflow.www.fab_security.sqla.manager import SecurityManager
-from airflow.www.utils import CustomSQLAInterface
-
-EXISTING_ROLES = {
Review Comment:
> Why would that be a breaking change
yes, it breaking change because I import this constant `from airflow.www.security import EXISTING_ROLES` in my security manager and after this changes my code will break.
> But if we want to avoid such things ... every change will be breaking change and doing
`from airflow.www.security import *` import path was very important for the implementing custom security manager and I feel it would be natural for many people to import `EXISTING_ROLES` if they need this rather than redefine so removing this suddenly is not good and may impact other users. I understand that maintaining backward compatibility for constant type this is painful but still, I would like to keep this maybe we can remove it when we decide to remove `airflow.www.security.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 a diff in pull request #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310279570
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -660,13 +660,13 @@ def _process_inner_views(self):
def init_appbuilder(app) -> AirflowAppBuilder:
"""Init `Flask App Builder <https://flask-appbuilder.readthedocs.io/en/latest/>`__."""
- from airflow.www.security import AirflowSecurityManager
+ from airflow.auth.managers.fab.security_manager.override import FabAirflowSecurityManagerOverride
- security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or AirflowSecurityManager
+ security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or FabAirflowSecurityManagerOverride
- if not issubclass(security_manager_class, AirflowSecurityManager):
+ if not issubclass(security_manager_class, FabAirflowSecurityManagerOverride):
Review Comment:
That is a breaking change. Anyone who's bringing their own security manager will have their environment failing with this change. My understanding was this whole change was made to keep the backward compatibility.
I dont think this function (`init_appbuilder`) needs to change. We will need later on to move it to the FAB auth manager but in terms of implementation, I dont think it requires a change
##########
airflow/auth/managers/fab/security_manager/modules/oauth.py:
##########
@@ -24,10 +24,12 @@
import re2
from flask import session
+from airflow.www.security_manager import AirflowSecurityManager
+
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideOauth:
+class FabAirflowSecurityManagerOverrideOauth(AirflowSecurityManager):
Review Comment:
Why doing this? This looks wrong to me
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
Why doing this? This looks wrong to me
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310577578
##########
airflow/www/security_manager.py:
##########
@@ -0,0 +1,759 @@
+# 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.
+from __future__ import annotations
+
+import warnings
+from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+
+from flask import g
+from sqlalchemy import or_, select
+from sqlalchemy.orm import joinedload
+
+from airflow.auth.managers.fab.models import Permission, Resource, Role, User
+from airflow.auth.managers.fab.views.permissions import (
+ ActionModelView,
+ PermissionPairModelView,
+ ResourceModelView,
+)
+from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
+from airflow.auth.managers.fab.views.user import (
+ CustomUserDBModelView,
+ CustomUserLDAPModelView,
+ CustomUserOAuthModelView,
+ CustomUserOIDModelView,
+ CustomUserRemoteUserModelView,
+)
+from airflow.auth.managers.fab.views.user_edit import (
+ CustomResetMyPasswordView,
+ CustomResetPasswordView,
+ CustomUserInfoEditView,
+)
+from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
+from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
+from airflow.models import DagBag, DagModel
+from airflow.security import permissions
+from airflow.utils.log.logging_mixin import LoggingMixin
+from airflow.utils.session import NEW_SESSION, provide_session
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+from airflow.www.fab_security.sqla.manager import SecurityManager
+from airflow.www.utils import CustomSQLAInterface
+
+EXISTING_ROLES = {
+ "Admin",
+ "Viewer",
+ "User",
+ "Op",
+ "Public",
+}
+
+if TYPE_CHECKING:
+ from sqlalchemy.orm import Session
+
+
+class AirflowSecurityManager(SecurityManager, LoggingMixin):
Review Comment:
I agree
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310433634
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -660,13 +660,13 @@ def _process_inner_views(self):
def init_appbuilder(app) -> AirflowAppBuilder:
"""Init `Flask App Builder <https://flask-appbuilder.readthedocs.io/en/latest/>`__."""
- from airflow.www.security import AirflowSecurityManager
+ from airflow.auth.managers.fab.security_manager.override import FabAirflowSecurityManagerOverride
- security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or AirflowSecurityManager
+ security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or FabAirflowSecurityManagerOverride
- if not issubclass(security_manager_class, AirflowSecurityManager):
+ if not issubclass(security_manager_class, FabAirflowSecurityManagerOverride):
Review Comment:
ah yes right 🤦 I was a bit too hasty ^^
I can add this check but just to print a deprecation warning
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1316314556
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -24,9 +24,11 @@
from airflow.utils.log.logging_mixin import LoggingMixin
if TYPE_CHECKING:
+ from flask import Flask
+
from airflow.auth.managers.models.base_user import BaseUser
from airflow.cli.cli_config import CLICommand
- from airflow.www.security import AirflowSecurityManager
+ from airflow.www.security_manager import AirflowSecurityManagerV2
class BaseAuthManager(LoggingMixin):
Review Comment:
hmm yeah ok. Though in the current state of things, a vanilla AirflowSecurityManagerV2 won't get you very far because it lacks some useful functions, but the end goal is that all auth providers shouldn't need to write their own security manager.
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1316311143
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -36,8 +38,9 @@ class BaseAuthManager(LoggingMixin):
Auth managers are responsible for any user management related operation such as login, logout, authz, ...
"""
- def __init__(self):
+ def __init__(self, app: Flask):
Review Comment:
Yeah I can do that but `__init__` methods are a bit special and also everyone know what they are, adding type annotation there (especially to say `None`) seems pointless to me ?
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1333260908
##########
airflow/www/security.py:
##########
@@ -16,785 +16,25 @@
# under the License.
from __future__ import annotations
-import warnings
-from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+from typing import TYPE_CHECKING
-from flask import g
-from sqlalchemy import or_, select
-from sqlalchemy.orm import joinedload
+from deprecated import deprecated
-from airflow.auth.managers.fab.models import Permission, Resource, Role, User
-from airflow.auth.managers.fab.views.permissions import (
- ActionModelView,
- PermissionPairModelView,
- ResourceModelView,
-)
-from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
-from airflow.auth.managers.fab.views.user import (
- CustomUserDBModelView,
- CustomUserLDAPModelView,
- CustomUserOAuthModelView,
- CustomUserOIDModelView,
- CustomUserRemoteUserModelView,
-)
-from airflow.auth.managers.fab.views.user_edit import (
- CustomResetMyPasswordView,
- CustomResetPasswordView,
- CustomUserInfoEditView,
-)
-from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
-from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
-from airflow.models import DagBag, DagModel
-from airflow.security import permissions
-from airflow.utils.log.logging_mixin import LoggingMixin
-from airflow.utils.session import NEW_SESSION, provide_session
-from airflow.www.extensions.init_auth_manager import get_auth_manager
-from airflow.www.fab_security.sqla.manager import SecurityManager
-from airflow.www.utils import CustomSQLAInterface
-
-EXISTING_ROLES = {
Review Comment:
I see ... I'll try to think about a solution. We might have to keep whatever is static (constants, variables, functions) in these files then ... Which I really dont like ... But it seems we do not have much choice if we want to be backward compatible with users bringing their own security managers. I wish this feature did not exist xD.
But anyway, thanks for the feedback. I'll probably create a PR to fix that and will tag you on 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] pankajastro commented on a diff in pull request #33901: Fix inheritance chain in security manager
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1333264646
##########
airflow/www/security.py:
##########
@@ -16,785 +16,25 @@
# under the License.
from __future__ import annotations
-import warnings
-from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+from typing import TYPE_CHECKING
-from flask import g
-from sqlalchemy import or_, select
-from sqlalchemy.orm import joinedload
+from deprecated import deprecated
-from airflow.auth.managers.fab.models import Permission, Resource, Role, User
-from airflow.auth.managers.fab.views.permissions import (
- ActionModelView,
- PermissionPairModelView,
- ResourceModelView,
-)
-from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
-from airflow.auth.managers.fab.views.user import (
- CustomUserDBModelView,
- CustomUserLDAPModelView,
- CustomUserOAuthModelView,
- CustomUserOIDModelView,
- CustomUserRemoteUserModelView,
-)
-from airflow.auth.managers.fab.views.user_edit import (
- CustomResetMyPasswordView,
- CustomResetPasswordView,
- CustomUserInfoEditView,
-)
-from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
-from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
-from airflow.models import DagBag, DagModel
-from airflow.security import permissions
-from airflow.utils.log.logging_mixin import LoggingMixin
-from airflow.utils.session import NEW_SESSION, provide_session
-from airflow.www.extensions.init_auth_manager import get_auth_manager
-from airflow.www.fab_security.sqla.manager import SecurityManager
-from airflow.www.utils import CustomSQLAInterface
-
-EXISTING_ROLES = {
Review Comment:
Thanks @vincbeck
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310635879
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
yeah ok but that's not a durable solution since the class is obsolete. We "added" it to give some time to the users to migrate, but we shouldn't use it internally.
Moreover, the class is empty, it's basically a shortcut to `FabAirflowSecurityManagerOverride`. In that case, might as well make `ApplessAirflowSecurityManager` inherit from `FabAirflowSecurityManagerOverride`
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1312007402
##########
airflow/api_connexion/endpoints/role_and_permission_endpoint.py:
##########
@@ -40,14 +40,14 @@
if TYPE_CHECKING:
from airflow.api_connexion.types import APIResponse, UpdateMask
- from airflow.www.security import AirflowSecurityManager
+ from airflow.www.security_manager import AirflowSecurityManagerV2
Review Comment:
Since on the long term we will deprecate `AirflowSecurityManager` in favor of `AirflowSecurityManagerV2` I am wondering if `AirflowSecurityManagerV2` is a good name since this name will stay. Should we keep the name `AirflowSecurityManager`? It can confusing but we can assume this is only temporary? Or should we come up with another name? `SimplifiedAirflowSecurityManager`? I dont like it either xD.
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310598042
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
`AirflowSecurityManager` used to bring everything with it because it was at the bottom of the graph. Now that it's in the middle, inheriting from "just" `AirflowSecurityManager` means a bunch of methods don't exist.
--
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 #33901: Fix inheritance chain in security manager
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #33901:
URL: https://github.com/apache/airflow/pull/33901#issuecomment-1705753093
I need to twist my head around it :). Will take a look tomorrow .
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310573724
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
yeah so one of the reasons is this
https://github.com/apache/airflow/blob/962870f891c03334227faa18e9cc00bee03c69fc/airflow/www/security_appless.py#L36
Appless cannot inherit from _only_ AirflowSecurityManager because it uses some methods in _db_.
I guess I could maybe make it inherit from FabAirflowSecurityManagerOverride ?
The right answer is probably to untangle this, see what it needs in _db_, and decouple it I suppose.
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310577327
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
Can you not leave it as is? Meaning `ApplessAirflowSecurityManager` inherits from `AirflowSecurityManager`?
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310665313
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
`ApplessAirflowSecurityManager` is not used only for testing, it's also used in the normal course of action, for instance in this call stack:
<img width="323" alt="image" src="https://github.com/apache/airflow/assets/114772123/6bb39272-4cc6-44a5-a232-1f7c2e5cb45b">
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1312153721
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -660,15 +661,24 @@ def _process_inner_views(self):
def init_appbuilder(app) -> AirflowAppBuilder:
"""Init `Flask App Builder <https://flask-appbuilder.readthedocs.io/en/latest/>`__."""
+ from airflow.auth.managers.fab.security_manager.override import FabAirflowSecurityManagerOverride
from airflow.www.security import AirflowSecurityManager
- security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or AirflowSecurityManager
+ sm_from_config = app.config.get("SECURITY_MANAGER_CLASS")
+ if sm_from_config:
+ if not issubclass(sm_from_config, AirflowSecurityManager):
+ raise Exception(
+ """Your CUSTOM_SECURITY_MANAGER must extend FabAirflowSecurityManagerOverride,
+ not FAB's own security manager."""
+ )
+ if not issubclass(sm_from_config, FabAirflowSecurityManagerOverride):
+ warnings.warn(
+ "Please make your custom security manager inherit from "
+ "FabAirflowSecurityManagerOverride instead of AirflowSecurityManager.",
+ DeprecationWarning,
+ )
Review Comment:
True, I have always wondered if it was needed to provide the app as argument of the auth manager constructor. Maybe this one tells us, it is indeed needed :).
If we want to do that, we need to change a bit how we instantiate the auth manager but I think it should be okay
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1312146155
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -660,15 +661,24 @@ def _process_inner_views(self):
def init_appbuilder(app) -> AirflowAppBuilder:
"""Init `Flask App Builder <https://flask-appbuilder.readthedocs.io/en/latest/>`__."""
+ from airflow.auth.managers.fab.security_manager.override import FabAirflowSecurityManagerOverride
from airflow.www.security import AirflowSecurityManager
- security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or AirflowSecurityManager
+ sm_from_config = app.config.get("SECURITY_MANAGER_CLASS")
+ if sm_from_config:
+ if not issubclass(sm_from_config, AirflowSecurityManager):
+ raise Exception(
+ """Your CUSTOM_SECURITY_MANAGER must extend FabAirflowSecurityManagerOverride,
+ not FAB's own security manager."""
+ )
+ if not issubclass(sm_from_config, FabAirflowSecurityManagerOverride):
+ warnings.warn(
+ "Please make your custom security manager inherit from "
+ "FabAirflowSecurityManagerOverride instead of AirflowSecurityManager.",
+ DeprecationWarning,
+ )
Review Comment:
hmm, the trouble is accessing the `app.config` from there.
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1316316171
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -24,9 +24,11 @@
from airflow.utils.log.logging_mixin import LoggingMixin
if TYPE_CHECKING:
+ from flask import Flask
+
from airflow.auth.managers.models.base_user import BaseUser
from airflow.cli.cli_config import CLICommand
- from airflow.www.security import AirflowSecurityManager
+ from airflow.www.security_manager import AirflowSecurityManagerV2
class BaseAuthManager(LoggingMixin):
Review Comment:
100% agree :)
--
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 pull request #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on PR #33901:
URL: https://github.com/apache/airflow/pull/33901#issuecomment-1699370312
> I would:
>
> * Remove dependency between the modules and `AirflowSecurityManager`
>
> * Add dependency between `FabAirflowSecurityManagerOverride` and `AirflowSecurityManager`
>
>
> WDYT?
I *think* there was a reason for this but then I forgot, I'll double check today and come back with a more convincing answer :p
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310547795
##########
airflow/www/security_manager.py:
##########
@@ -0,0 +1,759 @@
+# 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.
+from __future__ import annotations
+
+import warnings
+from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+
+from flask import g
+from sqlalchemy import or_, select
+from sqlalchemy.orm import joinedload
+
+from airflow.auth.managers.fab.models import Permission, Resource, Role, User
+from airflow.auth.managers.fab.views.permissions import (
+ ActionModelView,
+ PermissionPairModelView,
+ ResourceModelView,
+)
+from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
+from airflow.auth.managers.fab.views.user import (
+ CustomUserDBModelView,
+ CustomUserLDAPModelView,
+ CustomUserOAuthModelView,
+ CustomUserOIDModelView,
+ CustomUserRemoteUserModelView,
+)
+from airflow.auth.managers.fab.views.user_edit import (
+ CustomResetMyPasswordView,
+ CustomResetPasswordView,
+ CustomUserInfoEditView,
+)
+from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
+from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
+from airflow.models import DagBag, DagModel
+from airflow.security import permissions
+from airflow.utils.log.logging_mixin import LoggingMixin
+from airflow.utils.session import NEW_SESSION, provide_session
+from airflow.www.extensions.init_auth_manager import get_auth_manager
+from airflow.www.fab_security.sqla.manager import SecurityManager
+from airflow.www.utils import CustomSQLAInterface
+
+EXISTING_ROLES = {
+ "Admin",
+ "Viewer",
+ "User",
+ "Op",
+ "Public",
+}
+
+if TYPE_CHECKING:
+ from sqlalchemy.orm import Session
+
+
+class AirflowSecurityManager(SecurityManager, LoggingMixin):
Review Comment:
I'm thinking of renaming this one, like, `AirflowSecurityManager2` or something, because differentiating only by the imports makes the code very hard to read...
--
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 pull request #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on PR #33901:
URL: https://github.com/apache/airflow/pull/33901#issuecomment-1699621243
> The idea of modules I introduced in `FabAirflowSecurityManagerOverride` was to avoid to build a big monster class very hard to maintain, so I split it in multiple modules.
BTW, I'm wondering if this isn't causing more harm than good.
A big file is not necessarily hard to maintain, and I'd argue it's actually harder to maintain code that's spread over 3 different files.
Inheritance also comes with extra features that we don't want here. For instance, defining the same method/property/field in 2 files would result in one overriding the other rather than an error.
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1311999855
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -660,15 +661,24 @@ def _process_inner_views(self):
def init_appbuilder(app) -> AirflowAppBuilder:
"""Init `Flask App Builder <https://flask-appbuilder.readthedocs.io/en/latest/>`__."""
+ from airflow.auth.managers.fab.security_manager.override import FabAirflowSecurityManagerOverride
from airflow.www.security import AirflowSecurityManager
- security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or AirflowSecurityManager
+ sm_from_config = app.config.get("SECURITY_MANAGER_CLASS")
+ if sm_from_config:
+ if not issubclass(sm_from_config, AirflowSecurityManager):
+ raise Exception(
+ """Your CUSTOM_SECURITY_MANAGER must extend FabAirflowSecurityManagerOverride,
+ not FAB's own security manager."""
+ )
+ if not issubclass(sm_from_config, FabAirflowSecurityManagerOverride):
+ warnings.warn(
+ "Please make your custom security manager inherit from "
+ "FabAirflowSecurityManagerOverride instead of AirflowSecurityManager.",
+ DeprecationWarning,
+ )
Review Comment:
We should move this piece of code inside `get_security_manager_override_class` of `FabAuthManager`
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -660,15 +661,24 @@ def _process_inner_views(self):
def init_appbuilder(app) -> AirflowAppBuilder:
"""Init `Flask App Builder <https://flask-appbuilder.readthedocs.io/en/latest/>`__."""
+ from airflow.auth.managers.fab.security_manager.override import FabAirflowSecurityManagerOverride
from airflow.www.security import AirflowSecurityManager
- security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or AirflowSecurityManager
+ sm_from_config = app.config.get("SECURITY_MANAGER_CLASS")
+ if sm_from_config:
+ if not issubclass(sm_from_config, AirflowSecurityManager):
+ raise Exception(
+ """Your CUSTOM_SECURITY_MANAGER must extend FabAirflowSecurityManagerOverride,
+ not FAB's own security manager."""
+ )
+ if not issubclass(sm_from_config, FabAirflowSecurityManagerOverride):
+ warnings.warn(
+ "Please make your custom security manager inherit from "
+ "FabAirflowSecurityManagerOverride instead of AirflowSecurityManager.",
+ DeprecationWarning,
+ )
- if not issubclass(security_manager_class, AirflowSecurityManager):
- raise Exception(
- """Your CUSTOM_SECURITY_MANAGER must now extend AirflowSecurityManager,
- not FAB's security manager."""
- )
+ security_manager_class = sm_from_config or FabAirflowSecurityManagerOverride
Review Comment:
It should be as simple as that. We need to modify the method `get_security_manager_override_class` of `BaseAuthManager` to return `AirflowSecurityManager` by default
```suggestion
security_manager_class = get_auth_manager().get_security_manager_override_class()
```
--
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] uranusjr commented on a diff in pull request #33901: Fix inheritance chain in security manager
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1316512464
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -36,8 +38,9 @@ class BaseAuthManager(LoggingMixin):
Auth managers are responsible for any user management related operation such as login, logout, authz, ...
"""
- def __init__(self):
+ def __init__(self, app: Flask):
Review Comment:
Mypy would require an annotation in strict mode, i.o.w. it doesn’t think this is that special, compared to e.g. `self`. But what is special is of course subjective.
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #33901:
URL: https://github.com/apache/airflow/pull/33901#issuecomment-1707005235
We should double check it resolves: #34107
--
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] pankajastro commented on a diff in pull request #33901: Fix inheritance chain in security manager
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1332098485
##########
airflow/www/security.py:
##########
@@ -16,785 +16,25 @@
# under the License.
from __future__ import annotations
-import warnings
-from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+from typing import TYPE_CHECKING
-from flask import g
-from sqlalchemy import or_, select
-from sqlalchemy.orm import joinedload
+from deprecated import deprecated
-from airflow.auth.managers.fab.models import Permission, Resource, Role, User
-from airflow.auth.managers.fab.views.permissions import (
- ActionModelView,
- PermissionPairModelView,
- ResourceModelView,
-)
-from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
-from airflow.auth.managers.fab.views.user import (
- CustomUserDBModelView,
- CustomUserLDAPModelView,
- CustomUserOAuthModelView,
- CustomUserOIDModelView,
- CustomUserRemoteUserModelView,
-)
-from airflow.auth.managers.fab.views.user_edit import (
- CustomResetMyPasswordView,
- CustomResetPasswordView,
- CustomUserInfoEditView,
-)
-from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
-from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
-from airflow.models import DagBag, DagModel
-from airflow.security import permissions
-from airflow.utils.log.logging_mixin import LoggingMixin
-from airflow.utils.session import NEW_SESSION, provide_session
-from airflow.www.extensions.init_auth_manager import get_auth_manager
-from airflow.www.fab_security.sqla.manager import SecurityManager
-from airflow.www.utils import CustomSQLAInterface
-
-EXISTING_ROLES = {
Review Comment:
@vandonr-amz @vincbeck are we considering removing `EXISTING_ROLES` from here would a breaking change or I'm missing something here?
--
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] pankajastro commented on a diff in pull request #33901: Fix inheritance chain in security manager
Posted by "pankajastro (via GitHub)" <gi...@apache.org>.
pankajastro commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1332098485
##########
airflow/www/security.py:
##########
@@ -16,785 +16,25 @@
# under the License.
from __future__ import annotations
-import warnings
-from typing import TYPE_CHECKING, Any, Collection, Container, Iterable, Sequence
+from typing import TYPE_CHECKING
-from flask import g
-from sqlalchemy import or_, select
-from sqlalchemy.orm import joinedload
+from deprecated import deprecated
-from airflow.auth.managers.fab.models import Permission, Resource, Role, User
-from airflow.auth.managers.fab.views.permissions import (
- ActionModelView,
- PermissionPairModelView,
- ResourceModelView,
-)
-from airflow.auth.managers.fab.views.roles_list import CustomRoleModelView
-from airflow.auth.managers.fab.views.user import (
- CustomUserDBModelView,
- CustomUserLDAPModelView,
- CustomUserOAuthModelView,
- CustomUserOIDModelView,
- CustomUserRemoteUserModelView,
-)
-from airflow.auth.managers.fab.views.user_edit import (
- CustomResetMyPasswordView,
- CustomResetPasswordView,
- CustomUserInfoEditView,
-)
-from airflow.auth.managers.fab.views.user_stats import CustomUserStatsChartView
-from airflow.exceptions import AirflowException, RemovedInAirflow3Warning
-from airflow.models import DagBag, DagModel
-from airflow.security import permissions
-from airflow.utils.log.logging_mixin import LoggingMixin
-from airflow.utils.session import NEW_SESSION, provide_session
-from airflow.www.extensions.init_auth_manager import get_auth_manager
-from airflow.www.fab_security.sqla.manager import SecurityManager
-from airflow.www.utils import CustomSQLAInterface
-
-EXISTING_ROLES = {
Review Comment:
@vandonr-amz @vincbeck are we considering removing `EXISTING_ROLES` from here a breaking change or I'm missing something here?
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310433634
##########
airflow/www/extensions/init_appbuilder.py:
##########
@@ -660,13 +660,13 @@ def _process_inner_views(self):
def init_appbuilder(app) -> AirflowAppBuilder:
"""Init `Flask App Builder <https://flask-appbuilder.readthedocs.io/en/latest/>`__."""
- from airflow.www.security import AirflowSecurityManager
+ from airflow.auth.managers.fab.security_manager.override import FabAirflowSecurityManagerOverride
- security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or AirflowSecurityManager
+ security_manager_class = app.config.get("SECURITY_MANAGER_CLASS") or FabAirflowSecurityManagerOverride
- if not issubclass(security_manager_class, AirflowSecurityManager):
+ if not issubclass(security_manager_class, FabAirflowSecurityManagerOverride):
Review Comment:
ah yes right 🤦 I was a bit too hasty ^^
I can add this check but just to print a deprecation warning
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1310672827
##########
airflow/auth/managers/fab/security_manager/modules/db.py:
##########
@@ -28,11 +28,12 @@
from airflow import AirflowException
from airflow.auth.managers.fab.models import Action, Permission, Resource, Role
+from airflow.www.security_manager import AirflowSecurityManager
log = logging.getLogger(__name__)
-class FabAirflowSecurityManagerOverrideDb:
+class FabAirflowSecurityManagerOverrideDb(AirflowSecurityManager):
Review Comment:
Yep, this is the only one I found. That's why I said "mostly" :)
--
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 #33901: Fix inheritance chain in security manager
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1316009816
##########
airflow/auth/managers/base_auth_manager.py:
##########
@@ -24,9 +24,11 @@
from airflow.utils.log.logging_mixin import LoggingMixin
if TYPE_CHECKING:
+ from flask import Flask
+
from airflow.auth.managers.models.base_user import BaseUser
from airflow.cli.cli_config import CLICommand
- from airflow.www.security import AirflowSecurityManager
+ from airflow.www.security_manager import AirflowSecurityManagerV2
class BaseAuthManager(LoggingMixin):
Review Comment:
The method `get_security_manager_override_class` needs to be modified to return `AirflowSecurityManagerV2` by default
--
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 #33901: Fix inheritance chain in security manager
Posted by "vandonr-amz (via GitHub)" <gi...@apache.org>.
vandonr-amz commented on code in PR #33901:
URL: https://github.com/apache/airflow/pull/33901#discussion_r1312139207
##########
airflow/api_connexion/endpoints/role_and_permission_endpoint.py:
##########
@@ -40,14 +40,14 @@
if TYPE_CHECKING:
from airflow.api_connexion.types import APIResponse, UpdateMask
- from airflow.www.security import AirflowSecurityManager
+ from airflow.www.security_manager import AirflowSecurityManagerV2
Review Comment:
temporary things still stay around for a pretty long time to give users time to migrate...
Idk, I'm open to suggestions, but we can always rename it once we remove the old one.
--
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