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/01/11 19:51:45 UTC

[GitHub] [airflow] ryanahamilton opened a new pull request #13620: Configurable API response (CORS) headers

ryanahamilton opened a new pull request #13620:
URL: https://github.com/apache/airflow/pull/13620


   Employing the newly improved REST API from an independent web application is currently prohibited by browsers due to the lack of [CORS (Cross-Origin Resource Sharing) headers](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS) in the API response.
   
   This PR adds 3 configuration options to add the following headers:
   
   - `Access-Control-Allow-Headers` via `AIRFLOW__API__ACCESS_CONTROL_ALLOW_HEADERS`
   - `Access-Control-Allow-Methods` via `AIRFLOW__API__ACCESS_CONTROL_ALLOW_METHODS`
   - `Access-Control-Allow-Origin` via `AIRFLOW__API__ACCESS_CONTROL_ALLOW_ORIGIN`
   
   This only covers a minimum of all potential headers that could be utilized, but the added `set_cors_headers_on_response` function establishes an obvious place for it to be further extended in the future if needed.
   
   We did look into utilizing [Flask-CORS](https://github.com/corydolphin/flask-cors) to add this functionality, but ultimately found it to be overkill given we only want to add this to the API endpoint and not the entire Webserver application.
   
   I've added documentation of this feature to Security/API and also cross-linked to that documentation from within the API documentation as well.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ryanahamilton merged pull request #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
ryanahamilton merged pull request #13620:
URL: https://github.com/apache/airflow/pull/13620


   


----------------------------------------------------------------
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] ryanahamilton commented on pull request #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#issuecomment-758686630


   Thanks for the input @mik-laj! Ash helped me work out the handler registration.
   
   re: `Access-Control-Allow-Credentials` I don't have a strong opinion on this as I haven't yet encountered a use-case that requires it. I'm happy to add it if you think it would be prudent? Otherwise, this will establish an obvious place to do so if the need arises in the future.


----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#issuecomment-758327313


   [The Workflow run](https://github.com/apache/airflow/actions/runs/478414198) is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason.


----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555790125



##########
File path: airflow/www/extensions/init_views.py
##########
@@ -171,6 +185,7 @@ def _handle_api_error(ex):
     api_bp = connexion_app.add_api(
         specification='v1.yaml', base_path=base_path, validate_responses=True, strict_validation=True
     ).blueprint
+    app.after_request(set_cors_headers_on_response)

Review comment:
       ```suggestion
       # Like "api_bp.after_request", but the BP is already registered, so we have to
       # register it in the app directly.
       app.after_request_funcs.setdefault(api_bp.name, []).append(set_cors_headers_on_response)
   
   ```




----------------------------------------------------------------
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 pull request #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#issuecomment-758224277


   Should we support [https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Credentials](Access-Control-Allow-Credentials)?


----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555331607



##########
File path: docs/apache-airflow/security/api.rst
##########
@@ -135,6 +135,26 @@ section of ``airflow.cfg``.
 
 Additional options to your auth backend can be configured in ``airflow.cfg``, as a new option.
 
+Enabling CORS
+---------------
+
+[Cross-origin resource sharing (CORS)](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS)
+is a browser security feature that restricts HTTP requests that are initiated
+from scripts running in the browser.
+
+`Access-Control-Allow-Headers`, `Access-Control-Allow-Methods`, and
+`Access-Control-Allow-Origin` headers can be added by setting values for

Review comment:
       ```suggestion
   ``Access-Control-Allow-Headers``, ``Access-Control-Allow-Methods``, and
   ``Access-Control-Allow-Origin`` headers can be added by setting values for
   ```




----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555331842



##########
File path: docs/apache-airflow/security/api.rst
##########
@@ -135,6 +135,26 @@ section of ``airflow.cfg``.
 
 Additional options to your auth backend can be configured in ``airflow.cfg``, as a new option.
 
+Enabling CORS
+---------------
+
+[Cross-origin resource sharing (CORS)](https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS)

Review comment:
       Please use rst syntax for link.  https://sublime-and-sphinx-guide.readthedocs.io/en/latest/references.html




----------------------------------------------------------------
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] ryanahamilton commented on a change in pull request #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
ryanahamilton commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555340602



##########
File path: airflow/www/extensions/init_views.py
##########
@@ -147,6 +147,20 @@ def init_error_handlers(app: Flask):
     app.register_error_handler(404, views.circles)
 
 
+def set_cors_headers_on_response(response):

Review comment:
       Ah, sorry, my Python/Flask inexperience is showing… Would it be ideal for this to be moved to an external file and then imported into `init_api_connexion`?




----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555775884



##########
File path: airflow/www/extensions/init_views.py
##########
@@ -171,6 +185,7 @@ def _handle_api_error(ex):
     api_bp = connexion_app.add_api(
         specification='v1.yaml', base_path=base_path, validate_responses=True, strict_validation=True
     ).blueprint
+    app.after_request(set_cors_headers_on_response)

Review comment:
       Connexion is doing something funny -- this doesn't work (the after request is never called!)




----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555344211



##########
File path: airflow/www/extensions/init_views.py
##########
@@ -147,6 +147,20 @@ def init_error_handlers(app: Flask):
     app.register_error_handler(404, views.circles)
 
 
+def set_cors_headers_on_response(response):

Review comment:
       This code register this handler for each view.
   ```
   app.after_request(set_cors_headers_on_response)
   ```
   You should use [Blueprint.after_request](https://flask.palletsprojects.com/en/1.1.x/api/#flask.Blueprint.after_request).  The api_ap variable contains an `flask.Blueprint` object. 
   

##########
File path: airflow/www/extensions/init_views.py
##########
@@ -171,6 +185,7 @@ def _handle_api_error(ex):
     api_bp = connexion_app.add_api(
         specification='v1.yaml', base_path=base_path, validate_responses=True, strict_validation=True
     ).blueprint
+    app.after_request(set_cors_headers_on_response)

Review comment:
       ```suggestion
       api_bp.after_request(set_cors_headers_on_response)
   ```




----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555331241



##########
File path: airflow/www/extensions/init_views.py
##########
@@ -147,6 +147,20 @@ def init_error_handlers(app: Flask):
     app.register_error_handler(404, views.circles)
 
 
+def set_cors_headers_on_response(response):

Review comment:
       This function will run for all views, not just the API.




----------------------------------------------------------------
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 #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#issuecomment-759576409


   The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, 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] ashb commented on pull request #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#issuecomment-758695037


   > Thanks for the input @mik-laj! Ash helped me work out the handler registration.
   > 
   > re: `Access-Control-Allow-Credentials` I don't have a strong opinion on this as I haven't yet encountered a use-case that requires it. I'm happy to add it if you think it would be prudent? Otherwise, this will establish an obvious place to do so if the need arises in the future.
   
   Probably add it when we/someone have a use case for it 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [airflow] ashb commented on a change in pull request #13620: Configurable API response (CORS) headers

Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #13620:
URL: https://github.com/apache/airflow/pull/13620#discussion_r555787779



##########
File path: airflow/www/extensions/init_views.py
##########
@@ -171,6 +185,7 @@ def _handle_api_error(ex):
     api_bp = connexion_app.add_api(
         specification='v1.yaml', base_path=base_path, validate_responses=True, strict_validation=True
     ).blueprint
+    app.after_request(set_cors_headers_on_response)

Review comment:
       Ah, the problem is this blueprint is already registered when it's returned, so using `bp.after_request` doesn't get added to the flask app's callback list.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org