From dde68d9500fe36a5b331653fc2fe66dd04dfb4b6 Mon Sep 17 00:00:00 2001 From: AkanshuS Date: Wed, 20 Nov 2024 18:01:47 -0500 Subject: [PATCH 01/19] Update user.py Deletes the current_user using session.delete() --- routers/user.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/routers/user.py b/routers/user.py index df7d698..367b7da 100644 --- a/routers/user.py +++ b/routers/user.py @@ -77,4 +77,6 @@ async def delete_account( # Mark the user as deleted current_user.deleted = True session.commit() + #Deletes user + session.delete(current_user) return RedirectResponse(url="/", status_code=303) From 60468fea548bf65dfab232ce2a77fbd70c913612 Mon Sep 17 00:00:00 2001 From: AkanshuS Date: Wed, 20 Nov 2024 18:05:03 -0500 Subject: [PATCH 02/19] Update user.py --- routers/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/user.py b/routers/user.py index 367b7da..1904ec1 100644 --- a/routers/user.py +++ b/routers/user.py @@ -77,6 +77,6 @@ async def delete_account( # Mark the user as deleted current_user.deleted = True session.commit() - #Deletes user + # Deletes user session.delete(current_user) return RedirectResponse(url="/", status_code=303) From 8a628835991edb9606acff7971636a82ec3581d6 Mon Sep 17 00:00:00 2001 From: AkanshuS Date: Fri, 22 Nov 2024 16:38:31 -0500 Subject: [PATCH 03/19] Logs Out --- routers/user.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/routers/user.py b/routers/user.py index 1904ec1..86d3710 100644 --- a/routers/user.py +++ b/routers/user.py @@ -77,6 +77,8 @@ async def delete_account( # Mark the user as deleted current_user.deleted = True session.commit() + #Logs Out + router.get("/logout", response_class=RedirectResponse) # Deletes user session.delete(current_user) return RedirectResponse(url="/", status_code=303) From 9beee0e86931c85e2f59105f385bb4d202822fea Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Sun, 24 Nov 2024 02:05:55 +0000 Subject: [PATCH 04/19] Customization adjustments --- docs/customization.qmd | 285 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 285 insertions(+) diff --git a/docs/customization.qmd b/docs/customization.qmd index a3dfe2e..279bbaf 100644 --- a/docs/customization.qmd +++ b/docs/customization.qmd @@ -2,3 +2,288 @@ title: "Customization" --- +## Development workflow + +### Dependency management with Poetry + +The project uses Poetry to manage dependencies: + +- Add new dependency: `poetry add ` +- Add development dependency: `poetry add --dev ` +- Remove dependency: `poetry remove ` +- Update lock file: `poetry lock` +- Install dependencies: `poetry install` +- Update all dependencies: `poetry update` + +### Testing + +The project uses Pytest for unit testing. It's highly recommended to write and run tests before committing code to ensure nothing is broken! + +The following fixtures, defined in `tests/conftest.py`, are available in the test suite: + +- `engine`: Creates a new SQLModel engine for the test database. +- `set_up_database`: Sets up the test database before running the test suite by dropping all tables and recreating them to ensure a clean state. +- `session`: Provides a session for database operations in tests. +- `clean_db`: Cleans up the database tables before each test by deleting all entries in the `PasswordResetToken` and `User` tables. +- `client`: Provides a `TestClient` instance with the session fixture, overriding the `get_session` dependency to use the test session. +- `test_user`: Creates a test user in the database with a predefined name, email, and hashed password. + +To run the tests, use these commands: + +- Run all tests: `pytest` +- Run tests in debug mode (includes logs and print statements in console output): `pytest -s` +- Run particular test files by name: `pytest ` +- Run particular tests by name: `pytest -k ` + +### Type checking with mypy + +The project uses type annotations and mypy for static type checking. To run mypy, use this command from the root directory: + +```bash +mypy +``` + +We find that mypy is an enormous time-saver, catching many errors early and greatly reducing time spent debugging unit tests. However, note that mypy requires you type annotate every variable, function, and method in your code base, so taking advantage of it requires a lifestyle change! + +## Project structure + +### Customizable folders and files + +- FastAPI application entry point and GET routes: `main.py` +- FastAPI POST routes: `routers/` + - User authentication endpoints: `auth.py` + - User profile management endpoints: `user.py` + - Organization management endpoints: `organization.py` + - Role management endpoints: `role.py` +- Jinja2 templates: `templates/` +- Static assets: `static/` +- Unit tests: `tests/` +- Test database configuration: `docker-compose.yml` +- Helper functions: `utils/` + - Auth helpers: `auth.py` + - Database helpers: `db.py` + - Database models: `models.py` +- Environment variables: `.env` +- CI/CD configuration: `.github/` +- Project configuration: `pyproject.toml` +- Quarto documentation: + - Source: `index.qmd` + `docs/` + - Configuration: `_quarto.yml` + +Most everything else is auto-generated and should not be manually modified. + +### Defining a web backend with FastAPI + +We use FastAPI to define the "API endpoints" of our application. An API endpoint is simply a URL that accepts user requests and returns responses. When a user visits a page, their browser sends what's called a "GET" request to an endpoint, and the server processes it (often querying a database), and returns a response (typically HTML). The browser renders the HTML, displaying the page. + +We also create POST endpoints, which accept form submissions so the user can create, update, and delete data in the database. This template follows the Post-Redirect-Get (PRG) pattern to handle POST requests. When a form is submitted, the server processes the data and then returns a "redirect" response, which sends the user to a GET endpoint to re-render the page with the updated data. (See [Architecture](https://promptlytechnologies.com/fastapi-jinja2-postgres-webapp/docs/architecture.html) for more details.) + +#### Routing patterns in this template + +In this template, GET routes are defined in the main entry point for the application, `main.py`. POST routes are organized into separate modules within the `routers/` directory. + +We name our GET routes using the convention `read_`, where `` is the name of the page, to indicate that they are read-only endpoints that do not modify the database. + +We divide our GET routes into authenticated and unauthenticated routes, using commented section headers in our code that look like this: + +```python +# -- Authenticated Routes -- +``` + +Some of our routes take request parameters, which we pass as keyword arguments to the route handler. These parameters should be type annotated for validation purposes. + +Some parameters are shared across all authenticated or unauthenticated routes, so we define them in the `common_authenticated_parameters` and `common_unauthenticated_parameters` dependencies defined in `main.py`. + +### HTML templating with Jinja2 + +To generate the HTML pages to be returned from our GET routes, we use Jinja2 templates. Jinja2's hierarchical templates allow creating a base template (`templates/base.html`) that defines the overall layout of our web pages (e.g., where the header, body, and footer should go). Individual pages can then extend this base template. We can also template reusable components that can be injected into our layout or page templates. + +With Jinja2, we can use the `{% block %}` tag to define content blocks, and the `{% extends %}` tag to extend a base template. We can also use the `{% include %}` tag to include a component in a parent template. See the [Jinja2 documentation on template inheritance](https://jinja.palletsprojects.com/en/stable/templates/#template-inheritance) for more details. + +#### Context variables + +Context refers to Python variables passed to a template to populate the HTML. In a FastAPI GET route, we can pass context to a template using the `templates.TemplateResponse` method, which takes the request and any context data as arguments. For example: + +```python +@app.get("/welcome") +async def welcome(request: Request): + return templates.TemplateResponse( + "welcome.html", + {"username": "Alice"} + ) +``` + +In this example, the `welcome.html` template will receive two pieces of context: the user's `request`, which is always passed automatically by FastAPI, and a `username` variable, which we specify as "Alice". We can then use the `{{{ username }}}` syntax in the `welcome.html` template (or any of its parent or child templates) to insert the value into the HTML. + +#### Form validation strategy + +While this template includes comprehensive server-side validation through Pydantic models and custom validators, it's important to note that server-side validation should be treated as a fallback security measure. If users ever see the `validation_error.html` template, it indicates that our client-side validation has failed to catch invalid input before it reaches the server. + +Best practices dictate implementing thorough client-side validation via JavaScript and/or HTML `input` element `pattern` attributes to: +- Provide immediate feedback to users +- Reduce server load +- Improve user experience by avoiding round-trips to the server +- Prevent malformed data from ever reaching the backend + +Server-side validation remains essential as a security measure against malicious requests that bypass client-side validation, but it should rarely be encountered during normal user interaction. See `templates/authentication/register.html` for a client-side form validation example involving both JavaScript and HTML regex `pattern` matching. + +### Writing type annotated code + +Pydantic is used for data validation and serialization. It ensures that the data received in requests meets the expected format and constraints. Pydantic models are used to define the structure of request and response data, making it easy to validate and parse JSON payloads. + +If a user-submitted form contains data that has the wrong number, names, or types of fields, Pydantic will raise a `RequestValidationError`, which is caught by middleware and converted into an HTTP 422 error response. + +For other, custom validation logic, we add Pydantic `@field_validator` methods to our Pydantic request models and then add the models as dependencies in the signatures of corresponding POST routes. FastAPI's dependency injection system ensures that dependency logic is executed before the body of the route handler. + +#### Defining request models and custom validators + +For example, in the `UserRegister` request model in `routers/authentication.py`, we add a custom validation method to ensure that the `confirm_password` field matches the `password` field. If not, it raises a custom `PasswordMismatchError`: + +```python +class PasswordMismatchError(HTTPException): + def __init__(self, field: str = "confirm_password"): + super().__init__( + status_code=422, + detail={ + "field": field, + "message": "The passwords you entered do not match" + } + ) + +class UserRegister(BaseModel): + name: str + email: EmailStr + password: str + confirm_password: str + + # Custom validators are added as class attributes + @field_validator("confirm_password", check_fields=False) + def validate_passwords_match(cls, v: str, values: dict[str, Any]) -> str: + if v != values["password"]: + raise PasswordMismatchError() + return v + # ... +``` + +We then add this request model as a dependency in the signature of our POST route: + +```python +@app.post("/register") +async def register(request: UserRegister = Depends()): + # ... +``` + +When the user submits the form, Pydantic will first check that all expected fields are present and match the expected types. If not, it raises a `RequestValidationError`. Then, it runs our custom `field_validator`, `validate_passwords_match`. If it finds that the `confirm_password` field does not match the `password` field, it raises a `PasswordMismatchError`. These exceptions can then be caught and handled by our middleware. + +(Note that these examples are simplified versions of the actual code.) + +#### Converting form data to request models + +In addition to custom validation logic, we also need to define a method on our request models that converts form data into the request model. Here's what that looks like in the `UserRegister` request model from the previous example: + +```python +class UserRegister(BaseModel): + # ... + + @classmethod + async def as_form( + cls, + name: str = Form(...), + email: EmailStr = Form(...), + password: str = Form(...), + confirm_password: str = Form(...) + ): + return cls( + name=name, + email=email, + password=password, + confirm_password=confirm_password + ) +``` + +#### Middleware exception handling + +Middlewares—which process requests before they reach the route handlers and responses before they are sent back to the client—are defined in `main.py`. They are commonly used in web development for tasks such as error handling, authentication token validation, logging, and modifying request/response objects. + +This template uses middlewares exclusively for global exception handling; they only affect requests that raise an exception. This allows for consistent error responses and centralized error logging. Middleware can catch exceptions raised during request processing and return appropriate HTTP responses. + +Middleware functions are decorated with `@app.exception_handler(ExceptionType)` and are executed in the order they are defined in `main.py`, from most to least specific. + +Here's a middleware for handling the `PasswordMismatchError` exception from the previous example, which renders the `errors/validation_error.html` template with the error details: + +```python +@app.exception_handler(PasswordMismatchError) +async def password_mismatch_exception_handler(request: Request, exc: PasswordMismatchError): + return templates.TemplateResponse( + request, + "errors/validation_error.html", + { + "status_code": 422, + "errors": {"error": exc.detail} + }, + status_code=422, + ) +``` + +### Database configuration and access with SQLModel + +SQLModel is an Object-Relational Mapping (ORM) library that allows us to interact with our PostgreSQL database using Python classes instead of writing raw SQL. It combines the features of SQLAlchemy (a powerful database toolkit) with Pydantic's data validation. + +#### Models and relationships + +Our database models are defined in `utils/models.py`. Each model is a Python class that inherits from `SQLModel` and represents a database table. The key models are: + +- `Organization`: Represents a company or team +- `User`: Represents a user account +- `Role`: Represents a discrete set of user permissions within an organization +- `Permission`: Represents specific actions a user can perform +- `RolePermissionLink`: Maps roles to their allowed permissions +- `PasswordResetToken`: Manages password reset functionality + +Here's an entity-relationship diagram (ERD) of the current database schema, automatically generated from our SQLModel definitions: + +```{python} +#| echo: false +#| warning: false +import sys +sys.path.append("..") +from utils.models import * +from utils.db import engine +from sqlalchemy import MetaData +from sqlalchemy_schemadisplay import create_schema_graph + +# Create the directed graph +graph = create_schema_graph( + engine=engine, + metadata=SQLModel.metadata, + show_datatypes=True, + show_indexes=True, + rankdir='TB', + concentrate=False +) + +# Save the graph +graph.write_png('static/schema.png') +``` + +![Database Schema](static/schema.png) + + +#### Database operations + +Database operations are handled by helper functions in `utils/db.py`. Key functions include: + +- `set_up_db()`: Initializes the database schema and default data (which we do on every application start in `main.py`) +- `get_connection_url()`: Creates a database connection URL from environment variables in `.env` +- `get_session()`: Provides a database session for performing operations + +To perform database operations in route handlers, inject the database session as a dependency: + +```python +@app.get("/users") +async def get_users(session: Session = Depends(get_session)): + users = session.exec(select(User)).all() + return users +``` + +The session automatically handles transaction management, ensuring that database operations are atomic and consistent. From 4ea65097f929260a11822a95582f8581ba2e0a4c Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Sun, 24 Nov 2024 22:10:23 +0000 Subject: [PATCH 05/19] Don't return users with deleted column set to True --- routers/authentication.py | 26 +++++++-- routers/user.py | 47 +++++++++++----- templates/users/profile.html | 2 +- tests/test_authentication.py | 103 +++++++++++++++++++++++++++++++++++ utils/auth.py | 15 ++++- utils/models.py | 13 ++++- 6 files changed, 179 insertions(+), 27 deletions(-) diff --git a/routers/authentication.py b/routers/authentication.py index 0a1098b..5d75e1f 100644 --- a/routers/authentication.py +++ b/routers/authentication.py @@ -1,5 +1,5 @@ # auth.py -from logging import getLogger +from logging import getLogger, DEBUG from typing import Optional from datetime import datetime from fastapi import APIRouter, Depends, HTTPException, BackgroundTasks, Form @@ -22,6 +22,7 @@ ) logger = getLogger("uvicorn.error") +logger.setLevel(DEBUG) router = APIRouter(prefix="/auth", tags=["auth"]) @@ -126,7 +127,9 @@ async def register( session: Session = Depends(get_session), ) -> RedirectResponse: db_user = session.exec(select(User).where( - User.email == user.email)).first() + User.email == user.email, + User.deleted == False + )).first() if db_user: raise HTTPException(status_code=400, detail="Email already registered") @@ -156,7 +159,9 @@ async def login( session: Session = Depends(get_session), ) -> RedirectResponse: db_user = session.exec(select(User).where( - User.email == user.email)).first() + User.email == user.email, + User.deleted == False + )).first() if not db_user or not verify_password(user.password, db_user.hashed_password): raise HTTPException(status_code=400, detail="Invalid credentials") @@ -205,7 +210,9 @@ async def refresh_token( user_email = decoded_token.get("sub") db_user = session.exec(select(User).where( - User.email == user_email)).first() + User.email == user_email, + User.deleted == False + )).first() if not db_user: return RedirectResponse(url="/login", status_code=303) @@ -239,7 +246,9 @@ async def forgot_password( session: Session = Depends(get_session) ): db_user = session.exec(select(User).where( - User.email == user.email)).first() + User.email == user.email, + User.deleted == False + )).first() if db_user: background_tasks.add_task(send_reset_email, user.email, session) @@ -255,8 +264,13 @@ async def reset_password( authorized_user, reset_token = get_user_from_reset_token( user.email, user.token, session) - if not authorized_user or not reset_token: + logger.debug(f"authorized_user: {authorized_user}") + logger.debug(f"reset_token: {reset_token}") + + if not reset_token: raise HTTPException(status_code=400, detail="Invalid or expired token") + elif not authorized_user: + raise HTTPException(status_code=400, detail="User not found") # Update password and mark token as used authorized_user.hashed_password = get_password_hash(user.new_password) diff --git a/routers/user.py b/routers/user.py index 86d3710..4d360fa 100644 --- a/routers/user.py +++ b/routers/user.py @@ -37,6 +37,22 @@ async def as_form( return cls(confirm_delete_password=confirm_delete_password) +class UpdateProfile(BaseModel): + """Request model for updating user profile information""" + name: str + email: EmailStr + avatar_url: str + + @classmethod + async def as_form( + cls, + name: str = Form(...), + email: EmailStr = Form(...), + avatar_url: str = Form(...), + ): + return cls(name=name, email=email, avatar_url=avatar_url) + + # -- Routes -- @@ -48,18 +64,16 @@ async def view_profile( return {"user": current_user} -@router.post("/edit_profile", response_class=RedirectResponse) -async def edit_profile( - name: str = Form(...), - email: str = Form(...), - avatar_url: str = Form(...), +@router.post("/update_profile", response_class=RedirectResponse) +async def update_profile( + profile_update: UpdateProfile = Depends(UpdateProfile.as_form), current_user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): # Update user details - current_user.name = name - current_user.email = email - current_user.avatar_url = avatar_url + current_user.name = profile_update.name + current_user.email = profile_update.email + current_user.avatar_url = profile_update.avatar_url session.commit() session.refresh(current_user) return RedirectResponse(url="/profile", status_code=303) @@ -67,18 +81,21 @@ async def edit_profile( @router.post("/delete_account", response_class=RedirectResponse) async def delete_account( - confirm_delete_password: str = Form(...), + user_delete_account: UserDeleteAccount = Depends( + UserDeleteAccount.as_form), current_user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): - if not verify_password(confirm_delete_password, current_user.hashed_password): + if not verify_password(user_delete_account.confirm_delete_password, current_user.hashed_password): raise HTTPException(status_code=400, detail="Password is incorrect") # Mark the user as deleted current_user.deleted = True session.commit() - #Logs Out - router.get("/logout", response_class=RedirectResponse) - # Deletes user - session.delete(current_user) - return RedirectResponse(url="/", status_code=303) + + # Delete the user's access and refresh tokens to force a logout + response = RedirectResponse(url="/", status_code=303) + response.delete_cookie("access_token") + response.delete_cookie("refresh_token") + + return response diff --git a/templates/users/profile.html b/templates/users/profile.html index 5afa794..448c0d6 100644 --- a/templates/users/profile.html +++ b/templates/users/profile.html @@ -34,7 +34,7 @@

User Profile

Edit Profile
-
+
diff --git a/tests/test_authentication.py b/tests/test_authentication.py index eed349e..0e749fa 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -261,6 +261,10 @@ def test_logout_endpoint(client: TestClient): def test_register_with_existing_email(client: TestClient, test_user: User): + """Test that registration fails with an existing non-deleted user's email""" + # Ensure test user is not deleted + assert not test_user.deleted + response = client.post( "/auth/register", data={ @@ -273,6 +277,34 @@ def test_register_with_existing_email(client: TestClient, test_user: User): assert response.status_code == 400 +def test_register_with_deleted_user_email(client: TestClient, test_user: User, session: Session): + """Test that registration succeeds with a deleted user's email""" + # Mark test user as deleted + test_user.deleted = True + session.add(test_user) + session.commit() + + response = client.post( + "/auth/register", + data={ + "name": "New User", + "email": test_user.email, + "password": "Test123!@#", + "confirm_password": "Test123!@#" + }, + follow_redirects=False + ) + assert response.status_code == 303 + + # Verify new user was created + new_user = session.exec(select(User).where( + User.email == test_user.email, + User.deleted == False + )).first() + assert new_user is not None + assert new_user.id != test_user.id + + def test_login_with_invalid_credentials(client: TestClient, test_user: User): response = client.post( "/auth/login", @@ -361,3 +393,74 @@ def test_password_reset_email_url(client: TestClient, session: Session, test_use assert parsed.path == str(reset_password_path) assert query_params["email"][0] == test_user.email assert query_params["token"][0] == reset_token.token + + +def test_deleted_user_cannot_login(client: TestClient, test_user: User, session: Session): + """Test that a deleted user cannot log in""" + # First mark the user as deleted + test_user.deleted = True + session.add(test_user) + session.commit() + + response = client.post( + "/auth/login", + data={ + "email": test_user.email, + "password": "Test123!@#" + } + ) + assert response.status_code == 400 + + +def test_deleted_user_cannot_use_tokens(client: TestClient, test_user: User, session: Session): + """Test that a deleted user's tokens become invalid""" + # Create tokens before marking user as deleted + access_token = create_access_token({"sub": test_user.email}) + refresh_token = create_refresh_token({"sub": test_user.email}) + + # Mark user as deleted + test_user.deleted = True + session.add(test_user) + session.commit() + + # Set tokens in cookies + client.cookies.set("access_token", access_token) + client.cookies.set("refresh_token", refresh_token) + + # Try to refresh tokens + response = client.post("/auth/refresh", follow_redirects=False) + assert response.status_code == 303 # user is redirected to login + + +def test_deleted_user_cannot_use_reset_token(client: TestClient, session: Session, test_user: User): + """Test that a deleted user cannot use a previously issued reset token""" + # First create a reset token + response = client.post( + "/auth/forgot_password", + data={"email": test_user.email}, + follow_redirects=False + ) + assert response.status_code == 303 + + # Get the reset token + reset_token = session.exec(select(PasswordResetToken) + .where(PasswordResetToken.user_id == test_user.id)).first() + assert reset_token is not None + + # Now mark user as deleted + test_user.deleted = True + session.add(test_user) + session.commit() + + # Try to use the reset token + response = client.post( + "/auth/reset_password", + data={ + "email": test_user.email, + "token": reset_token.token, + "new_password": "NewPass123!@#", + "confirm_new_password": "NewPass123!@#" + }, + follow_redirects=False + ) + assert response.status_code == 400 diff --git a/utils/auth.py b/utils/auth.py index 3bf7dac..a5b956c 100644 --- a/utils/auth.py +++ b/utils/auth.py @@ -180,7 +180,9 @@ def validate_token_and_get_user( if decoded_token: user_email = decoded_token.get("sub") user = session.exec(select(User).where( - User.email == user_email)).first() + User.email == user_email, + User.deleted == False + )).first() if user: if token_type == "refresh": new_access_token = create_access_token( @@ -275,7 +277,10 @@ def generate_password_reset_url(email: str, token: str) -> str: def send_reset_email(email: str, session: Session): # Check for an existing unexpired token - user = session.exec(select(User).where(User.email == email)).first() + user = session.exec(select(User).where( + User.email == email, + User.deleted == False + )).first() if user: existing_token = session.exec( select(PasswordResetToken) @@ -327,7 +332,11 @@ def get_user_from_reset_token(email: str, token: str, session: Session) -> tuple user = session.exec(select(User).where( User.email == email, - User.id == reset_token.user_id + User.id == reset_token.user_id, + User.deleted == False )).first() + if not user: + return None, None + return user, reset_token diff --git a/utils/models.py b/utils/models.py index 5941f3f..902269b 100644 --- a/utils/models.py +++ b/utils/models.py @@ -3,7 +3,7 @@ from datetime import datetime, UTC, timedelta from typing import Optional, List from sqlmodel import SQLModel, Field, Relationship -from sqlalchemy import Column, Enum as SQLAlchemyEnum +from sqlalchemy import Column, Enum as SQLAlchemyEnum, Index def utc_time(): @@ -89,7 +89,7 @@ class PasswordResetToken(SQLModel, table=True): class User(SQLModel, table=True): id: Optional[int] = Field(default=None, primary_key=True) name: str - email: str = Field(index=True, unique=True) + email: str = Field(index=True) hashed_password: str avatar_url: Optional[str] = None organization_id: Optional[int] = Field( @@ -105,6 +105,15 @@ class User(SQLModel, table=True): password_reset_tokens: List["PasswordResetToken"] = Relationship( back_populates="user") + __table_args__ = ( + Index( + 'ix_user_email_unique_active', + 'email', + unique=True, + postgresql_where=(deleted == False) + ), + ) + class UserOrganizationLink(SQLModel, table=True): id: Optional[int] = Field(default=None, primary_key=True) From 30551b7ef435ec22247004a84d63856fd341b703 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Sun, 24 Nov 2024 22:21:29 +0000 Subject: [PATCH 06/19] Use a mock to send email in unit test --- tests/test_authentication.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 0e749fa..35f4144 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -432,7 +432,7 @@ def test_deleted_user_cannot_use_tokens(client: TestClient, test_user: User, ses assert response.status_code == 303 # user is redirected to login -def test_deleted_user_cannot_use_reset_token(client: TestClient, session: Session, test_user: User): +def test_deleted_user_cannot_use_reset_token(client: TestClient, session: Session, test_user: User, mock_resend_send): """Test that a deleted user cannot use a previously issued reset token""" # First create a reset token response = client.post( From e058660edd4757bba71de50be2b0d9503ee8ce6b Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Sun, 24 Nov 2024 22:33:45 +0000 Subject: [PATCH 07/19] Revert "Use a mock to send email in unit test" This reverts commit 30551b7ef435ec22247004a84d63856fd341b703. --- tests/test_authentication.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 35f4144..0e749fa 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -432,7 +432,7 @@ def test_deleted_user_cannot_use_tokens(client: TestClient, test_user: User, ses assert response.status_code == 303 # user is redirected to login -def test_deleted_user_cannot_use_reset_token(client: TestClient, session: Session, test_user: User, mock_resend_send): +def test_deleted_user_cannot_use_reset_token(client: TestClient, session: Session, test_user: User): """Test that a deleted user cannot use a previously issued reset token""" # First create a reset token response = client.post( From b603e734399a05006925aef93dc0bea891d26ad6 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Sun, 24 Nov 2024 22:33:46 +0000 Subject: [PATCH 08/19] Revert "Don't return users with deleted column set to True" This reverts commit 4ea65097f929260a11822a95582f8581ba2e0a4c. --- routers/authentication.py | 26 ++------- routers/user.py | 47 +++++----------- templates/users/profile.html | 2 +- tests/test_authentication.py | 103 ----------------------------------- utils/auth.py | 15 +---- utils/models.py | 13 +---- 6 files changed, 27 insertions(+), 179 deletions(-) diff --git a/routers/authentication.py b/routers/authentication.py index 5d75e1f..0a1098b 100644 --- a/routers/authentication.py +++ b/routers/authentication.py @@ -1,5 +1,5 @@ # auth.py -from logging import getLogger, DEBUG +from logging import getLogger from typing import Optional from datetime import datetime from fastapi import APIRouter, Depends, HTTPException, BackgroundTasks, Form @@ -22,7 +22,6 @@ ) logger = getLogger("uvicorn.error") -logger.setLevel(DEBUG) router = APIRouter(prefix="/auth", tags=["auth"]) @@ -127,9 +126,7 @@ async def register( session: Session = Depends(get_session), ) -> RedirectResponse: db_user = session.exec(select(User).where( - User.email == user.email, - User.deleted == False - )).first() + User.email == user.email)).first() if db_user: raise HTTPException(status_code=400, detail="Email already registered") @@ -159,9 +156,7 @@ async def login( session: Session = Depends(get_session), ) -> RedirectResponse: db_user = session.exec(select(User).where( - User.email == user.email, - User.deleted == False - )).first() + User.email == user.email)).first() if not db_user or not verify_password(user.password, db_user.hashed_password): raise HTTPException(status_code=400, detail="Invalid credentials") @@ -210,9 +205,7 @@ async def refresh_token( user_email = decoded_token.get("sub") db_user = session.exec(select(User).where( - User.email == user_email, - User.deleted == False - )).first() + User.email == user_email)).first() if not db_user: return RedirectResponse(url="/login", status_code=303) @@ -246,9 +239,7 @@ async def forgot_password( session: Session = Depends(get_session) ): db_user = session.exec(select(User).where( - User.email == user.email, - User.deleted == False - )).first() + User.email == user.email)).first() if db_user: background_tasks.add_task(send_reset_email, user.email, session) @@ -264,13 +255,8 @@ async def reset_password( authorized_user, reset_token = get_user_from_reset_token( user.email, user.token, session) - logger.debug(f"authorized_user: {authorized_user}") - logger.debug(f"reset_token: {reset_token}") - - if not reset_token: + if not authorized_user or not reset_token: raise HTTPException(status_code=400, detail="Invalid or expired token") - elif not authorized_user: - raise HTTPException(status_code=400, detail="User not found") # Update password and mark token as used authorized_user.hashed_password = get_password_hash(user.new_password) diff --git a/routers/user.py b/routers/user.py index 4d360fa..86d3710 100644 --- a/routers/user.py +++ b/routers/user.py @@ -37,22 +37,6 @@ async def as_form( return cls(confirm_delete_password=confirm_delete_password) -class UpdateProfile(BaseModel): - """Request model for updating user profile information""" - name: str - email: EmailStr - avatar_url: str - - @classmethod - async def as_form( - cls, - name: str = Form(...), - email: EmailStr = Form(...), - avatar_url: str = Form(...), - ): - return cls(name=name, email=email, avatar_url=avatar_url) - - # -- Routes -- @@ -64,16 +48,18 @@ async def view_profile( return {"user": current_user} -@router.post("/update_profile", response_class=RedirectResponse) -async def update_profile( - profile_update: UpdateProfile = Depends(UpdateProfile.as_form), +@router.post("/edit_profile", response_class=RedirectResponse) +async def edit_profile( + name: str = Form(...), + email: str = Form(...), + avatar_url: str = Form(...), current_user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): # Update user details - current_user.name = profile_update.name - current_user.email = profile_update.email - current_user.avatar_url = profile_update.avatar_url + current_user.name = name + current_user.email = email + current_user.avatar_url = avatar_url session.commit() session.refresh(current_user) return RedirectResponse(url="/profile", status_code=303) @@ -81,21 +67,18 @@ async def update_profile( @router.post("/delete_account", response_class=RedirectResponse) async def delete_account( - user_delete_account: UserDeleteAccount = Depends( - UserDeleteAccount.as_form), + confirm_delete_password: str = Form(...), current_user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): - if not verify_password(user_delete_account.confirm_delete_password, current_user.hashed_password): + if not verify_password(confirm_delete_password, current_user.hashed_password): raise HTTPException(status_code=400, detail="Password is incorrect") # Mark the user as deleted current_user.deleted = True session.commit() - - # Delete the user's access and refresh tokens to force a logout - response = RedirectResponse(url="/", status_code=303) - response.delete_cookie("access_token") - response.delete_cookie("refresh_token") - - return response + #Logs Out + router.get("/logout", response_class=RedirectResponse) + # Deletes user + session.delete(current_user) + return RedirectResponse(url="/", status_code=303) diff --git a/templates/users/profile.html b/templates/users/profile.html index 448c0d6..5afa794 100644 --- a/templates/users/profile.html +++ b/templates/users/profile.html @@ -34,7 +34,7 @@

User Profile

Edit Profile
- +
diff --git a/tests/test_authentication.py b/tests/test_authentication.py index 0e749fa..eed349e 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -261,10 +261,6 @@ def test_logout_endpoint(client: TestClient): def test_register_with_existing_email(client: TestClient, test_user: User): - """Test that registration fails with an existing non-deleted user's email""" - # Ensure test user is not deleted - assert not test_user.deleted - response = client.post( "/auth/register", data={ @@ -277,34 +273,6 @@ def test_register_with_existing_email(client: TestClient, test_user: User): assert response.status_code == 400 -def test_register_with_deleted_user_email(client: TestClient, test_user: User, session: Session): - """Test that registration succeeds with a deleted user's email""" - # Mark test user as deleted - test_user.deleted = True - session.add(test_user) - session.commit() - - response = client.post( - "/auth/register", - data={ - "name": "New User", - "email": test_user.email, - "password": "Test123!@#", - "confirm_password": "Test123!@#" - }, - follow_redirects=False - ) - assert response.status_code == 303 - - # Verify new user was created - new_user = session.exec(select(User).where( - User.email == test_user.email, - User.deleted == False - )).first() - assert new_user is not None - assert new_user.id != test_user.id - - def test_login_with_invalid_credentials(client: TestClient, test_user: User): response = client.post( "/auth/login", @@ -393,74 +361,3 @@ def test_password_reset_email_url(client: TestClient, session: Session, test_use assert parsed.path == str(reset_password_path) assert query_params["email"][0] == test_user.email assert query_params["token"][0] == reset_token.token - - -def test_deleted_user_cannot_login(client: TestClient, test_user: User, session: Session): - """Test that a deleted user cannot log in""" - # First mark the user as deleted - test_user.deleted = True - session.add(test_user) - session.commit() - - response = client.post( - "/auth/login", - data={ - "email": test_user.email, - "password": "Test123!@#" - } - ) - assert response.status_code == 400 - - -def test_deleted_user_cannot_use_tokens(client: TestClient, test_user: User, session: Session): - """Test that a deleted user's tokens become invalid""" - # Create tokens before marking user as deleted - access_token = create_access_token({"sub": test_user.email}) - refresh_token = create_refresh_token({"sub": test_user.email}) - - # Mark user as deleted - test_user.deleted = True - session.add(test_user) - session.commit() - - # Set tokens in cookies - client.cookies.set("access_token", access_token) - client.cookies.set("refresh_token", refresh_token) - - # Try to refresh tokens - response = client.post("/auth/refresh", follow_redirects=False) - assert response.status_code == 303 # user is redirected to login - - -def test_deleted_user_cannot_use_reset_token(client: TestClient, session: Session, test_user: User): - """Test that a deleted user cannot use a previously issued reset token""" - # First create a reset token - response = client.post( - "/auth/forgot_password", - data={"email": test_user.email}, - follow_redirects=False - ) - assert response.status_code == 303 - - # Get the reset token - reset_token = session.exec(select(PasswordResetToken) - .where(PasswordResetToken.user_id == test_user.id)).first() - assert reset_token is not None - - # Now mark user as deleted - test_user.deleted = True - session.add(test_user) - session.commit() - - # Try to use the reset token - response = client.post( - "/auth/reset_password", - data={ - "email": test_user.email, - "token": reset_token.token, - "new_password": "NewPass123!@#", - "confirm_new_password": "NewPass123!@#" - }, - follow_redirects=False - ) - assert response.status_code == 400 diff --git a/utils/auth.py b/utils/auth.py index a5b956c..3bf7dac 100644 --- a/utils/auth.py +++ b/utils/auth.py @@ -180,9 +180,7 @@ def validate_token_and_get_user( if decoded_token: user_email = decoded_token.get("sub") user = session.exec(select(User).where( - User.email == user_email, - User.deleted == False - )).first() + User.email == user_email)).first() if user: if token_type == "refresh": new_access_token = create_access_token( @@ -277,10 +275,7 @@ def generate_password_reset_url(email: str, token: str) -> str: def send_reset_email(email: str, session: Session): # Check for an existing unexpired token - user = session.exec(select(User).where( - User.email == email, - User.deleted == False - )).first() + user = session.exec(select(User).where(User.email == email)).first() if user: existing_token = session.exec( select(PasswordResetToken) @@ -332,11 +327,7 @@ def get_user_from_reset_token(email: str, token: str, session: Session) -> tuple user = session.exec(select(User).where( User.email == email, - User.id == reset_token.user_id, - User.deleted == False + User.id == reset_token.user_id )).first() - if not user: - return None, None - return user, reset_token diff --git a/utils/models.py b/utils/models.py index 902269b..5941f3f 100644 --- a/utils/models.py +++ b/utils/models.py @@ -3,7 +3,7 @@ from datetime import datetime, UTC, timedelta from typing import Optional, List from sqlmodel import SQLModel, Field, Relationship -from sqlalchemy import Column, Enum as SQLAlchemyEnum, Index +from sqlalchemy import Column, Enum as SQLAlchemyEnum def utc_time(): @@ -89,7 +89,7 @@ class PasswordResetToken(SQLModel, table=True): class User(SQLModel, table=True): id: Optional[int] = Field(default=None, primary_key=True) name: str - email: str = Field(index=True) + email: str = Field(index=True, unique=True) hashed_password: str avatar_url: Optional[str] = None organization_id: Optional[int] = Field( @@ -105,15 +105,6 @@ class User(SQLModel, table=True): password_reset_tokens: List["PasswordResetToken"] = Relationship( back_populates="user") - __table_args__ = ( - Index( - 'ix_user_email_unique_active', - 'email', - unique=True, - postgresql_where=(deleted == False) - ), - ) - class UserOrganizationLink(SQLModel, table=True): id: Optional[int] = Field(default=None, primary_key=True) From fbcb5504ab2541efc034652ab6fc0214e4237936 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Sun, 24 Nov 2024 22:46:49 +0000 Subject: [PATCH 09/19] Remove deleted attributes in the database models and actually delete the records instead --- routers/authentication.py | 1 - routers/organization.py | 5 +---- routers/role.py | 10 +++------- routers/user.py | 12 +++++------- tests/test_user.py | 6 ++++++ utils/models.py | 12 ++++++------ 6 files changed, 21 insertions(+), 25 deletions(-) create mode 100644 tests/test_user.py diff --git a/routers/authentication.py b/routers/authentication.py index 0a1098b..d487575 100644 --- a/routers/authentication.py +++ b/routers/authentication.py @@ -114,7 +114,6 @@ class UserRead(BaseModel): organization_id: Optional[int] created_at: datetime updated_at: datetime - deleted: bool # -- Routes -- diff --git a/routers/organization.py b/routers/organization.py index 9d4f1de..ea48a94 100644 --- a/routers/organization.py +++ b/routers/organization.py @@ -27,7 +27,6 @@ class OrganizationRead(BaseModel): name: str created_at: datetime updated_at: datetime - deleted: bool class OrganizationUpdate(BaseModel): @@ -113,9 +112,7 @@ def delete_organization( if not db_org: raise HTTPException(status_code=404, detail="Organization not found") - db_org.deleted = True - db_org.updated_at = datetime.utcnow() - session.add(db_org) + session.delete(db_org) session.commit() return RedirectResponse(url="/organizations", status_code=303) diff --git a/routers/role.py b/routers/role.py index cb2488f..a6429c4 100644 --- a/routers/role.py +++ b/routers/role.py @@ -31,7 +31,6 @@ class RoleRead(BaseModel): name: str created_at: datetime updated_at: datetime - deleted: bool permissions: List[ValidPermissions] @@ -74,7 +73,7 @@ def create_role( @router.get("/{role_id}", response_model=RoleRead) def read_role(role_id: int, session: Session = Depends(get_session)): db_role: Role | None = session.get(Role, role_id) - if not db_role or not db_role.id or db_role.deleted: + if not db_role or not db_role.id: raise HTTPException(status_code=404, detail="Role not found") permissions = [ @@ -88,7 +87,6 @@ def read_role(role_id: int, session: Session = Depends(get_session)): name=db_role.name, created_at=db_role.created_at, updated_at=db_role.updated_at, - deleted=db_role.deleted, permissions=permissions ) @@ -99,7 +97,7 @@ def update_role( session: Session = Depends(get_session) ) -> RedirectResponse: db_role: Role | None = session.get(Role, role.id) - if not db_role or not db_role.id or db_role.deleted: + if not db_role or not db_role.id: raise HTTPException(status_code=404, detail="Role not found") role_data = role.model_dump(exclude_unset=True) for key, value in role_data.items(): @@ -131,8 +129,6 @@ def delete_role( db_role = session.get(Role, role_id) if not db_role: raise HTTPException(status_code=404, detail="Role not found") - db_role.deleted = True - db_role.updated_at = utc_time() - session.add(db_role) + session.delete(db_role) session.commit() return RedirectResponse(url="/roles", status_code=303) diff --git a/routers/user.py b/routers/user.py index 86d3710..3e1ab4e 100644 --- a/routers/user.py +++ b/routers/user.py @@ -74,11 +74,9 @@ async def delete_account( if not verify_password(confirm_delete_password, current_user.hashed_password): raise HTTPException(status_code=400, detail="Password is incorrect") - # Mark the user as deleted - current_user.deleted = True - session.commit() - #Logs Out - router.get("/logout", response_class=RedirectResponse) - # Deletes user + # Delete the user session.delete(current_user) - return RedirectResponse(url="/", status_code=303) + session.commit() + + # Log out the user + return RedirectResponse(url="/logout", status_code=303) diff --git a/tests/test_user.py b/tests/test_user.py new file mode 100644 index 0000000..26c4dba --- /dev/null +++ b/tests/test_user.py @@ -0,0 +1,6 @@ +import pytest +from fastapi.testclient import TestClient +from sqlmodel import Session, select + +from main import app +from utils.models import User diff --git a/utils/models.py b/utils/models.py index 5941f3f..d487395 100644 --- a/utils/models.py +++ b/utils/models.py @@ -29,7 +29,6 @@ class Organization(SQLModel, table=True): name: str created_at: datetime = Field(default_factory=utc_time) updated_at: datetime = Field(default_factory=utc_time) - deleted: bool = Field(default=False) users: List["User"] = Relationship(back_populates="organization") @@ -41,7 +40,6 @@ class Role(SQLModel, table=True): default=None, foreign_key="organization.id") created_at: datetime = Field(default_factory=utc_time) updated_at: datetime = Field(default_factory=utc_time) - deleted: bool = Field(default=False) users: List["User"] = Relationship(back_populates="role") role_permission_links: List["RolePermissionLink"] = Relationship( @@ -54,7 +52,6 @@ class Permission(SQLModel, table=True): sa_column=Column(SQLAlchemyEnum(ValidPermissions, create_type=False))) created_at: datetime = Field(default_factory=utc_time) updated_at: datetime = Field(default_factory=utc_time) - deleted: bool = Field(default=False) role_permission_links: List["RolePermissionLink"] = Relationship( back_populates="permission") @@ -83,7 +80,9 @@ class PasswordResetToken(SQLModel, table=True): used: bool = Field(default=False) user: Optional["User"] = Relationship( - back_populates="password_reset_tokens") + back_populates="password_reset_tokens", + sa_relationship_kwargs={"cascade": "all, delete-orphan"} + ) class User(SQLModel, table=True): @@ -97,13 +96,14 @@ class User(SQLModel, table=True): role_id: Optional[int] = Field(default=None, foreign_key="role.id") created_at: datetime = Field(default_factory=utc_time) updated_at: datetime = Field(default_factory=utc_time) - deleted: bool = Field(default=False) organization: Optional["Organization"] = Relationship( back_populates="users") role: Optional["Role"] = Relationship(back_populates="users") password_reset_tokens: List["PasswordResetToken"] = Relationship( - back_populates="user") + back_populates="user", + sa_relationship_kwargs={"cascade": "all, delete-orphan"} + ) class UserOrganizationLink(SQLModel, table=True): From 9d8c5b3b85d879f4147a24af67ba2191a58a3822 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 00:54:32 +0000 Subject: [PATCH 10/19] Added tests for a couple read routes in main.py --- tests/test_main.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 tests/test_main.py diff --git a/tests/test_main.py b/tests/test_main.py new file mode 100644 index 0000000..a8dd554 --- /dev/null +++ b/tests/test_main.py @@ -0,0 +1,21 @@ +from fastapi.testclient import TestClient + +from utils.models import User +from main import app + + +def test_read_profile_unauthorized(unauth_client: TestClient): + """Test that unauthorized users cannot view profile""" + response = unauth_client.get(app.url_path_for( + "read_profile"), follow_redirects=False) + assert response.status_code == 303 # Redirect to login + assert response.headers["location"] == app.url_path_for("read_login") + + +def test_read_profile_authorized(auth_client: TestClient, test_user: User): + """Test that authorized users can view their profile""" + response = auth_client.get(app.url_path_for("read_profile")) + assert response.status_code == 200 + # Check that the response contains the expected HTML content + assert test_user.email in response.text + assert test_user.name in response.text From 4ed648addcd2fb7a94e5475b4e28aa7156e0b417 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 00:55:46 +0000 Subject: [PATCH 11/19] Added request models for user profile update --- routers/user.py | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/routers/user.py b/routers/user.py index 3e1ab4e..043509d 100644 --- a/routers/user.py +++ b/routers/user.py @@ -11,7 +11,8 @@ # -- Server Request and Response Models -- -class UserProfile(BaseModel): +class UpdateProfile(BaseModel): + """Request model for updating user profile information""" name: str email: EmailStr avatar_url: str @@ -40,26 +41,16 @@ async def as_form( # -- Routes -- -@router.get("/profile", response_class=RedirectResponse) -async def view_profile( - current_user: User = Depends(get_authenticated_user) -): - # Render the profile page with the current user's data - return {"user": current_user} - - -@router.post("/edit_profile", response_class=RedirectResponse) -async def edit_profile( - name: str = Form(...), - email: str = Form(...), - avatar_url: str = Form(...), +@router.post("/update_profile", response_class=RedirectResponse) +async def update_profile( + user_profile: UpdateProfile = Depends(UpdateProfile.as_form), current_user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): # Update user details - current_user.name = name - current_user.email = email - current_user.avatar_url = avatar_url + current_user.name = user_profile.name + current_user.email = user_profile.email + current_user.avatar_url = user_profile.avatar_url session.commit() session.refresh(current_user) return RedirectResponse(url="/profile", status_code=303) @@ -67,16 +58,23 @@ async def edit_profile( @router.post("/delete_account", response_class=RedirectResponse) async def delete_account( - confirm_delete_password: str = Form(...), + user_delete_account: UserDeleteAccount = Depends( + UserDeleteAccount.as_form), current_user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): - if not verify_password(confirm_delete_password, current_user.hashed_password): - raise HTTPException(status_code=400, detail="Password is incorrect") + if not verify_password( + user_delete_account.confirm_delete_password, + current_user.hashed_password + ): + raise HTTPException( + status_code=400, + detail="Password is incorrect" + ) # Delete the user session.delete(current_user) session.commit() # Log out the user - return RedirectResponse(url="/logout", status_code=303) + return RedirectResponse(url="/auth/logout", status_code=303) From 7ea6d5127e30ba630f22c5178d907c7324533c27 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 00:56:20 +0000 Subject: [PATCH 12/19] Fixed a SQLAlchemy model problem where cascade was going the wrong way --- utils/models.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/utils/models.py b/utils/models.py index d487395..cd43f34 100644 --- a/utils/models.py +++ b/utils/models.py @@ -80,9 +80,7 @@ class PasswordResetToken(SQLModel, table=True): used: bool = Field(default=False) user: Optional["User"] = Relationship( - back_populates="password_reset_tokens", - sa_relationship_kwargs={"cascade": "all, delete-orphan"} - ) + back_populates="password_reset_tokens") class User(SQLModel, table=True): From 3cc67dd3f8ef67e47e6e6711d9186481fac89c6b Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 00:57:23 +0000 Subject: [PATCH 13/19] Redirect unauthed users with 303 rather than 307 so POST requests are changed to GET --- utils/auth.py | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/utils/auth.py b/utils/auth.py index 3bf7dac..8fbae0b 100644 --- a/utils/auth.py +++ b/utils/auth.py @@ -12,6 +12,7 @@ from datetime import UTC, datetime, timedelta from typing import Optional from fastapi import Depends, Cookie, HTTPException, status +from fastapi.responses import RedirectResponse from utils.db import get_session from utils.models import User, PasswordResetToken @@ -180,7 +181,8 @@ def validate_token_and_get_user( if decoded_token: user_email = decoded_token.get("sub") user = session.exec(select(User).where( - User.email == user_email)).first() + User.email == user_email + )).first() if user: if token_type == "refresh": new_access_token = create_access_token( @@ -215,6 +217,14 @@ def get_user_from_tokens( return None, None, None +class AuthenticationError(HTTPException): + def __init__(self): + super().__init__( + status_code=status.HTTP_303_SEE_OTHER, + headers={"Location": "/login"} + ) + + def get_authenticated_user( tokens: tuple[Optional[str], Optional[str] ] = Depends(oauth2_scheme_cookie), @@ -228,11 +238,7 @@ def get_authenticated_user( raise NeedsNewTokens(user, new_access_token, new_refresh_token) return user - # If both tokens are invalid or missing, redirect to login - raise HTTPException( - status_code=status.HTTP_307_TEMPORARY_REDIRECT, - headers={"Location": "/login"} - ) + raise AuthenticationError() def get_optional_user( @@ -275,7 +281,9 @@ def generate_password_reset_url(email: str, token: str) -> str: def send_reset_email(email: str, session: Session): # Check for an existing unexpired token - user = session.exec(select(User).where(User.email == email)).first() + user = session.exec(select(User).where( + User.email == email + )).first() if user: existing_token = session.exec( select(PasswordResetToken) @@ -316,18 +324,19 @@ def send_reset_email(email: str, session: Session): def get_user_from_reset_token(email: str, token: str, session: Session) -> tuple[Optional[User], Optional[PasswordResetToken]]: - reset_token = session.exec(select(PasswordResetToken).where( - PasswordResetToken.token == token, - PasswordResetToken.expires_at > datetime.now(UTC), - PasswordResetToken.used == False - )).first() + result = session.exec( + select(User, PasswordResetToken) + .where( + User.email == email, + PasswordResetToken.token == token, + PasswordResetToken.expires_at > datetime.now(UTC), + PasswordResetToken.used == False, + PasswordResetToken.user_id == User.id + ) + ).first() - if not reset_token: + if not result: return None, None - user = session.exec(select(User).where( - User.email == email, - User.id == reset_token.user_id - )).first() - + user, reset_token = result return user, reset_token From bf63caf9f96a9af17d28996c97204ec203b252b1 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 00:58:39 +0000 Subject: [PATCH 14/19] New authentication error handler in main.py --- main.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/main.py b/main.py index 676808e..5642f6a 100644 --- a/main.py +++ b/main.py @@ -8,7 +8,7 @@ from fastapi.exceptions import RequestValidationError, HTTPException, StarletteHTTPException from sqlmodel import Session from routers import authentication, organization, role, user -from utils.auth import get_authenticated_user, get_optional_user, NeedsNewTokens, get_user_from_reset_token, PasswordValidationError +from utils.auth import get_authenticated_user, get_optional_user, NeedsNewTokens, get_user_from_reset_token, PasswordValidationError, AuthenticationError from utils.models import User from utils.db import get_session, set_up_db @@ -37,6 +37,15 @@ async def lifespan(app: FastAPI): # -- Exception Handling Middlewares -- +# Handle AuthenticationError by redirecting to login page +@app.exception_handler(AuthenticationError) +async def authentication_error_handler(request: Request, exc: AuthenticationError): + return RedirectResponse( + url="/login", + status_code=status.HTTP_303_SEE_OTHER + ) + + # Handle NeedsNewTokens by setting new tokens and redirecting to same page @app.exception_handler(NeedsNewTokens) async def needs_new_tokens_handler(request: Request, exc: NeedsNewTokens): @@ -104,10 +113,6 @@ async def validation_exception_handler(request: Request, exc: RequestValidationE # Handle StarletteHTTPException (including 404, 405, etc.) by rendering the error page @app.exception_handler(StarletteHTTPException) async def http_exception_handler(request: Request, exc: StarletteHTTPException): - # Don't handle redirects - if exc.status_code in [301, 302, 303, 307, 308]: - raise exc - return templates.TemplateResponse( request, "errors/error.html", From 2fe1570d2abd7ee8e8cc6bd4320a3482fc1af364 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 00:59:04 +0000 Subject: [PATCH 15/19] Fix misnamed endpoint in profile.html template --- templates/users/profile.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/templates/users/profile.html b/templates/users/profile.html index 5afa794..448c0d6 100644 --- a/templates/users/profile.html +++ b/templates/users/profile.html @@ -34,7 +34,7 @@

User Profile

Edit Profile
- +
From a6fd524825cfe09db5e4a50d4223b197160ca328 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 00:59:37 +0000 Subject: [PATCH 16/19] New test fixtures for authed and unauthed clients, tests for user endpoints --- tests/conftest.py | 56 +++++++++++++++++-------- tests/test_authentication.py | 75 ++++++++++++++++----------------- tests/test_user.py | 81 +++++++++++++++++++++++++++++++++++- 3 files changed, 155 insertions(+), 57 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index da1eec2..f9b90ae 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,7 +4,7 @@ from fastapi.testclient import TestClient from utils.db import get_connection_url, set_up_db, tear_down_db, get_session from utils.models import User, PasswordResetToken -from utils.auth import get_password_hash +from utils.auth import get_password_hash, create_access_token, create_refresh_token from main import app load_dotenv() @@ -54,22 +54,6 @@ def clean_db(session: Session): session.commit() -# Test client fixture -@pytest.fixture() -def client(session: Session): - """ - Provides a TestClient instance with the session fixture. - Overrides the get_session dependency to use the test session. - """ - def get_session_override(): - return session - - app.dependency_overrides[get_session] = get_session_override - client = TestClient(app) - yield client - app.dependency_overrides.clear() - - # Test user fixture @pytest.fixture() def test_user(session: Session): @@ -85,3 +69,41 @@ def test_user(session: Session): session.commit() session.refresh(user) return user + + +# Unauthenticated client fixture +@pytest.fixture() +def unauth_client(session: Session): + """ + Provides a TestClient instance without authentication. + """ + def get_session_override(): + return session + + app.dependency_overrides[get_session] = get_session_override + client = TestClient(app) + yield client + app.dependency_overrides.clear() + + +# Authenticated client fixture +@pytest.fixture() +def auth_client(session: Session, test_user: User): + """ + Provides a TestClient instance with valid authentication tokens. + """ + def get_session_override(): + return session + + app.dependency_overrides[get_session] = get_session_override + client = TestClient(app) + + # Create and set valid tokens + access_token = create_access_token({"sub": test_user.email}) + refresh_token = create_refresh_token({"sub": test_user.email}) + + client.cookies.set("access_token", access_token) + client.cookies.set("refresh_token", refresh_token) + + yield client + app.dependency_overrides.clear() diff --git a/tests/test_authentication.py b/tests/test_authentication.py index ac0df9e..0ba2331 100644 --- a/tests/test_authentication.py +++ b/tests/test_authentication.py @@ -86,9 +86,9 @@ def test_invalid_token_type(): # --- API Endpoint Tests --- -def test_register_endpoint(client: TestClient, session: Session): - response = client.post( - "/auth/register", +def test_register_endpoint(unauth_client: TestClient, session: Session): + response = unauth_client.post( + app.url_path_for("register"), data={ "name": "New User", "email": "new@example.com", @@ -107,9 +107,9 @@ def test_register_endpoint(client: TestClient, session: Session): assert verify_password("NewPass123!@#", user.hashed_password) -def test_login_endpoint(client: TestClient, test_user: User): - response = client.post( - "/auth/login", +def test_login_endpoint(unauth_client: TestClient, test_user: User): + response = unauth_client.post( + app.url_path_for("login"), data={ "email": test_user.email, "password": "Test123!@#" @@ -124,18 +124,18 @@ def test_login_endpoint(client: TestClient, test_user: User): assert "refresh_token" in cookies -def test_refresh_token_endpoint(client: TestClient, test_user: User): - # Create expired access token and valid refresh token - access_token = create_access_token( +def test_refresh_token_endpoint(auth_client: TestClient, test_user: User): + # Override just the access token to be expired, keeping the valid refresh token + expired_access_token = create_access_token( {"sub": test_user.email}, timedelta(minutes=-10) ) - refresh_token = create_refresh_token({"sub": test_user.email}) + auth_client.cookies.set("access_token", expired_access_token) - client.cookies.set("access_token", access_token) - client.cookies.set("refresh_token", refresh_token) - - response = client.post("/auth/refresh", follow_redirects=False) + response = auth_client.post( + app.url_path_for("refresh_token"), + follow_redirects=False + ) assert response.status_code == 303 # Check for new tokens in headers @@ -155,10 +155,10 @@ def test_refresh_token_endpoint(client: TestClient, test_user: User): assert decoded["sub"] == test_user.email -def test_password_reset_flow(client: TestClient, session: Session, test_user: User, mock_resend_send): +def test_password_reset_flow(unauth_client: TestClient, session: Session, test_user: User, mock_resend_send): # Test forgot password request - response = client.post( - "/auth/forgot_password", + response = unauth_client.post( + app.url_path_for("forgot_password"), data={"email": test_user.email}, follow_redirects=False ) @@ -188,8 +188,8 @@ def test_password_reset_flow(client: TestClient, session: Session, test_user: Us assert not reset_token.used # Test password reset - response = client.post( - "/auth/reset_password", + response = unauth_client.post( + app.url_path_for("reset_password"), data={ "email": test_user.email, "token": reset_token.token, @@ -207,12 +207,11 @@ def test_password_reset_flow(client: TestClient, session: Session, test_user: Us assert reset_token.used -def test_logout_endpoint(client: TestClient): - # First set some cookies - client.cookies.set("access_token", "some_access_token") - client.cookies.set("refresh_token", "some_refresh_token") - - response = client.get("/auth/logout", follow_redirects=False) +def test_logout_endpoint(auth_client: TestClient): + response = auth_client.get( + app.url_path_for("logout"), + follow_redirects=False + ) assert response.status_code == 303 # Check for cookie deletion in headers @@ -226,9 +225,9 @@ def test_logout_endpoint(client: TestClient): # --- Error Case Tests --- -def test_register_with_existing_email(client: TestClient, test_user: User): - response = client.post( - "/auth/register", +def test_register_with_existing_email(unauth_client: TestClient, test_user: User): + response = unauth_client.post( + app.url_path_for("register"), data={ "name": "Another User", "email": test_user.email, @@ -239,9 +238,9 @@ def test_register_with_existing_email(client: TestClient, test_user: User): assert response.status_code == 400 -def test_login_with_invalid_credentials(client: TestClient, test_user: User): - response = client.post( - "/auth/login", +def test_login_with_invalid_credentials(unauth_client: TestClient, test_user: User): + response = unauth_client.post( + app.url_path_for("login"), data={ "email": test_user.email, "password": "WrongPass123!@#" @@ -250,9 +249,9 @@ def test_login_with_invalid_credentials(client: TestClient, test_user: User): assert response.status_code == 400 -def test_password_reset_with_invalid_token(client: TestClient, test_user: User): - response = client.post( - "/auth/reset_password", +def test_password_reset_with_invalid_token(unauth_client: TestClient, test_user: User): + response = unauth_client.post( + app.url_path_for("reset_password"), data={ "email": test_user.email, "token": "invalid_token", @@ -263,7 +262,7 @@ def test_password_reset_with_invalid_token(client: TestClient, test_user: User): assert response.status_code == 400 -def test_password_reset_url_generation(client: TestClient): +def test_password_reset_url_generation(unauth_client: TestClient): """ Tests that the password reset URL is correctly formatted and contains the required query parameters. @@ -290,12 +289,12 @@ def test_password_reset_url_generation(client: TestClient): assert query_params["token"][0] == test_token -def test_password_reset_email_url(client: TestClient, session: Session, test_user: User, mock_resend_send): +def test_password_reset_email_url(unauth_client: TestClient, session: Session, test_user: User, mock_resend_send): """ Tests that the password reset email contains a properly formatted reset URL. """ - response = client.post( - "/auth/forgot_password", + response = unauth_client.post( + app.url_path_for("forgot_password"), data={"email": test_user.email}, follow_redirects=False ) diff --git a/tests/test_user.py b/tests/test_user.py index 26c4dba..294b9eb 100644 --- a/tests/test_user.py +++ b/tests/test_user.py @@ -1,6 +1,83 @@ -import pytest from fastapi.testclient import TestClient -from sqlmodel import Session, select +from fastapi.responses import Response +from sqlmodel import Session from main import app from utils.models import User + + +def test_update_profile_unauthorized(unauth_client: TestClient): + """Test that unauthorized users cannot edit profile""" + response: Response = unauth_client.post( + app.url_path_for("update_profile"), + data={ + "name": "New Name", + "email": "new@example.com", + "avatar_url": "https://example.com/avatar.jpg" + }, + follow_redirects=False + ) + assert response.status_code == 303 # Redirect to login + assert response.headers["location"] == app.url_path_for("read_login") + + +def test_update_profile_authorized(auth_client: TestClient, test_user: User, session: Session): + """Test that authorized users can edit their profile""" + + # Update profile + response: Response = auth_client.post( + app.url_path_for("update_profile"), + data={ + "name": "Updated Name", + "email": "updated@example.com", + "avatar_url": "https://example.com/new-avatar.jpg" + }, + follow_redirects=False + ) + assert response.status_code == 303 + assert response.headers["location"] == app.url_path_for("read_profile") + + # Verify changes in database + session.refresh(test_user) + assert test_user.name == "Updated Name" + assert test_user.email == "updated@example.com" + assert test_user.avatar_url == "https://example.com/new-avatar.jpg" + + +def test_delete_account_unauthorized(unauth_client: TestClient): + """Test that unauthorized users cannot delete account""" + response: Response = unauth_client.post( + app.url_path_for("delete_account"), + data={"confirm_delete_password": "Test123!@#"}, + follow_redirects=False + ) + assert response.status_code == 303 # Redirect to login + assert response.headers["location"] == app.url_path_for("read_login") + + +def test_delete_account_wrong_password(auth_client: TestClient, test_user: User): + """Test that account deletion fails with wrong password""" + response: Response = auth_client.post( + app.url_path_for("delete_account"), + data={"confirm_delete_password": "WrongPassword123!"}, + follow_redirects=False + ) + assert response.status_code == 400 + assert "Password is incorrect" in response.text + + +def test_delete_account_success(auth_client: TestClient, test_user: User, session: Session): + """Test successful account deletion""" + + # Delete account + response: Response = auth_client.post( + app.url_path_for("delete_account"), + data={"confirm_delete_password": "Test123!@#"}, + follow_redirects=False + ) + assert response.status_code == 303 + assert response.headers["location"] == app.url_path_for("logout") + + # Verify user is deleted from database + user = session.get(User, test_user.id) + assert user is None From 26073b460a239a8607d454f920fe996e52836d22 Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 01:12:02 +0000 Subject: [PATCH 17/19] Fixed a type lint error and updated pytest docs --- docs/customization.qmd | 3 ++- tests/test_user.py | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/customization.qmd b/docs/customization.qmd index 279bbaf..00e9685 100644 --- a/docs/customization.qmd +++ b/docs/customization.qmd @@ -25,7 +25,8 @@ The following fixtures, defined in `tests/conftest.py`, are available in the tes - `set_up_database`: Sets up the test database before running the test suite by dropping all tables and recreating them to ensure a clean state. - `session`: Provides a session for database operations in tests. - `clean_db`: Cleans up the database tables before each test by deleting all entries in the `PasswordResetToken` and `User` tables. -- `client`: Provides a `TestClient` instance with the session fixture, overriding the `get_session` dependency to use the test session. +- `auth_client`: Provides a `TestClient` instance with access and refresh token cookies set, overriding the `get_session` dependency to use the `session` fixture. +- `unauth_client`: Provides a `TestClient` instance without authentication cookies set, overriding the `get_session` dependency to use the `session` fixture. - `test_user`: Creates a test user in the database with a predefined name, email, and hashed password. To run the tests, use these commands: diff --git a/tests/test_user.py b/tests/test_user.py index 294b9eb..d6f42d0 100644 --- a/tests/test_user.py +++ b/tests/test_user.py @@ -1,5 +1,5 @@ from fastapi.testclient import TestClient -from fastapi.responses import Response +from httpx import Response from sqlmodel import Session from main import app @@ -63,7 +63,7 @@ def test_delete_account_wrong_password(auth_client: TestClient, test_user: User) follow_redirects=False ) assert response.status_code == 400 - assert "Password is incorrect" in response.text + assert "Password is incorrect" in response.text.strip() def test_delete_account_success(auth_client: TestClient, test_user: User, session: Session): From e1ee3d75ab23d7d05408830667a1f9f826ab1f0f Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 01:34:04 +0000 Subject: [PATCH 18/19] Adjusted password strength regex to allow forward slashes --- templates/authentication/register.html | 2 +- utils/auth.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/templates/authentication/register.html b/templates/authentication/register.html index 69320a1..ceb8aac 100644 --- a/templates/authentication/register.html +++ b/templates/authentication/register.html @@ -25,7 +25,7 @@
diff --git a/utils/auth.py b/utils/auth.py index 8fbae0b..5793c3e 100644 --- a/utils/auth.py +++ b/utils/auth.py @@ -12,7 +12,6 @@ from datetime import UTC, datetime, timedelta from typing import Optional from fastapi import Depends, Cookie, HTTPException, status -from fastapi.responses import RedirectResponse from utils.db import get_session from utils.models import User, PasswordResetToken @@ -76,7 +75,7 @@ def validate_password_strength(v: str) -> str: """ logger.debug(f"Validating password for {field_name}") pattern = re.compile( - r"(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[@$!%*?&{}<>.,\\'#\-_=+\(\)\[\]:;|~])[A-Za-z\d@$!%*?&{}<>.,\\'#\-_=+\(\)\[\]:;|~]{8,}") + r"(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[@$!%*?&{}<>.,\\'#\-_=+\(\)\[\]:;|~/])[A-Za-z\d@$!%*?&{}<>.,\\'#\-_=+\(\)\[\]:;|~/]{8,}") if not pattern.match(v): logger.debug(f"Password for { field_name} does not satisfy the security policy") From b165e0a2ede652c8df7c7e2ccd5327e723747afe Mon Sep 17 00:00:00 2001 From: Christopher Carroll Smith Date: Mon, 25 Nov 2024 23:14:11 +0000 Subject: [PATCH 19/19] Document that we need to use -v flag when running docker compose down --- docs/installation.qmd | 21 ++++++++++++++++++++- index.qmd | 2 ++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/docs/installation.qmd b/docs/installation.qmd index 7167589..2014027 100644 --- a/docs/installation.qmd +++ b/docs/installation.qmd @@ -20,6 +20,8 @@ If you use VSCode with Docker to develop in a container, the following VSCode De Simply create a `.devcontainer` folder in the root of the project and add a `devcontainer.json` file in the folder with the above content. VSCode may prompt you to install the Dev Container extension if you haven't already, and/or to open the project in a container. If not, you can manually select "Dev Containers: Reopen in Container" from View > Command Palette. +*IMPORTANT: If using this dev container configuration, you will need to set the `DB_HOST` environment variable to "host.docker.internal" in the `.env` file.* + ## Install development dependencies manually ### Python and Docker @@ -103,15 +105,32 @@ Set your desired database name, username, and password in the .env file. To use password recovery, register a [Resend](https://resend.com/) account, verify a domain, get an API key, and paste the API key into the .env file. +If using the dev container configuration, you will need to set the `DB_HOST` environment variable to "host.docker.internal" in the .env file. Otherwise, set `DB_HOST` to "localhost" for local development. (In production, `DB_HOST` will be set to the hostname of the database server.) + ## Start development database +To start the development database, run the following command in your terminal from the root directory: + ``` bash docker compose up -d ``` +If at any point you change the environment variables in the .env file, you will need to stop the database service *and tear down the volume*: + +``` bash +# Don't forget the -v flag to tear down the volume! +docker compose down -v +``` + +You may also need to restart the terminal session to pick up the new environment variables. You can also add the `--force-recreate` and `--build` flags to the startup command to ensure the container is rebuilt: + +``` bash +docker compose up -d --force-recreate --build +``` + ## Run the development server -Make sure the development database is running and tables and default permissions/roles are created first. +Before running the development server, make sure the development database is running and tables and default permissions/roles are created first. Then run the following command in your terminal from the root directory: ``` bash uvicorn main:app --host 0.0.0.0 --port 8000 --reload diff --git a/index.qmd b/index.qmd index 0266e7b..b23f439 100644 --- a/index.qmd +++ b/index.qmd @@ -107,6 +107,8 @@ To use password recovery, register a [Resend](https://resend.com/) account, veri ### Start development database +To start the development database, run the following command in your terminal from the root directory: + ``` bash docker compose up -d ```