diff --git a/backend/pyproject.toml b/backend/pyproject.toml index dc8065aef..d4420c7ce 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -25,6 +25,7 @@ test = [ "Faker==20.1.0", "httpx==0.25.1", "pytest==7.4.3", + "freezegun==1.4.0", ] deploy = ['appointment[cli]', 'appointment[db]'] diff --git a/backend/src/appointment/controller/auth.py b/backend/src/appointment/controller/auth.py index df6aa94dd..f6bc255be 100644 --- a/backend/src/appointment/controller/auth.py +++ b/backend/src/appointment/controller/auth.py @@ -6,8 +6,23 @@ import os import hashlib import hmac +import datetime -from ..database import repo, schemas +from sqlalchemy.orm import Session + +from .apis.fxa_client import FxaClient +from ..database import repo, schemas, models + + +def logout(db: Session, subscriber: models.Subscriber, fxa_client: FxaClient | None, deny_previous_tokens=True): + """Sets a minimum valid issued at time (time). This prevents access tokens issued earlier from working.""" + if deny_previous_tokens: + subscriber.minimum_valid_iat_time = datetime.datetime.now(datetime.UTC) + db.add(subscriber) + db.commit() + + if os.getenv('AUTH_SCHEME') == 'fxa': + fxa_client.logout() def sign_url(url: str): diff --git a/backend/src/appointment/database/models.py b/backend/src/appointment/database/models.py index e9d092478..36937f215 100644 --- a/backend/src/appointment/database/models.py +++ b/backend/src/appointment/database/models.py @@ -92,6 +92,9 @@ class Subscriber(Base): google_state_expires_at = Column(DateTime) short_link_hash = Column(StringEncryptedType(String, secret, AesEngine, "pkcs5", length=255), index=False) + # Only accept the times greater than the one specified in the `iat` claim of the jwt token + minimum_valid_iat_time = Column('minimum_valid_iat_time', StringEncryptedType(DateTime, secret, AesEngine, "pkcs5", length=255)) + calendars = relationship("Calendar", cascade="all,delete", back_populates="owner") slots = relationship("Slot", cascade="all,delete", back_populates="subscriber") external_connections = relationship("ExternalConnections", cascade="all,delete", back_populates="owner") diff --git a/backend/src/appointment/database/repo.py b/backend/src/appointment/database/repo.py index a9fad0fdf..50ced4223 100644 --- a/backend/src/appointment/database/repo.py +++ b/backend/src/appointment/database/repo.py @@ -50,7 +50,7 @@ def delete_attendees_by_subscriber(db: Session, subscriber_id: int): """ -def get_subscriber(db: Session, subscriber_id: int): +def get_subscriber(db: Session, subscriber_id: int) -> models.Subscriber | None: """retrieve subscriber by id""" return db.get(models.Subscriber, subscriber_id) @@ -610,7 +610,7 @@ def delete_external_connections_by_type_id(db: Session, subscriber_id: int, type return True -def get_external_connections_by_type(db: Session, subscriber_id: int, type: models.ExternalConnectionType, type_id: str | None): +def get_external_connections_by_type(db: Session, subscriber_id: int, type: models.ExternalConnectionType, type_id: str | None) -> models.ExternalConnections | None: """Return a subscribers external connections by type, and optionally type id""" query = ( db.query(models.ExternalConnections) diff --git a/backend/src/appointment/dependencies/auth.py b/backend/src/appointment/dependencies/auth.py index 21b12e50f..f1b33711d 100644 --- a/backend/src/appointment/dependencies/auth.py +++ b/backend/src/appointment/dependencies/auth.py @@ -18,13 +18,22 @@ def get_user_from_token(db, token: str): try: payload = jwt.decode(token, os.getenv('JWT_SECRET'), algorithms=[os.getenv('JWT_ALGO')]) sub = payload.get("sub") + iat = payload.get("iat") if sub is None: raise InvalidTokenException() except JWTError: raise InvalidTokenException() id = sub.replace('uid-', '') - return repo.get_subscriber(db, int(id)) + subscriber = repo.get_subscriber(db, int(id)) + + # Token has been expired by us - temp measure to avoid spinning a refresh system, or a deny list for this issue + if subscriber.minimum_valid_iat_time and not iat: + raise InvalidTokenException() + elif subscriber.minimum_valid_iat_time and subscriber.minimum_valid_iat_time.timestamp() > int(iat): + raise InvalidTokenException() + + return subscriber def get_subscriber( diff --git a/backend/src/appointment/migrations/versions/2024_01_09_1652-ad7cc2de5ff8_add_minimum_valid_iat_time_to_subscribers.py b/backend/src/appointment/migrations/versions/2024_01_09_1652-ad7cc2de5ff8_add_minimum_valid_iat_time_to_subscribers.py new file mode 100644 index 000000000..38f14fd19 --- /dev/null +++ b/backend/src/appointment/migrations/versions/2024_01_09_1652-ad7cc2de5ff8_add_minimum_valid_iat_time_to_subscribers.py @@ -0,0 +1,31 @@ +"""add minimum_valid_iat_time to subscribers + +Revision ID: ad7cc2de5ff8 +Revises: 0dc429ca07f5 +Create Date: 2024-01-09 16:52:20.941572 + +""" +import os +from alembic import op +import sqlalchemy as sa +from sqlalchemy_utils import StringEncryptedType +from sqlalchemy_utils.types.encrypted.encrypted_type import AesEngine + + +def secret(): + return os.getenv("DB_SECRET") + + +# revision identifiers, used by Alembic. +revision = 'ad7cc2de5ff8' +down_revision = '0dc429ca07f5' +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column('subscribers', sa.Column('minimum_valid_iat_time', StringEncryptedType(sa.DateTime, secret, AesEngine, "pkcs5", length=255))) + + +def downgrade() -> None: + op.drop_column('subscribers', 'minimum_valid_iat_time') diff --git a/backend/src/appointment/routes/auth.py b/backend/src/appointment/routes/auth.py index 4b2a8e890..cfc6f36bb 100644 --- a/backend/src/appointment/routes/auth.py +++ b/backend/src/appointment/routes/auth.py @@ -19,6 +19,7 @@ from ..dependencies.database import get_db from ..dependencies.auth import get_subscriber +from ..controller import auth from ..controller.apis.fxa_client import FxaClient from ..dependencies.fxa import get_fxa_client from ..exceptions.fxa_api import NotInAllowListException @@ -42,7 +43,10 @@ def create_access_token(data: dict, expires_delta: timedelta | None = None): expire = datetime.now(UTC) + expires_delta else: expire = datetime.now(UTC) + timedelta(minutes=15) - to_encode.update({"exp": expire}) + to_encode.update({ + "exp": expire, + "iat": int(datetime.now(UTC).timestamp()) + }) encoded_jwt = jwt.encode(to_encode, os.getenv('JWT_SECRET'), algorithm=os.getenv('JWT_ALGO')) return encoded_jwt @@ -194,15 +198,14 @@ def token( @router.get('/logout') -def logout(subscriber: Subscriber = Depends(get_subscriber), fxa_client: FxaClient = Depends(get_fxa_client)): +def logout(db: Session = Depends(get_db), subscriber: Subscriber = Depends(get_subscriber), fxa_client: FxaClient = Depends(get_fxa_client)): """Logout a given subscriber session""" - # We don't actually have to do anything for non-fxa schemes - if os.getenv('AUTH_SCHEME') != 'fxa': - return True + if os.getenv('AUTH_SCHEME') == 'fxa': + fxa_client.setup(subscriber.id, subscriber.get_external_connection(ExternalConnectionType.fxa).token) - fxa_client.setup(subscriber.id, subscriber.get_external_connection(ExternalConnectionType.fxa).token) - fxa_client.logout() + # Don't set a minimum_valid_iat_time here. + auth.logout(db, subscriber, fxa_client, deny_previous_tokens=False) return True diff --git a/backend/src/appointment/routes/webhooks.py b/backend/src/appointment/routes/webhooks.py index da15aa297..7bf52b471 100644 --- a/backend/src/appointment/routes/webhooks.py +++ b/backend/src/appointment/routes/webhooks.py @@ -5,10 +5,12 @@ from fastapi import APIRouter, Depends, Request from sqlalchemy.orm import Session +from ..controller import auth, data from ..controller.apis.fxa_client import FxaClient -from ..database import repo, models +from ..database import repo, models, schemas from ..dependencies.database import get_db from ..dependencies.fxa import get_webhook_auth, get_fxa_client +from ..exceptions.account_api import AccountDeletionSubscriberFail from ..exceptions.fxa_api import MissingRefreshTokenException router = APIRouter() @@ -16,7 +18,6 @@ @router.post("/fxa-process") def fxa_process( - request: Request, db: Session = Depends(get_db), decoded_token: dict = Depends(get_webhook_auth), fxa_client: FxaClient = Depends(get_fxa_client) @@ -42,20 +43,41 @@ def fxa_process( break try: - fxa_client.logout() + auth.logout(db, subscriber, fxa_client) except MissingRefreshTokenException: logging.warning("Subscriber doesn't have refresh token.") except requests.exceptions.HTTPError as ex: logging.error(f"Error logging out user: {ex.response}") case 'https://schemas.accounts.firefox.com/event/profile-change': if event_data.get('email') is not None: - # Update the subscriber's email (and username for now) + # Update the subscriber's email, we do this first in case there's a problem with get_profile() subscriber.email = event_data.get('email') - subscriber.username = subscriber.email db.add(subscriber) db.commit() + + try: + profile = fxa_client.get_profile() + # Update profile with fxa info + repo.update_subscriber(db, schemas.SubscriberIn( + avatar_url=profile['avatar'], + name=profile['displayName'] if 'displayName' in profile else profile['email'].split('@')[0], + username=subscriber.username + ), subscriber.id) + except Exception as ex: + logging.error(f"Error updating user: {ex}") + + # Finally log the subscriber out + try: + auth.logout(db, subscriber, fxa_client) + except MissingRefreshTokenException: + logging.warning("Subscriber doesn't have refresh token.") + except requests.exceptions.HTTPError as ex: + logging.error(f"Error logging out user: {ex.response}") case 'https://schemas.accounts.firefox.com/event/delete-user': - # TODO: We have a delete function, but it's not up-to-date - logging.warning(f"Deletion request came in for {subscriber.id}") + try: + data.delete_account(db, subscriber) + except AccountDeletionSubscriberFail as ex: + logging.error(f"Account deletion webhook failed: {ex.message}") + case _: logging.warning(f"Ignoring event {event}") diff --git a/backend/test/factory/subscriber_factory.py b/backend/test/factory/subscriber_factory.py index 2ff5cafea..94ff925e5 100644 --- a/backend/test/factory/subscriber_factory.py +++ b/backend/test/factory/subscriber_factory.py @@ -14,7 +14,7 @@ def _make_subscriber(level, name=FAKER_RANDOM_VALUE, username=FAKER_RANDOM_VALUE subscriber = repo.create_subscriber(db, schemas.SubscriberBase( name=name if factory_has_value(name) else fake.name(), username=username if factory_has_value(username) else fake.name(), - email=email if factory_has_value(FAKER_RANDOM_VALUE) else fake.email(), + email=email if factory_has_value(email) else fake.email(), level=level, timezone='America/Vancouver' )) diff --git a/backend/test/integration/test_webhooks.py b/backend/test/integration/test_webhooks.py new file mode 100644 index 000000000..f708878cb --- /dev/null +++ b/backend/test/integration/test_webhooks.py @@ -0,0 +1,162 @@ +import datetime + +from freezegun import freeze_time +from appointment.database import models, repo + +from appointment.dependencies.fxa import get_webhook_auth +from defines import FXA_CLIENT_PATCH + + +class TestFXAWebhooks: + def test_fxa_process_change_password(self, with_db, with_client, make_pro_subscriber, make_external_connections): + """Ensure the change password event is handled correctly""" + FXA_USER_ID = 'abc-123' + + def override_get_webhook_auth(): + return { + "iss": "https://accounts.firefox.com/", + "sub": FXA_USER_ID, + "aud": "REMOTE_SYSTEM", + "iat": 1565720808, + "jti": "e19ed6c5-4816-4171-aa43-56ffe80dbda1", + "events": { + "https://schemas.accounts.firefox.com/event/password-change": { + "changeTime": 1565721242227 + } + } + } + + # Override get_webhook_auth so we don't have to mock up a valid jwt token + with_client.app.dependency_overrides[get_webhook_auth] = override_get_webhook_auth + + # make a guy + subscriber = make_pro_subscriber() + subscriber_id = subscriber.id + make_external_connections(subscriber_id, type=models.ExternalConnectionType.fxa, type_id=FXA_USER_ID) + + assert subscriber.minimum_valid_iat_time is None + + # Freeze time to before the changeTime timestamp and test the password change works correctly + with freeze_time('Aug 13th 2019'): + # Update the external connection time to match our freeze_time + with with_db() as db: + fxa_connection = repo.get_external_connections_by_type(db, subscriber_id, models.ExternalConnectionType.fxa, FXA_USER_ID)[0] + fxa_connection.time_updated = datetime.datetime.now() + db.add(fxa_connection) + db.commit() + + response = with_client.post( + "/webhooks/fxa-process", + ) + assert response.status_code == 200, response.text + + with with_db() as db: + subscriber = repo.get_subscriber(db, subscriber_id) + assert subscriber.minimum_valid_iat_time is not None + + # Update the external connection time to match our current time + # This will make the change password event out of date + with with_db() as db: + fxa_connection = repo.get_external_connections_by_type(db, subscriber_id, models.ExternalConnectionType.fxa, FXA_USER_ID)[0] + fxa_connection.time_updated = datetime.datetime.now() + db.add(fxa_connection) + db.commit() + + # Reset our minimum_valid_iat_time, so we can ensure it stays None + subscriber = repo.get_subscriber(db, subscriber_id) + subscriber.minimum_valid_iat_time = None + db.add(subscriber) + db.commit() + + # Finally test that minimum_valid_iat_time stays the same due to an outdated password change event + response = with_client.post( + "/webhooks/fxa-process", + ) + assert response.status_code == 200, response.text + with with_db() as db: + subscriber = repo.get_subscriber(db, subscriber_id) + assert subscriber.minimum_valid_iat_time is None + + def test_fxa_process_change_primary_email(self, with_db, with_client, make_pro_subscriber, make_external_connections): + """Ensure the change primary email event is handled correctly""" + + FXA_USER_ID = 'abc-456' + OLD_EMAIL = 'xBufferFan94x@example.org' + NEW_EMAIL = 'john.butterfly@example.org' + + def override_get_webhook_auth(): + return { + "iss": "https://accounts.firefox.com/", + "sub": FXA_USER_ID, + "aud": "REMOTE_SYSTEM", + "iat": 1565720808, + "jti": "e19ed6c5-4816-4171-aa43-56ffe80dbda1", + "events": { + "https://schemas.accounts.firefox.com/event/profile-change": { + "email": NEW_EMAIL + } + } + } + + # Override get_webhook_auth so we don't have to mock up a valid jwt token + with_client.app.dependency_overrides[get_webhook_auth] = override_get_webhook_auth + + # Make a guy with a middleschool-era email they would like to change + subscriber = make_pro_subscriber(email=OLD_EMAIL) + subscriber_id = subscriber.id + make_external_connections(subscriber_id, type=models.ExternalConnectionType.fxa, type_id=FXA_USER_ID) + + assert subscriber.minimum_valid_iat_time is None + assert subscriber.email == OLD_EMAIL + assert subscriber.avatar_url != FXA_CLIENT_PATCH.get('subscriber_avatar_url') + assert subscriber.name != FXA_CLIENT_PATCH.get('subscriber_display_name') + + response = with_client.post( + "/webhooks/fxa-process", + ) + assert response.status_code == 200, response.text + + # Refresh the subscriber and test minimum_valid_iat_time (they should be logged out), and email address + with with_db() as db: + subscriber = repo.get_subscriber(db, subscriber_id) + assert subscriber.email == NEW_EMAIL + assert subscriber.minimum_valid_iat_time is not None + + # Ensure our profile update occured + assert subscriber.avatar_url == FXA_CLIENT_PATCH.get('subscriber_avatar_url') + assert subscriber.name == FXA_CLIENT_PATCH.get('subscriber_display_name') + + def test_fxa_process_delete_user(self, with_db, with_client, make_pro_subscriber, make_external_connections, make_appointment, make_caldav_calendar): + """Ensure the delete user event is handled correctly""" + FXA_USER_ID = 'abc-789' + + def override_get_webhook_auth(): + return { + "iss": "https://accounts.firefox.com/", + "sub": FXA_USER_ID, + "aud": "REMOTE_SYSTEM", + "iat": 1565720810, + "jti": "1b3d623a-300a-4ab8-9241-855c35586809", + "events": { + "https://schemas.accounts.firefox.com/event/delete-user": {} + } + } + + # Override get_webhook_auth so we don't have to mock up a valid jwt token + with_client.app.dependency_overrides[get_webhook_auth] = override_get_webhook_auth + + subscriber = make_pro_subscriber() + make_external_connections(subscriber.id, type=models.ExternalConnectionType.fxa, type_id=FXA_USER_ID) + calendar = make_caldav_calendar(subscriber.id) + appointment = make_appointment(calendar_id=calendar.id) + + response = with_client.post( + "/webhooks/fxa-process", + ) + assert response.status_code == 200, response.text + + with with_db() as db: + # Make sure everything we created is gone. A more exhaustive check is done in the delete account test + assert repo.get_subscriber(db, subscriber.id) is None + assert repo.get_calendar(db, calendar.id) is None + assert repo.get_appointment(db, appointment.id) is None diff --git a/backend/test/unit/test_auth_dependency.py b/backend/test/unit/test_auth_dependency.py new file mode 100644 index 000000000..9d6824788 --- /dev/null +++ b/backend/test/unit/test_auth_dependency.py @@ -0,0 +1,63 @@ +import datetime +import os + +import pytest +from freezegun import freeze_time + +from appointment.database import repo +from appointment.dependencies.auth import get_user_from_token +from appointment.exceptions.validation import InvalidTokenException +from appointment.routes.auth import create_access_token + + +class TestAuthDependency: + + def test_get_user_from_token(self, with_db, with_l10n, make_pro_subscriber): + subscriber = make_pro_subscriber() + access_token_expires = datetime.timedelta(minutes=float(os.getenv('JWT_EXPIRE_IN_MINS'))) + + # Ensure we don't have a minimum_valid_iat_time, that test comes later. + assert subscriber.minimum_valid_iat_time is None + + # Create the access token and test it + with freeze_time("Jan 9th 2024"): + access_token = create_access_token(data={"sub": f"uid-{subscriber.id}"}, expires_delta=access_token_expires) + + assert access_token + + with with_db() as db: + subscriber_from_token = get_user_from_token(db, access_token) + + assert subscriber_from_token + assert subscriber_from_token == subscriber_from_token + + # The access token should still be valid the next day + with freeze_time("Jan 10th 2024"): + with with_db() as db: + subscriber_from_token = get_user_from_token(db, access_token) + + assert subscriber_from_token + assert subscriber_from_token == subscriber_from_token + + # Pick a time outside the token expiry window, and ensure it breaks + with freeze_time("Feb 1st 2024"): + with with_db() as db: + # Internally raises ExpiredSignatureError, but we catch it and send a HTTPException instead. + with pytest.raises(InvalidTokenException): + get_user_from_token(db, access_token) + + # Update the subscriber to have a minimum_valid_iat_time + with freeze_time("Jan 10th 2024"): + with with_db() as db: + # We need to pull down the subscriber in this db session, otherwise we can't save it. + subscriber = repo.get_subscriber(db, subscriber.id) + subscriber.minimum_valid_iat_time = datetime.datetime.now(datetime.UTC) + db.add(subscriber) + db.commit() + + # Now the access token should be invalid + with freeze_time("Jan 9th 2024"): + with with_db() as db: + # Internally raises ExpiredSignatureError, but we catch it and send a HTTPException instead. + with pytest.raises(InvalidTokenException): + get_user_from_token(db, access_token) diff --git a/frontend/src/views/LoginView.vue b/frontend/src/views/LoginView.vue index cd7a8d505..478460421 100644 --- a/frontend/src/views/LoginView.vue +++ b/frontend/src/views/LoginView.vue @@ -61,7 +61,8 @@ const login = async () => { } if (isFxaAuth) { - const { error, data } = await call(`fxa_login?email=${username.value}&timezone=${dj.tz.guess()}`).get().json(); + const email = encodeURIComponent(username.value); + const { error, data } = await call(`fxa_login?email=${email}&timezone=${dj.tz.guess()}`).get().json(); const { url } = data.value; if (error.value) {