Merge pull request #66 from ScilifelabDataCentre/organisation-in-user-model

SQ-805: Add organisation + user role to registration form

Author: RMCrean

docs/development/database-migrations.md

@@ -101,6 +101,32 @@ You can also run the `pytest-alembic` tests to further validate the newly create
 pytest tests/migrations
 ```
 
+## Migrations in local development (swapping between branches/downgrading)
+
+- If you pull a branch with new migrations, you will need to restart the stack to apply the new migrations (see above).
+- If you work on a branch with new migrations, and then switch back to main (or another branch without those migrations), you will need to downgrade the database to the previous migration version, and then restart the stack to apply the downgraded schema, here are the steps you can follows:
+
+### Protocol
+
+1. Be in the branch that has the latest migration script (as you need the downgrade function from the latest migration script so trying this from any other branch will not work).
+
+2. Use the db-migrator container to run the downgrade command (so adjust the command/entry point in the docker compose file to something like this:
+
+    ```bash
+    # From: docker/divbase_compose.yaml command: ["alembic", "upgrade", "head"]
+    ["alembic", "downgrade", "<REVISION_ID_TO_DOWNGRADE_TO>"] # or -1 to downgrade one step
+    ```
+
+3. Down everything
+
+4. Undo the change to the db-migrator container in the docker compose file.
+
+5. Swap to the other branch (e.g. main) that does not have the new migration script.
+
+6. docker compose up/watch and it should just work as you will now be at "head" again.
+
+The validation run by the fastapi lifespan event on startup will help tell you if you succeded or not, as it will error if you are not at the head revision for that branch. So if fastapi is working, it's probably working.
+
 ## Production Deployment
 
 Documentation on how to run migrations in production/deployed environments is covered in our [private repository, argocd-divbase](https://github.com/ScilifelabDataCentre/argocd-divbase).
@@ -121,3 +147,27 @@ Documentation on how to run migrations in production/deployed environments is co
 ### Starlette admin/admin panel not showing the models
 
 - You need the models in your src to match the postgres schema. So if you have pending changes (that you have or have not created migrations for) they won't display until you've actually done the migration.
+
+### Migrations that include new fields with non-nullable constraints
+
+One way to solve this is to modify the migration scripts upgrade command as follows:
+
+In this example, we're adding a new column "organisation" to the user table, and it cannot be null (and we already have users...), so we need to provide a default value for existing rows.
+
+```python
+def upgrade() -> None:
+    # Add the server_default param to the add operation for the new column
+    # "server_default=sa.text("'Not specified'") was added to below command
+    # We use sa.text to ensure the default value is set as a string in the database,
+    # see e.g. here for why: https://github.com/sqlalchemy/alembic/discussions/1433
+    op.add_column('user', sa.Column('organisation', sa.String(length=200), nullable=False, server_default=sa.text("'Not specified'"),))
+
+    # Any rows in the database table that existed before the migration will now be populated with the default value 'Not specified'.
+    # At the end of the same migration script, remove the server_default param from the column (with `server_default=None`) - so our models match exactly with the database schema.
+    # New rows will now require the application to provide a value for the organisation column.
+    op.alter_column('user', 'organisation', server_default=None)
+```
+
+**You don't need to set the server_default in the models.py, just in the migration script.**
+
+Alternatively you could set the server_default in the models.py, but then you should be comfortable with it always having a default value. There are also other ways to solve this problem...

packages/divbase-api/src/divbase_api/admin_panel.py

@@ -82,6 +82,8 @@ class UserView(ModelView):
         "id",
         StringField("name", required=True, help_text="Full name of the user."),
         EmailField("email", required=True, help_text="Email address of the user."),
+        StringField("organisation", required=True, help_text="Organisation of the user."),
+        StringField("organisation_role", required=True, help_text="Role of the user within their organisation."),
         StringField("password", required=True, help_text="Password for the user."),
         StringField(
             "hashed_password",
@@ -114,15 +116,32 @@ class UserView(ModelView):
         ),
     ]
 
-    exclude_fields_from_list = ["hashed_password", "password"]
+    exclude_fields_from_list = [
+        "hashed_password",
+        "password",
+        "organisation_role",
+        "date_deleted",
+        "last_password_change",
+        "created_at",
+        "updated_at",
+    ]
     exclude_fields_from_create = [
         "project_memberships",
         "is_deleted",
         "is_active",
         "last_password_change",
         "date_deleted",
-    ]  # hashed_password wont be defined yet but needs to be included.
-    exclude_fields_from_edit = ["project_memberships", "password", "hashed_password", "last_password_change"]
+        "created_at",
+        "updated_at",
+    ]
+    exclude_fields_from_edit = [
+        "project_memberships",
+        "password",
+        "hashed_password",  # hashed_password wont be defined yet but needs to be included here.
+        "last_password_change",
+        "created_at",
+        "updated_at",
+    ]
     exclude_fields_from_detail = ["hashed_password", "password"]
 
     def can_delete(self, request: Request) -> bool:

packages/divbase-api/src/divbase_api/api_constants.py

@@ -0,0 +1,52 @@
+"""
+DivBase API constants.
+"""
+
+# List of Swedish universities for the dropdown in the registration/update profile form.
+SWEDISH_UNIVERSITIES = [
+    "Blekinge Institute of Technology",
+    "Chalmers University of Technology",
+    "Dalarna University",
+    "Göteborgs University",
+    "Halmstad University",
+    "Jönköping University",
+    "KTH Royal Institute of Technology",
+    "Karlstad University",
+    "Karolinska institutet (KI)",
+    "Kristianstad University",
+    "Linköpings University",
+    "Linnaeus University",
+    "Luleå University of Technology",
+    "Lund University",
+    "Malmö University",
+    "Mid Sweden University",
+    "Mälardalen University",
+    "Stockholm School of Economics",
+    "Stockholms University",
+    "Swedish Defence University",
+    "Swedish School of Sport and Health Sciences",
+    "Swedish University of Agricultural Sciences",
+    "Södertörn University",
+    "Umeå University",
+    "University West",
+    "University of Borås",
+    "University of Gävle",
+    "University of Skövde",
+    "Uppsala University",
+    "Örebro University",
+]
+
+# List of known job roles for the dropdown in the registration/update profile form.
+KNOWN_JOB_ROLES = [
+    "Bachelor's Student",
+    "Bioinformatician",
+    "Infrastructure Support Staff",
+    "Master's Student",
+    "PhD Candidate",
+    "Postdoctoral Researcher",
+    "Principal Investigator",
+    "Research Associate",
+    "Researcher",
+    "Scientist",
+    "Technician",
+]

packages/divbase-api/src/divbase_api/crud/auth.py

@@ -127,7 +127,6 @@ async def update_user_password(db: AsyncSession, user_id: int, password_data: Us
 
 def delete_auth_cookies(response: Response) -> Response:
     """Helper to delete auth cookies from a response (e.g. on logout)."""
-    # TODO - when token blacklisting is implemented, blacklist the tokens here too. (make async at that point too...)
     response.delete_cookie(TokenType.ACCESS.value)
     response.delete_cookie(TokenType.REFRESH.value)
     return response

packages/divbase-api/src/divbase_api/crud/users.py

@@ -58,7 +58,7 @@ async def create_user(
             internal_logging_message=f"Attempt made to register new account with existing email: {proposed_email}"
         )
 
-    user_dict = user_data.model_dump(exclude={"password"})
+    user_dict = user_data.model_dump(exclude={"password", "confirm_password"})
     hashed_password = get_password_hash(user_data.password)
 
     user = UserDB(**user_dict, hashed_password=hashed_password, is_admin=is_admin, email_verified=email_verified)
@@ -119,3 +119,18 @@ async def update_user_profile(db: AsyncSession, user_data: UserUpdate, user_id:
     await db.commit()
     await db.refresh(user)
     return user
+
+
+def resolve_dropdown_form_input(dropdown_value: str, other_value: str | None) -> str | None:
+    """
+    Helper to resolve "Other" fields in registration/profile update forms.
+    These fields have a dropdown with predefined options and an "Other" option that reveals a text input.
+    If "Other" selected, we take the value from the text input instead. If the text input is empty or too short, we return None to indicate an error (e.g. in the registration/profile update endpoint), otherwise we return the resolved value. If "Other" is not selected, we just return the original value.
+
+    If non-resolvable, returns None, so calling function can return an error response.
+    """
+    if dropdown_value != "Other":
+        return dropdown_value.strip()
+    if not other_value or len(other_value.strip()) < 3:
+        return None
+    return other_value.strip()

packages/divbase-api/src/divbase_api/db.py

@@ -110,6 +110,9 @@ async def create_first_admin_user() -> None:
             name="First Admin",
             email=admin_email,
             password=admin_password,
+            confirm_password=admin_password,
+            organisation="DivBase University",
+            organisation_role="Developer",
         )
 
         admin_user = await create_user(

packages/divbase-api/src/divbase_api/exception_handlers.py

@@ -83,13 +83,21 @@ async def global_exception_handler(request: Request, exc: Exception):
     Handle unexpected exceptions globally. - in the ideal world this is never be triggered
     """
     logger.error(f"Unexpected Error occurred for: {request.method} {request.url.path}: {exc}", exc_info=True)
-    return JSONResponse(
-        status_code=500,
-        content={
-            "detail": "An unexpected error occurred. Please try again later.",
-            "type": "server_error",
-        },
-    )
+
+    if is_api_request(request):
+        return JSONResponse(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            content={
+                "detail": "An unexpected error occurred. Please try again later.",
+                "type": "server_error",
+            },
+        )
+    else:
+        return await render_error_page(
+            request,
+            "An unexpected error occurred. Please try again later.",
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+        )
 
 
 async def authentication_error_handler(request: Request, exc: AuthenticationError):

packages/divbase-api/src/divbase_api/frontend_routes/auth.py

@@ -10,6 +10,7 @@
 from sqlalchemy.ext.asyncio import AsyncSession
 
 from divbase_api.api_config import settings
+from divbase_api.api_constants import KNOWN_JOB_ROLES, SWEDISH_UNIVERSITIES
 from divbase_api.crud.auth import (
     authenticate_user,
     check_user_email_verified,
@@ -18,7 +19,7 @@
     update_user_password,
 )
 from divbase_api.crud.revoked_tokens import revoke_token_on_logout, revoke_used_password_reset_token, token_is_revoked
-from divbase_api.crud.users import create_user, get_user_by_email, get_user_by_id_or_raise
+from divbase_api.crud.users import create_user, get_user_by_email, get_user_by_id_or_raise, resolve_dropdown_form_input
 from divbase_api.db import get_db
 from divbase_api.deps import get_current_user_from_cookie_optional
 from divbase_api.exceptions import AuthenticationError
@@ -128,7 +129,15 @@ async def get_register(request: Request, current_user: UserDB | None = Depends(g
     """Render the registration page."""
     if current_user:
         return RedirectResponse(url="/", status_code=status.HTTP_302_FOUND)
-    return templates.TemplateResponse(request=request, name="auth_pages/register.html", context={"current_user": None})
+    return templates.TemplateResponse(
+        request=request,
+        name="auth_pages/register.html",
+        context={
+            "current_user": None,
+            "swedish_universities": SWEDISH_UNIVERSITIES,
+            "known_job_roles": KNOWN_JOB_ROLES,
+        },
+    )
 
 
 @fr_auth_router.post("/register", response_class=HTMLResponse)
@@ -137,6 +146,10 @@ async def post_register(
     background_tasks: BackgroundTasks,
     name: str = Form(...),
     email: str = Form(...),
+    organisation: str = Form(...),
+    organisation_other: str | None = Form(None),
+    role: str = Form(...),
+    role_other: str | None = Form(None),
     password: str = Form(...),
     confirm_password: str = Form(...),
     db: AsyncSession = Depends(get_db),
@@ -148,25 +161,56 @@ def registration_failed_response(error_message: str):
         return templates.TemplateResponse(
             request=request,
             name="auth_pages/register.html",
-            context={"error": error_message, "name": name, "email": email},
+            context={
+                "error": error_message,
+                "name": name,
+                "email": email,
+                "organisation": organisation,
+                "organisation_other": organisation_other,
+                "role": role,
+                "role_other": role_other,
+                "swedish_universities": SWEDISH_UNIVERSITIES,
+                "known_job_roles": KNOWN_JOB_ROLES,
+            },
         )
 
-    if password != confirm_password:
-        return registration_failed_response("Passwords do not match")
+    resolved_organisation = resolve_dropdown_form_input(dropdown_value=organisation, other_value=organisation_other)
+    if not resolved_organisation:
+        return registration_failed_response("Please specify your organisation, it must be at least 3 characters long.")
+
+    resolved_role = resolve_dropdown_form_input(dropdown_value=role, other_value=role_other)
+    if not resolved_role:
+        return registration_failed_response("Please specify your role, it must be at least 3 characters long.")
 
-    existing_user = await get_user_by_email(db=db, email=email)
+    existing_user = await get_user_by_email(db=db, email=email.strip())
     if existing_user:  # Not recommended to specify why failed, just say failed.
+        logger.warning(f"Attempt to register new account with already registered email: {email}")
         return registration_failed_response("Registration failed, please try again.")
 
     try:
-        user_data = UserCreate(name=name, email=email, password=SecretStr(password))
+        user_data = UserCreate(
+            name=name.strip(),
+            email=email.strip(),
+            organisation=resolved_organisation.strip(),
+            organisation_role=resolved_role.strip(),
+            confirm_password=SecretStr(confirm_password),
+            password=SecretStr(password),
+        )
         user = await create_user(db=db, user_data=user_data, is_admin=False)
+    except ValidationError as e:
+        error_msg = " ".join(err["msg"] for err in e.errors())
+        # We warn about this because the frontend should have prevented it.
+        # so if we get validation errors here it could be because:
+        # Some mismatch in backend vs frontend validation logic or
+        # someone bypassing client side validation (could be accidently or intentionally).
+        # Don't think it is a good idea to return the validation error messages to the user as it could leak info.
+        logger.warning(f"User registration failed backend validation for email: {email} - {error_msg}")
+        return registration_failed_response("Registration failed, please try again.")
     except Exception as e:
-        logger.error(f"Error creating user: {e}")
+        logger.error(f"Unexpected error during user registration for email: {email} - {str(e)}")
         return registration_failed_response("Registration failed, please try again.")
 
     background_tasks.add_task(send_verification_email, email_to=user.email, user_id=user.id)
-
     logger.info(f"New user registered: {user_data.email=}")
     return templates.TemplateResponse(
         request=request,