From cc6bbdb794b5351c1ae6e4e58f9a0aec5afd17c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Voron?= Date: Thu, 2 Jan 2025 14:37:31 +0100 Subject: [PATCH] server/oauth2: raise OAuth2Client.client_id/client_secret length limits --- ...36_raise_oauth2client_client_id_client_.py | 56 +++++++++++++++++++ server/polar/models/oauth2_client.py | 2 + server/tests/oauth2/endpoints/test_oauth2.py | 6 +- .../oauth2/service/test_oauth2_client.py | 14 ++--- 4 files changed, 65 insertions(+), 13 deletions(-) create mode 100644 server/migrations/versions/2025-01-02-1436_raise_oauth2client_client_id_client_.py diff --git a/server/migrations/versions/2025-01-02-1436_raise_oauth2client_client_id_client_.py b/server/migrations/versions/2025-01-02-1436_raise_oauth2client_client_id_client_.py new file mode 100644 index 0000000000..5b57724d38 --- /dev/null +++ b/server/migrations/versions/2025-01-02-1436_raise_oauth2client_client_id_client_.py @@ -0,0 +1,56 @@ +"""Raise OAuth2Client.client_id/client_secret length limits + +Revision ID: d23cb1d45208 +Revises: eaf307b21bd9 +Create Date: 2025-01-02 14:36:38.357087 + +""" + +import sqlalchemy as sa +from alembic import op + +# Polar Custom Imports + +# revision identifiers, used by Alembic. +revision = "d23cb1d45208" +down_revision = "eaf307b21bd9" +branch_labels: tuple[str] | None = None +depends_on: tuple[str] | None = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column( + "oauth2_clients", + "client_id", + existing_type=sa.VARCHAR(length=48), + type_=sa.String(length=52), + nullable=False, + ) + op.alter_column( + "oauth2_clients", + "client_secret", + existing_type=sa.VARCHAR(length=120), + type_=sa.String(length=52), + nullable=False, + ) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column( + "oauth2_clients", + "client_secret", + existing_type=sa.String(length=52), + type_=sa.VARCHAR(length=120), + nullable=True, + ) + op.alter_column( + "oauth2_clients", + "client_id", + existing_type=sa.String(length=52), + type_=sa.VARCHAR(length=48), + nullable=True, + ) + # ### end Alembic commands ### diff --git a/server/polar/models/oauth2_client.py b/server/polar/models/oauth2_client.py index d675815699..a37a7ef62d 100644 --- a/server/polar/models/oauth2_client.py +++ b/server/polar/models/oauth2_client.py @@ -15,6 +15,8 @@ class OAuth2Client(RecordModel, OAuth2ClientMixin): __tablename__ = "oauth2_clients" __table_args__ = (UniqueConstraint("client_id"),) + client_id: Mapped[str] = mapped_column(String(52), index=True, nullable=False) + client_secret: Mapped[str] = mapped_column(String(52), nullable=False) registration_access_token: Mapped[str] = mapped_column( String, index=True, nullable=False ) diff --git a/server/tests/oauth2/endpoints/test_oauth2.py b/server/tests/oauth2/endpoints/test_oauth2.py index 08b04b4052..7979c71edd 100644 --- a/server/tests/oauth2/endpoints/test_oauth2.py +++ b/server/tests/oauth2/endpoints/test_oauth2.py @@ -1,5 +1,3 @@ -from typing import cast - import pytest import pytest_asyncio from httpx import AsyncClient @@ -567,7 +565,7 @@ async def test_allow( sync_session, sub_type=SubType.user, sub_id=user.id, - client_id=cast(str, oauth2_client.client_id), + client_id=oauth2_client.client_id, ) assert grant is not None assert grant.scopes == ["openid", "profile", "email"] @@ -676,7 +674,7 @@ async def test_organization_allow( sync_session, sub_type=SubType.organization, sub_id=organization.id, - client_id=cast(str, oauth2_client.client_id), + client_id=oauth2_client.client_id, ) assert grant is not None assert grant.scopes == ["openid", "profile", "email"] diff --git a/server/tests/oauth2/service/test_oauth2_client.py b/server/tests/oauth2/service/test_oauth2_client.py index 439f00a2a6..10668ed62a 100644 --- a/server/tests/oauth2/service/test_oauth2_client.py +++ b/server/tests/oauth2/service/test_oauth2_client.py @@ -1,4 +1,3 @@ -from typing import cast from unittest.mock import MagicMock import pytest @@ -54,13 +53,10 @@ async def test_true_positive( oauth2_client: OAuth2Client, enqueue_email_mock: MagicMock, ) -> None: - token = cast( - str, - ( - oauth2_client.client_secret - if token_type == TokenType.client_secret - else oauth2_client.registration_access_token - ), + token = ( + oauth2_client.client_secret + if token_type == TokenType.client_secret + else oauth2_client.registration_access_token ) result = await oauth2_client_service.revoke_leaked( @@ -72,7 +68,7 @@ async def test_true_positive( assert updated_oauth2_client is not None if token_type == TokenType.client_secret: - assert cast(str, updated_oauth2_client.client_secret) != token + assert updated_oauth2_client.client_secret != token else: assert updated_oauth2_client.registration_access_token != token