Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-3947] Flash msg for no DAG-level access error #4767

Merged

Conversation

XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Feb 25, 2019

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

In FAB UI, when user clicks a page to which he/she doesn't have access, there will be a "Access is Denied" flash message.

But for the DAG-level access control: when the user clicks a DAG to which he/she doesn't have access, he/she would be redirected to the main page WITHOUT any flash message. This may be confusing to the user.

This PR adds proper flash warning message in the UI for this. Users will see flash message "DAG-level access is denied" when they click on a DAG to which he/she doesn't have "can_dag_view"/"can_dag_edit" permissions.

alt text

@XD-DENG
Copy link
Member Author

XD-DENG commented Feb 25, 2019

@feng-tao PTAL.

@codecov-io
Copy link

codecov-io commented Feb 25, 2019

Codecov Report

Merging #4767 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4767      +/-   ##
==========================================
+ Coverage   74.44%   74.45%   +<.01%     
==========================================
  Files         450      450              
  Lines       28973    28974       +1     
==========================================
+ Hits        21570    21572       +2     
+ Misses       7403     7402       -1
Impacted Files Coverage Δ
airflow/www/decorators.py 74.5% <100%> (+0.5%) ⬆️
airflow/contrib/operators/ssh_operator.py 83.54% <0%> (+1.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfa81b5...afec11b. Read the comment docs.

@feng-tao
Copy link
Member

lgtm, thanks @XD-DENG

@@ -120,6 +120,7 @@ def wrapper(self, *args, **kwargs):
dag_id)))):
return f(self, *args, **kwargs)
else:
flash("DAG-level access is denied", "danger")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about the message change to "User can't access {} DAG.".format(dag_id) ?

Copy link
Member

@feng-tao feng-tao Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when I implemented the feature, the user can't even see the DAGs that he doesn't have permissions from the landing page(or can only see the DAG he can access). Is there something changed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Let me update it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. But based on what I tested/observed now, users can see the DAGs to which that he/she doesn't have permissions.

May you check & confirm from your side as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I tested was to delete the two "all_dags" permissions from role "User", then log in as "User" role. I can still see all the DAGs (all are the "built-in" example DAGs though).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@feng-tao , updated as advised. PTAL.

I think it's good to have this flash message, no matter if the user can or can not see the DAGs that he/she doesn't have permissions from the landing page (there may be cases that he/she is given a URL like http://localhost:8080/tree?dag_id=<dag_to_check> while he/she doesn't have access to <dag_to_check>).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am afk and will merge tomorrow morning . And could you create a jira for that issue and assign it to me?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But when I implemented the feature, the user can't even see the DAGs that he doesn't have permissions from the landing page(or can only see the DAG he can access). Is there something changed here?

Created https://issues.apache.org/jira/browse/AIRFLOW-3949 to track the issue discussed above.

It will show and remind user when a user clicks on a DAG that
he/she doesn't have can_dag_read or can_dag_edit permissions.
@XD-DENG XD-DENG force-pushed the access_flash_for_decorator_has_dag_access branch from ae1e094 to f9eff53 Compare February 25, 2019 08:19
@XD-DENG
Copy link
Member Author

XD-DENG commented Feb 25, 2019

Hi @feng-tao , I updated the flash message contents to "User can't access DAG {}.".format(dag_id) as advised, but it's causing some issues (https://travis-ci.org/apache/airflow/jobs/498037088#L5528): 5 tests failed because the dag id appears in the html contents while it should not according to the test definition.

It may not be worth changing the test cases for this PR, so I changed the flash msg to "Access is Denied" (1. it's consistent with the default msg in FAB; 2. "DAG-level access is denied" may cause wrong impression that the current user doesn't have dag-level access to any dag).

@feng-tao
Copy link
Member

thanks

@feng-tao feng-tao merged commit bb91246 into apache:master Feb 25, 2019
@XD-DENG XD-DENG deleted the access_flash_for_decorator_has_dag_access branch February 26, 2019 01:47
ashb pushed a commit that referenced this pull request Apr 2, 2019
* [AIRFLOW-3947] Flash msg for no DAG-level access error

It will show and remind user when a user clicks on a DAG that
he/she doesn't have can_dag_read or can_dag_edit permissions.

* Change the flash msg contents
andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
* [AIRFLOW-3947] Flash msg for no DAG-level access error

It will show and remind user when a user clicks on a DAG that
he/she doesn't have can_dag_read or can_dag_edit permissions.

* Change the flash msg contents
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
* [AIRFLOW-3947] Flash msg for no DAG-level access error

It will show and remind user when a user clicks on a DAG that
he/she doesn't have can_dag_read or can_dag_edit permissions.

* Change the flash msg contents
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants