/role PATCH endpoint bug when name gets changed to a role that already exists

[alex-astronomer]

Apache Airflow version

2.2.0b2 (beta snapshot)

Operating System

macOS Big Sur 11.3.1

Versions of Apache Airflow Providers

n/a

Deployment

Virtualenv installation

Deployment details

stock airflow installation with newest main branch

What happened

When the PATCH verb is called for /roles/{role_name} endpoint, the body sent allows for a name to be changed. For example:

PATCH http://localhost:8080/api/v1/roles/Viewer

Body:

{
    "name": "test",
    "actions": [
        {
            "action": {
                "name": "can_delete"
            },
            "resource": {
                "name": "DAGs"
            }
        }
    ]
}

If the role test already exists in the system, no changes will be made to the Viewer role, but the changes in the body will be made to the role that has the name specified in the body.

What you expected to happen

I believe that this is a problem because when we run PATCH /roles/Viewer we expect the changes being made to be to the Viewer, and not the role with the name from the body.

I expect that an error message would be thrown if the name from the body already exists as a role in the Airflow meta DB.

How to reproduce

1. Create a role called test with no permissions.
2. Run PATCH /roles/Viewer with the body

{
    "name": "test",
    "actions": [
        {
            "action": {
                "name": "can_delete"
            },
            "resource": {
                "name": "DAGs"
            }
        }
    ]
}

3. Refresh the Airlfow UI and notice that there were no changes to the Viewer, but the test role now has can delete on DAGs permissions.

Anything else

No response

Are you willing to submit PR?

• Yes I am willing to submit a PR!

Code of Conduct

• I agree to follow this project's Code of Conduct

[uranusjr]

Since the web UI allows changing a permission's name (albeit changing one of the default permissions will cause issues… that's not what we need to care here), specifying the name field should trigger a rename of that role, similar to #18757 for users.

[uranusjr]

I added the following test checks but can't reproduce:

 def test_patch_should_respond_200(self, payload, expected_name, expected_actions):
    role = create_role(self.app, 'mytestrole')
    response = self.client.patch(
        f"/api/v1/roles/{role.name}", json=payload, environ_overrides={'REMOTE_USER': "test"}
    )
    assert response.status_code == 200
    assert response.json['name'] == expected_name
    assert response.json["actions"] == expected_actions

    with create_session() as session:
        role_names = {name for name, in session.query(Role.name)}
    assert expected_name in role_names
    assert "mytestrole" not in role_names

The old role was renamed correctly and tests pass.

I'm wondering if it makes a difference you are editing a built-in role? IIRC Airflow has some special treatments for those role names because they need to exist for the web UI and API to operate correctly.

[alex-astronomer]

I'll give further reproduction steps with examples from the source for why the problem happens. Sorry about the confusion. Let me reproduce again just to make sure and then I'll post here.

EDIT: I'll also pull down your PR branch to make sure that I can still reproduce with those changes since it directly affects the part of the source where this problem exists.

[alex-astronomer]

Actually @uranusjr, the changes that you made for #18820 fixed this issue! I'm going to write some test cases that confirm this behavior as a part of this issue, but otherwise looks really good!

[uranusjr]

Fixed as a part of #18820.