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 2020/08/09 22:26:53 UTC
[GitHub] [airflow] mik-laj opened a new pull request #10267: Add Authentication for Stable API
mik-laj opened a new pull request #10267:
URL: https://github.com/apache/airflow/pull/10267
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ 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] houqp commented on a change in pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
houqp commented on a change in pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#discussion_r468873335
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -151,6 +156,7 @@ def get_dag_runs_batch(session):
total_entries=total_entries))
+@security.requires_authentication
Review comment:
if no auth is the exception here (i.e. only /health and /version), wouldn't it be safer to make auth required the default, then turn off auth specifically for those two endpoints?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#discussion_r468646811
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -151,6 +156,7 @@ def get_dag_runs_batch(session):
total_entries=total_entries))
+@security.requires_authentication
Review comment:
Perfect!
----------------------------------------------------------------
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 #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#discussion_r468618657
##########
File path: airflow/api_connexion/security.py
##########
@@ -0,0 +1,37 @@
+# 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 functools import wraps
+from typing import Callable, TypeVar, cast
+
+from flask import Response, current_app
+
+from airflow.api_connexion.exceptions import Unauthenticated
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
Review comment:
Any letter of the alphabet can be used as a variable name, so I don't think that's a good idea. We don't have a lot of such situations in the code. You might get this feeling because I started using them more, but overall we have less than 10 such situations in the codebase.
----------------------------------------------------------------
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] kaxil commented on pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-671957861
cc @jhtimmins
----------------------------------------------------------------
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 #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#discussion_r468615302
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -151,6 +156,7 @@ def get_dag_runs_batch(session):
total_entries=total_entries))
+@security.requires_authentication
Review comment:
Yes, We can configure it on a higher layer, but prefer to be explicit than implicit, because we have two views that do not require authorization - `/health` and `/version`.
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#discussion_r468603778
##########
File path: airflow/api_connexion/security.py
##########
@@ -0,0 +1,37 @@
+# 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 functools import wraps
+from typing import Callable, TypeVar, cast
+
+from flask import Response, current_app
+
+from airflow.api_connexion.exceptions import Unauthenticated
+
+T = TypeVar("T", bound=Callable) # pylint: disable=invalid-name
Review comment:
Is this disabled necessary? If yes, maybe we can add this `T` to accepted names as we are using it quite often?
----------------------------------------------------------------
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] potiuk commented on a change in pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#discussion_r468457128
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -151,6 +156,7 @@ def get_dag_runs_batch(session):
total_entries=total_entries))
+@security.requires_authentication
Review comment:
Is it possible to detect automatically if we have an API call without @security.requires_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] mik-laj merged pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #10267:
URL: https://github.com/apache/airflow/pull/10267
----------------------------------------------------------------
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 #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#discussion_r469187890
##########
File path: airflow/api_connexion/endpoints/dag_run_endpoint.py
##########
@@ -151,6 +156,7 @@ def get_dag_runs_batch(session):
total_entries=total_entries))
+@security.requires_authentication
Review comment:
I was based on how the experimental API is realized, but I will prepare a patch that will update the stable and experimental 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] ashb commented on pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672223632
Does this do everything we need for https://github.com/apache/airflow/issues/8111?
Additionally why did you do this PR without first checking in with James when that ticket was assigned to him?
----------------------------------------------------------------
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] potiuk commented on pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672799228
@ashb The main reason is that we have one week left for @ephraimbuddy and @OmairK to finish their work and they were blocked by this one to complete some of the open PR so we went ahead with that so that they can simple complete their tasks - while we are still OK with iterating over the authentication part. I think it would be great to get comments from @jhtimmins and I think we are all more than happy to apply the review as a follow up -PR.
I hope this is OK.
----------------------------------------------------------------
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] potiuk commented on pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672804883
Ah.I see it now. Apologies @jthimmins. It was simply an overlook from my side. When we discussed it a the last call with our interns (with @kaxil as well), I think we simply did not realize (or rather forgot) that you were assigned to the ticket that was already created. Sorry for that, we should have actually check the ticket.
----------------------------------------------------------------
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] kaxil commented on pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672810192
hmm I was under the impression that James is working on Auth and permissions model based on the discussions in https://github.com/apache/airflow/issues/8111 . If I gave any wrong impressions, apologies for that.
Anyways, on positive side this work is done so now @jhtimmins can focus on permissions model. Can you both sync on that please @mik-laj and @jhtimmins :) on who is doing what, you could setup up a call.
----------------------------------------------------------------
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] potiuk edited a comment on pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672799228
@ashb The main reason is that we have one week left for @ephraimbuddy and @OmairK to finish their work (their internship ends and they need to wrap-up) and they were blocked by this one to complete some of the open PR so we went ahead with that so that they can simple complete their tasks - while we are still OK with iterating over the authentication part. I think it would be great to get comments from @jhtimmins and I think we are all more than happy to apply the review as a follow up -PR.
I hope this is OK.
----------------------------------------------------------------
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] kaxil edited a comment on pull request #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672810192
hmm I was under the impression that James is working on Auth and permissions model based on the discussions in https://github.com/apache/airflow/issues/8111 . If I gave any wrong impressions, apologies for that.
Anyways, on the positive side this work is done so now @jhtimmins can focus on permissions model. Can you both sync on that please @mik-laj and @jhtimmins :) on who is doing what, you could setup up a call to make sure there is no duplication of work.
----------------------------------------------------------------
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 #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672816411
> Does this do everything we need for #8111?
@jhtimmins is working on related changes that don't have a separate ticket.
> I will work on simple auth mechanisms for users that don't have an auth proxy.
----------------------------------------------------------------
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 #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672799764
Asking in advance would have been polite
----------------------------------------------------------------
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 #10267: Add Authentication for Stable API
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #10267:
URL: https://github.com/apache/airflow/pull/10267#issuecomment-672811818
I'm sorry, but I should have called @jhtimmins sooner, but from what I found out with him, he didn't work on that ticket anymore. From the last discussion, we agreed that we will use the authentication based on the authentication from the experimental API. The code in this change is very similar to the one I presented in my last comment.
@jhtimmins If you have any comments on this change, I will gladly implement them in the follow-up PR. Sorry again for not pinging you when I made the PR. This is an oversight on my part, as I remember that you want to focus on other authentication methods once the core is implemented.
----------------------------------------------------------------
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