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-4028] Avoid Adding DAG_PERMS of All DAGs to Admin Role #4856

Conversation

XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Mar 6, 2019

Jira

Description

Currently Admin’s permissions will always be updated so it has ALL permissions, including each DAG’s “can_dag_view” and “can_dag_edit”.

Let’s say we have 1000 DAGs now: then 2000 permissions will be added to Admin role in DB, and all of them will also be shown in UI if we go to Security->Roles.

This may not be necessary as Admin role already has “can_dag_view” and “can_dag_edit” on special view “all_dags”.

For Admin users, they already have DAG permissions on special view "all_dags".
It's not necessary to add DAG permissions of each single DAG to Admin role,
which may add too many records into DB table (say there are 1000 DAGs, there
are 2000 records to be written into permission table currently)
@XD-DENG
Copy link
Member Author

XD-DENG commented Mar 6, 2019

Hi @feng-tao , this PR is to address the issue I mentioned in #4569 (comment) .

Understand that you don't want to go for hacky solution. Please take a look at this PR and decide if it looks good to you. Feel free to close it if it's hacky for you (as you understand the FAB SecurityManager part better than I do).

Thanks.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4856      +/-   ##
==========================================
+ Coverage   74.77%   74.77%   +<.01%     
==========================================
  Files         449      449              
  Lines       28963    28963              
==========================================
+ Hits        21657    21658       +1     
+ Misses       7306     7305       -1
Impacted Files Coverage Δ
airflow/www/security.py 92.82% <100%> (ø) ⬆️
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 36a6305...3b93b6d. Read the comment docs.

@feng-tao
Copy link
Member

feng-tao commented Mar 7, 2019

@XD-DENG , I felt the solution is a bit of hacky as it relies on Admin should use all_dags view_menu. It will break if tomorrow we decide to use something else for Admin role. And normally I don't think the Admin user will change what admin permissions.

@XD-DENG
Copy link
Member Author

XD-DENG commented Mar 7, 2019

@feng-tao , understand and agree.

Let me close this PR but leave the JIRA ticket open.

@XD-DENG XD-DENG closed this Mar 7, 2019
@XD-DENG XD-DENG deleted the avoid_adding_dag_perms_of_each_dag_to_admin branch March 7, 2019 05:54
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