You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/09 00:29:11 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #15295: Prevent creating flask sessions on REST API requests
ephraimbuddy opened a new pull request #15295:
URL: https://github.com/apache/airflow/pull/15295
Currently, flask sessions are created on API requests. This PR prevents it
by setting a value in flask global object g and customizing cookie session
creation
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611214862
##########
File path: tests/api_connexion/test_security.py
##########
@@ -0,0 +1,48 @@
+# 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.
+
+import pytest
+
+from airflow.security import permissions
+from tests.test_utils.api_connexion_utils import create_user, delete_user
+
+
+@pytest.fixture(scope="module")
+def configured_app(minimal_app_for_api):
+ app = minimal_app_for_api
+ create_user(
+ app, # type:ignore
+ username="test",
+ role_name="Test",
+ permissions=[(permissions.ACTION_CAN_READ, permissions.RESOURCE_CONFIG)], # type: ignore
+ )
+
+ yield minimal_app_for_api
+
+ delete_user(app, username="test") # type: ignore
+
+
+class TestSession:
+ @pytest.fixture(autouse=True)
+ def setup_attrs(self, configured_app) -> None:
+ self.app = configured_app
+ self.client = self.app.test_client() # type:ignore
+
+ def test_session_not_created_on_api_request(self):
+ self.client.get("api/v1/dags", environ_overrides={'REMOTE_USER': "test"})
+ cookie = next((cookie for cookie in self.client.cookie_jar if cookie.name == "session"), None)
+ assert cookie is None
Review comment:
```suggestion
assert all(cookie for cookie in self.client.cookie_jar if cookie.name != "session")
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816614188
> There are two approaches I would choose from to deal with the session interface setup. The first is to make the session interface an setup global to the entire Airflow web app. The session interface would simply be named `AirflowSessionInterface`, and the `app.session_intercace = AirflowSessionInterface()` line moved to `airflow.www.app.create_app` to reflect the fact the configuration is not local to `api_connextion`.
>
> The other approach is to take a composite approach, and make `app.session_interface` additive instead of overwriting it, so the session interface provided by `api_connextion` add to the existing interface instead of overwriting. Something like:
>
> ```python
> class APIConnexionSessionInterface:
> """Session interface that avoids creating session from API requests.
>
> Breifly explain how this is done (by setting ``g.login_from_api`` on user creation,
> and reading this before session creation to avoid it when we already loaded a user).
> """
> def __init__(self, wrapped: SessionInterface) -> None:
> self._wrapped = wrapped
>
> @user_loaded_from_header.connect
> def user_loaded_from_header(self, user=None):
> g.login_from_api = True
>
> def open_session(self, app, request):
> return self._wrapped.open_session(app, request)
>
> def save_session(self, app, session, response):
> if g.get('login_from_api'):
> return None
> return self._wrapped.save_session(app, session, response)
>
> def init_api_connexion(app: Flask) -> None:
> ...
> app.session_interface = APIConnexionInterface(app.session_interface)
> ```
>
> This makes the custom session interface local to the `api_connextion` module, and allows other modules to also modify the session interface by composition.
I think that we should have this implementation as it is. The reason is that apart from the REST API we would not have the need of modifying the 'flask' session or stopping its creation in another module. What do you think?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr edited a comment on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816464793
There are two approaches I would choose from to deal with the session interface setup. The first is to make the session interface an setup global to the entire Airflow web app. The session interface would simply be named `AirflowSessionInterface`, and the `app.session_intercace = AirflowSessionInterface()` line moved to `airflow.www.app.create_app` to reflect the fact the configuration is not local to `api_connextion`.
The other approach is to take a composite approach, and make `app.session_interface` additive instead of overwriting it, so the session interface provided by `api_connextion` add to the existing interface instead of overwriting. Something like:
```python
class APIConnexionSessionInterface:
"""Session interface that avoids creating session from API requests.
Breifly explain how this is done (by setting ``g.login_from_api`` on user creation,
and reading this before session creation to avoid it when we already loaded a user).
"""
def __init__(self, wrapped: SessionInterface) -> None:
self._wrapped = wrapped
@user_loaded_from_header.connect
def user_loaded_from_header(self, user=None):
g.login_from_api = True
def open_session(self, app, request):
return self._wrapped.open_session(app, request)
def save_session(self, app, session, response):
if g.get('login_from_api'):
return None
return self._wrapped.save_session(app, session, response)
def init_api_connexion(app: Flask) -> None:
...
app.session_interface = APIConnexionInterface(app.session_interface)
```
This makes the custom session interface local to the `api_connextion` module, and allows other modules to also modify the session interface by composition.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611439964
##########
File path: airflow/www/security.py
##########
@@ -170,7 +190,11 @@ def __init__(self, appbuilder):
if not view or not getattr(view, 'datamodel', None):
continue
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
+ app = self.appbuilder.get_app
self.perms = None
+ # Custom cookie session interface
+ # Override to implement your custom cookie session interface
+ app.session_interface = DefaultSessionInterface()
Review comment:
This shouldn't be set/change in here, but instead in a function in airflow.www.extensions.init_session (that you will need to call inside create_app in airflow.www.app
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611433820
##########
File path: airflow/www/security.py
##########
@@ -44,6 +46,24 @@
}
+class DefaultSessionInterface(SecureCookieSessionInterface):
+ """
+ Default cookie session interface.
+ This prevents creating flask sessions on REST API requests.
+ """
+
+ def save_session(self, *args, **kwargs):
+ """Prevent creating session from REST API requests."""
+ if g.get('login_from_api'):
Review comment:
Maybe we should do check here `if request.url.startswith ("/api/")`? This will then simplify the code a bit. And it will also prevent discovery endpoints(non-login-required) e.g. `/healtth`, `/version` from creating sessions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611215495
##########
File path: airflow/api_connexion/security.py
##########
@@ -42,6 +42,7 @@ def requires_access(permissions: Optional[Sequence[Tuple[str, str]]] = None) ->
def requires_access_decorator(func: T):
@wraps(func)
def decorated(*args, **kwargs):
+ g.login_from_api = True
Review comment:
Can you add a one-line comment explaining why this is needed? The relationship between this decorator and the `DefaultSessionInterface` class is not obvious.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-817823533
> This is actually for a case where login is configured to be processed through the header. Airflow did not configure this, but users may configure it.
Do you mean this would read the `Autorization` HTTP header? If so, maybe we should make `requires_access()` etc. able to handle this instead. It really feels weird that different authentication methods need to be handled in different places.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r612078540
##########
File path: airflow/www/security.py
##########
@@ -170,7 +190,11 @@ def __init__(self, appbuilder):
if not view or not getattr(view, 'datamodel', None):
continue
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
+ app = self.appbuilder.get_app
self.perms = None
+ # Custom cookie session interface
+ # Override to implement your custom cookie session interface
+ app.session_interface = DefaultSessionInterface()
Review comment:
The security manager is already in "god class" territory, so I think we should avoid moving anything else into it.
If we really want users to be able to customize everything (which I don't necessarily think is the case), we can easily allow customization of the init functions in other ways.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611433820
##########
File path: airflow/www/security.py
##########
@@ -44,6 +46,24 @@
}
+class DefaultSessionInterface(SecureCookieSessionInterface):
+ """
+ Default cookie session interface.
+ This prevents creating flask sessions on REST API requests.
+ """
+
+ def save_session(self, *args, **kwargs):
+ """Prevent creating session from REST API requests."""
+ if g.get('login_from_api'):
Review comment:
Maybe we should do check here `if request.url.startswith ("/ api")`? This will then simplify the code a bit. And it will also prevent discovery endpoints(non-login-required) from generating sessions.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611461639
##########
File path: airflow/www/security.py
##########
@@ -170,7 +190,11 @@ def __init__(self, appbuilder):
if not view or not getattr(view, 'datamodel', None):
continue
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
+ app = self.appbuilder.get_app
self.perms = None
+ # Custom cookie session interface
+ # Override to implement your custom cookie session interface
+ app.session_interface = DefaultSessionInterface()
Review comment:
Cool. But I'm of the opinion that users won't be able to modify the session when set at airflow.www.extensions.init_session?
Also, I think that some of the initialization at airflow.www.extensions can be refactored by moving them into the security manager. The security manager also gets called in create_app through airflow.www.extensions.init_appbuilder.
So it's still the same I think but users can be able to override a lot using just the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816634247
> > The reason is that apart from the REST API we would not have the need of modifying the 'flask' session or stopping its creation in another module.
>
> RIght now we don't. But modifying the session interface is still a global override, and doing it inside `api_connextion` would waste future contributors time debugging why their override doesn't work (because it's overridden again during `init_api_connexion()`), or mysteriously breaks the API (because their session interface overrides the setup in `init_api_connexion()`). Putting the override in `create_app()` helps prevent that.
Agreed! Thanks. I will work out a solution on the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611436504
##########
File path: airflow/www/security.py
##########
@@ -44,6 +46,24 @@
}
+class DefaultSessionInterface(SecureCookieSessionInterface):
+ """
+ Default cookie session interface.
+ This prevents creating flask sessions on REST API requests.
+ """
+
+ def save_session(self, *args, **kwargs):
+ """Prevent creating session from REST API requests."""
+ if g.get('login_from_api'):
Review comment:
> Maybe we should do check here `if request.url.startswith ("/api/")`? This will then simplify the code a bit. And it will also prevent discovery endpoints(non-login-required) e.g. `/healtth`, `/version` from creating sessions.
And I think using this will also get rid of 'login_from_api'. Thanks! testing
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-818309241
> > if user decides to configure it, then the user should also configure session interface
>
> Sounds good to me!
>
> Instead of detecting the `/api/` prefix, would it be better to rely on Flask’s routing for detection instead? IIRC API views are grouped in Blueprints already, so maybe `request.blueprint` would be useful. I’m not strong on this though; hard-coding URLs is usually not a good idea, but Airflow is already doing that in may places anyway (eh).
I have used request.blueprint. Maybe we should refactor code later on and not hard-code the URL.
Let me know if this is what you expected.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816399553
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy edited a comment on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy edited a comment on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816713328
Hi @uranusjr, can you take a look at this implementation. I figured if we need modification, we can just modify the DefaultSessionInterface since it's global. No need to have different instances of it modified by separate modules but all changes would be on this Interface.
Users would also be able to override this and implement their custom modification
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611438996
##########
File path: airflow/www/security.py
##########
@@ -44,6 +46,24 @@
}
+class DefaultSessionInterface(SecureCookieSessionInterface):
+ """
+ Default cookie session interface.
+ This prevents creating flask sessions on REST API requests.
+ """
+
+ def save_session(self, *args, **kwargs):
+ """Prevent creating session from REST API requests."""
+ if g.get('login_from_api'):
+ return None
+ return super().save_session(*args, **kwargs)
+
+ @user_loaded_from_header.connect
+ def user_loaded_from_header(self, user=None): # pylint: disable=unused-argument
+ """Set login_from_api in g"""
Review comment:
This comment does't add any "value" -- we can see that it's doing this.
Either don't have a docstring here, or say _why_ we are setting this.
Additionlay: why do we need this and the change in airflow/api_connexion/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611447356
##########
File path: airflow/www/security.py
##########
@@ -44,6 +46,24 @@
}
+class DefaultSessionInterface(SecureCookieSessionInterface):
+ """
+ Default cookie session interface.
+ This prevents creating flask sessions on REST API requests.
+ """
+
+ def save_session(self, *args, **kwargs):
+ """Prevent creating session from REST API requests."""
+ if g.get('login_from_api'):
+ return None
+ return super().save_session(*args, **kwargs)
+
+ @user_loaded_from_header.connect
+ def user_loaded_from_header(self, user=None): # pylint: disable=unused-argument
+ """Set login_from_api in g"""
Review comment:
This is actually for a case where login is configured to be processed through the header. Airflow did not configure this, but users may configure it. We can remove it. Should I?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-822794542
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-817607888
> I can never understand Flask contexts and will just ask the dump question: Can (should?) this use the request context instead and why (not)? API authentication feels like a request-level thing to me, although Flask’s application context is actually per-request so the request context might not actually meant to be used for request-level stuff anyway, I have no idea.
From the docs, `flask.g` is a good place to store resources during a request: https://flask.palletsprojects.com/en/1.1.x/api/#flask.g.
Even though it's in the application context, it gets destroyed after each request. It's possible to store in request context but I think that storing it in flask.g is also good since they also admitted that it's a good place to store resources during a request.
> BTW why does g.login_from_api need to be set in two places? I’d assume there’s a callback that all requests flow through that can be used to set context for API authentication?
This is because login can be configured to be processed through the header in flask login and appbuilder uses flask login. It's not configured in airflow but some other persons may configure login through the header.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-817873964
> > This is actually for a case where login is configured to be processed through the header. Airflow did not configure this, but users may configure it.
>
> Do you mean this would read the `Autorization` HTTP header? If so, maybe we should make `requires_access()` etc. able to handle this instead. It really feels weird that different authentication methods need to be handled in different places.
It's even better we remove the flask.g, and check the request URL if it has '/api/' in it. Also yes, in some configurations for flask_login, it can accept HTTP header 'Autorization'. I'm deciding to leave this out, if user decides to configure it, then the user should also configure session interface
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r610343223
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -156,6 +158,21 @@ def set_cors_headers_on_response(response):
return response
+class CustomSessionInterface(SecureCookieSessionInterface):
Review comment:
I would prefer a better name on the class interface but I can't come up with any..
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816464793
There are two approaches I would choose from to deal with the session interface setup. The first is to make the session interface an setup global to the entire Airflow web app. The session interface would simply be named `AirflowSessionInterface`, and the `app.session_intercace = AirflowSessionInterface()` line moved to `airflow.www.app.create_app` to reflect the fact the configuration is not local to `api_connextion`.
The other approach is to take a composite approach, and make `app.session_interface` additive instead of overwriting it, so the session interface provided by `api_connextion` add to the existing interface instead of overwriting. Something like:
```python
class APIConnexionSessionInterface:
"""Session interface that avoids creating session from API requests.
Breifly explain how this is done (by setting ``g.login_from_api`` on user creation,
and reading this before session creation to avoid it when we already loaded a user.
"""
def __init__(self, wrapped: SessionInterface) -> None:
self._wrapped = wrapped
@user_loaded_from_header.connect
def user_loaded_from_header(self, user=None):
g.login_from_api = True
def open_session(self, app, request):
return self._wrapped.open_session(app, request)
def save_session(self, app, session, response):
if g.get('login_from_api'):
return None
return self._wrapped.save_session(app, session, response)
def init_api_connexion(app: Flask) -> None:
...
app.session_interface = APIConnexionInterface(app.session_interface)
```
This makes the custom session interface local to the `api_connextion` module, and allows other modules to also modify the session interface by composition.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-817569707
I can never understand Flask contexts and will just ask the dump question: Can (should?) this use the request context instead and why (not)? API authentication feels like a request-level thing to me, although Flask’s application context is actually per-request so the request context might not actually meant to be used for request-level stuff anyway, I have no idea.
BTW why does `g.login_from_api` need to be set in two places? I’d assume there’s a callback that all requests flow through that can be used to set context for API authentication?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr edited a comment on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
uranusjr edited a comment on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816616829
> The reason is that apart from the REST API we would not have the need of modifying the 'flask' session or stopping its creation in another module.
RIght now we don't. But modifying the session interface is still a global override, and doing it inside `api_connextion` would waste future contributors time debugging why their override doesn't work (because it's overridden again during `init_api_connexion()`), or mysteriously breaks the API (because their session interface overrides the setup in `init_api_connexion()`). Putting the override in `create_app()` helps prevent that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] xinbinhuang commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
xinbinhuang commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r610343223
##########
File path: airflow/www/extensions/init_views.py
##########
@@ -156,6 +158,21 @@ def set_cors_headers_on_response(response):
return response
+class CustomSessionInterface(SecureCookieSessionInterface):
Review comment:
```suggestion
class CustomSessionInterface(SecureCookieSessionInterface):
```
I would prefer a better name on the class interface but I can't come up with any..
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816713328
Hi @uranusjr, can you take a look at this implementation. I figured if we need modification, we can just modify the DefaultSessionInterface since it's global. No need to have different instances of it modified by separate modules but all changes would be on this Interface.
Users would also be able to override this and implement their own custom modification
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-816616829
> The reason is that apart from the REST API we would not have the need of modifying the 'flask' session or stopping its creation in another module.
RIght now we don't. But Modifying the session interface is still a global override, and doing it inside `api_connextion` would waste future contributors time debugging why their override doesn't work, or mysteriously breaks the API. Putting the override in `create_app()` helps prevent that.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] jhtimmins merged pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
jhtimmins merged pull request #15295:
URL: https://github.com/apache/airflow/pull/15295
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#issuecomment-817958781
> if user decides to configure it, then the user should also configure session interface
Sounds good to me!
----
Instead of detecting the `/api/` prefix, would it be better to rely on Flask’s routing for detection instead? IIRC API views are grouped in Blueprints already, so maybe `request.blueprint` would be useful. I’m not strong on this though; hard-coding URLs is usually not a good idea, but Airflow is already doing that in may places anyway (eh).
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r611214862
##########
File path: tests/api_connexion/test_security.py
##########
@@ -0,0 +1,48 @@
+# 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.
+
+import pytest
+
+from airflow.security import permissions
+from tests.test_utils.api_connexion_utils import create_user, delete_user
+
+
+@pytest.fixture(scope="module")
+def configured_app(minimal_app_for_api):
+ app = minimal_app_for_api
+ create_user(
+ app, # type:ignore
+ username="test",
+ role_name="Test",
+ permissions=[(permissions.ACTION_CAN_READ, permissions.RESOURCE_CONFIG)], # type: ignore
+ )
+
+ yield minimal_app_for_api
+
+ delete_user(app, username="test") # type: ignore
+
+
+class TestSession:
+ @pytest.fixture(autouse=True)
+ def setup_attrs(self, configured_app) -> None:
+ self.app = configured_app
+ self.client = self.app.test_client() # type:ignore
+
+ def test_session_not_created_on_api_request(self):
+ self.client.get("api/v1/dags", environ_overrides={'REMOTE_USER': "test"})
+ cookie = next((cookie for cookie in self.client.cookie_jar if cookie.name == "session"), None)
+ assert cookie is None
Review comment:
```suggestion
assert all(cookie.name != "session" for cookie in self.client.cookie_jar)
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #15295: Prevent creating flask sessions on REST API requests
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #15295:
URL: https://github.com/apache/airflow/pull/15295#discussion_r612232404
##########
File path: airflow/www/security.py
##########
@@ -170,7 +190,11 @@ def __init__(self, appbuilder):
if not view or not getattr(view, 'datamodel', None):
continue
view.datamodel = CustomSQLAInterface(view.datamodel.obj)
+ app = self.appbuilder.get_app
self.perms = None
+ # Custom cookie session interface
+ # Override to implement your custom cookie session interface
+ app.session_interface = DefaultSessionInterface()
Review comment:
Hi James, I discussed with Ash about this and the plans I have for also invalidating sessions.
I do not have strong opinion on this but having studied appbuider and that we also use it, I believe it's not antipattern and security manager is a good place to place security related components.
I would argue that what we did in airflow.www.extensions is actually an antipattern following how appbuider is built and also other flask packages.
Ash advised everything session related should be in one place and I would like if you can take more look at this.
I'll be working on session invalidation and I will be moving it to security manager too if you do not object and have all session related components moved too to the security manager.
We can schedule a session and discuss this if you want.
Thanks!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org