Another bunch of fixes to correctly delete users (#918)

* Another bunch of fixes to correctly delete users

* Additional test

* oops

* Fix bug in tests

mwdb/model/api_key.py

@@ -18,7 +18,9 @@ class APIKey(db.Model):
         db.Integer, db.ForeignKey("user.id", ondelete="CASCADE"), nullable=False
     )
     issued_on = db.Column(db.DateTime, default=datetime.datetime.utcnow, nullable=False)
-    issued_by = db.Column(db.Integer, db.ForeignKey("user.id"), nullable=False)
+    issued_by = db.Column(
+        db.Integer, db.ForeignKey("user.id", ondelete="SET NULL"), nullable=True
+    )
     name = db.Column(db.String(length=100), nullable=False)
 
     issuer = db.relationship("User", foreign_keys=[issued_by], uselist=False)

mwdb/model/migrations/versions/1a46a79d9108_set_nullable_fk_to_correctly_delete_.py

@@ -0,0 +1,43 @@
+"""Set nullable FK to correctly delete users
+
+Revision ID: 1a46a79d9108
+Revises: a81513c2a4bf
+Create Date: 2024-02-23 16:13:48.780670
+
+"""
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "1a46a79d9108"
+down_revision = "a81513c2a4bf"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.alter_column("api_key", "issued_by", existing_type=sa.INTEGER(), nullable=True)
+    op.drop_constraint("api_key_issued_by_fkey", "api_key", type_="foreignkey")
+    op.create_foreign_key(
+        None, "api_key", "user", ["issued_by"], ["id"], ondelete="SET NULL"
+    )
+    op.drop_constraint("user_registered_by_fkey", "user", type_="foreignkey")
+    op.create_foreign_key(
+        None, "user", "user", ["registered_by"], ["id"], ondelete="SET NULL"
+    )
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.drop_constraint(None, "user", type_="foreignkey")
+    op.create_foreign_key(
+        "user_registered_by_fkey", "user", "user", ["registered_by"], ["id"]
+    )
+    op.drop_constraint(None, "api_key", type_="foreignkey")
+    op.create_foreign_key(
+        "api_key_issued_by_fkey", "api_key", "user", ["issued_by"], ["id"]
+    )
+    op.alter_column("api_key", "issued_by", existing_type=sa.INTEGER(), nullable=False)
+    # ### end Alembic commands ###

mwdb/model/user.py

@@ -40,7 +40,7 @@ class User(db.Model):
 
     requested_on = db.Column(db.DateTime)
     registered_on = db.Column(db.DateTime)
-    registered_by = db.Column(db.Integer, db.ForeignKey("user.id"))
+    registered_by = db.Column(db.Integer, db.ForeignKey("user.id", ondelete="SET NULL"))
     logged_on = db.Column(db.DateTime)
     set_password_on = db.Column(db.DateTime)
 
@@ -84,7 +84,10 @@ class User(db.Model):
         "APIKey", foreign_keys="APIKey.user_id", backref="user", cascade="all, delete"
     )
     registrar = db.relationship(
-        "User", foreign_keys="User.registered_by", remote_side=[id], uselist=False
+        "User",
+        foreign_keys="User.registered_by",
+        remote_side=[id],
+        uselist=False,
     )
 
     # used to load-balance the malware processing pipeline

tests/backend/test_api.py

@@ -235,12 +235,24 @@ def test_zero_starting_crc32(admin_session):
 def test_user_delete(admin_session):
     # Make user and use it to comment new sample
     case = RelationTestCase(admin_session)
-    alice = case.new_user("Alice", capabilities=["adding_comments"])
+    alice = case.new_user("Alice", capabilities=["adding_comments", "manage_users"])
+    # Upload sample
     sample = case.new_sample("Sample")
     sample.create(alice)
+    # Set comment for sample
     alice.session.add_comment(sample.dhash, "random comment")
+    # Register new user using Alice
+    bob_identity = rand_string(16)
+    alice.session.register_user(bob_identity, bob_identity)
+    # Issue new API key for bob
+    bob_api_key_resp = alice.session.api_key_create(bob_identity, "new api key").json()
+    bob_api_key_id = bob_api_key_resp["id"]
     # Then try to remove that user
     admin_session.remove_user(alice.identity)
+    # Bob should still exist
+    bob_data = admin_session.get_user(bob_identity)
+    assert bob_data["registrar_login"] is None
+    assert bob_data["api_keys"][0]["id"] == bob_api_key_id
     # Object should be still reachable
     admin_session.get_sample(sample.dhash)
     # And should still have one comment (with nulled author)

tests/backend/utils.py

@@ -90,11 +90,6 @@ def api_key_create(self, login, name):
         res.raise_for_status()
         return res
 
-    def api_key_get(self, api_key):
-        res = self.session.get(self.mwdb_url + "/api_key/" + api_key)
-        res.raise_for_status()
-        return res
-
     def api_key_delete(self, api_key):
         res = self.session.delete(self.mwdb_url + "/api_key/" + api_key)
         res.raise_for_status()
@@ -111,6 +106,11 @@ def get_group(self, name):
         res.raise_for_status()
         return res.json()
 
+    def get_user(self, login):
+        res = self.session.get(self.mwdb_url + '/user/' + login)
+        res.raise_for_status()
+        return res.json()
+
     def remove_group(self, name):
         res = self.session.delete(self.mwdb_url + "/group/" + name)
         res.raise_for_status()
@@ -154,7 +154,6 @@ def remove_member(self, name, username):
 
     def register_user(self, username, password, capabilities=None):
         capabilities = capabilities or []
-        self.login()
         res = self.session.post(
             self.mwdb_url + "/user/" + username,
             json={