Make it possible to delete a permission from a role using the stable REST API

[alex-astronomer]

Description

I'd like to be able to delete a permission from a role using the stable REST API. Right now, when using the PATCH verb on the /roles/{role_name} endpoint, the actions and resources combo that gets added through the body is merged into the database, making it possible to add permissions but not delete them from what I can see.

I'd like to have the option to delete a permission from a role, which I don't believe is currently possible using PATCH.

Use case/motivation

I want to be able to better modify Roles using the API. Maybe as part of CI/CD I can set up the roles to my liking by using a script which calls the API, and can change roles. Having more control over the Roles through the API would be a good feature for users of Airflow and useful specifically for my use-case involving the Astronomer platform.

Related issues

No response

Are you willing to submit a PR?

• [X] Yes I am willing to submit a PR!

Code of Conduct

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

[uranusjr]

the actions and resources combo that gets added through the body is merged into the database, making it possible to add permissions but not delete them from what I can see.

This feels like a bug to me. If permissions is provided, it should replace the entire permission list instead of merging the given permissions into the existing ones.

[alex-astronomer]

Might be just a problem with my misunderstanding of this endpoint, also see #18713 for my confusion 👀

[alex-astronomer]

Yeah the behavior that I'm seeing is that the permission list is being merged rather than replacing the entire thing. This is corroborated by security_manager.update_role function call that is happening which is running a merge on the DB with the role. You think that this is a bug @uranusjr ? I'm thinking the same thing, but really don't know a ton about the nature of these PATCH endpoints to be able to make that kind of distinction.

[uranusjr]

REST API is only a guide line without hard rules, but PATCH generally means the user can submit a subset of fields, and the missing fields would be filled by existing values on the model. It is a natural variant to PUT, which replaces all fields on the model with new values specified in the request.

https://datatracker.ietf.org/doc/html/rfc5789#section-2

The difference between the PUT and PATCH requests is reflected in the way the server processes the enclosed entity to modify the resource identified by the Request-URI. In a PUT request, the enclosed entity is considered to be a modified version of the resource stored on the origin server, and the client is requesting that the stored version be replaced. With PATCH, however, the enclosed entity contains a set of instructions describing how a resource currently residing on the origin server should be modified to produce a new version. The PATCH method affects the resource identified by the Request-URI, and it also MAY have side effects on other resources; i.e., new resources may be created, or existing ones modified, by the application of a PATCH.

Now it is entirely undefined whether sending a list in PATCH should mean merging the lists in the request and the existing object, or replacing the entire list on the existing object with the one in the request. But the merging behaviour does not really make common sense since it would make deleting entries impossible. So I'm inclined to think the current behaviour is wrong.

[alex-astronomer]

Maybe we make a new PUT endpoint that has the behavior that we expect?

[uranusjr]

We can, although the current PATCH behaviour is still somehow unintuitive and should at least be documented.

[alex-astronomer]

Heard that. I'll make that a part of this PR that I started working on for this.

[erdrix]

Hello, any news on this topic ? It's quite blocking if we want to set up a management logic outside the UI for the roles.

[potiuk]

@erdrix - how about providing a PR to that one? This is the easiest way to do it and you can become one of the ~ 2000 contributors to Airlfow.

[potiuk]

Especially if it is business-critical for you, it only seems reasonable to pay-back for the free software you get by contributing back this small feature.

[andreyolv]

I was investigating the possible cause of the problem.

I was confused because the name of the patch_role function uses the PUT method in the decorator above, but the behavior is actually PATCH and not PUT, as this function calls the bulk_sync_roles function which only adds the roles and to have the PUT behavior it should have delete_role inside this function.

I don't have in-depth knowledge of Python development, so I could be saying something wrong, I'm just trying to help identify the possible cause and contribute in some way to this issue