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

Add readonly REST API endpoint for roles and permissions #14664

Merged
merged 28 commits into from
Mar 16, 2021

Conversation

ephraimbuddy
Copy link
Contributor

This PR seeks to add readonly endpoints for roles and permissions.
Because this two endpoints share a lot together, I decided to have the code for both in one file instead of separate files. Let me know if this should be separated


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Mar 8, 2021
@github-actions
Copy link

github-actions bot commented Mar 8, 2021

The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the workflow link to check the reason.

@ephraimbuddy ephraimbuddy force-pushed the add-role-and-permission-endpoints branch 2 times, most recently from 958b92d to 0f98cc7 Compare March 9, 2021 22:43
@ephraimbuddy ephraimbuddy marked this pull request as ready for review March 10, 2021 14:50
@ephraimbuddy
Copy link
Contributor Author

I'm lost at what permissions should be used to protect these views @jhtimmins

@ephraimbuddy ephraimbuddy force-pushed the add-role-and-permission-endpoints branch from 0f98cc7 to 460bbb4 Compare March 10, 2021 21:24
@mik-laj
Copy link
Member

mik-laj commented Mar 11, 2021

do these endpoints work properly when environments use LDAP or Auth proxy?

@ephraimbuddy
Copy link
Contributor Author

ephraimbuddy commented Mar 11, 2021

do these endpoints work properly when environments use LDAP or Auth proxy?

These endpoints are similar to what you would see when you go to Security > List Roles on the webserver. So it will work properly with any auth type since it just reads DB roles and permissions which are part of airflow. That's what I understand, unless I'm missing something. What do you think?

@mik-laj
Copy link
Member

mik-laj commented Mar 11, 2021

These endpoints are similar to what you would see when you go to Security > List Roles on the webserver. So it will work properly with any auth type since it just reads DB roles and permissions which are part of airflow. That's what I understand, unless I'm missing something. What do you think?

This is very confusing to end users. They don't know all the limitations we have in the project. A similar situation occurs when the user uses the Backend Secret. See: https://apache-airflow.slack.com/archives/CCR6P6JRL/p1615394435242400 We should think about how to limit similar surprises because it takes users hours/days to discover that everything works as intended, but they misunderstood our product. This was one of the reasons this endpoint did not appear in the first version of the API.

API for optional components

The goal is not to develop an API for non-fundamental components. There are expectations to develop an API that provides additional features. For example:

allows access to node information when using CeleryExecutor,
for deeper integration between Airflow and Kubernetes when KubernetesExecutor is used
for monitoring
Integration with these components will be covered in other documents.

https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-32%3A+Airflow+REST+API

As the simplest solution, we can disable this endpoint if it doesn't work properly in a current configuration, but we can also write a smart solution.

@ephraimbuddy
Copy link
Contributor Author

ephraimbuddy commented Mar 11, 2021

As the simplest solution, we can disable this endpoint if it doesn't work properly in a current configuration, but we can also write a smart solution.

These would certainly cause confusion for people checking roles and permissions just as it currently does on the UI.
Curious on what a smart solution would be. Is there a way we can see Roles and permissions from an LDAP server through airflow? My understanding is that we can get roles and permissions for the current_user but not all that're available in the server. 🤔

@mik-laj
Copy link
Member

mik-laj commented Mar 11, 2021

These would certainly cause confusion for people checking roles and permissions just as it currently does on the UI.

In Web UI, we should also disable it if it doesn't work properly.

Is there a way we can see Roles and permissions from an LDAP server through airflow?

Probably, when you pass * as the username to _search_ldap method, the list of all users will be stored in the search_resulsts variable, but I did not check it.
https://github.com/dpgaspar/Flask-AppBuilder/blob/dbe1eded6369c199b777836eb08d829ba37634d7/flask_appbuilder/security/manager.py#L845

@ephraimbuddy
Copy link
Contributor Author

These would certainly cause confusion for people checking roles and permissions just as it currently does on the UI.

In Web UI, we should also disable it if it doesn't work properly.

Is there a way we can see Roles and permissions from an LDAP server through airflow?

Probably, when you pass * as the username to _search_ldap method, the list of all users will be stored in the search_resulsts variable, but I did not check it.
https://github.com/dpgaspar/Flask-AppBuilder/blob/dbe1eded6369c199b777836eb08d829ba37634d7/flask_appbuilder/security/manager.py#L845

Hi @mik-laj , after going through the code, I see that we are good with this implementation.
Whatever authentication method that is been used, roles for users are set with AUTH_ROLES_MAPPING config or AUTH_USER_REGISTRATION_ROLE if users are to be registered to the DB.

These roles must be one of the roles in FAB DB, that's why it's calculated here for LDAP.

And you can see where roles in AUTH_ROLES_MAPPING are being searched here

I believe we are good returning roles available in airflow because that's what auths use.

The same thing applies to users endpoint, if user registration is set, users are created in FAB DB, if not, then no users in DB and there'll not be accident of returning users that was not added through LDAP or remote user.

So I think this is different from secrets 🙁

@ephraimbuddy ephraimbuddy force-pushed the add-role-and-permission-endpoints branch 2 times, most recently from 2a478bc to 0643192 Compare March 11, 2021 23:47
@ephraimbuddy
Copy link
Contributor Author

Removed mistakenly added users endpoint

nullable: false

ActionResource:
description: The Action-Resource permission item
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the different between a permission and an action resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ephraimbuddy ephraimbuddy force-pushed the add-role-and-permission-endpoints branch from 5b64a50 to b78873e Compare March 15, 2021 21:37
tests/test_utils/api_connexion_utils.py Outdated Show resolved Hide resolved
tests/test_utils/fab_utils.py Outdated Show resolved Hide resolved
@ashb ashb merged commit 0e13458 into apache:master Mar 16, 2021
@ashb ashb deleted the add-role-and-permission-endpoints branch March 16, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-38 Modern Web Application area:API Airflow's REST/HTTP API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants