ChickenBenny opened a new pull request, #76972: URL: https://github.com/apache/airflow/pull/76972
Renaming a role to a name another role already has returned 200, but only the permissions actually changed. `patch_role` replaced the permissions first and renamed second. Each permission change is committed on its own, so when the rename then hit the unique constraint on `ab_role.name`, `update_role` rolled back only the rename and returned `None`. The service ignored that and returned the requested name and permissions. The client thought the update worked, while the role kept its old name with a new set of permissions. Changes: - Rename before touching permissions. Permissions are still validated first, so a bad action or resource still returns 400 without writing anything. - Return 409 if the new name belongs to another role, same as `create_role`. The check compares ids, so a case-only rename on MySQL (where the lookup finds the role itself) still works. - Return 500 if `update_role` fails anyway, e.g. a concurrent rename, instead of 200. - Add 409 to the OpenAPI responses and regenerate the spec. Not fixed here (existing behaviour, follow-up PR): - Permission updates are not atomic. `add_permission_to_role` / `remove_permission_from_role` swallow errors, so a failure halfway leaves the role partly updated and still returns 200. - Concurrent PATCHes on the same role can overwrite each other: the last rename wins, and permission diffs computed from stale reads can end up mixed. - A rename that loses a race to another role gets 500 instead of 409, since `update_role` returns `None` for any error. The plan is a security manager method that locks the row (`SELECT ... FOR UPDATE`) and does the rename and permission update in one transaction, mapping unique violations to 409. Testing: the new 409/500 unit tests and a `db_test` reproducing the bug fail on `main`. The case-only-rename case passes on `main`; it covers the id check. `services/test_roles.py` and `routes/test_roles.py` pass on SQLite (59 passed). `prek run --stage pre-commit` and `breeze run mypy` on the changed files pass. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
