fix(auth): make username a required field (#4734)

Co-authored-by: Mikyo King <[email protected]>

app/global-setup.ts

@@ -29,6 +29,7 @@ async function globalSetup(config: FullConfig) {
 
   // Add the user
   await page.getByLabel("Email").fill("[email protected]");
+  await page.getByLabel("Username *").fill("member");
   await page.getByLabel("Password *", { exact: true }).fill("member123");
   await page.getByLabel("Confirm Password").fill("member123");
 

app/schema.graphql

@@ -153,7 +153,7 @@ type CreateUserApiKeyMutationPayload {
 
 input CreateUserInput {
   email: String!
-  username: String
+  username: String!
   password: String!
   role: UserRoleInput!
 }

app/src/components/settings/UserForm.tsx

@@ -12,7 +12,7 @@ const MIN_PASSWORD_LENGTH = 4;
 
 export type UserFormParams = {
   email: string;
-  username: string | null;
+  username: string;
   password: string;
   confirmPassword: string;
   role: UserRole;
@@ -36,7 +36,7 @@ export function UserForm({
   } = useForm<UserFormParams>({
     defaultValues: {
       email: email ?? "",
-      username: username ?? null,
+      username: username ?? "",
       password: password ?? "",
       confirmPassword: "",
       role: role ?? UserRole.MEMBER,
@@ -87,14 +87,18 @@ export function UserForm({
           <Controller
             name="username"
             control={control}
+            rules={{
+              required: "Email is required",
+            }}
             render={({
               field: { name, onChange, onBlur, value },
               fieldState: { invalid, error },
             }) => (
               <TextField
                 label="Username"
                 name={name}
-                description="The user's username. Optional."
+                isRequired
+                description="A unique username."
                 errorMessage={error?.message}
                 validationState={invalid ? "invalid" : "valid"}
                 onChange={onChange}

app/tests/user-management.spec.ts

@@ -18,6 +18,7 @@ test("can create a user", async ({ page }) => {
   const email = `member-${randomUUID()}@localhost.com`;
   // Add the user
   await page.getByLabel("Email").fill(email);
+  await page.getByLabel("Username *").fill(email);
   await page.getByLabel("Password *", { exact: true }).fill("member123");
   await page.getByLabel("Confirm Password").fill("member123");
   await page.getByRole("dialog").getByLabel("member", { exact: true }).click();

integration_tests/_helpers.py

@@ -92,7 +92,7 @@
 class _Profile:
     email: _Email
     password: _Password
-    username: Optional[_Username] = None
+    username: _Username
 
 
 class _String(str, ABC):

integration_tests/auth/test_auth.py

@@ -422,18 +422,6 @@ def test_only_admin_can_create_user(
             new_user.log_in()
             assert _will_be_asked_to_reset_password(new_user)
 
-    @pytest.mark.parametrize("role_or_user", [_ADMIN, _DEFAULT_ADMIN])
-    @pytest.mark.parametrize("role", list(UserRoleInput))
-    def test_username_is_optional(
-        self,
-        role_or_user: _RoleOrUser,
-        role: UserRoleInput,
-        _get_user: _GetUser,
-        _profiles: Iterator[_Profile],
-    ) -> None:
-        profile = replace(next(_profiles), username=None)
-        _get_user(role_or_user).create_user(role, profile=profile)
-
 
 class TestPatchViewer:
     @pytest.mark.parametrize("role_or_user", [_MEMBER, _ADMIN, _DEFAULT_ADMIN])

src/phoenix/auth.py

@@ -227,6 +227,8 @@ def validate(
 DEFAULT_ADMIN_USERNAME = "admin"
 DEFAULT_ADMIN_EMAIL = "admin@localhost"
 DEFAULT_ADMIN_PASSWORD = "admin"
+DEFAULT_SYSTEM_USERNAME = "system"
+DEFAULT_SYSTEM_EMAIL = "system@localhost"
 DEFAULT_SECRET_LENGTH = 32
 DEFAULT_PASSWORD_RESET_TOKEN_EXPIRY_MINUTES = 15
 DEFAULT_ACCESS_TOKEN_EXPIRY_MINUTES = 10

src/phoenix/db/facilitator.py

@@ -16,6 +16,8 @@
     DEFAULT_ADMIN_PASSWORD,
     DEFAULT_ADMIN_USERNAME,
     DEFAULT_SECRET_LENGTH,
+    DEFAULT_SYSTEM_EMAIL,
+    DEFAULT_SYSTEM_USERNAME,
     compute_password_hash,
 )
 from phoenix.db import models
@@ -84,7 +86,8 @@ async def _ensure_user_roles(session: AsyncSession) -> None:
     ) is not None:
         system_user = models.User(
             user_role_id=system_role_id,
-            email="system@localhost",
+            username=DEFAULT_SYSTEM_USERNAME,
+            email=DEFAULT_SYSTEM_EMAIL,
             reset_password=False,
         )
         session.add(system_user)

src/phoenix/db/migrations/versions/cd164e83824f_users_and_tokens.py

@@ -40,7 +40,7 @@ def upgrade() -> None:
             nullable=False,
             index=True,
         ),
-        sa.Column("username", sa.String, nullable=True, unique=True, index=True),
+        sa.Column("username", sa.String, nullable=False, unique=True, index=True),
         sa.Column("email", sa.String, nullable=False, unique=True, index=True),
         sa.Column("profile_picture_url", sa.String, nullable=True),
         sa.Column("password_hash", sa.LargeBinary, nullable=True),

src/phoenix/db/models.py

@@ -642,7 +642,7 @@ class User(Base):
         index=True,
     )
     role: Mapped["UserRole"] = relationship("UserRole", back_populates="users")
-    username: Mapped[Optional[str]] = mapped_column(nullable=True, unique=True, index=True)
+    username: Mapped[str] = mapped_column(nullable=False, unique=True, index=True)
     email: Mapped[str] = mapped_column(nullable=False, unique=True, index=True)
     profile_picture_url: Mapped[Optional[str]]
     password_hash: Mapped[Optional[bytes]]

src/phoenix/server/api/mutations/user_mutations.py

@@ -35,7 +35,7 @@
 @strawberry.input
 class CreateUserInput:
     email: str
-    username: Optional[str] = UNSET
+    username: str
     password: str
     role: UserRoleInput
 
@@ -93,7 +93,7 @@ async def create_user(
         password_hash = await info.context.hash_password(password, salt)
         user = models.User(
             reset_password=True,
-            username=input.username or None,
+            username=input.username,
             email=email,
             password_hash=password_hash,
             password_salt=salt,