From 6e8cafd78c65a14c557f5fa235333add66683e24 Mon Sep 17 00:00:00 2001 From: Shawn Zivontsis Date: Sat, 3 Aug 2024 22:33:55 -0400 Subject: [PATCH 1/4] Make client_secret nullable --- aioauth_fastapi_demo/admin/models.py | 2 +- aioauth_fastapi_demo/oauth2/models.py | 2 +- ...l_migrations.py => 21eb480f8485_initial_migrations.py} | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) rename aioauth_fastapi_demo/storage/migrations/versions/{07a7ace268a7_initial_migrations.py => 21eb480f8485_initial_migrations.py} (98%) diff --git a/aioauth_fastapi_demo/admin/models.py b/aioauth_fastapi_demo/admin/models.py index d74af67..7971e2e 100644 --- a/aioauth_fastapi_demo/admin/models.py +++ b/aioauth_fastapi_demo/admin/models.py @@ -6,7 +6,7 @@ class ClientCreate(BaseModel): client_id: str - client_secret: str + client_secret: Optional[str] grant_types: List[GrantType] response_types: List[ResponseType] redirect_uris: List[str] diff --git a/aioauth_fastapi_demo/oauth2/models.py b/aioauth_fastapi_demo/oauth2/models.py index ada7f23..eaed284 100644 --- a/aioauth_fastapi_demo/oauth2/models.py +++ b/aioauth_fastapi_demo/oauth2/models.py @@ -13,7 +13,7 @@ class Client(BaseTable, table=True): # type: ignore client_id: str - client_secret: str + client_secret: Optional[str] grant_types: List[str] = Field(sa_column=Column(ARRAY(String))) response_types: List[str] = Field(sa_column=Column(ARRAY(String))) redirect_uris: List[str] = Field(sa_column=Column(ARRAY(String))) diff --git a/aioauth_fastapi_demo/storage/migrations/versions/07a7ace268a7_initial_migrations.py b/aioauth_fastapi_demo/storage/migrations/versions/21eb480f8485_initial_migrations.py similarity index 98% rename from aioauth_fastapi_demo/storage/migrations/versions/07a7ace268a7_initial_migrations.py rename to aioauth_fastapi_demo/storage/migrations/versions/21eb480f8485_initial_migrations.py index 2fe0862..c28cbe4 100644 --- a/aioauth_fastapi_demo/storage/migrations/versions/07a7ace268a7_initial_migrations.py +++ b/aioauth_fastapi_demo/storage/migrations/versions/21eb480f8485_initial_migrations.py @@ -1,8 +1,8 @@ """Initial migrations -Revision ID: 07a7ace268a7 +Revision ID: 21eb480f8485 Revises: -Create Date: 2021-10-02 22:50:10.418498 +Create Date: 2024-08-03 21:35:56.307146 """ import sqlalchemy as sa @@ -10,7 +10,7 @@ from alembic import op # revision identifiers, used by Alembic. -revision = "07a7ace268a7" +revision = "21eb480f8485" down_revision = None branch_labels = None depends_on = None @@ -125,7 +125,7 @@ def upgrade(): sa.Column("redirect_uris", sa.ARRAY(sa.String()), nullable=True), sa.Column("id", sqlmodel.sql.sqltypes.GUID(), nullable=False), sa.Column("client_id", sqlmodel.sql.sqltypes.AutoString(), nullable=False), - sa.Column("client_secret", sqlmodel.sql.sqltypes.AutoString(), nullable=False), + sa.Column("client_secret", sqlmodel.sql.sqltypes.AutoString(), nullable=True), sa.Column("scope", sqlmodel.sql.sqltypes.AutoString(), nullable=False), sa.Column("user_id", sqlmodel.sql.sqltypes.GUID(), nullable=False), sa.ForeignKeyConstraint( From e452fafb978381acef8b6f7d0911c756d1660c13 Mon Sep 17 00:00:00 2001 From: Shawn Zivontsis Date: Sat, 3 Aug 2024 22:35:02 -0400 Subject: [PATCH 2/4] Add tests for incorrect client_secret --- tests/conftest.py | 32 ++++++++++++---- tests/test_oauth2.py | 91 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 111 insertions(+), 12 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index ec69ad4..88f0abe 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ import logging -from uuid import uuid4 +from typing import Optional +from uuid import UUID, uuid4 import pytest from alembic.config import main @@ -65,16 +66,19 @@ async def user(db: "SQLAlchemyStorage", user_password: str) -> User: return user -@pytest.fixture -async def client(db: "SQLAlchemyStorage", user: "User") -> Client: - client_id = uuid4() - client_secret = uuid4() +async def _create_client( + db: "SQLAlchemyStorage", + user: "User", + client_id: UUID, + client_secret: Optional[UUID], +) -> Client: grant_types = [ "authorization_code", - "client_credentials", "password", "refresh_token", ] + if client_secret is not None: + grant_types.append("client_credentials") response_types = [ "code", "id_token", @@ -88,7 +92,7 @@ async def client(db: "SQLAlchemyStorage", user: "User") -> Client: client = Client( client_id=str(client_id), - client_secret=str(client_secret), + client_secret=str(client_secret) if client_secret is not None else None, response_types=response_types, grant_types=grant_types, redirect_uris=redirect_uris, @@ -99,3 +103,17 @@ async def client(db: "SQLAlchemyStorage", user: "User") -> Client: await db.add(client) return client + + +@pytest.fixture +async def client(db: "SQLAlchemyStorage", user: "User") -> Client: + client_id = uuid4() + client_secret = uuid4() + return await _create_client(db, user, client_id, client_secret) + + +@pytest.fixture +async def public_client(db: "SQLAlchemyStorage", user: "User") -> Client: + client_id = uuid4() + client_secret = None + return await _create_client(db, user, client_id, client_secret) diff --git a/tests/test_oauth2.py b/tests/test_oauth2.py index 9fc0928..2f05e43 100644 --- a/tests/test_oauth2.py +++ b/tests/test_oauth2.py @@ -9,10 +9,7 @@ from aioauth_fastapi_demo.users.models import User -@pytest.mark.asyncio -async def test_authorization_code_flow( - http_client: TestClient, user: "User", client: "Client" -): +async def _get_authorization_code(http_client, user, client): access_token, _ = get_jwt(user) response = await http_client.get( @@ -37,6 +34,15 @@ async def test_authorization_code_flow( assert "scope" in parsed_qs.keys() assert "id_token" in parsed_qs.keys() + return parsed_qs["code"][0] + + +@pytest.mark.asyncio +async def test_authorization_code_flow( + http_client: TestClient, user: "User", client: "Client" +): + authorization_code = await _get_authorization_code(http_client, user, client) + response = await http_client.post( "/oauth2/token", form={ @@ -44,7 +50,7 @@ async def test_authorization_code_flow( "redirect_uri": client.redirect_uris[0], "client_id": client.client_id, "client_secret": client.client_secret, - "code": parsed_qs["code"][0], + "code": authorization_code, }, ) @@ -79,6 +85,81 @@ async def test_authorization_code_flow( ), "re-trying to revoke an already revoked token should be rejected" +@pytest.mark.asyncio +async def test_authorization_code_no_secret( + http_client: TestClient, user: "User", client: "Client" +): + authorization_code = await _get_authorization_code(http_client, user, client) + + response = await http_client.post( + "/oauth2/token", + form={ + "grant_type": "authorization_code", + "redirect_uri": client.redirect_uris[0], + "client_id": client.client_id, + "code": authorization_code, + }, + ) + + assert ( + response.status_code == HTTPStatus.UNAUTHORIZED + ), "no client secret for a confidential client should be rejected" + + +@pytest.mark.asyncio +async def test_authorization_code_wrong_secret( + http_client: TestClient, user: "User", client: "Client" +): + authorization_code = await _get_authorization_code(http_client, user, client) + + response = await http_client.post( + "/oauth2/token", + form={ + "grant_type": "authorization_code", + "redirect_uri": client.redirect_uris[0], + "client_id": client.client_id, + "client_secret": f"not {client.client_secret}", + "code": authorization_code, + }, + ) + + assert ( + response.status_code == HTTPStatus.UNAUTHORIZED + ), "wrong client secret for a confidential client should be rejected" + + +@pytest.mark.asyncio +async def test_authorization_code_public_client( + http_client: TestClient, user: "User", public_client: "Client" +): + authorization_code = await _get_authorization_code(http_client, user, public_client) + + response = await http_client.post( + "/oauth2/token", + form={ + "grant_type": "authorization_code", + "redirect_uri": public_client.redirect_uris[0], + "client_id": public_client.client_id, + "code": authorization_code, + }, + ) + + assert "access_token" in response.json() + + refresh_token = response.json()["refresh_token"] + + response = await http_client.post( + "/oauth2/token", + form={ + "grant_type": "refresh_token", + "refresh_token": refresh_token, + "client_id": public_client.client_id, + }, + ) + + assert response.status_code == HTTPStatus.OK + + @pytest.mark.asyncio async def test_implicit_flow(http_client: TestClient, user: "User", client: "Client"): access_token, _ = get_jwt(user) From c0c773abffac445e18956f25edcd171634e65b94 Mon Sep 17 00:00:00 2001 From: Shawn Zivontsis Date: Sat, 3 Aug 2024 22:35:33 -0400 Subject: [PATCH 3/4] Validate client_secret in get_client --- aioauth_fastapi_demo/oauth2/storage.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/aioauth_fastapi_demo/oauth2/storage.py b/aioauth_fastapi_demo/oauth2/storage.py index 7bde2bd..c64ad4b 100644 --- a/aioauth_fastapi_demo/oauth2/storage.py +++ b/aioauth_fastapi_demo/oauth2/storage.py @@ -216,6 +216,11 @@ async def get_client( if not client_record: return None + if client_secret is not None and client_record.client_secret is not None: + # validate the client_secret + if client_secret != client_record.client_secret: + return None + return Client( client_id=client_record.client_id, client_secret=client_record.client_secret, From bc4bebe9e86cf262f5e7a5bc0cc85515e7770345 Mon Sep 17 00:00:00 2001 From: Shawn Zivontsis Date: Sun, 4 Aug 2024 09:16:24 -0400 Subject: [PATCH 4/4] Upgrade python version in CI --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7d582ac..2d737ff 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,7 +30,7 @@ jobs: - name: Set up Python uses: actions/setup-python@v1 with: - python-version: "3.10.10" + python-version: "3.10.14" - name: Install dependencies run: | make dev-install