Merge pull request #18379 from jdavcs/241_fix_usernames_in_db

[24.1] Fix empty usernames in database + bug in username generation

lib/galaxy/managers/users.py

@@ -197,7 +197,7 @@ def purge(self, user, flush=True):
         private_role = self.app.security_agent.get_private_user_role(user)
         if private_role is None:
             raise exceptions.InconsistentDatabase(
-                "User '%s' private role is missing while attempting to purge deleted user." % user.email
+                f"User {user.email} private role is missing while attempting to purge deleted user."
             )
         # Delete History
         for active_history in user.active_histories:
@@ -664,23 +664,11 @@ def get_or_create_remote_user(self, remote_user_email):
                     self.app.security_agent.user_set_default_permissions(user)
                     self.app.security_agent.user_set_default_permissions(user, history=True, dataset=True)
         elif user is None:
-            username = remote_user_email.split("@", 1)[0].lower()
             random.seed()
             user = self.app.model.User(email=remote_user_email)
             user.set_random_password(length=12)
             user.external = True
-            # Replace invalid characters in the username
-            for char in [x for x in username if x not in f"{string.ascii_lowercase + string.digits}-."]:
-                username = username.replace(char, "-")
-            # Find a unique username - user can change it later
-            stmt = select(self.app.model.User).filter_by(username=username).limit(1)
-            if self.session().scalars(stmt).first():
-                i = 1
-                stmt = select(self.app.model.User).filter_by(username=f"{username}-{str(i)}").limit(1)
-                while self.session().scalars(stmt).first():
-                    i += 1
-                username += f"-{str(i)}"
-            user.username = username
+            user.username = username_from_email(self.session(), remote_user_email, self.app.model.User)
             self.session().add(user)
             with transaction(self.session()):
                 self.session().commit()
@@ -896,3 +884,31 @@ def get_user_by_email(session, email: str, model_class=User, case_sensitive=True
 def get_user_by_username(session, username: str, model_class=User):
     stmt = select(model_class).filter(model_class.username == username).limit(1)
     return session.scalars(stmt).first()
+
+
+def username_from_email(session, email, model_class=User):
+    """Get next available username generated based on email"""
+    username = email.split("@", 1)[0].lower()
+    username = filter_out_invalid_username_characters(username)
+    if username_exists(session, username, model_class):
+        username = generate_next_available_username(session, username, model_class)
+    return username
+
+
+def filter_out_invalid_username_characters(username):
+    """Replace invalid characters in username"""
+    for char in [x for x in username if x not in f"{string.ascii_lowercase + string.digits}-."]:
+        username = username.replace(char, "-")
+    return username
+
+
+def username_exists(session, username: str, model_class=User):
+    return bool(get_user_by_username(session, username, model_class))
+
+
+def generate_next_available_username(session, username, model_class=User):
+    """Generate unique username; user can change it later"""
+    i = 1
+    while session.execute(select(model_class).where(model_class.username == f"{username}-{i}")).first():
+        i += 1
+    return f"{username}-{i}"

lib/galaxy/model/migrations/alembic/versions_gxy/c63848676caf_update_username_column_schema_and_data.py

@@ -0,0 +1,89 @@
+"""Update username column schema and data
+
+Revision ID: c63848676caf
+Revises: c14a3c93d66b
+Create Date: 2024-06-11 13:41:36.411803
+
+"""
+
+import string
+
+from alembic import op
+from sqlalchemy import (
+    or_,
+    select,
+    update,
+)
+
+from galaxy.model import User
+from galaxy.model.migrations.util import (
+    alter_column,
+    transaction,
+)
+
+# revision identifiers, used by Alembic.
+revision = "c63848676caf"
+down_revision = "c14a3c93d66b"
+branch_labels = None
+depends_on = None
+
+table_name = "galaxy_user"
+column_name = "username"
+
+
+def upgrade():
+    with transaction():
+        _generate_missing_usernames()
+        alter_column(table_name, column_name, nullable=False)
+
+
+def downgrade():
+    with transaction():
+        # The data migration is one-way: we can change back the database schema,
+        # but we can't remove the generated username values.
+        alter_column(table_name, column_name, nullable=True)
+
+
+def _generate_missing_usernames():
+    stmt = select(User.id, User.email).where(or_(User.username.is_(None), User.username == ""))
+    connection = op.get_bind()
+    users = connection.execute(stmt).all()
+    for id, email in users:
+        new_username = username_from_email(connection, email)
+        update_stmt = update(User).where(User.id == id).values(username=new_username)
+        connection.execute(update_stmt)
+
+
+# The code below is a near-duplicate of similar code in managers.users. The duplication is
+# intentional: we want to preserve this logic in the migration script. The only differences are:
+# (1) this code uses a Connection instead of a Session;
+# (2) the username_exists function inlines the Select statement from managers.users::get_user_by_username.
+
+
+def username_from_email(connection, email, model_class=User):
+    # This function is also called from database revision scripts, which do not provide a session.
+    username = email.split("@", 1)[0].lower()
+    username = filter_out_invalid_username_characters(username)
+    if username_exists(connection, username, model_class):
+        username = generate_next_available_username(connection, username, model_class)
+    return username
+
+
+def filter_out_invalid_username_characters(username):
+    """Replace invalid characters in username"""
+    for char in [x for x in username if x not in f"{string.ascii_lowercase + string.digits}-."]:
+        username = username.replace(char, "-")
+    return username
+
+
+def username_exists(connection, username: str, model_class=User):
+    stmt = select(model_class).filter(model_class.username == username).limit(1)
+    return bool(connection.execute(stmt).first())
+
+
+def generate_next_available_username(connection, username, model_class=User):
+    """Generate unique username; user can change it later"""
+    i = 1
+    while connection.execute(select(model_class).where(model_class.username == f"{username}-{i}")).first():
+        i += 1
+    return f"{username}-{i}"

lib/galaxy/model/migrations/alembic/versions_gxy/ddbdbc40bdc1_add_workflow_comment_table.py

@@ -1,4 +1,5 @@
 """add workflow_comment table
+
 Revision ID: ddbdbc40bdc1
 Revises: 92fb564c7095
 Create Date: 2023-08-14 13:41:59.442243

lib/galaxy/model/migrations/dbscript.py

@@ -42,8 +42,8 @@
     "23.2": "8a19186a6ee7",
     "release_24.0": "55f02fd8ab6c",
     "24.0": "55f02fd8ab6c",
-    "release_24.1": "c14a3c93d66b",
-    "24.1": "c14a3c93d66b",
+    "release_24.1": "c63848676caf",
+    "24.1": "c63848676caf",
 }