diff --git a/alembic/versions/cf94844cecc1_allow_email1_to_be_nullable.py b/alembic/versions/cf94844cecc1_allow_email1_to_be_nullable.py new file mode 100644 index 0000000..9edaeff --- /dev/null +++ b/alembic/versions/cf94844cecc1_allow_email1_to_be_nullable.py @@ -0,0 +1,88 @@ +"""Allow email1 to be nullable + +Revision ID: cf94844cecc1 +Revises: 6450d7ab8865 +Create Date: 2026-04-20 11:48:33.276491 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision: str = 'cf94844cecc1' +down_revision: Union[str, Sequence[str], None] = '6450d7ab8865' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + """Upgrade schema.""" + # ### commands auto generated by Alembic - please adjust! ### + op.add_column('user_form', sa.Column('email', sa.String(length=255), nullable=True)) + op.alter_column('users', 'email1', + existing_type=sa.VARCHAR(length=255), + nullable=True) + + # ### commands generated by human ### + + # This view exists, drop and create anew + op.execute("DROP VIEW IF EXISTS user_applications") + + # Create view with email field + op.execute(""" + CREATE VIEW user_applications AS + SELECT f.id, + f.form_type, + f.status, + f.created_by, + f.created_at, + f.updated_by, + f.updated_at, + uf.email, + uf.pi_id, + uf.pi_name, + uf.pi_email, + uf.position, + uf.content + FROM forms f + JOIN user_form uf ON f.id = uf.id + WHERE f.form_type = 'USER' + """) + # ### end Alembic commands ### + + +def downgrade() -> None: + """Downgrade schema.""" + # ### commands generated by human ### + op.execute("DROP VIEW IF EXISTS user_applications") + op.execute(""" + CREATE VIEW user_applications AS + SELECT f.id, + f.form_type, + f.status, + f.created_by, + f.created_at, + f.updated_by, + f.updated_at, + uf.pi_id, + uf.pi_name, + uf.pi_email, + uf.position, + uf.content + FROM forms f + JOIN user_form uf ON f.id = uf.id + WHERE f.form_type = 'USER' + """) + # ### end Alembic commands ### + + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('users', 'email1', + existing_type=sa.VARCHAR(length=255), + nullable=False) + op.drop_column('user_form', 'email') + # ### end Alembic commands ### + + diff --git a/userapp/api/routes/forms/_util.py b/userapp/api/routes/forms/_util.py index 4d9cbc0..5027b66 100644 --- a/userapp/api/routes/forms/_util.py +++ b/userapp/api/routes/forms/_util.py @@ -9,6 +9,8 @@ from userapp.core.schemas.user_project import UserProjectTableSchema from userapp.api.routes._util import _patch_user_submit_nodes +CHTC_TICKETING_EMAIL = "chtc@cs.wisc.edu" + ON_USER_FORM_SUBMIT_EMAIL_TEMPLATE = """ Dear {name}, @@ -48,12 +50,13 @@ The CHTC Research Computing Facilitation team """.strip() -async def on_user_form_submit(session: AsyncSession, form_id: int, form: None) -> None: - user_form = await session.scalar( - select(UserFormTable) - .where(UserFormTable.id == form_id) - ) +async def on_user_form_submit(session: AsyncSession, form_id: int, user_form: UserFormTable) -> None: + + # Get an email for the application submission user = user_form.base_form.created_by_user + email = user.email1 or user_form.email + if email is None: + raise HTTPException(status_code=400, detail="User form must have an email address if user account does not.") # Try sending the user an email try: @@ -61,7 +64,7 @@ async def on_user_form_submit(session: AsyncSession, form_id: int, form: None) - ON_USER_FORM_SUBMIT_EMAIL_TEMPLATE, name=user.name or "user", ) - send_email("chtc@cs.wisc.edu", user.email1, "CHTC Account Application Received", text) + send_email(CHTC_TICKETING_EMAIL, email, CHTC_TICKETING_EMAIL, "CHTC Account Application Received", text) except Exception: # we don't care if the email send fails pass @@ -83,6 +86,12 @@ async def on_user_form_accept(session: AsyncSession, form_id: int, form: UserFor user.active = True + # Set the users email if not already set + if user.email1 is None: + if form.email is None: + raise HTTPException(status_code=400, detail="User has no email address and no email provided in form patch") + user.email1 = form.email + # If we are not preserving the users data dump all of their groups if not form.preserve_existing_data: @@ -117,7 +126,7 @@ async def on_user_form_accept(session: AsyncSession, form_id: int, form: UserFor ON_USER_FORM_APPROVAL_EMAIL_TEMPLATE, name=user.name or "user", ) - send_email("chtc@cs.wisc.edu", user.email1, "CHTC Account Application Approved", text) + send_email(CHTC_TICKETING_EMAIL, user.email1, CHTC_TICKETING_EMAIL, "CHTC Account Application Approved", text) except Exception: # we don't care if the email send fails pass diff --git a/userapp/api/routes/forms/user_application.py b/userapp/api/routes/forms/user_application.py index 57fe688..9d2caad 100644 --- a/userapp/api/routes/forms/user_application.py +++ b/userapp/api/routes/forms/user_application.py @@ -124,13 +124,14 @@ async def create_user_form( } user_form_schema = UserFormTableSchema( id=created_base_form.id, + email=form.email, pi_id=form.pi_id, pi_name=form.pi_name, pi_email=form.pi_email, position=form.position, content=form_content ) - await create_one_endpoint(session, UserFormTable, user_form_schema) + user_form = await create_one_endpoint(session, UserFormTable, user_form_schema) # Flush session so we can get all the fields when we send the objects back as a view session.flush() @@ -138,7 +139,7 @@ async def create_user_form( # Trigger the None->Pending transition trigger = form_triggers.get((FormTypeEnum.USER, None, FormStatusEnum.PENDING)) if trigger: - await trigger(session, created_base_form.id, None) + await trigger(session, created_base_form.id, user_form) user_application_form = await get_one_endpoint(session, UserApplicationViewTable, created_base_form.id) return user_application_form diff --git a/userapp/api/routes/security.py b/userapp/api/routes/security.py index 6f5cc4d..f8a55bb 100644 --- a/userapp/api/routes/security.py +++ b/userapp/api/routes/security.py @@ -458,12 +458,11 @@ async def oidc_callback(request: Request, response: Response, session=Depends(se ) user_info_resp.raise_for_status() user_info = user_info_resp.json() - print(user_info) 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"], + email1=user_info.get("email", None), netid=user_info.get("sub"), active=False, is_admin=False, diff --git a/userapp/api/tests/test_forms.py b/userapp/api/tests/test_forms.py index 9d86a7e..53f76a0 100644 --- a/userapp/api/tests/test_forms.py +++ b/userapp/api/tests/test_forms.py @@ -1,10 +1,14 @@ import random +from typing import Callable, Generator, Any from unittest.mock import AsyncMock +import pytest from httpx import Client +from starlette.testclient import TestClient from userapp.api.routes import forms as forms_routes from userapp.core.models.enum import FormStatusEnum, FormTypeEnum, RoleEnum from userapp.api.tests.fake_data import user_form_data_f, user_form_approval_data_f +from userapp.api.tests.conftest import _make_auth_client def create_submit_node(admin_client: Client) -> dict: @@ -507,3 +511,202 @@ def test_approve_activates_user_and_assigns_access( assert updated_user["active"] is True assert any(project_membership["project_id"] == project["id"] for project_membership in updated_user["projects"]) assert any(user_submit["submit_node_name"] == submit_node["name"] for user_submit in updated_user["submit_nodes"]) + + +@pytest.fixture +def user_without_email(existing_admin_client: Client, project_factory, group_factory, user_factory) -> dict: + """Fixture that creates a test user and clears their email1 to simulate an OIDC user with no email.""" + + project = project_factory() + user = user_factory(0, project_id=project["id"]) + + # Clear email1 to simulate a user with no email from the OIDC provider + patch_response = existing_admin_client.patch(f"/users/{user['id']}", json={"email1": None}) + assert patch_response.status_code == 200, ( + f"Clearing email1 should return 200, got {patch_response.status_code}: {patch_response.text}" + ) + + user = existing_admin_client.get(f"/users/{user['id']}").json() + assert user["email1"] is None, "User email1 should be None for this fixture" + + yield user + + existing_admin_client.delete(f"/users/{user['id']}") + + +class TestEmailOnForm: + """Tests for the case where a user has no email address from their OIDC provider + and must supply one via the application form.""" + + def test_submit_with_email_succeeds_when_user_has_no_email( + self, + user_without_email: dict, + admin_client: Client, + ): + """A user without an email address can submit a form by providing email in the form.""" + + # Mark inactive so they can submit + r = admin_client.patch(f"/users/{user_without_email['id']}", json={"active": False}) + assert r.status_code == 200 + + form_data = user_form_data_f() + form_data["email"] = "noemail_user@example.com" + + client = next(_make_auth_client(user_without_email)) + response = client.post("/forms/user-applications", json=form_data) + + assert response.status_code == 201, ( + f"POST /forms/user-applications with email field should return 201 for user without email1, " + f"got {response.status_code}: {response.text}" + ) + + def test_submit_without_email_fails_when_user_has_no_email( + self, + user_without_email: dict, + admin_client: Client, + ): + """A user without an email address gets an error when they don't provide email in the form.""" + + # Mark inactive so they can submit + r = admin_client.patch(f"/users/{user_without_email['id']}", json={"active": False}) + assert r.status_code == 200 + + form_data = user_form_data_f() + # Explicitly omit email + form_data.pop("email", None) + + client = next(_make_auth_client(user_without_email)) + response = client.post("/forms/user-applications", json=form_data) + + assert response.status_code != 201, ( + f"POST /forms/user-applications without email should not return 201 when user has no email1, " + f"got {response.status_code}: {response.text}" + ) + + def test_approve_with_email_sets_user_email( + self, + user_without_email: dict, + admin_client: Client, + project_factory, + ): + """When admin approves a form and the user has no email1, providing email in the patch sets user.email1.""" + + # Mark inactive + r = admin_client.patch(f"/users/{user_without_email['id']}", json={"active": False}) + assert r.status_code == 200 + + form_data = user_form_data_f() + form_data["email"] = "new_email@example.com" + + client = next(_make_auth_client(user_without_email)) + create_response = client.post("/forms/user-applications", json=form_data) + + assert create_response.status_code == 201, ( + f"POST /forms/user-applications should return 201, got {create_response.status_code}: {create_response.text}" + ) + form_id = create_response.json()["id"] + + project = project_factory() + submit_node = create_submit_node(admin_client) + approval_data = user_form_approval_data_f(project["id"], [{"submit_node_id": submit_node["id"]}]) + approval_data["email"] = "new_email@example.com" + + update_response = admin_client.patch(f"/forms/user-applications/{form_id}", json=approval_data) + + assert update_response.status_code == 200, ( + f"Admin PATCH /forms/user-applications/{form_id} with email should return 200, " + f"got {update_response.status_code}: {update_response.text}" + ) + + user_response = admin_client.get(f"/users/{user_without_email['id']}") + assert user_response.status_code == 200 + updated_user = user_response.json() + + assert updated_user["email1"] == "new_email@example.com", ( + f"User email1 should be set to 'new_email@example.com' after approval, got {updated_user['email1']}" + ) + assert updated_user["active"] is True + + def test_approve_without_email_fails_when_user_has_no_email( + self, + user_without_email: dict, + admin_client: Client, + project_factory, + ): + """When admin approves a form without providing email and user has no email1, it should return 400.""" + + # Mark inactive + r = admin_client.patch(f"/users/{user_without_email['id']}", json={"active": False}) + assert r.status_code == 200 + + form_data = user_form_data_f() + form_data["email"] = "temp@example.com" + + client = next(_make_auth_client(user_without_email)) + create_response = client.post("/forms/user-applications", json=form_data) + + assert create_response.status_code == 201, ( + f"POST /forms/user-applications should return 201, got {create_response.status_code}: {create_response.text}" + ) + form_id = create_response.json()["id"] + + project = project_factory() + submit_node = create_submit_node(admin_client) + # Do NOT include email in the approval patch + approval_data = user_form_approval_data_f(project["id"], [{"submit_node_id": submit_node["id"]}]) + + update_response = admin_client.patch(f"/forms/user-applications/{form_id}", json=approval_data) + + assert update_response.status_code == 400, ( + f"Admin PATCH /forms/user-applications/{form_id} without email for user with no email1 should return 400, " + f"got {update_response.status_code}: {update_response.text}" + ) + + def test_form_email_field_is_stored( + self, + user_without_email: dict, + admin_client: Client, + ): + """The email provided in a form submission should be stored and retrievable.""" + + # Mark inactive + r = admin_client.patch(f"/users/{user_without_email['id']}", json={"active": False}) + assert r.status_code == 200 + + submitted_email = "stored_email@example.com" + form_data = user_form_data_f() + form_data["email"] = submitted_email + + client = next(_make_auth_client(user_without_email)) + create_response = client.post("/forms/user-applications", json=form_data) + + assert create_response.status_code == 201 + response_json = create_response.json() + assert response_json.get("email") == submitted_email, ( + f"Submitted email should be stored in the form response, got {response_json.get('email')}" + ) + + def test_user_with_email_can_submit_without_form_email( + self, + user: dict, + nonadmin_client: Client, + admin_client: Client, + ): + """A user who already has email1 from their OIDC provider doesn't need to provide email on the form.""" + + # Mark inactive + r = admin_client.patch(f"/users/{user['id']}", json={"active": False}) + assert r.status_code == 200 + + form_data = user_form_data_f() + # Ensure email is not in the form + form_data.pop("email", None) + + response = nonadmin_client.post("/forms/user-applications", json=form_data) + + assert response.status_code == 201, ( + f"A user with email1 should be able to submit a form without providing email, " + f"got {response.status_code}: {response.text}" + ) + + diff --git a/userapp/api/util.py b/userapp/api/util.py index 2413a50..39be3f0 100644 --- a/userapp/api/util.py +++ b/userapp/api/util.py @@ -176,10 +176,18 @@ def format_escaped_template(template: str, **kwargs) -> str: } return template.format(**escaped_kwargs) -def send_email(send_from: str, send_to: Union[str, list], subject: str, text: str, server=SMTP_SERVER): +def send_email(send_from: str, send_to: Union[str, list], cc: Union[str, list], subject: str, text: str, server=SMTP_SERVER): + + # Don't send emails outside of production + if os.getenv("PYTHON_ENV") != "production": + print(send_from, send_to, cc, subject, text) + return + + msg = MIMEMultipart() msg['From'] = send_from msg['To'] = send_to if isinstance(send_to, str) else ', '.join(send_to) # Handle the two types + msg["Cc"] = cc if isinstance(cc, str) else ', '.join(cc) # Handle the two types msg['Date'] = formatdate(localtime=True) msg['Subject'] = subject diff --git a/userapp/core/models/tables.py b/userapp/core/models/tables.py index 06363ed..30edcbd 100644 --- a/userapp/core/models/tables.py +++ b/userapp/core/models/tables.py @@ -94,7 +94,7 @@ class User(Base): id = Column(Integer, primary_key=True, index=True) name = Column(String(255), nullable=False) - email1 = Column(String(255), nullable=False) + email1 = Column(String(255)) email2 = Column(String(255)) netid = Column(String(255)) # Made unique via a Table constraint netid_exp_datetime = Column(TIMESTAMP) @@ -258,6 +258,7 @@ class UserForm(Base): id = Column(Integer, ForeignKey('forms.id', ondelete="CASCADE"), primary_key=True) # These can be tied into the existing data system + email = Column(String(255), nullable=True) pi_id = Column(Integer, ForeignKey('users.id', ondelete="SET NULL"), nullable=True) pi_name = Column(String(255), nullable=True) pi_email = Column(String(255), nullable=True) diff --git a/userapp/core/models/views.py b/userapp/core/models/views.py index 6fdbcb0..a2c22ba 100644 --- a/userapp/core/models/views.py +++ b/userapp/core/models/views.py @@ -89,6 +89,7 @@ class UserApplicationView(Base): updated_at = Column(TIMESTAMP) # UserForm columns + email = Column(String(255)) pi_id = Column(Integer) pi_name = Column(String(255)) pi_email = Column(String(255)) diff --git a/userapp/core/schemas/general.py b/userapp/core/schemas/general.py index f31ea9a..d59cb01 100644 --- a/userapp/core/schemas/general.py +++ b/userapp/core/schemas/general.py @@ -24,7 +24,7 @@ class PiProjectView(BaseModel): name: Optional[str] = Field(default=None) project_id: int project_name: str - email1: Optional[str] = Field(default=None) + email1: Optional[EmailStr] = Field(default=None) phone1: Optional[str] = Field(default=None) netid: Optional[str] = Field(default=None) @@ -39,7 +39,7 @@ class JoinedProjectView(BaseModel): project_accounting_group: Optional[str] = Field(default=None) is_primary: Optional[bool] = Field(default=None) name: 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) @@ -74,6 +74,7 @@ class UserApplicationView(BaseModel): updated_at: datetime # UserForm fields + email: Optional[EmailStr] = Field(default=None) pi_id: Optional[int] = Field(default=None) pi_name: Optional[str] = Field(default=None) pi_email: Optional[EmailStr] = Field(default=None) diff --git a/userapp/core/schemas/user_application_form.py b/userapp/core/schemas/user_application_form.py index 0ef5b6e..f0d325b 100644 --- a/userapp/core/schemas/user_application_form.py +++ b/userapp/core/schemas/user_application_form.py @@ -1,7 +1,7 @@ from datetime import datetime from typing import Optional -from pydantic import Field, model_validator +from pydantic import Field, model_validator, EmailStr from userapp.core.models.enum import FormStatusEnum, FormTypeEnum, PositionEnum, RoleEnum from userapp.core.schemas.general import BaseModel @@ -12,6 +12,7 @@ class UserFormPatch(BaseModel): status: FormStatusEnum preserve_existing_data: Optional[bool] = Field(default=False) + email: Optional[EmailStr] = Field(default=None) project_id: Optional[int] = Field(default=None) user_position: Optional[PositionEnum] = Field(default=None) submit_nodes: Optional[list["UserSubmitPost"]] = Field(default=None) @@ -20,6 +21,7 @@ class UserFormPatch(BaseModel): class UserFormTableSchema(BaseModel): """Schema for the database representation of a UserForm.""" id: int + email: Optional[EmailStr] = Field(default=None) pi_id: Optional[int] = Field(default=None) pi_name: Optional[str] = Field(default=None) pi_email: Optional[str] = Field(default=None) @@ -28,6 +30,7 @@ class UserFormTableSchema(BaseModel): class UserFormGet(BaseModel): + email: Optional[EmailStr] = Field(default=None) pi_id: Optional[int] = Field(default=None) pi_name: Optional[str] = Field(default=None) pi_email: Optional[str] = Field(default=None) @@ -36,6 +39,7 @@ class UserFormGet(BaseModel): class UserFormPost(BaseModel): + email: Optional[EmailStr] = Field(default=None) pi_id: Optional[int] = Field(default=None) pi_name: Optional[str] = Field(default=None) pi_email: Optional[str] = Field(default=None) diff --git a/userapp/core/schemas/users.py b/userapp/core/schemas/users.py index 7e92b7c..47ab05b 100644 --- a/userapp/core/schemas/users.py +++ b/userapp/core/schemas/users.py @@ -18,7 +18,7 @@ class UserTableSchema(BaseModel): id: Optional[int] = Field(default=None) name: 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) @@ -41,7 +41,7 @@ class UserGet(BaseModel): id: Optional[int] = Field(default=None) name: str username: None = Field(default=None) - 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) @@ -139,7 +139,6 @@ class RestrictedUserPatch(BaseModel): model_config = ConfigDict(extra='ignore') name: Optional[str] = Field(default=None) - email1: Optional[EmailStr] = Field(default=None) email2: Optional[EmailStr] = Field(default=None) phone1: Optional[str] = Field(default=None) phone2: Optional[str] = Field(default=None)