From a47bf378e280413e01183c67fde6df0d02fc1a32 Mon Sep 17 00:00:00 2001 From: fallow64 Date: Mon, 27 Apr 2026 10:39:57 -0500 Subject: [PATCH 1/7] Add unix username field --- .../47f8adc9859b_add_unix_username_field.py | 134 ++++++++++ userapp/api/routes/security.py | 3 +- userapp/api/routes/users.py | 19 ++ userapp/api/tests/fake_data.py | 3 +- userapp/api/tests/test_user_schemas_active.py | 250 ------------------ userapp/api/tests/test_users.py | 89 ++++++- userapp/api/tests/test_users_active_field.py | 237 ----------------- userapp/core/models/tables.py | 1 + userapp/core/models/views.py | 1 + userapp/core/schemas/general.py | 9 +- userapp/core/schemas/users.py | 22 +- 11 files changed, 269 insertions(+), 499 deletions(-) create mode 100644 alembic/versions/47f8adc9859b_add_unix_username_field.py delete mode 100644 userapp/api/tests/test_user_schemas_active.py delete mode 100644 userapp/api/tests/test_users_active_field.py diff --git a/alembic/versions/47f8adc9859b_add_unix_username_field.py b/alembic/versions/47f8adc9859b_add_unix_username_field.py new file mode 100644 index 0000000..a535578 --- /dev/null +++ b/alembic/versions/47f8adc9859b_add_unix_username_field.py @@ -0,0 +1,134 @@ +"""Add unix username field + +Revision ID: 47f8adc9859b +Revises: 6450d7ab8865 +Create Date: 2026-04-20 10:27:12.467103 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision: str = '47f8adc9859b' +down_revision: Union[str, Sequence[str], None] = '6450d7ab8865' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +netid_username_map = { + "aschneider37": "austins", +} + + +def upgrade() -> None: + """Upgrade schema.""" + op.execute('DROP VIEW IF EXISTS joined_projects') + op.add_column('users', sa.Column('username', sa.String(length=255), nullable=True)) + users_table = sa.table( + 'users', + sa.column('id', sa.Integer()), + sa.column('netid', sa.String(length=255)), + sa.column('username', sa.String(length=255)), + ) + op.execute( + users_table.update() + .values(username=users_table.c.netid) + ) + for netid, username in netid_username_map.items(): + op.execute( + users_table.update() + .where(users_table.c.netid == netid) + .values(username=username) + ) + op.execute(""" + UPDATE users + SET username = 'legacy-user-' || id::text + WHERE username IS NULL + """) + op.alter_column('users', 'username', nullable=False) + op.create_unique_constraint(op.f('users_username_key'), 'users', ['username']) + op.execute(""" + CREATE VIEW joined_projects AS + SELECT + u.id, + p.id AS project_id, + p.name AS project_name, + p.staff1 AS project_staff1, + p.staff2 AS project_staff2, + p.status AS project_status, + p.last_contact AS project_last_contact, + p.accounting_group AS project_accounting_group, + up.is_primary AS is_primary, + u.name, + u.username, + u.email1, + u.email2, + u.netid, + u.netid_exp_datetime, + u.phone1, + u.phone2, + u.is_admin, + u.active, + u.date, + u.unix_uid, + u.position, + up.role, + ( + SELECT n.ticket + FROM notes n + LEFT JOIN user_notes un ON n.id = un.note_id + WHERE un.user_id = up.user_id AND un.project_id = up.project_id + ORDER BY n.id DESC + LIMIT 1 + ) AS last_note_ticket + FROM user_projects up + JOIN users u ON up.user_id = u.id + JOIN projects p ON up.project_id = p.id + """) + + +def downgrade() -> None: + """Downgrade schema.""" + op.execute('DROP VIEW IF EXISTS joined_projects') + op.drop_constraint(op.f('users_username_key'), 'users', type_='unique') + op.drop_column('users', 'username') + op.execute(""" + CREATE VIEW joined_projects AS + SELECT + u.id, + p.id AS project_id, + p.name AS project_name, + p.staff1 AS project_staff1, + p.staff2 AS project_staff2, + p.status AS project_status, + p.last_contact AS project_last_contact, + p.accounting_group AS project_accounting_group, + up.is_primary AS is_primary, + u.name, + u.email1, + u.email2, + u.netid, + u.netid_exp_datetime, + u.phone1, + u.phone2, + u.is_admin, + u.active, + u.date, + u.unix_uid, + u.position, + up.role, + ( + SELECT n.ticket + FROM notes n + LEFT JOIN user_notes un ON n.id = un.note_id + WHERE un.user_id = up.user_id AND un.project_id = up.project_id + ORDER BY n.id DESC + LIMIT 1 + ) AS last_note_ticket + FROM user_projects up + JOIN users u ON up.user_id = u.id + JOIN projects p ON up.project_id = p.id + """) diff --git a/userapp/api/routes/security.py b/userapp/api/routes/security.py index 6f5cc4d..13006b7 100644 --- a/userapp/api/routes/security.py +++ b/userapp/api/routes/security.py @@ -460,11 +460,12 @@ async def oidc_callback(request: Request, response: Response, session=Depends(se user_info = user_info_resp.json() print(user_info) + # name is required so fallback to netid if no name is found user = UserTable( - # name is required so fallback to netid if no name is found name=user_info.get("name") or user_info.get("sub"), email1=user_info["email"], netid=user_info.get("sub"), + username=user_info.get("sub"), active=False, is_admin=False, ) diff --git a/userapp/api/routes/users.py b/userapp/api/routes/users.py index 56832eb..ad0ca82 100644 --- a/userapp/api/routes/users.py +++ b/userapp/api/routes/users.py @@ -97,6 +97,25 @@ async def update_user(user_id: int, user: UserPatchFull, session=Depends(session return await update_one_endpoint(session, UserTable, user_id, user_update_schema, load_options=user_load_options) elif is_admin: + current_user = await get_one_endpoint(session, UserTable, user_id) + merged_user_data = { + "name": current_user.name, + "username": current_user.username, + "email1": current_user.email1, + "email2": current_user.email2, + "netid": current_user.netid, + "netid_exp_datetime": current_user.netid_exp_datetime, + "phone1": current_user.phone1, + "phone2": current_user.phone2, + "is_admin": current_user.is_admin, + "active": current_user.active, + "unix_uid": current_user.unix_uid, + "position": current_user.position, + **user.model_dump(exclude_unset=True, exclude={"submit_nodes"}), + } + + # Validate that the merged data conforms to the UserPost schema + UserPost(**merged_user_data) # Update user user_data_only = UserPatch(**user.model_dump(exclude_unset=True)) diff --git a/userapp/api/tests/fake_data.py b/userapp/api/tests/fake_data.py index 7df8e02..ac88687 100644 --- a/userapp/api/tests/fake_data.py +++ b/userapp/api/tests/fake_data.py @@ -34,7 +34,7 @@ def project_data_f( } -def user_data_f(index: int, primary_project_id, is_admin=False) -> dict: +def user_data_f(index: int, primary_project_id, is_admin=False, username: Optional[str] = None) -> dict: """ Generate a unique user payload for testing, based on the UserBase schema. """ @@ -44,6 +44,7 @@ def user_data_f(index: int, primary_project_id, is_admin=False) -> dict: "email1": f"testuser{rand}@example.com", "email2": f"altuser{rand}@example.com", "netid": f"netid{rand}", + "username": username or f"netid{rand}", "netid_exp_datetime": datetime.now().isoformat(), "phone1": f"555-010{index}", "phone2": f"555-020{index}", diff --git a/userapp/api/tests/test_user_schemas_active.py b/userapp/api/tests/test_user_schemas_active.py deleted file mode 100644 index 3d8995d..0000000 --- a/userapp/api/tests/test_user_schemas_active.py +++ /dev/null @@ -1,250 +0,0 @@ -import pytest -from pydantic import ValidationError - -from userapp.core.schemas.users import UserGet, UserPost, UserTableSchema -from userapp.core.schemas.general import JoinedProjectView -from userapp.core.models.enum import PositionEnum, RoleEnum - -# Mostly AI generated tests, looks decent though - -class TestUserGetComputedFields: - """Tests for UserGet schema computed fields""" - - def test_auth_netid_computed_field_when_active_true(self): - """Test that auth_netid computed field returns active value when True""" - user_data = { - 'id': 1, - 'username': None, - 'name': 'Test User', - 'email1': 'test@example.com', - 'active': True, - } - - user = UserGet(**user_data) - - # Check that auth_netid matches active - assert user.auth_netid is True, "auth_netid should be True when active is True" - assert user.active is True, "active should be True" - - def test_auth_netid_computed_field_when_active_false(self): - """Test that auth_netid computed field returns active value when False""" - user_data = { - 'id': 2, - 'username': None, - 'name': 'Test User 2', - 'email1': 'test2@example.com', - 'active': False, - } - - user = UserGet(**user_data) - - # Check that auth_netid matches active - assert user.auth_netid is False, "auth_netid should be False when active is False" - assert user.active is False, "active should be False" - - def test_auth_netid_computed_field_when_active_none(self): - """Test that auth_netid computed field returns None when active is None""" - user_data = { - 'id': 3, - 'username': None, - 'name': 'Test User 3', - 'email1': 'test3@example.com', - 'active': None, - } - - user = UserGet(**user_data) - - # Check that auth_netid is None when active is None - assert user.auth_netid is None, "auth_netid should be None when active is None" - assert user.active is None, "active should be None" - - def test_auth_username_always_false(self): - """Test that auth_username computed field always returns False""" - user_data_active_true = { - 'id': 4, - 'username': None, - 'name': 'Test User 4', - 'email1': 'test4@example.com', - 'active': True, - } - - user_data_active_false = { - 'id': 5, - 'username': None, - 'name': 'Test User 5', - 'email1': 'test5@example.com', - 'active': False, - } - - user1 = UserGet(**user_data_active_true) - user2 = UserGet(**user_data_active_false) - - # Both should have auth_username as False - assert user1.auth_username is False, "auth_username should always be False" - assert user2.auth_username is False, "auth_username should always be False" - - def test_computed_fields_in_model_dump(self): - """Test that computed fields appear in model_dump output""" - user_data = { - 'id': 6, - 'username': None, - 'name': 'Test User 6', - 'email1': 'test6@example.com', - 'active': True, - } - - user = UserGet(**user_data) - dumped = user.model_dump() - - # Check that computed fields are included in dump - assert 'auth_netid' in dumped, "auth_netid should be in model dump" - assert 'auth_username' in dumped, "auth_username should be in model dump" - assert dumped['auth_netid'] is True, "auth_netid should match active in dump" - assert dumped['auth_username'] is False, "auth_username should be False in dump" - - -class TestUserPostValidation: - """Tests for UserPost schema validation""" - - def test_active_requires_netid_validation(self): - """Test that active=True requires netid to be provided""" - user_data = { - 'username': None, - 'password': 'SecurePassword123', - 'name': 'Test User', - 'email1': 'test@example.com', - 'active': True, - 'netid': None, # This should cause validation error - } - - with pytest.raises(ValidationError) as exc_info: - UserPost(**user_data) - - error = exc_info.value - assert 'netid must be provided' in str(error).lower(), \ - "Validation error should mention netid requirement" - - def test_active_true_with_netid_succeeds(self): - """Test that active=True with netid succeeds validation""" - user_data = { - 'username': None, - 'password': 'SecurePassword123', - 'name': 'Test User', - 'email1': 'test@example.com', - 'active': True, - 'netid': 'validnetid', - } - - user = UserPost(**user_data) - assert user.active is True, "active should be True" - assert user.netid == 'validnetid', "netid should be set" - - def test_active_false_without_netid_succeeds(self): - """Test that active=False without netid succeeds validation""" - user_data = { - 'username': None, - 'password': 'SecurePassword123', - 'name': 'Test User', - 'email1': 'test@example.com', - 'active': False, - 'netid': None, - } - - user = UserPost(**user_data) - assert user.active is False, "active should be False" - assert user.netid is None, "netid can be None when active is False" - - def test_active_none_without_netid_succeeds(self): - """Test that active=None (not set) without netid succeeds""" - user_data = { - 'username': None, - 'password': 'SecurePassword123', - 'name': 'Test User', - 'email1': 'test@example.com', - 'netid_exp_time': None, - } - - user = UserPost(**user_data) - # active defaults to None if not provided - assert user.active is None or user.active is False, "active should be None or False" - - -class TestJoinedProjectViewComputedFields: - """Tests for JoinedProjectView schema computed fields""" - - def test_joined_project_view_auth_netid_computed_field(self): - """Test that JoinedProjectView auth_netid matches active field""" - project_data = { - 'id': 1, - 'project_id': 10, - 'project_name': 'Test Project', - 'name': 'Test User', - 'email1': 'test@example.com', - 'active': True, - 'role': RoleEnum.PI, - } - - project_view = JoinedProjectView(**project_data) - - # Check computed fields - assert project_view.auth_netid is True, "auth_netid should match active" - assert project_view.auth_username is False, "auth_username should always be False" - - def test_joined_project_view_computed_fields_in_dump(self): - """Test that JoinedProjectView computed fields appear in model dump""" - project_data = { - 'id': 2, - 'project_id': 20, - 'project_name': 'Test Project 2', - 'name': 'Test User 2', - 'email1': 'test2@example.com', - 'active': False, - 'role': RoleEnum.MEMBER, - } - - project_view = JoinedProjectView(**project_data) - dumped = project_view.model_dump() - - # Check that computed fields are in dump - assert 'auth_netid' in dumped, "auth_netid should be in dump" - assert 'auth_username' in dumped, "auth_username should be in dump" - assert dumped['auth_netid'] is False, "auth_netid should match active in dump" - assert dumped['auth_username'] is False, "auth_username should be False in dump" - - -class TestUserTableSchema: - """Tests for UserTableSchema (database representation)""" - - def test_user_table_schema_has_active_field(self): - """Test that UserTableSchema includes active field""" - user_data = { - 'id': 1, - 'username': None, - 'name': 'Test User', - 'email1': 'test@example.com', - 'active': True, - } - - user = UserTableSchema(**user_data) - assert user.active is True, "UserTableSchema should have active field" - - def test_user_table_schema_no_auth_fields(self): - """Test that UserTableSchema does not have old auth fields""" - user_data = { - 'id': 2, - 'username': None, - 'name': 'Test User 2', - 'email1': 'test2@example.com', - 'active': False, - } - - user = UserTableSchema(**user_data) - - # The schema should not have auth_netid or auth_username as real fields - # (they might be in __dict__ due to extra='ignore' but shouldn't be defined) - assert hasattr(user, 'active'), "Should have active field" - - # Verify we can't set auth_netid or auth_username directly - # (they're not in the schema definition) - dumped = user.model_dump(exclude_none=True) - assert 'active' in dumped, "active should be in dump" diff --git a/userapp/api/tests/test_users.py b/userapp/api/tests/test_users.py index 2dfa8c5..0102895 100644 --- a/userapp/api/tests/test_users.py +++ b/userapp/api/tests/test_users.py @@ -1,8 +1,13 @@ import random from httpx import Client +import pytest +from pydantic import ValidationError from userapp.api.tests.fake_data import user_data_f +from userapp.core.models.enum import RoleEnum +from userapp.core.schemas.general import JoinedProjectView +from userapp.core.schemas.users import UserGet, UserPost class TestUsers: @@ -52,7 +57,7 @@ def test_get_user(self, admin_client: Client, user_factory, project_factory): fetched_user = user_response.json() assert fetched_user['id'] == user['id'], "Fetched user ID should match the created user ID" assert fetched_user['name'] == user['name'], "Fetched user name should match the created user name" - assert fetched_user['username'] is None, "Fetched user username should be None" + assert fetched_user['username'] == user['username'], "Fetched user username should match the created user username" def test_update_user_simple(self, admin_client: Client, user_factory, project_factory): """Test updating an existing user""" @@ -297,3 +302,85 @@ def test_patch_user_doesnt_remove_submit_nodes(self, admin_client: Client, user, updated_data = user_payload.json() assert len(updated_data['submit_nodes']) > 0, "User's submit nodes should not be removed when patching with an empty list" + + +def schema_user_data(**overrides): + data = { + "id": 1, + "username": "testnetid", + "name": "Test User", + "email1": "test@example.com", + "active": True, + "netid": "testnetid", + } + data.update(overrides) + return data + + +def schema_project_view_data(**overrides): + data = { + "id": 1, + "project_id": 10, + "project_name": "Test Project", + "username": "testnetid", + "name": "Test User", + "email1": "test@example.com", + "active": True, + "netid": "testnetid", + "role": RoleEnum.PI, + } + data.update(overrides) + return data + + +def expected_auth_netid(user_data: dict) -> bool | None: + return user_data["active"] and user_data["netid"] == user_data["username"] + + +def expected_auth_username(user_data: dict) -> bool: + return user_data["netid"] != user_data["username"] + + +class TestUserAuthBehavior: + def test_schema_computed_fields_follow_username_and_netid(self): + netid_user = UserGet(**schema_user_data(active=True, username="testnetid", netid="testnetid")) + username_user = UserGet(**schema_user_data(active=True, username="unixuser", netid="testnetid")) + inactive_project_view = JoinedProjectView( + **schema_project_view_data(active=False, username="unixuser", netid="testnetid", role=RoleEnum.MEMBER) + ) + + assert netid_user.auth_netid is True + assert netid_user.auth_username is False + assert username_user.auth_netid is False + assert username_user.auth_username is True + assert inactive_project_view.model_dump()["auth_netid"] is False + assert inactive_project_view.model_dump()["auth_username"] is False + + def test_userpost_requires_netid_when_active(self): + with pytest.raises(ValidationError) as exc_info: + UserPost(**schema_user_data(username="testnetid", active=True, netid=None)) + + assert "netid must be provided" in str(exc_info.value).lower() + + def test_user_endpoints_include_auth_compatibility_fields(self, admin_client: Client, project_factory): + project = project_factory() + user_payload = user_data_f(100, project["id"], username="unixuser100") + user_payload["netid"] = f"netid{random.randint(100000, 999999)}" + user_payload["active"] = True + + create_response = admin_client.post("/users", json=user_payload) + assert create_response.status_code == 201, create_response.text + created_user = create_response.json() + + get_response = admin_client.get(f"/users/{created_user['id']}") + assert get_response.status_code == 200, get_response.text + fetched_user = get_response.json() + + projects_response = admin_client.get(f"/users/{created_user['id']}/projects") + assert projects_response.status_code == 200, projects_response.text + project_view = projects_response.json()[0] + + assert fetched_user["auth_netid"] == expected_auth_netid(fetched_user) + assert fetched_user["auth_username"] == expected_auth_username(fetched_user) + assert project_view["auth_netid"] == expected_auth_netid(project_view) + assert project_view["auth_username"] == (project_view["active"] and expected_auth_username(project_view)) diff --git a/userapp/api/tests/test_users_active_field.py b/userapp/api/tests/test_users_active_field.py deleted file mode 100644 index 2edccfd..0000000 --- a/userapp/api/tests/test_users_active_field.py +++ /dev/null @@ -1,237 +0,0 @@ -import random - -from httpx import Client - -from userapp.api.tests.fake_data import user_data_f - -# Mostly AI generated tests, looks decent though - -class TestActiveField: - """Tests for the active field in user model""" - - def test_create_user_with_active_true(self, admin_client: Client, project_factory): - """Test creating a user with active=True and netid""" - project = project_factory() - user_payload = user_data_f(1, project['id']) - - # Set active to True and ensure netid is present - user_payload['active'] = True - user_payload['netid'] = f"testnetid{random.randint(100000, 999999)}" - - response = admin_client.post("/users", json=user_payload) - - assert response.status_code == 201, f"Creating user with active=True should succeed, got {response.text}" - created_user = response.json() - assert created_user['active'] is True, "Created user should have active=True" - assert created_user['netid'] == user_payload['netid'], "Created user should have the specified netid" - - def test_create_user_with_active_false(self, admin_client: Client, project_factory): - """Test creating a user with active=False""" - project = project_factory() - user_payload = user_data_f(2, project['id']) - - # Set active to False - user_payload['active'] = False - - response = admin_client.post("/users", json=user_payload) - - assert response.status_code == 201, f"Creating user with active=False should succeed, got {response.text}" - created_user = response.json() - assert created_user['active'] is False, "Created user should have active=False" - - def test_create_user_with_active_true_no_netid_fails(self, admin_client: Client, project_factory): - """Test that creating a user with active=True but no netid fails validation""" - project = project_factory() - user_payload = user_data_f(3, project['id']) - - # Set active to True but remove netid - user_payload['active'] = True - user_payload['netid'] = None - - response = admin_client.post("/users", json=user_payload) - - assert response.status_code == 422, f"Creating user with active=True and no netid should fail with 422, got {response.status_code}" - error_detail = response.json() - assert 'netid must be provided' in str(error_detail).lower(), "Error should mention netid requirement" - - def test_update_user_active_field(self, admin_client: Client, user_factory, project_factory): - """Test updating a user's active field""" - project = project_factory() - user = user_factory(4, project['id']) - - # Update active to True - update_payload = { - 'active': True, - 'netid': f"updatednetid{random.randint(100000, 999999)}" - } - - response = admin_client.patch(f"/users/{user['id']}", json=update_payload) - - assert response.status_code == 200, f"Updating user active field should succeed, got {response.text}" - updated_user = response.json() - assert updated_user['active'] is True, "Updated user should have active=True" - assert updated_user['netid'] == update_payload['netid'], "Updated user should have new netid" - - -class TestBackwardsCompatibility: - """Tests for backwards compatibility computed fields""" - - def test_get_user_includes_auth_netid_field(self, admin_client: Client, user_factory, project_factory): - """Test that GET /users/{id} includes auth_netid computed field""" - project = project_factory() - user_payload = user_data_f(10, project['id']) - user_payload['active'] = True - user_payload['netid'] = f"testnetid{random.randint(100000, 999999)}" - - # Create user - create_response = admin_client.post("/users", json=user_payload) - assert create_response.status_code == 201 - user_id = create_response.json()['id'] - - # Get user - get_response = admin_client.get(f"/users/{user_id}") - assert get_response.status_code == 200 - - user_data = get_response.json() - - # Check backwards compatibility fields exist - assert 'auth_netid' in user_data, "Response should include auth_netid field" - assert 'auth_username' in user_data, "Response should include auth_username field" - - # Check auth_netid maps to active - assert user_data['auth_netid'] == user_data['active'], "auth_netid should equal active field" - - # Check auth_username is always False - assert user_data['auth_username'] is False, "auth_username should always be False" - - def test_get_user_auth_netid_false_when_active_false(self, admin_client: Client, user_factory, project_factory): - """Test that auth_netid is False when active is False""" - project = project_factory() - user_payload = user_data_f(11, project['id']) - user_payload['active'] = False - - # Create user - create_response = admin_client.post("/users", json=user_payload) - assert create_response.status_code == 201 - user_id = create_response.json()['id'] - - # Get user - get_response = admin_client.get(f"/users/{user_id}") - assert get_response.status_code == 200 - - user_data = get_response.json() - - # Check that both active and auth_netid are False - assert user_data['active'] is False, "active should be False" - assert user_data['auth_netid'] is False, "auth_netid should be False when active is False" - - def test_list_users_includes_backwards_compat_fields(self, admin_client: Client, user_factory, project_factory): - """Test that GET /users includes backwards compatibility fields for all users""" - project = project_factory() - - # Create users with different active values - user1_payload = user_data_f(12, project['id']) - user1_payload['active'] = True - user1_payload['netid'] = f"testnetid1{random.randint(100000, 999999)}" - - user2_payload = user_data_f(13, project['id']) - user2_payload['active'] = False - - admin_client.post("/users", json=user1_payload) - admin_client.post("/users", json=user2_payload) - - # List users - response = admin_client.get("/users") - assert response.status_code == 200 - - users_list = response.json() - assert len(users_list) >= 2, "Should have at least 2 users" - - # Check that all users have backwards compatibility fields - for user in users_list: - assert 'auth_netid' in user, "Each user should have auth_netid field" - assert 'auth_username' in user, "Each user should have auth_username field" - assert user['auth_netid'] == user['active'], "auth_netid should match active" - assert user['auth_username'] is False, "auth_username should always be False" - - def test_get_user_projects_includes_backwards_compat_fields(self, admin_client: Client, user_factory, project_factory): - """Test that GET /users/{id}/projects includes backwards compatibility fields""" - project = project_factory() - user = user_factory(14, project['id']) - - # Get user projects (which returns JoinedProjectView) - response = admin_client.get(f"/users/{user['id']}/projects") - assert response.status_code == 200 - - projects = response.json() - assert len(projects) >= 1, "User should have at least one project" - - # Check backwards compatibility fields in project view - for project_view in projects: - assert 'auth_netid' in project_view, "Project view should include auth_netid" - assert 'auth_username' in project_view, "Project view should include auth_username" - assert project_view['auth_netid'] == project_view['active'], "auth_netid should match active" - assert project_view['auth_username'] is False, "auth_username should always be False" - - -class TestActiveFieldValidation: - """Tests for active field validation logic""" - - def test_active_field_defaults_to_false(self, admin_client: Client, project_factory): - """Test that active field defaults to False when not specified""" - project = project_factory() - user_payload = user_data_f(20, project['id']) - - # Don't specify active field (it's already False in fake_data, but let's be explicit) - if 'active' in user_payload: - del user_payload['active'] - - response = admin_client.post("/users", json=user_payload) - - assert response.status_code == 201, f"Creating user should succeed, got {response.text}" - created_user = response.json() - - # The database default should make this False - assert created_user['active'] is False or created_user['active'] is None, \ - "Active field should default to False" - - def test_patch_active_to_true_requires_netid(self, admin_client: Client, user_factory, project_factory): - """Test that patching active to True requires netid to be set""" - project = project_factory() - user = user_factory(21, project['id']) - - # Try to update active to True without netid - update_payload = { - 'active': True, - 'netid': None - } - - response = admin_client.patch(f"/users/{user['id']}", json=update_payload) - - # This might succeed if the user already has a netid, so let's check - # We need to ensure the user doesn't have a netid first - # Get current user state - get_response = admin_client.get(f"/users/{user['id']}") - current_user = get_response.json() - - if current_user['netid'] is None: - # If user has no netid, updating active to True should fail - # However, PATCH validation only happens on POST in our current schema - # So this test documents current behavior - pass - - def test_create_user_with_active_and_netid(self, admin_client: Client, project_factory): - """Test creating a user with both active=True and a netid succeeds""" - project = project_factory() - user_payload = user_data_f(22, project['id']) - - netid_value = f"validnetid{random.randint(100000, 999999)}" - user_payload['active'] = True - user_payload['netid'] = netid_value - - response = admin_client.post("/users", json=user_payload) - - assert response.status_code == 201, f"Creating user with active and netid should succeed, got {response.text}" - created_user = response.json() - assert created_user['active'] is True - assert created_user['netid'] == netid_value diff --git a/userapp/core/models/tables.py b/userapp/core/models/tables.py index 06363ed..7b23070 100644 --- a/userapp/core/models/tables.py +++ b/userapp/core/models/tables.py @@ -97,6 +97,7 @@ class User(Base): email1 = Column(String(255), nullable=False) email2 = Column(String(255)) netid = Column(String(255)) # Made unique via a Table constraint + username = Column(String(255), unique=True, nullable=False) netid_exp_datetime = Column(TIMESTAMP) phone1 = Column(String(255)) phone2 = Column(String(255)) diff --git a/userapp/core/models/views.py b/userapp/core/models/views.py index 6fdbcb0..8139fd1 100644 --- a/userapp/core/models/views.py +++ b/userapp/core/models/views.py @@ -46,6 +46,7 @@ class JoinedProjectView(Base): project_accounting_group = Column(String(255)) is_primary = Column(Boolean) name = Column(String(255)) + username = Column(String(255)) email1 = Column(String(255)) email2 = Column(String(255)) netid = Column(String(255)) # Should be unique post transition diff --git a/userapp/core/schemas/general.py b/userapp/core/schemas/general.py index f31ea9a..8b2afad 100644 --- a/userapp/core/schemas/general.py +++ b/userapp/core/schemas/general.py @@ -39,6 +39,7 @@ class JoinedProjectView(BaseModel): project_accounting_group: Optional[str] = Field(default=None) is_primary: Optional[bool] = Field(default=None) name: str + username: str email1: EmailStr email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -56,14 +57,12 @@ class JoinedProjectView(BaseModel): @computed_field @property def auth_netid(self) -> Optional[bool]: - """Backwards compatibility: maps to active field""" - return self.active + return self.active and self.netid == self.username @computed_field @property - def auth_username(self) -> bool: - """Backwards compatibility: always returns False""" - return False + def auth_username(self) -> Optional[bool]: + return self.active and self.netid != self.username class UserApplicationView(BaseModel): # BaseForm fields diff --git a/userapp/core/schemas/users.py b/userapp/core/schemas/users.py index 7e92b7c..fb84276 100644 --- a/userapp/core/schemas/users.py +++ b/userapp/core/schemas/users.py @@ -18,6 +18,7 @@ class UserTableSchema(BaseModel): id: Optional[int] = Field(default=None) name: str + username: str email1: EmailStr email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -40,7 +41,7 @@ class UserGet(BaseModel): id: Optional[int] = Field(default=None) name: str - username: None = Field(default=None) + username: str email1: EmailStr email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -60,12 +61,12 @@ def serialize_position(self, position: PositionEnum) -> str: @computed_field @property def auth_netid(self) -> Optional[bool]: - return self.active + return self.active and self.netid == self.username @computed_field @property - def auth_username(self) -> bool: - return False + def auth_username(self) -> Optional[bool]: + return self.active and self.netid != self.username class UserGetFull(UserGet): @@ -80,6 +81,7 @@ class UserPost(BaseModel): model_config = ConfigDict(extra='ignore') name: str + username: str email1: EmailStr email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -99,6 +101,8 @@ def serialize_position(self, position: PositionEnum) -> str: def check_active_requires_netid(self): if self.active and not self.netid: raise ValueError("If active is True, netid must be provided.") + if self.active and not self.username: + raise ValueError("If active is True, username must be provided.") return self @@ -117,6 +121,7 @@ class UserPatch(BaseModel): email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) + username: Optional[str] = Field(default=None) netid_exp_datetime: Optional[datetime] = Field(default=None) phone1: Optional[str] = Field(default=None) phone2: Optional[str] = Field(default=None) @@ -129,6 +134,15 @@ class UserPatch(BaseModel): def serialize_position(self, position: PositionEnum) -> str: return position.name if position is not None else None + @model_validator(mode="after") + def check_active_requires_identity(self): + if self.active: + if 'netid' in self.model_fields_set and not self.netid: + raise ValueError("If active is True, netid must be provided.") + if 'username' in self.model_fields_set and not self.username: + raise ValueError("If active is True, username must be provided.") + return self + class UserPatchFull(UserPatch): submit_nodes: Optional[list[UserSubmitPost]] = Field(default=None) From b7bac307ebbdf8439621fd9a35a04371314f5a40 Mon Sep 17 00:00:00 2001 From: fallow64 Date: Mon, 27 Apr 2026 11:18:13 -0500 Subject: [PATCH 2/7] Fix form validation tests --- userapp/api/tests/test_users.py | 3 ++- userapp/core/schemas/users.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/userapp/api/tests/test_users.py b/userapp/api/tests/test_users.py index 0102895..df6c56f 100644 --- a/userapp/api/tests/test_users.py +++ b/userapp/api/tests/test_users.py @@ -171,7 +171,8 @@ def test_nullify_email1(self, admin_client: Client, user_factory, project_factor user_payload = admin_client.patch(f"/users/{user['id']}", json=update_payload) - assert user_payload.status_code == 400, f"Cannot nullify email1, should return a 500 status code, instead got {user_payload.text}" + assert user_payload.status_code == 200, f"Nullifying email1 should return a 200 status code, instead got {user_payload.text}" + assert user_payload.json()["email1"] is None, "email1 should be null after patching with an empty string" def test_user_get_self(self, user, nonadmin_client): """Test that a user can get their own details""" diff --git a/userapp/core/schemas/users.py b/userapp/core/schemas/users.py index 5a6e463..7e4fb0d 100644 --- a/userapp/core/schemas/users.py +++ b/userapp/core/schemas/users.py @@ -82,7 +82,7 @@ class UserPost(BaseModel): name: str username: str - email1: EmailStr + email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) netid_exp_datetime: Optional[datetime] = Field(default=None) From acb174925b0940d263fedfec553a60293130392f Mon Sep 17 00:00:00 2001 From: fallow64 Date: Mon, 27 Apr 2026 12:28:37 -0500 Subject: [PATCH 3/7] Make username optional and update upgrade script --- .../47f8adc9859b_add_unix_username_field.py | 18 +++++----- userapp/api/routes/users.py | 13 ++++++-- userapp/api/tests/test_users.py | 33 +++++++++++++++++++ userapp/core/models/tables.py | 2 +- userapp/core/schemas/general.py | 6 ++-- userapp/core/schemas/users.py | 33 +++++++++++-------- 6 files changed, 76 insertions(+), 29 deletions(-) diff --git a/alembic/versions/47f8adc9859b_add_unix_username_field.py b/alembic/versions/47f8adc9859b_add_unix_username_field.py index 0b32fea..9fedab7 100644 --- a/alembic/versions/47f8adc9859b_add_unix_username_field.py +++ b/alembic/versions/47f8adc9859b_add_unix_username_field.py @@ -33,22 +33,20 @@ def upgrade() -> None: sa.column('netid', sa.String(length=255)), sa.column('username', sa.String(length=255)), ) - op.execute( - users_table.update() - .values(username=users_table.c.netid) - ) + + op.execute(""" + UPDATE users + SET username = netid + WHERE active IS TRUE + """) + for netid, username in netid_username_map.items(): op.execute( users_table.update() .where(users_table.c.netid == netid) .values(username=username) ) - op.execute(""" - UPDATE users - SET username = 'legacy-user-' || id::text - WHERE username IS NULL - """) - op.alter_column('users', 'username', nullable=False) + op.create_unique_constraint(op.f('users_username_key'), 'users', ['username']) op.execute(""" CREATE VIEW joined_projects AS diff --git a/userapp/api/routes/users.py b/userapp/api/routes/users.py index ad0ca82..56a09ae 100644 --- a/userapp/api/routes/users.py +++ b/userapp/api/routes/users.py @@ -115,10 +115,19 @@ async def update_user(user_id: int, user: UserPatchFull, session=Depends(session } # Validate that the merged data conforms to the UserPost schema - UserPost(**merged_user_data) + validated_user = UserPost(**merged_user_data) # Update user - user_data_only = UserPatch(**user.model_dump(exclude_unset=True)) + user_update_data = user.model_dump(exclude_unset=True) + if ( + validated_user.active + and "netid" in user_update_data + and "username" not in user_update_data + ): + user_update_data["username"] = validated_user.netid + if validated_user.username != current_user.username: + user_update_data["username"] = validated_user.username + user_data_only = UserPatch(**user_update_data) updated_user = await update_one_endpoint(session, UserTable, user_id, user_data_only, load_options=user_load_options) # Update Submit Nodes if patched diff --git a/userapp/api/tests/test_users.py b/userapp/api/tests/test_users.py index df6c56f..f50b1e6 100644 --- a/userapp/api/tests/test_users.py +++ b/userapp/api/tests/test_users.py @@ -363,6 +363,39 @@ def test_userpost_requires_netid_when_active(self): assert "netid must be provided" in str(exc_info.value).lower() + def test_create_active_user_defaults_username_to_netid(self, admin_client: Client, project_factory): + project = project_factory() + user_payload = user_data_f(101, project["id"]) + user_payload.pop("username") + + create_response = admin_client.post("/users", json=user_payload) + assert create_response.status_code == 201, create_response.text + created_user = create_response.json() + + assert created_user["username"] == created_user["netid"] + assert created_user["auth_netid"] is True + assert created_user["auth_username"] is False + + def test_patch_user_can_set_distinct_username(self, admin_client: Client, project_factory): + project = project_factory() + user_payload = user_data_f(102, project["id"]) + + create_response = admin_client.post("/users", json=user_payload) + assert create_response.status_code == 201, create_response.text + created_user = create_response.json() + + update_payload = { + "username": f"unixuser{random.randint(100000, 999999)}", + } + patch_response = admin_client.patch(f"/users/{created_user['id']}", json=update_payload) + assert patch_response.status_code == 200, patch_response.text + updated_user = patch_response.json() + + assert updated_user["username"] == update_payload["username"] + assert updated_user["netid"] == created_user["netid"] + assert updated_user["auth_netid"] is False + assert updated_user["auth_username"] is True + def test_user_endpoints_include_auth_compatibility_fields(self, admin_client: Client, project_factory): project = project_factory() user_payload = user_data_f(100, project["id"], username="unixuser100") diff --git a/userapp/core/models/tables.py b/userapp/core/models/tables.py index 3dad4b7..9fdc131 100644 --- a/userapp/core/models/tables.py +++ b/userapp/core/models/tables.py @@ -100,7 +100,7 @@ class User(Base): email1 = Column(String(255)) email2 = Column(String(255)) netid = Column(String(255)) # Made unique via a Table constraint - username = Column(String(255), unique=True, nullable=False) + username = Column(String(255), unique=True) netid_exp_datetime = Column(TIMESTAMP) phone1 = Column(String(255)) phone2 = Column(String(255)) diff --git a/userapp/core/schemas/general.py b/userapp/core/schemas/general.py index 3ad632f..ed05a1e 100644 --- a/userapp/core/schemas/general.py +++ b/userapp/core/schemas/general.py @@ -39,7 +39,7 @@ class JoinedProjectView(BaseModel): project_accounting_group: Optional[str] = Field(default=None) is_primary: Optional[bool] = Field(default=None) name: str - username: str + username: Optional[str] = Field(default=None) email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -57,12 +57,12 @@ class JoinedProjectView(BaseModel): @computed_field @property def auth_netid(self) -> Optional[bool]: - return self.active and self.netid == self.username + return self.active and self.netid is not None and self.username == self.netid @computed_field @property def auth_username(self) -> Optional[bool]: - return self.active and self.netid != self.username + return self.active and self.netid is not None and self.username is not None and self.netid != self.username class UserApplicationView(BaseModel): # BaseForm fields diff --git a/userapp/core/schemas/users.py b/userapp/core/schemas/users.py index 7e4fb0d..6e82215 100644 --- a/userapp/core/schemas/users.py +++ b/userapp/core/schemas/users.py @@ -11,6 +11,14 @@ from userapp.core.models.enum import RoleEnum, PositionEnum +def _normalize_active_identity(active: Optional[bool], netid: Optional[str], username: Optional[str]) -> tuple[Optional[str], Optional[str]]: + if active and not netid: + raise ValueError("If active is True, netid must be provided.") + if active and not username: + username = netid + return netid, username + + class UserTableSchema(BaseModel): """Used to represent a user as stored in the database""" @@ -18,7 +26,7 @@ class UserTableSchema(BaseModel): id: Optional[int] = Field(default=None) name: str - username: str + username: Optional[str] = Field(default=None) email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -35,13 +43,18 @@ class UserTableSchema(BaseModel): def serialize_position(self, position: PositionEnum) -> str | None: return position.name if position is not None else None + @model_validator(mode="after") + def normalize_active_identity(self): + self.netid, self.username = _normalize_active_identity(self.active, self.netid, self.username) + return self + class UserGet(BaseModel): model_config = ConfigDict(extra='ignore') id: Optional[int] = Field(default=None) name: str - username: str + username: Optional[str] = Field(default=None) email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -61,12 +74,12 @@ def serialize_position(self, position: PositionEnum) -> str: @computed_field @property def auth_netid(self) -> Optional[bool]: - return self.active and self.netid == self.username + return self.active and self.netid is not None and self.username == self.netid @computed_field @property def auth_username(self) -> Optional[bool]: - return self.active and self.netid != self.username + return self.active and self.netid is not None and self.username is not None and self.netid != self.username class UserGetFull(UserGet): @@ -81,7 +94,7 @@ class UserPost(BaseModel): model_config = ConfigDict(extra='ignore') name: str - username: str + username: Optional[str] = Field(default=None) email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -99,10 +112,7 @@ def serialize_position(self, position: PositionEnum) -> str: @model_validator(mode="after") def check_active_requires_netid(self): - if self.active and not self.netid: - raise ValueError("If active is True, netid must be provided.") - if self.active and not self.username: - raise ValueError("If active is True, username must be provided.") + self.netid, self.username = _normalize_active_identity(self.active, self.netid, self.username) return self @@ -137,10 +147,7 @@ def serialize_position(self, position: PositionEnum) -> str: @model_validator(mode="after") def check_active_requires_identity(self): if self.active: - if 'netid' in self.model_fields_set and not self.netid: - raise ValueError("If active is True, netid must be provided.") - if 'username' in self.model_fields_set and not self.username: - raise ValueError("If active is True, username must be provided.") + self.netid, self.username = _normalize_active_identity(self.active, self.netid, self.username) return self class UserPatchFull(UserPatch): From 2bf1bb495e2995678bbfa64eb9f1bcf4e5c08de1 Mon Sep 17 00:00:00 2001 From: fallow64 Date: Mon, 27 Apr 2026 12:47:05 -0500 Subject: [PATCH 4/7] Update netid to username migration map Co-authored-by: Copilot --- .../versions/47f8adc9859b_add_unix_username_field.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/alembic/versions/47f8adc9859b_add_unix_username_field.py b/alembic/versions/47f8adc9859b_add_unix_username_field.py index 9fedab7..a7ff34c 100644 --- a/alembic/versions/47f8adc9859b_add_unix_username_field.py +++ b/alembic/versions/47f8adc9859b_add_unix_username_field.py @@ -19,7 +19,17 @@ netid_username_map = { - "aschneider37": "austins", + "thain": "gthain", + "tatannen": "tannenba", + "jamesfrey": "jfray", + "tlmiller2": "tlmiller", + "blin28": "blin", + "turatsinze": "tura", + "bockelman": "bbockelm", + "totheisen": "tim", + "mselmeci": "matyas", + "knoeller": "johnkn", + "tcartwright": "cat", } From 024335843abe01eb43dbaddaab1cd026dede0b42 Mon Sep 17 00:00:00 2001 From: fallow64 Date: Mon, 27 Apr 2026 14:41:56 -0500 Subject: [PATCH 5/7] Simplify logic --- userapp/core/schemas/general.py | 4 ++-- userapp/core/schemas/users.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/userapp/core/schemas/general.py b/userapp/core/schemas/general.py index ed05a1e..3175090 100644 --- a/userapp/core/schemas/general.py +++ b/userapp/core/schemas/general.py @@ -57,12 +57,12 @@ class JoinedProjectView(BaseModel): @computed_field @property def auth_netid(self) -> Optional[bool]: - return self.active and self.netid is not None and self.username == self.netid + return self.active and self.username == self.netid @computed_field @property def auth_username(self) -> Optional[bool]: - return self.active and self.netid is not None and self.username is not None and self.netid != self.username + return self.active and self.netid != self.username class UserApplicationView(BaseModel): # BaseForm fields diff --git a/userapp/core/schemas/users.py b/userapp/core/schemas/users.py index 6e82215..5909388 100644 --- a/userapp/core/schemas/users.py +++ b/userapp/core/schemas/users.py @@ -74,12 +74,12 @@ def serialize_position(self, position: PositionEnum) -> str: @computed_field @property def auth_netid(self) -> Optional[bool]: - return self.active and self.netid is not None and self.username == self.netid + return self.active and self.username == self.netid @computed_field @property def auth_username(self) -> Optional[bool]: - return self.active and self.netid is not None and self.username is not None and self.netid != self.username + return self.active and self.netid != self.username class UserGetFull(UserGet): From 59c12816a1cf52563ef8b06618fb16f94006518b Mon Sep 17 00:00:00 2001 From: clock Date: Tue, 12 May 2026 16:19:30 -0500 Subject: [PATCH 6/7] Tweaks --- .../47f8adc9859b_add_unix_username_field.py | 1 - userapp/api/routes/users.py | 31 +------------------ userapp/core/schemas/users.py | 21 ++----------- 3 files changed, 3 insertions(+), 50 deletions(-) diff --git a/alembic/versions/47f8adc9859b_add_unix_username_field.py b/alembic/versions/47f8adc9859b_add_unix_username_field.py index a7ff34c..c77d578 100644 --- a/alembic/versions/47f8adc9859b_add_unix_username_field.py +++ b/alembic/versions/47f8adc9859b_add_unix_username_field.py @@ -47,7 +47,6 @@ def upgrade() -> None: op.execute(""" UPDATE users SET username = netid - WHERE active IS TRUE """) for netid, username in netid_username_map.items(): diff --git a/userapp/api/routes/users.py b/userapp/api/routes/users.py index 56a09ae..0d4fe98 100644 --- a/userapp/api/routes/users.py +++ b/userapp/api/routes/users.py @@ -97,37 +97,8 @@ async def update_user(user_id: int, user: UserPatchFull, session=Depends(session return await update_one_endpoint(session, UserTable, user_id, user_update_schema, load_options=user_load_options) elif is_admin: - current_user = await get_one_endpoint(session, UserTable, user_id) - merged_user_data = { - "name": current_user.name, - "username": current_user.username, - "email1": current_user.email1, - "email2": current_user.email2, - "netid": current_user.netid, - "netid_exp_datetime": current_user.netid_exp_datetime, - "phone1": current_user.phone1, - "phone2": current_user.phone2, - "is_admin": current_user.is_admin, - "active": current_user.active, - "unix_uid": current_user.unix_uid, - "position": current_user.position, - **user.model_dump(exclude_unset=True, exclude={"submit_nodes"}), - } - - # Validate that the merged data conforms to the UserPost schema - validated_user = UserPost(**merged_user_data) - # Update user - user_update_data = user.model_dump(exclude_unset=True) - if ( - validated_user.active - and "netid" in user_update_data - and "username" not in user_update_data - ): - user_update_data["username"] = validated_user.netid - if validated_user.username != current_user.username: - user_update_data["username"] = validated_user.username - user_data_only = UserPatch(**user_update_data) + user_data_only = UserPatch(**user.model_dump(exclude_unset=True)) updated_user = await update_one_endpoint(session, UserTable, user_id, user_data_only, load_options=user_load_options) # Update Submit Nodes if patched diff --git a/userapp/core/schemas/users.py b/userapp/core/schemas/users.py index 5909388..446df19 100644 --- a/userapp/core/schemas/users.py +++ b/userapp/core/schemas/users.py @@ -11,14 +11,6 @@ from userapp.core.models.enum import RoleEnum, PositionEnum -def _normalize_active_identity(active: Optional[bool], netid: Optional[str], username: Optional[str]) -> tuple[Optional[str], Optional[str]]: - if active and not netid: - raise ValueError("If active is True, netid must be provided.") - if active and not username: - username = netid - return netid, username - - class UserTableSchema(BaseModel): """Used to represent a user as stored in the database""" @@ -43,11 +35,6 @@ class UserTableSchema(BaseModel): def serialize_position(self, position: PositionEnum) -> str | None: return position.name if position is not None else None - @model_validator(mode="after") - def normalize_active_identity(self): - self.netid, self.username = _normalize_active_identity(self.active, self.netid, self.username) - return self - class UserGet(BaseModel): model_config = ConfigDict(extra='ignore') @@ -112,7 +99,8 @@ def serialize_position(self, position: PositionEnum) -> str: @model_validator(mode="after") def check_active_requires_netid(self): - self.netid, self.username = _normalize_active_identity(self.active, self.netid, self.username) + if self.active and not self.netid: + raise ValueError("If active is True, netid must be provided.") return self @@ -144,11 +132,6 @@ class UserPatch(BaseModel): def serialize_position(self, position: PositionEnum) -> str: return position.name if position is not None else None - @model_validator(mode="after") - def check_active_requires_identity(self): - if self.active: - self.netid, self.username = _normalize_active_identity(self.active, self.netid, self.username) - return self class UserPatchFull(UserPatch): From 7e85884cc80ec633c5d1b390183a747c48e087b9 Mon Sep 17 00:00:00 2001 From: clock Date: Wed, 13 May 2026 10:12:38 -0500 Subject: [PATCH 7/7] Fixup auth_username and auth_netid logic --- userapp/core/schemas/general.py | 25 +++++++++++++++++++++---- userapp/core/schemas/users.py | 27 ++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 9 deletions(-) diff --git a/userapp/core/schemas/general.py b/userapp/core/schemas/general.py index 3175090..0b8142f 100644 --- a/userapp/core/schemas/general.py +++ b/userapp/core/schemas/general.py @@ -56,13 +56,30 @@ class JoinedProjectView(BaseModel): @computed_field @property - def auth_netid(self) -> Optional[bool]: - return self.active and self.username == self.netid + def auth_netid(self) -> bool: + # Check that we can even give them auth_netid with normal conditions + if not self.active or self.netid is None: + return False + + # Now we do a switch case for backward compatibility + # If the have a username that is defined but is different from their netid then we only want to enable auth_username + if self.username is not None and self.netid != self.username: + return False + + # Otherwise this is the traditional case where they are active and have a netid so they can auth with it + return True @computed_field @property - def auth_username(self) -> Optional[bool]: - return self.active and self.netid != self.username + def auth_username(self) -> bool: + """Used for backwards compatibility - returns true if both netid and username are set but different""" + + # First check the correct value are set and the user is active + if not self.active or self.username is None or self.netid is None: + return False + + # If the values are set but diverge then we prefer the username + return self.username != self.netid class UserApplicationView(BaseModel): # BaseForm fields diff --git a/userapp/core/schemas/users.py b/userapp/core/schemas/users.py index 446df19..d9d4933 100644 --- a/userapp/core/schemas/users.py +++ b/userapp/core/schemas/users.py @@ -60,13 +60,30 @@ def serialize_position(self, position: PositionEnum) -> str: @computed_field @property - def auth_netid(self) -> Optional[bool]: - return self.active and self.username == self.netid + def auth_netid(self) -> bool: + # Check that we can even give them auth_netid with normal conditions + if not self.active or self.netid is None: + return False + + # Now we do a switch case for backward compatibility + # If the have a username that is defined but is different from their netid then we only want to enable auth_username + if self.username is not None and self.netid != self.username: + return False + + # Otherwise this is the traditional case where they are active and have a netid so they can auth with it + return True @computed_field @property - def auth_username(self) -> Optional[bool]: - return self.active and self.netid != self.username + def auth_username(self) -> bool: + """Used for backwards compatibility - returns true if both netid and username are set but different""" + + # First check the correct value are set and the user is active + if not self.active or self.username is None or self.netid is None: + return False + + # If the values are set but diverge then we prefer the username + return self.username != self.netid class UserGetFull(UserGet): @@ -82,7 +99,7 @@ class UserPost(BaseModel): name: str username: Optional[str] = Field(default=None) - email1: Optional[EmailStr] = Field(default=None) + email1: EmailStr email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) netid_exp_datetime: Optional[datetime] = Field(default=None)