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

Anonymous users aren't able to view DAGs even with Admin Role #13340

Closed
AmarEL opened this issue Dec 28, 2020 · 12 comments · Fixed by #14042
Closed

Anonymous users aren't able to view DAGs even with Admin Role #13340

AmarEL opened this issue Dec 28, 2020 · 12 comments · Fixed by #14042
Assignees
Labels
affected_version:2.0 Issues Reported for 2.0 area:auth kind:bug This is a clearly a bug
Milestone

Comments

@AmarEL
Copy link
Contributor

AmarEL commented Dec 28, 2020

Apache Airflow version: 2.0.0 (Current master)

Kubernetes version (if you are using kubernetes) (use kubectl version):

Environment:

  • Cloud provider or hardware configuration:
  • OS (e.g. from /etc/os-release): Ubuntu 20.04.1 LTS
  • Kernel (e.g. uname -a): Linux ubuntu 5.4.0-58-generic Hive Metastore Browser plugin #64-Ubuntu SMP Wed Dec 9 08:16:25 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
  • Install tools:
  • Others:

webserver_config.py file config:

# Uncomment to setup Public role name, no authentication needed
AUTH_ROLE_PUBLIC = 'Admin'

What happened:

After disabling the authentication, all users are identified as "Anonymous User" and no dags are load on the screen because there is a method that returns an empty set for roles when a user is anonymous.

views.py file:

# Get all the dag id the user could access
filter_dag_ids = current_app.appbuilder.sm.get_accessible_dag_ids(g.user)

security.py file:

    def get_accessible_dags(self, user_actions, user, session=None):
        """Generic function to get readable or writable DAGs for authenticated user."""
        if user.is_anonymous:
            return set()

        user_query = (
            session.query(User)
            .options(
                joinedload(User.roles)
                .subqueryload(Role.permissions)
                .options(joinedload(PermissionView.permission), joinedload(PermissionView.view_menu))
            )
            .filter(User.id == user.id)
            .first()
        )
        resources = set()
        for role in user_query.roles:
        ...

What you expected to happen:

Since the option to disable login exists, I expect that all anonymous users have the Role specified in the webserver_config.py file in the AUTH_ROLE_PUBLIC entry.

It will make anonymous users able to see/edit dags if the roles specified as the default for anonymous users match the DAG roles.

How to reproduce it:

Set the following entry in webserver_config.py file config to disable authentication and make all users anonymous with the 'Admin" role:

# Uncomment to setup Public role name, no authentication needed
AUTH_ROLE_PUBLIC = 'Admin'

With the current master branch installed, run
airflow webserver

No DAGs will appear:

image

Anything else we need to know:

The methods have explicit comments about being used for authenticated user:

def get_accessible_dags(self, user_actions, user, session=None):
"""Generic function to get readable or writable DAGs for authenticated user."""

But there is no way for anonymous users to be able to see DAGs on the screen without modifying the behavior of this method.

@AmarEL AmarEL added the kind:bug This is a clearly a bug label Dec 28, 2020
@potiuk
Copy link
Member

potiuk commented Dec 28, 2020

I believe this is intended behaviour, not sure if that was the intention, but I would see it highly problematic (and dangerous) to give the anonymous user the Admin (or any in that respect) right. I believe the whole purpose of RBAC interface is to enforce authentication for airflow users. @jhtimmins @ryanahamilton WDYT?

@mik-laj
Copy link
Member

mik-laj commented Dec 28, 2020

This looks like something we can fix. I am able to imagine several environments where authentication is not needed because the access control is done on a different layer. This configuration is supported by Flask App Builder, so it makes sense for Airflow to support it too.

@mik-laj
Copy link
Member

mik-laj commented Dec 28, 2020

@AmarEL Are you willing to submit a PR?

@AmarEL
Copy link
Contributor Author

AmarEL commented Dec 28, 2020

Yep!

@potiuk
Copy link
Member

potiuk commented Dec 28, 2020

This looks like something we can fix. I am able to imagine several environments where authentication is not needed because the access control is done on a different layer. This configuration is supported by Flask App Builder, so it makes sense for Airflow to support it too.

Yeah. It is supported - but is it intended ? This is my question. I am not sure if this is something that we want to, that's why I would love to hear other's opinion on that if it was a deliberate choice or an acident (I honestly do not know).

@mik-laj
Copy link
Member

mik-laj commented Dec 28, 2020

@potiuk I can imagine that some DAGs will be read-only to the public. Giving full administrator privileges is an edge case, but giving read-only one or two DAGs is something that makes sense to support. I looked at the code quickly and I don't find it difficult to add support for this use case.

I think if we improve this one SQL query and use get_user_roles instead of our own SQL query it should work.

If the user can configure their own role it looks as if this was provided functionality by Flask App Builder. Some actions then require an additional user.is_anonymous case to be considered, but it has low impact

@potiuk
Copy link
Member

potiuk commented Dec 28, 2020

Yeah. The 'read' use case is s convincing for sure. I thought about it myself.

Just want to make sure that there was no reason behind not letting it in the first place and that there is no reason (like potential security problem which we might not be aware of) for disallowing it.

@AmarEL
Copy link
Contributor Author

AmarEL commented Dec 29, 2020

Maybe I exaggerated in my example with an Admin role rsrsrs
But we have some cases here at my job with unauthenticated users sometimes accessing the webserver to take a look in some dag run status (we teached them how to "read" tree view). And of course, our webserver is running on a private network that make us less worried about disable the authentication option.

@mik-laj I already did some tests using the get_user_roles instead method and it works to show the dags on the home page, but for accessing dag another error occurred. I will continue working on it.

@mik-laj
Copy link
Member

mik-laj commented Dec 29, 2020

@AmarEL I assigned you to this ticket.

@potiuk
Copy link
Member

potiuk commented Dec 29, 2020

Maybe I exaggerated in my example with an Admin role rsrsrs

So maybe part of the PR will be to only allow READ access for unauthenticated users and fail configuration if any WRITE access is configured? I think this might be both - much more secure and serve the use case that you are talking about ?

@XD-DENG
Copy link
Member

XD-DENG commented Jan 2, 2021

I doubt if we want to have this behaviour change: if the Admin wants to allow Public Role to view specific DAGs (or all DAGs), it's very easy to configure it in the UI as Admin role. There is no need to make any explicit change in my opinion.

@AmarEL
Copy link
Contributor Author

AmarEL commented Jan 2, 2021

The actual behavior without changes is problematic too because if someone sets the AUTH_ROLE_PUBLIC = "Admin, the DAGs will not be displayed, but the anonymous user still able to view and access some Menus like the "Admin". So, an Anonymous user with the role Admin can view/edit some sensitive data, but can't view/edit dags.

Or another example could be if I create a new role "Anonymous" and attach just the View Dags Permission to this role, the user will still not able to view the dags without changing the code.

So, I think that some change is necessary.

And I appreciated the @potiuk suggestion and I still looking for it in another branch, but for

to only allow READ access for unauthenticated users and fail configuration if any WRITE access is configured

more changes are necessary for methods related to Menu Views and other stuff.

I'm not sure if is better to fix the behavior that I wrote in this issue and document all these details very well to let the developer configure this correctly if he wants to give READ permission for an anonymous user or deny any READ permission for an anonymous user (it needs to be very well documented too).

@vikramkoka vikramkoka added affected_version:2.0 Issues Reported for 2.0 area:auth labels Jan 18, 2021
@kaxil kaxil modified the milestones: Airflow 2.1, Airflow 2.0.2 Feb 1, 2021
kaxil pushed a commit that referenced this issue Feb 4, 2021
Fixes the issue wherein regardless of what role anonymous users are assigned (via the `AUTH_ROLE_PUBLIC` env var), they can't see any DAGs.

Current behavior causes:
Anonymous users are handled as a special case by Airflow's DAG-related security methods (`.has_access()` and `.get_accessible_dags()`). Rather than checking the `AUTH_ROLE_PUBLIC` value to check for role permissions, the methods reject access to view or edit any DAGs.

Changes in this PR:
Rather than hardcoding permission rules inside the security methods, this change checks the `AUTH_ROLE_PUBLIC` value and gives anonymous users all permissions linked to the designated role. 

**This places security in the hands of the Airflow users. If the value is set to `Admin`, anonymous users will have full admin functionality.**

This also changes how the `Public` role is created. Currently, the `Public` role is created automatically by Flask App Builder. This PR explicitly declares `Public` as a default role with no permissions in `security.py`. This change makes it easier to test.

closes: #13340
kaxil pushed a commit that referenced this issue Feb 4, 2021
Fixes the issue wherein regardless of what role anonymous users are assigned (via the `AUTH_ROLE_PUBLIC` env var), they can't see any DAGs.

Current behavior causes:
Anonymous users are handled as a special case by Airflow's DAG-related security methods (`.has_access()` and `.get_accessible_dags()`). Rather than checking the `AUTH_ROLE_PUBLIC` value to check for role permissions, the methods reject access to view or edit any DAGs.

Changes in this PR:
Rather than hardcoding permission rules inside the security methods, this change checks the `AUTH_ROLE_PUBLIC` value and gives anonymous users all permissions linked to the designated role.

**This places security in the hands of the Airflow users. If the value is set to `Admin`, anonymous users will have full admin functionality.**

This also changes how the `Public` role is created. Currently, the `Public` role is created automatically by Flask App Builder. This PR explicitly declares `Public` as a default role with no permissions in `security.py`. This change makes it easier to test.

closes: #13340
(cherry picked from commit 78aa921)
kaxil pushed a commit that referenced this issue Feb 4, 2021
Fixes the issue wherein regardless of what role anonymous users are assigned (via the `AUTH_ROLE_PUBLIC` env var), they can't see any DAGs.

Current behavior causes:
Anonymous users are handled as a special case by Airflow's DAG-related security methods (`.has_access()` and `.get_accessible_dags()`). Rather than checking the `AUTH_ROLE_PUBLIC` value to check for role permissions, the methods reject access to view or edit any DAGs.

Changes in this PR:
Rather than hardcoding permission rules inside the security methods, this change checks the `AUTH_ROLE_PUBLIC` value and gives anonymous users all permissions linked to the designated role.

**This places security in the hands of the Airflow users. If the value is set to `Admin`, anonymous users will have full admin functionality.**

This also changes how the `Public` role is created. Currently, the `Public` role is created automatically by Flask App Builder. This PR explicitly declares `Public` as a default role with no permissions in `security.py`. This change makes it easier to test.

closes: #13340
(cherry picked from commit 78aa921)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affected_version:2.0 Issues Reported for 2.0 area:auth kind:bug This is a clearly a bug
Projects
None yet
6 participants