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..c77d578 --- /dev/null +++ b/alembic/versions/47f8adc9859b_add_unix_username_field.py @@ -0,0 +1,141 @@ +"""Add unix username field + +Revision ID: 47f8adc9859b +Revises: 4aa2a7d34719 +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] = '4aa2a7d34719' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +netid_username_map = { + "thain": "gthain", + "tatannen": "tannenba", + "jamesfrey": "jfray", + "tlmiller2": "tlmiller", + "blin28": "blin", + "turatsinze": "tura", + "bockelman": "bbockelm", + "totheisen": "tim", + "mselmeci": "matyas", + "knoeller": "johnkn", + "tcartwright": "cat", +} + + +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(""" + UPDATE users + SET username = netid + """) + + for netid, username in netid_username_map.items(): + op.execute( + users_table.update() + .where(users_table.c.netid == netid) + .values(username=username) + ) + + 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 f8a55bb..dcf533b 100644 --- a/userapp/api/routes/security.py +++ b/userapp/api/routes/security.py @@ -459,11 +459,12 @@ async def oidc_callback(request: Request, response: Response, session=Depends(se user_info_resp.raise_for_status() user_info = user_info_resp.json() + # 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.get("email", None), 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..0d4fe98 100644 --- a/userapp/api/routes/users.py +++ b/userapp/api/routes/users.py @@ -97,7 +97,6 @@ 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: - # Update user 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) 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..f50b1e6 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""" @@ -166,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""" @@ -297,3 +303,118 @@ 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_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") + 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 d0a8292..9fdc131 100644 --- a/userapp/core/models/tables.py +++ b/userapp/core/models/tables.py @@ -100,6 +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) 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 a2c22ba..bf59f0f 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 d59cb01..0b8142f 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: Optional[str] = Field(default=None) email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -55,15 +56,30 @@ class JoinedProjectView(BaseModel): @computed_field @property - def auth_netid(self) -> Optional[bool]: - """Backwards compatibility: maps to active field""" - return self.active + 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) -> bool: - """Backwards compatibility: always returns False""" - return False + """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 47ab05b..d9d4933 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: Optional[str] = Field(default=None) email1: Optional[EmailStr] = Field(default=None) 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: Optional[str] = Field(default=None) email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -59,13 +60,30 @@ def serialize_position(self, position: PositionEnum) -> str: @computed_field @property - def auth_netid(self) -> Optional[bool]: - return self.active + 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) -> bool: - return False + """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): @@ -80,6 +98,7 @@ class UserPost(BaseModel): model_config = ConfigDict(extra='ignore') name: str + username: Optional[str] = Field(default=None) email1: EmailStr email2: Optional[EmailStr] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -117,6 +136,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 +149,7 @@ class UserPatch(BaseModel): def serialize_position(self, position: PositionEnum) -> str: return position.name if position is not None else None + class UserPatchFull(UserPatch): submit_nodes: Optional[list[UserSubmitPost]] = Field(default=None)