EnxDev commented on code in PR #44866:
URL: https://github.com/apache/superset/pull/44866#discussion_r4163384685
##########
superset/views/users/api.py:
##########
@@ -50,7 +50,6 @@ class CurrentUserRestApi(BaseSupersetApi):
def pre_update(self, item: User, data: Dict[str, Any]) -> None:
item.changed_on = datetime.now()
- item.changed_by_fk = g.user.id
Review Comment:
This stops new self-references, but any user who already edited their
profile on 6.0/6.1 still has `changed_by_fk == id` and stays undeletable after
upgrade. The admin path from the issue also still hits it: FAB's
`UserApi.pre_update` sets `changed_by_fk = current_user.id`, and
`SupersetUserApi.pre_update` (`superset/security/manager.py:518`) calls
`super()`, so an Admin editing their own row via `PUT
/api/v1/security/users/<own id>` gets the same poisoned row.
Could we also null a self-referencing `changed_by_fk` / `created_by_fk` in
`SupersetUserApi.pre_delete`? That one guard covers existing rows and both
write paths.
##########
tests/unit_tests/views/test_current_user_api.py:
##########
@@ -245,3 +245,26 @@ def
test_update_me_password_change_clears_flag_in_the_same_unit_of_work(
mock_commit.assert_not_called()
assert attr.password_must_change is False
assert check_password_hash(admin_user.password, "BrandNewPassw0rd!")
+
+
+def test_update_me_does_not_set_self_referential_changed_by(
+ admin_user: User, # noqa: F811
+ after_each: None, # noqa: F811
+) -> None:
+ """A user editing their own profile must not end up with ``changed_by_fk``
+ pointing at their own row. ``User.changed_by`` is a self-referential
+ relationship without ``post_update``, so SQLAlchemy cannot order the
+ DELETE of such a row and raises ``CircularDependencyError``, which made
+ the user undeletable.
+ """
+ db.session.flush()
+
+ _run_update_me(admin_user, {"first_name": "Changed"})
+ db.session.flush()
+
+ assert admin_user.first_name == "Changed"
+ assert admin_user.changed_by_fk != admin_user.id
Review Comment:
This guards the `/me` path, but nothing covers a row that already points at
itself.
If the `pre_delete` guard goes in, worth a test that sets `changed_by_fk =
user.id` directly and then deletes through `SupersetUserApi`, since that's the
state existing installs are in.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
