specify / specify7

Specify 7
https://www.specifysoftware.org/products/specify-7/
GNU General Public License v2.0
63 stars 36 forks source link

Concurrency issues when changing user permissions fails #2192

Open maxpatiiuk opened 2 years ago

maxpatiiuk commented 2 years ago

Saving a user sends the following requests:

  1. /api/set_agents/<userId> - to change assigned agents
  2. /permissions/user_policies/institution/<userId> - to change institutional policies
  3. /api/set_password/<userId> - to set password
  4. Then, all of these in parallel:
    1. /permissions/user_roles/<collectionId>/<userId> - to change user roles within a collection. Called for each collection in which roles were modified
    2. /permissions/user_policies/<collectionId>/<userId> - to change user policies within a collection. Called for each collection in which policies were modified.
  5. /api/specify/specifyuser/<userId> - save the user itself

It all works fine most of the time, but things start to fall apart when some requests fail due to user not having an agent assigned in a given collection.

If some requests fail, the user record is left in an inconsistent state with some changes applied and some not.

What is worse, the order in which requests are executed determins whether an error occurs or not. For example, if you unassigned an agent and also removed collection access for user in that collection, then if /api/set_agents/<userId> request is sent first, it would fail because the user would still have collection access at that point. If, however, that request is sent after /permissions/user_policies/<collectionId>/<userId>, no error occurs

Proposed solutions:

  1. (the simplest for the front-end) Aggregate changing all users permissions under a single endpoint to make the action atomic (as far as the front-end is concerned. the back-end can execute the actions in any order). This returns a single error message if any request fails
  2. (can be done without back-end changes) Run all of the above requests in a dry run mode, where each action is applied in a sequence, but if any fails, all actions are reverted (either automatically by the back-end, or by front-end in the subsequent request)
  3. (probably the best solution) Allow to temporarily disable the errors about user missing an agent in an accessible collection. After all requests were sent and processed by the back-end, the front-end would send a final request verifying if the configuration is valid and displaying an error message if it is not. The biggest issue with this approach is that the user can just ignore the error message, and the user preference would remain in an invalid configuration. However, making any future permission edits for this user would resurface the error.
  4. (a compromise) execute all requests one after another (rather than in parallel). If any fails, stop, but don't revert changes. Some valid changes would throw missing agent error, but at least the behavior would be predictable.
benanhalt commented 2 years ago

I think (1) makes the most sense to me. If clicking "save" in the UI is supposed to perform all of those actions, they should probably be handled atomically. I can make an end point to do that. Can you draft an OpenAPI spec of how you would like the request to be structured?