[AIRFLOW-3745] Fix viewer not able to view dag details #4569

feng-tao · 2019-01-21T19:20:29Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-3745
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.

Description

Here are some details about my PR, including screenshots of any UI changes:
To stick with existing RBAC which Admin, Op, User, Viewer view all dags.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

Passes flake8

feng-tao · 2019-01-21T19:20:56Z

PTAL @XD-DENG , @kaxil

Could you test with this pr and see the issue still exists?

kaxil

Can we add test by creating dummy users with each role & check they have correct permissions?

XD-DENG · 2019-01-22T01:29:36Z

Hi @feng-tao , I tried to cherry-pick this change into 1.10.2rc3 (I copied the change in your PR in airflow/www_rbac/security.py of 1.10.2rc3), seems something is wrong:

I tried to log in using accounts with User/Viewer role, I can see Security/Admin tabs, and can even create new user or change permission of each roles.

I believe this is not desired. Please try and let me know if you can reproduce this issue.

feng-tao · 2019-01-22T01:35:13Z

thanks @XD-DENG , @kaxil , I know what is the issue now. I will work on a pr with test tonight.

feng-tao · 2019-01-22T01:35:43Z

@XD-DENG correct, I don't think that is the correct behavior.

XD-DENG · 2019-01-22T01:36:49Z

Thanks @feng-tao

feng-tao · 2019-01-22T03:10:28Z

PTAL @XD-DENG , @kaxil , code updated with unit test. I have tried on my end which works(viewer couldn't see the Admin role menu). Could you test it on your end and see if there are any issues?

codecov-io · 2019-01-22T03:19:39Z

Codecov Report

❗ No coverage uploaded for pull request base (master@98e8522). Click here to learn what that means.
The diff coverage is 93.33%.

@@            Coverage Diff            @@
##             master    #4569   +/-   ##
=========================================
  Coverage          ?   74.13%           
=========================================
  Files             ?      421           
  Lines             ?    27714           
  Branches          ?        0           
=========================================
  Hits              ?    20546           
  Misses            ?     7168           
  Partials          ?        0

Impacted Files	Coverage Δ
airflow/www/security.py	`92.85% <93.33%> (ø)`

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 98e8522...27ff881. Read the comment docs.

codecov-io · 2019-01-22T03:19:40Z

Codecov Report

Merging #4569 into master will increase coverage by 0.01%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #4569      +/-   ##
==========================================
+ Coverage   74.12%   74.13%   +0.01%     
==========================================
  Files         421      421              
  Lines       27700    27715      +15     
==========================================
+ Hits        20533    20547      +14     
- Misses       7167     7168       +1

Impacted Files	Coverage Δ
airflow/www/security.py	`92.89% <94.11%> (+0.04%)`	⬆️
airflow/models/__init__.py	`92.33% <0%> (-0.05%)`	⬇️
airflow/utils/dag_processing.py	`59.92% <0%> (+0.17%)`	⬆️

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 98e8522...31bfb65. Read the comment docs.

feng-tao · 2019-01-22T04:45:44Z

CI pass(k8s one is random, transient)

XD-DENG · 2019-01-22T05:31:35Z

Hi @feng-tao , I tried to apply your latest change. But the issue seems still there. Using Viewer/User, I can still see Security/Admin menus, and edit inside.

I'm using SQLite. Before testing, I have deleted the earlier .db file and run initdb again.

Not sure if I missed anything. @kaxil please help test as well so we can confirm.

Thanks!

feng-tao · 2019-01-22T05:37:03Z

This is the UI for a viewer / user (which doesn't have the security tab) on my end. Do you apply the latest one?

feng-tao · 2019-01-22T05:39:30Z

@XD-DENG , could you confirm if you apply this commit(31bfb65)?

XD-DENG · 2019-01-22T06:27:48Z

@feng-tao yes, that's what I applied. But please note what I did was applying it against the www_rbac/security.py in 1.10.2rc3 (on biz trip with a laptop that I can't install npm). This may be why we got different results?

I trust your testing. May you please test this fix on 1-10-test as well in case any conflict? Thank you.

feng-tao · 2019-01-22T07:01:08Z

@XD-DENG , I think it shouldn't matter with npm. We only use npm to build the static access which may just cause UI off. But I try with v-10-test branch which I don't see any issues.

@kaxil , do you think you could try out this branch on top v-10-test and see if you still see the issues or not?

cc @seelmann and see if you could try out and see whether the viewer issue still exists or not.

kaxil · 2019-01-22T09:51:25Z

I will test this tonight. Sorry, we have an Airflow meetup tomorrow so preparing for that as well and I am on the client side as well so can't work on Airflow from 9-5.

feng-tao · 2019-01-22T16:08:22Z

cc @jgao54 in case you have time to try out.

ashb

Does this change mean that the default roles that are created are to some extend un-editable?

feng-tao · 2019-01-22T16:13:07Z

@ashb , the default roles are all editable. This pr is to keep the behavior same as 10.1 which all the default roles could view the dag info detail page without Admin manually intervenes.

ashb · 2019-01-22T16:17:05Z

@ashb , the default roles are all editable. This pr is to keep the behavior same as 10.1 which all the default roles could view the dag info detail page without Admin manually intervenes.

Thanks. I haven't quite grokked how the RBAC roles and the Security Manager code works yet.

jgao54 · 2019-01-22T19:15:30Z

@feng-tao I think the same thing could be achieved by adding dag_perms to viewer under ROLE_CONFIGS... which should generate can_dag_read on all_dags/can_dag_edit on all_dags when init_role() method is called in sync_roles, assuming that's the goal of this PR.

feng-tao · 2019-01-22T20:40:32Z

thanks @jgao54 , correct, either way should work :)

jgao54 · 2019-01-22T22:57:48Z

In that case, less (code) is more :)
I don't think we need to explicitly sync all_dag access. We explicitly sync admin is because it's not part of ROLE_CONFIGS

feng-tao · 2019-01-25T01:32:51Z

thanks @jgao54 . Will update the pr tonight.

feng-tao · 2019-01-25T06:26:29Z

@jgao54 @XD-DENG Code is refactored. I tested it locally and worked as expected. Let me know if there is any concern.

@XD-DENG if possible, it would be good if you could test it on your side to verify as well.

feng-tao · 2019-03-05T19:08:20Z

@ashb , let's consolidate all the discussion in one place. And I could do some investigations in my night time(PST time zone). From what I understanding, your issues are:

you couldn't pause DAG with op role(https://github.com/apache/airflow/blob/master/airflow/www/views.py#L1657) ? Have you checked whether the refactored UI could pass the is_paused param to Flask? For pause, I am pretty sure it is not related to this pr.
your comment in [AIRFLOW-3271] Airflow RBAC Permissions modification via UI do not persist #4118 (comment) which all_dags permissions not shown until roles created. The latest version in master has changed which create the all_dag view-menu when the webserver starts(https://github.com/apache/airflow/blob/master/airflow/www/security.py#L460). And one thing we could do with https://github.com/apache/airflow/pull/4118/files is to make sure all_dags permissions are always exist in the existing roles which means user could only remove non dag permissions. WDYT?

ashb · 2019-03-05T19:39:03Z

It never gets to L1657, @has_dag_access issues a 302 before the view fn is ever run.
I'll cherry-pick this PR (which adds the change you suggest) in to 1.10.3 which will fix the problem for new installs. Adding all_dags to the existing roles (I assume you mean Admin,User,Op, not all roles in the DB?) would "work" but it seems counter to the purpose of [AIRFLOW-3271] Airflow RBAC Permissions modification via UI do not persist #4118, of letting those roles be editable by the admin of the site.

Let me have a look at how we might do this with a migration. If we could (easily, without loads of code) replace all the role syncing code at run time, which would also have the benefit of removing the mildly annoying "duplicate" log lines:

[2019-03-05 19:38:25,622] {security.py:437} INFO - Start syncing user roles.
[2019-03-05 19:38:25,634] {security.py:437} INFO - Start syncing user roles.
[2019-03-05 19:38:25,677] {security.py:437} INFO - Start syncing user roles.
[2019-03-05 19:38:25,684] {security.py:437} INFO - Start syncing user roles.
[2019-03-05 19:38:25,988] {security.py:195} INFO - Existing permissions for the role:Viewer within the database will persist.
[2019-03-05 19:38:26,085] {security.py:195} INFO - Existing permissions for the role:Viewer within the database will persist.
[2019-03-05 19:38:26,172] {security.py:195} INFO - Existing permissions for the role:Viewer within the database will persist.
[2019-03-05 19:38:26,183] {security.py:195} INFO - Existing permissions for the role:Viewer within the database will persist.
[2019-03-05 19:38:26,520] {security.py:195} INFO - Existing permissions for the role:User within the database will persist.
[2019-03-05 19:38:26,538] {security.py:195} INFO - Existing permissions for the role:User within the database will persist.
[2019-03-05 19:38:26,692] {security.py:195} INFO - Existing permissions for the role:User within the database will persist.
[2019-03-05 19:38:26,784] {security.py:195} INFO - Existing permissions for the role:User within the database will persist.
[2019-03-05 19:38:27,158] {security.py:195} INFO - Existing permissions for the role:Op within the database will persist.
[2019-03-05 19:38:27,165] {security.py:195} INFO - Existing permissions for the role:Op within the database will persist.
[2019-03-05 19:38:27,166] {security.py:195} INFO - Existing permissions for the role:Op within the database will persist.
[2019-03-05 19:38:27,169] {security.py:346} INFO - Fetching a set of all permission, view_menu from FAB meta-table
[2019-03-05 19:38:27,170] {security.py:346} INFO - Fetching a set of all permission, view_menu from FAB meta-table
[2019-03-05 19:38:27,173] {security.py:195} INFO - Existing permissions for the role:Op within the database will persist.
[2019-03-05 19:38:27,177] {security.py:346} INFO - Fetching a set of all permission, view_menu from FAB meta-table
[2019-03-05 19:38:27,203] {security.py:346} INFO - Fetching a set of all permission, view_menu from FAB meta-table
[2019-03-05 19:38:28,254] {security.py:299} INFO - Cleaning faulty perms
[2019-03-05 19:38:28,266] {security.py:299} INFO - Cleaning faulty perms
[2019-03-05 19:38:28,268] {security.py:299} INFO - Cleaning faulty perms
[2019-03-05 19:38:28,289] {security.py:299} INFO - Cleaning faulty perms

(Cos each worker runs the checks, we do this for every worker at start, and then every 30s as the new worker cycles).

feng-tao · 2019-03-05T19:57:16Z

@ashb , I see. Your deploy is based on 1.10.2 not on master? I think I fail to get this pr into 1.10.2 as the release timeline was pretty tight around the time. And I think someone said that just use UI to modify the permissions as a work around.

Currently it only adds to those default roles to keep it the same behaviour as 1.10.1, but not those DAG level roles. And we do plan to write a blog on how we do DAG level access at Lyft, but just don't have enough time ATM.

In terms of migrations from 1.10.1 to 1.10.2, I am not sure if it is easy to do as Airflow doesn't have the FAB model code which makes the db migration difficult. Let me know if you figure it out the right approach.

ashb · 2019-03-05T21:05:08Z

Correct, yeah I've been testing the v1-10-stable branch to prepare 1.10.3 - though the problem would also apply to any upgraded install from 1.10 to what becomes master.

So I feel we need to fix this in some way that gives an upgrade path form 1.10.x now to 2.0 as it will be - that doesn't involve manually adding things in the UI.

feng-tao · 2019-03-05T21:06:58Z

@ashb , #4845 for removing viewer edit permissions.

feng-tao · 2019-03-05T21:07:59Z

correct, we shouldn't rely on UI for the operations...

ashb · 2019-03-05T21:19:03Z

I'm sort of picturing something like (very pseudo code):

from something import role_migrations as rm

def up():
    p = rm.add_permission('all_dags', 'can_dag_edit')
    rm.add_to_role('Op', p)

def down():
    rm.remove_from_role('Op', 'all_dags', 'can_dag_edit')

I haven't worked out the details yet, still a bit hazy (and I'm not 100% on the FAB data model around permissions, view menus, roles and all the many-to-many tables between them.)

feng-tao · 2019-03-05T21:30:35Z

@ashb https://flask-appbuilder.readthedocs.io/en/latest/diagrams.html FYI

XD-DENG · 2019-03-05T23:02:03Z

I have a question to seek @feng-tao ‘s input given we have started the discussion on this: 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 “all_dags”. (but from another side, I also understand it’s easier to just simply give Admin ALL permissions)

May you let me know your thoughts on this?

feng-tao · 2019-03-05T23:06:07Z

@XD-DENG , I don't think it will add 2000 entries to the db for admin role. It only adds the "all_dags" view-menu with the 2 permissions into the table.

XD-DENG · 2019-03-05T23:08:20Z

Thanks @feng-tao .
let me double-check the code later today and will let you know if I still have confusion on it.

ashb · 2019-03-05T23:08:50Z

Oh thanks, I hadn't seen that diagram!

XD-DENG · 2019-03-06T00:50:27Z

@XD-DENG , I don't think it will add 2000 entries to the db for admin role. It only adds the "all_dags" view-menu with the 2 permissions into the table.

Hi @feng-tao please refer to

airflow/airflow/www/security.py

Lines 440 to 453 in dda309e

    
               def update_admin_perm_view(self): 
        
                   """ 
        
                   Admin should have all the permission-views. 
        
                   Add the missing ones to the table for admin. 
        
                   :return: None. 
        
                   """ 
        
                   pvms = self.get_session.query(sqla_models.PermissionView).all() 
        
                   pvms = [p for p in pvms if p.permission and p.view_menu] 
        
                   admin = self.find_role('Admin') 
        
                   admin.permissions = list(set(admin.permissions) | set(pvms)) 
        
                   self.get_session.commit()

By checking the code itself, seems it's simply adding all whatever permissions to Admin role.

Below is the version before my set-related refactoring:

airflow/airflow/www/security.py

Lines 440 to 452 in c552996

    
                   :return: None. 
        
                   """ 
        
                   pvms = self.get_session.query(sqla_models.PermissionView).all() 
        
                   pvms = [p for p in pvms if p.permission and p.view_menu] 
        
                   admin = self.find_role('Admin') 
        
                   existing_perms_vms = set(admin.permissions) 
        
                   for p in pvms: 
        
                       if p not in existing_perms_vms: 
        
                           existing_perms_vms.add(p) 
        
                   admin.permissions = list(existing_perms_vms) 
        
                   self.get_session.commit()

feng-tao · 2019-03-06T05:51:11Z

@XD-DENG somehow I thought it has only all_dags , but it seems it has everything. ATM, I didn't have time to dig deep to do this without a hacky solution.

I would suggest let's leave it as it is for now.

XD-DENG · 2019-03-06T06:21:18Z

@feng-tao sure. Maybe let me create a ticket for this later maybe. Will tag you then for our reference.

ashb · 2019-03-20T19:06:54Z

@feng-tao I haven't managed to finish it but I got somewhere with migrations to deal with perms:

https://gist.github.com/ashb/f43741740fb0eae59948d52634cda575

The biggest issue is working out 1) what query to run, or 2) working out how to call the right classes/methods from within fab.security.* to update.

feng-tao · 2019-03-22T04:41:05Z

@ashb , thanks. BTW, what do you think of moving https://github.com/apache/airflow/blob/master/airflow/www/security.py#L35-L140 inside AirflowSecurityManager so that we could redefine permission on different roles by subclassing the security manager?

ashb · 2019-03-22T10:35:35Z

Yes that probably makes sense to do.

feng-tao · 2019-03-23T03:26:38Z

@ashb , pr posted (#4960)

All of the code for this has been already applied in resolving conflicts in previous cherry picks, so this just brings along the tests. Mostly so that future PRs don't conflict (cherry picked from commit c2f48ed)

kaxil reviewed Jan 21, 2019

View reviewed changes

feng-tao force-pushed the tfeng_fix_perm branch from b059ecd to bec7b10 Compare January 22, 2019 02:34

feng-tao force-pushed the tfeng_fix_perm branch from bec7b10 to 27ff881 Compare January 22, 2019 03:17

feng-tao force-pushed the tfeng_fix_perm branch from 27ff881 to 31bfb65 Compare January 22, 2019 03:19

ashb reviewed Jan 22, 2019

View reviewed changes

[AIRFLOW-3745] Fix viewer not able to view dag details

dd25379

feng-tao force-pushed the tfeng_fix_perm branch from 31bfb65 to dd25379 Compare January 25, 2019 06:22

ashb mentioned this pull request Mar 5, 2019

[AIRFLOW-3271] Airflow RBAC Permissions modification via UI do not persist #4118

Merged

6 tasks

feng-tao mentioned this pull request Mar 6, 2019

[AIRFLOW-2694] Declare permissions in DAG definition #4642

Merged

XD-DENG mentioned this pull request Mar 6, 2019

[AIRFLOW-4028] Avoid Adding DAG_PERMS of All DAGs to Admin Role #4856

Closed

wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-3745] Fix viewer not able to view dag details (apache#4569)

676b570

[AIRFLOW-3745] Fix viewer not able to view dag details #4569

[AIRFLOW-3745] Fix viewer not able to view dag details #4569

Conversation

feng-tao commented Jan 21, 2019

Jira

Description

Tests

Commits

Documentation

Code Quality

feng-tao commented Jan 21, 2019

kaxil left a comment

Choose a reason for hiding this comment

XD-DENG commented Jan 22, 2019 • edited Loading

feng-tao commented Jan 22, 2019

feng-tao commented Jan 22, 2019

XD-DENG commented Jan 22, 2019

feng-tao commented Jan 22, 2019

codecov-io commented Jan 22, 2019

Codecov Report

codecov-io commented Jan 22, 2019 • edited Loading

Codecov Report

feng-tao commented Jan 22, 2019

XD-DENG commented Jan 22, 2019

feng-tao commented Jan 22, 2019 • edited Loading

feng-tao commented Jan 22, 2019

XD-DENG commented Jan 22, 2019

feng-tao commented Jan 22, 2019

kaxil commented Jan 22, 2019 • edited Loading

feng-tao commented Jan 22, 2019

ashb left a comment

Choose a reason for hiding this comment

feng-tao commented Jan 22, 2019

ashb commented Jan 22, 2019

jgao54 commented Jan 22, 2019 • edited Loading

feng-tao commented Jan 22, 2019

jgao54 commented Jan 22, 2019

feng-tao commented Jan 25, 2019

feng-tao commented Jan 25, 2019

feng-tao commented Mar 5, 2019

ashb commented Mar 5, 2019

feng-tao commented Mar 5, 2019 • edited Loading

ashb commented Mar 5, 2019

feng-tao commented Mar 5, 2019

feng-tao commented Mar 5, 2019

ashb commented Mar 5, 2019

feng-tao commented Mar 5, 2019

XD-DENG commented Mar 5, 2019

feng-tao commented Mar 5, 2019

XD-DENG commented Mar 5, 2019

ashb commented Mar 5, 2019

XD-DENG commented Mar 6, 2019 • edited Loading

feng-tao commented Mar 6, 2019

XD-DENG commented Mar 6, 2019

ashb commented Mar 20, 2019

feng-tao commented Mar 22, 2019

ashb commented Mar 22, 2019

feng-tao commented Mar 23, 2019

XD-DENG commented Jan 22, 2019 •

edited

Loading

codecov-io commented Jan 22, 2019 •

edited

Loading

feng-tao commented Jan 22, 2019 •

edited

Loading

kaxil commented Jan 22, 2019 •

edited

Loading

jgao54 commented Jan 22, 2019 •

edited

Loading

feng-tao commented Mar 5, 2019 •

edited

Loading

XD-DENG commented Mar 6, 2019 •

edited

Loading