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/12/06 02:21:32 UTC
[GitHub] [airflow] jhtimmins opened a new pull request #12846: Fix login error warning
jhtimmins opened a new pull request #12846:
URL: https://github.com/apache/airflow/pull/12846
Currently, when a user is logged out, the `DAGs` link shows in the menu and the `Not Logged In` warning shows if the user has been directed to `/login` from any other page.
This hides the `DAGs` menu item unless the user is logged in, and doesn't show a warning if an unauthenticated user redirects to `/login`.
----------------------------------------------------------------
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 merged pull request #12846: Fix login error warning
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #12846:
URL: https://github.com/apache/airflow/pull/12846
----------------------------------------------------------------
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 #12846: Fix login error warning
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on a change in pull request #12846:
URL: https://github.com/apache/airflow/pull/12846#discussion_r537141002
##########
File path: airflow/www/auth.py
##########
@@ -32,9 +32,6 @@ def decorated(*args, **kwargs):
appbuilder = current_app.appbuilder
if appbuilder.sm.check_authorization(permissions, request.args.get('dag_id', None)):
return func(*args, **kwargs)
- else:
- access_denied = "Access is Denied"
- flash(access_denied, "danger")
Review comment:
@ashb ok I updated it. I realized a lot of CRUD paths use the built-in FAB routing and access controls and never even use this function. So instead I just modified `flash.html` to hide this specific error on the login page. Not the prettiest solution, but at least it's consistent.
----------------------------------------------------------------
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 #12846: Fix login error warning
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12846:
URL: https://github.com/apache/airflow/pull/12846#issuecomment-739591833
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] ashb commented on a change in pull request #12846: Fix login error warning
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12846:
URL: https://github.com/apache/airflow/pull/12846#discussion_r537148700
##########
File path: airflow/www/templates/appbuilder/flash.html
##########
@@ -41,10 +41,12 @@
{% if regular_alerts %}
{% for category, m in regular_alerts %}
- <div class="alert alert-{{ category if category else 'info' }}">
- <button type="button" class="close" data-dismiss="alert">×</button>
- {{ m }}
- </div>
+ {% if not ('login' in request.path and 'access is denied' in m.lower()) %}
Review comment:
Hmmm, don't feel great about this. But not sure I have a better idea.
Let's at least add a (jinja) comment `{# #}` saying why we are doing this.
Better than just `'login' in request.path` is
```suggestion
{% if not (request.path == appbuilder.get_url_for_login and 'access is denied' in m.lower()) %}
```
----------------------------------------------------------------
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 #12846: Fix login error warning
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12846:
URL: https://github.com/apache/airflow/pull/12846#discussion_r537088763
##########
File path: airflow/www/auth.py
##########
@@ -32,9 +32,6 @@ def decorated(*args, **kwargs):
appbuilder = current_app.appbuilder
if appbuilder.sm.check_authorization(permissions, request.args.get('dag_id', None)):
return func(*args, **kwargs)
- else:
- access_denied = "Access is Denied"
- flash(access_denied, "danger")
Review comment:
So, this isn't quite right.
This helps when the user isn't logged in, but if you are a non-admin user, and you, for example, try to go to `http://localhost:8080/connection/list/` then it will redirect you (in a _very_ round about way via /login -> /) to /home and show this "access is deined" flash.
I think without that flash message it would be very confusing.
So we probably need to do something like looking at current_user.is_anonymous (I think that's the property) -- if it's anon, don't show the flash, but if there is a user then, yes, you can't actually access this page.
----------------------------------------------------------------
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