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

Remove deprecated usage of init_role() from API #18820

Merged
merged 3 commits into from
Oct 12, 2021

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Oct 8, 2021

Some cleanups found when investigating #18813.

init_role() has been replaced by bulk_update_roles(). This PR also slightly tweaked the ordering of operations so bulk_sync_roles() is only called when there's a need to update permissions, and added a few lines in the test to make sure role rename is performed successfully.

@uranusjr uranusjr requested a review from ephraimbuddy October 8, 2021 02:59
@uranusjr uranusjr requested review from kaxil and mik-laj as code owners October 8, 2021 02:59
@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Oct 8, 2021
@kaxil kaxil added this to the Airflow 2.2.1 milestone Oct 8, 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 Oct 8, 2021
@github-actions
Copy link

github-actions bot commented Oct 8, 2021

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.

@uranusjr
Copy link
Member Author

uranusjr commented Oct 8, 2021

I just realised instead of "name" not in data, this should check for emptiness and equality like the user endpoint.

@alex-astronomer
Copy link
Contributor

alex-astronomer commented Oct 9, 2021

Not sure how to request permission to add to this PR but will you please add this test to confirm that #18813 was fixed?

    def test_patch_updates_correct_roles_permissions(self):
        """
        Issue described in #18813, fixed by #18820
        """
        role_to_change = create_role(self.app, "patched_role")
        existing_role = create_role(self.app, "already_exists")
        assert len(role_to_change.permissions) == 0
        assert len(existing_role.permissions) == 0
        payload = {
            "name": "already_exists",
            "actions": [{"action": {"name": "can_delete"}, "resource": {"name": "XComs"}}],
        }
        response = self.client.patch(
            f"/api/v1/roles/{role_to_change.name}", json=payload, environ_overrides={"REMOTE_USER": "test"}
        )
        assert response.status_code == 200
        # have to use find_role because the session becomes detached during the test from the
        # previous role definitions
        assert len(self.app.appbuilder.sm.find_role("patched_role").permissions) == 1
        assert len(self.app.appbuilder.sm.find_role("already_exists").permissions) == 0
        expected_permission = role_to_change.permissions[0]
        assert (expected_permission.permission.name, expected_permission.view_menu.name) == (
            "can_delete",
            "XComs",
        )

This test fails on main but is corrected with #18820 (this PR)

The function has been replaced by bulk_update_roles(). This PR also
slightly tweaked the ordering of operations so bulk_sync_roles() is only
called when there's a need to update permissions, and added a few lines
in the test to make sure role rename is performed successfully.
@uranusjr uranusjr force-pushed the api-user-patch-modern branch from 5668ed4 to 01ab438 Compare October 12, 2021 00:23
@uranusjr uranusjr merged commit 9c2eb9d into apache:main Oct 12, 2021
@uranusjr uranusjr deleted the api-user-patch-modern branch October 12, 2021 01:14
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API okay to merge It's ok to merge this PR as it does not require more tests type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants