Add PATCH to _MAP_METHOD_NAME_TO_FAB_ACTION_NAME

[rawwar]

related to #42360

related comment: #47062 (comment)

[pierrejeambrun]

Yes required for the permissions on the server api. (there are some patch there)

[pierrejeambrun]

cc: @vincbeck

[vincbeck]

Why do we need both? What is the different between PUT and PATCH? Maybe using HTTP terms in auth manager was not that much of a good idea actually but here we just want to express action, what kind of action the user is trying to do?

• Create (POST)
• Edit (PUT)
• Delete (DELETE)
• Read (GET)

If you want to add PATCH just because you have PATCH routes defined in the fastapi application I think this is wrong. We should then have a layer that translate fastapi method to auth manager action and convert PATCH to PUT in that layer

[pierrejeambrun]

There are some subtle differences (idempotency) I won't go into details and it depends on how we implement them, but both PUT and PATCH could be used in the rest API to update a resource. (For now we only have PATCH in the new server api).

I think both http method PUT & PATCH should be mapped to the can_edit action

[vincbeck]

I understand this. What I am saying is, on the auth manager side, we do not need to have both, we do not need to say: "I want to give user X permissions for PUT but not for PATCH". Do we? On the auth manager side we should limit the number of actions and I dont think it is needed to add that new action. I dont think we need this granularity?

[pierrejeambrun]

We are not adding an action. We are mapping the PATCH HTTP method to the same existing can_edit action. So basically once the mapping is used to get the FAB action (that already exists) from this PATCH method, everything is untouched from the fab auth manager perspective.

Do you have an alternative in mind ?

We should then have a layer that translate fastapi method to auth manager action and convert PATCH to PUT in that layer

This is exactly what is happening get_fab_action_from_method_map will map both HTTP methods (PUT PATCH) to the same FAB action.

Maybe I missed something.

[vincbeck]

No, you are adding a new method to ResourceMethod which configures which are the action available in auth managers (I am not talking about FAB auth manager specifically here but auth manager in general). By adding this method then you can do things like auth_manager.is_authorized_configuration(method=ResourceMethod.PUT) and auth_manager.is_authorized_configuration(method=ResourceMethod.PATCH) You need to understand that FAB auth manager is not the only auth manager.

You are right get_fab_action_from_method_map translate ResourceMethod to FAB action but again here the issue is, we should not add a new action to the list of actions in ResourceMethod which are shared across all auth managers.

I think we are on the same page but I think this implementation is wrong. What we want to do is to translate HTTP method PATCH to the resource method ResourceMethod.PUT.

[pierrejeambrun]

Ohhh, yes you are talking about ResourceMethod = Literal["GET", "POST", "PUT", "PATCH", "DELETE", "MENU"].

I was focusing on "PATCH": ACTION_CAN_EDIT,, I understand, yes the ResourceMethod change is not good.

Then I guess we can do the mapping on the fastapi side. So that PATCH will map to ResourceMethod.PUT.

Or don't do anything specific, and we keep things as they were. A patch endpoint will be decorated with a @security.requires_access_dag("PUT"), cf patch_dags in the legacy api. I don't think we need that change at all for now after all.

(Even if I admit this is confusing that every single other HTTP method got their counterpart ResourceMethod, but PATCH do not and will use PUT, at first it looks like a mistake, anyway, not needed).

[pierrejeambrun]

Waiting for @vincbeck input but I think we will most likely close this one.

[vincbeck]

💯 We are on the same page :) The two choices you listed are valid to me and I'll let you chose the best option