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

Fix inconsitencies in checking edit permissions for a DAG #20346

Merged
merged 3 commits into from
Jan 14, 2023

Conversation

kaxil
Copy link
Member

@kaxil kaxil commented Dec 16, 2021

We were short-circuting permission in the views instead of letting the security manager handle that. A user will find it inconsistent as the Graph and other views check "per-dag" permissions via

dag.can_edit = current_app.appbuilder.sm.can_edit_dag(dag.dag_id)

so if someone uses Custom Security Manager that will end up with user not being able to "pause" DAG from individual dag page but would be able to do so from Homepage. This PR fixes this inconsistency and gives back this responsibility of permissions to security manager instead if Views.


^ 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.

@kaxil kaxil requested review from ashb and jhtimmins December 16, 2021 13:54
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Dec 16, 2021
@bbovenzi
Copy link
Contributor

This looks right to me but I had issues with my local setup to test nor is python my strong suit.

@jedcunningham
Copy link
Member

Fwiw, this isn't how permissions are documented:
https://airflow.apache.org/docs/apache-airflow/stable/security/access-control.html#dag-level-permissions

Have we tried this in an instance with a large number of DAGs?

@kaxil
Copy link
Member Author

kaxil commented Dec 17, 2021

Fwiw, this isn't how permissions are documented: https://airflow.apache.org/docs/apache-airflow/stable/security/access-control.html#dag-level-permissions

Have we tried this in an instance with a large number of DAGs?

current_app.appbuilder.sm.get_editable_dag_ids(g.user) logically does the same thing what existed before the PR change too -- however instead of checking the permission in the view, it hands of the permissions of doing it to the Security Manager. So if you have "can_edit" permission on the "DAG" resource, a user will by default still be able to edit/pause all the dags.

re: Optimization -- we have a short-circuit too, i.e. as soon as we find "can_edit" on "DAG" we return, check the following source:

Source Code:

def get_editable_dag_ids(self, user) -> Set[str]:
"""Gets the DAG IDs editable by authenticated user."""
return self.get_accessible_dag_ids(user, [permissions.ACTION_CAN_EDIT])

resource = permission.resource.name
if resource == permissions.RESOURCE_DAG:
return {dag.dag_id for dag in session.query(DagModel.dag_id)}

@jedcunningham
Copy link
Member

I'm just saying that, regardless of what security manager is in use, global can_edit should be expected to give edit on all DAGs. We shouldn't assume all things will use get_editable_dags_ids. Maybe we add a new can_edit_all_dags as a consistent way to check for that (and that assumption-breaking security managers can always return as false)?

It'll eventually short-circuit, but in the worst case scenario what impact do we have by not using the more efficient short circuit?

@kaxil
Copy link
Member Author

kaxil commented Dec 18, 2021

Add generic all read and all edit function

What do you think about - 66a7033?

@jedcunningham
Copy link
Member

Looks good.

@kaxil kaxil added this to the Airflow 2.2.4 milestone Dec 18, 2021
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Dec 18, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Another test error is

sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Permission at 0x7efe63460b10>
is not bound to a Session; lazy load operation of attribute 'action' cannot proceed

It’s emitted from get_current_user_permissions, but I’m struggling to find where this PR changed could cause this incorrect join.

airflow/www/security.py Outdated Show resolved Hide resolved
@jedcunningham jedcunningham removed this from the Airflow 2.2.4 milestone Feb 1, 2022
@joshuayeung
Copy link
Contributor

Should we make the can_create_dag_run also handled by the security manager?

@eladkal
Copy link
Contributor

eladkal commented May 20, 2022

Do we want to include this in 2.3.1?

@jedcunningham
Copy link
Member

Too late for 2.3.1 I'm afraid.

@ashb ashb added this to the Airflow 2.3.2 milestone May 21, 2022
@jedcunningham
Copy link
Member

@kaxil, looks like we lost changes when this happened: e20e8f6 to ea7f928

Kicking this to 2.3.4, but we can pull it back if this gets done soon enough.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@ashb ashb added this to the Airflow 2.4.1 milestone Sep 8, 2022
@kaxil
Copy link
Member Author

kaxil commented Oct 4, 2022

Yeah still needed

@kaxil
Copy link
Member Author

kaxil commented Oct 4, 2022

I will try to get to it in next couple of weeks but if I don't get to it feel free to take-over / close PR -- it is a minor one

@ephraimbuddy
Copy link
Contributor

cc: @kaxil

@ephraimbuddy ephraimbuddy modified the milestones: Airflow 2.4.3, Airflow 2.4.4 Nov 9, 2022
@potiuk
Copy link
Member

potiuk commented Nov 14, 2022

Needs rebase I am afraid.

@ephraimbuddy ephraimbuddy modified the milestones: Airflow 2.4.4, Airflow 2.5.0, Airflow 2.5.1 Nov 23, 2022
@kaxil kaxil force-pushed the dag-level-ediit branch 2 times, most recently from 6488e72 to 9d27cda Compare December 28, 2022 18:34
We were short-circuting permission in the views instead of letting the security manager handle that. A user will find it inconsistent as the Graph and other views check "per-dag" permissions via https://github.com/apache/airflow/blob/174681911f96f17d41a4f560ca08d5e200944f7f/airflow/www/views.py#L579

so if someone uses Custom Security Manager that will end up with user not being able to "pause" DAG from individual dag page but would be able to do so from Homepage. This PR fixes this inconsistency and gives back this responsibility of permissions to security manager instead if Views.
@kaxil kaxil merged commit 87ea8ac into apache:main Jan 14, 2023
@kaxil kaxil deleted the dag-level-ediit branch January 14, 2023 21:21
@pierrejeambrun pierrejeambrun added the type:bug-fix Changelog: Bug Fixes label Feb 27, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 6, 2023
We were short-circuting permission in the views instead of letting the security manager handle that. A user will find it inconsistent as the Graph and other views check "per-dag" permissions via https://github.com/apache/airflow/blob/174681911f96f17d41a4f560ca08d5e200944f7f/airflow/www/views.py#L579

so if someone uses Custom Security Manager that will end up with user not being able to "pause" DAG from individual dag page but would be able to do so from Homepage. This PR fixes this inconsistency and gives back this responsibility of permissions to security manager instead if Views.

(cherry picked from commit 87ea8ac)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants