Skip to content

Commit

Permalink
[FIX] Ensure token forwarded to n-APIs does not include an extra sche…
Browse files Browse the repository at this point in the history
…me string (#134)

* strip auth scheme from token before forwarding

* add positive test for verify_token

* rename func and variable for clarity

* rename test fixtures
  • Loading branch information
alyssadai authored Nov 1, 2024
1 parent c89f188 commit f191115
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 18 deletions.
4 changes: 2 additions & 2 deletions app/api/routers/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from .. import crud, security
from ..models import CombinedQueryResponse, QueryModel
from ..security import verify_token
from ..security import verify_and_extract_token

# from fastapi.security import open_id_connect_url

Expand Down Expand Up @@ -61,7 +61,7 @@ async def get_query(
status_code=status.HTTP_403_FORBIDDEN,
detail="Not authenticated",
)
verify_token(token)
token = verify_and_extract_token(token)

response_dict = await crud.get(
query.min_age,
Expand Down
12 changes: 8 additions & 4 deletions app/api/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,23 @@ def check_client_id():
)


def verify_token(token: str):
"""Verify the Google ID token. Raise an HTTPException if the token is invalid."""
def verify_and_extract_token(token: str) -> str:
"""
Verify and return the Google ID token with the authorization scheme stripped.
Raise an HTTPException if the token is invalid.
"""
# Adapted from https://developers.google.com/identity/gsi/web/guides/verify-google-id-token#python
try:
# Extract the token from the "Bearer" scheme
# (See https://github.com/tiangolo/fastapi/blob/master/fastapi/security/oauth2.py#L473-L485)
# TODO: Check also if scheme of token is "Bearer"?
_, param = get_authorization_scheme_param(token)
_, extracted_token = get_authorization_scheme_param(token)
id_info = id_token.verify_oauth2_token(
param, requests.Request(), CLIENT_ID
extracted_token, requests.Request(), CLIENT_ID
)
# TODO: Remove print statement or turn into logging
print("Token verified: ", id_info)
return extracted_token
except (GoogleAuthError, ValueError) as exc:
raise HTTPException(
status_code=status.HTTP_401_UNAUTHORIZED,
Expand Down
13 changes: 8 additions & 5 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,23 @@ def disable_auth(monkeypatch):


@pytest.fixture()
def mock_verify_token():
def mock_verify_and_extract_token():
"""Mock a successful token verification that does not raise any exceptions."""

def _verify_token(token):
def _verify_and_extract_token(token):
return None

return _verify_token
return _verify_and_extract_token


@pytest.fixture()
def set_mock_verify_token(monkeypatch, mock_verify_token):
def set_mock_verify_and_extract_token(
monkeypatch, mock_verify_and_extract_token
):
"""Set the verify_token function to a mock that does not raise any exceptions."""
monkeypatch.setattr(
"app.api.routers.query.verify_token", mock_verify_token
"app.api.routers.query.verify_and_extract_token",
mock_verify_and_extract_token,
)


Expand Down
8 changes: 4 additions & 4 deletions tests/test_query.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def test_partial_node_failure_responses_handled_gracefully(
set_valid_test_federation_nodes,
mocked_single_matching_dataset_result,
mock_token,
set_mock_verify_token,
set_mock_verify_and_extract_token,
caplog,
):
"""
Expand Down Expand Up @@ -101,7 +101,7 @@ def test_partial_node_request_failures_handled_gracefully(
set_valid_test_federation_nodes,
mocked_single_matching_dataset_result,
mock_token,
set_mock_verify_token,
set_mock_verify_and_extract_token,
error_to_raise,
expected_node_message,
caplog,
Expand Down Expand Up @@ -155,7 +155,7 @@ def test_all_nodes_failure_handled_gracefully(
test_app,
mock_failed_connection_httpx_get,
mock_token,
set_mock_verify_token,
set_mock_verify_and_extract_token,
set_valid_test_federation_nodes,
caplog,
):
Expand Down Expand Up @@ -193,7 +193,7 @@ def test_all_nodes_success_handled_gracefully(
set_valid_test_federation_nodes,
mocked_single_matching_dataset_result,
mock_token,
set_mock_verify_token,
set_mock_verify_and_extract_token,
):
"""
Test that when queries sent to all nodes succeed, the federation API response includes an overall success status and no errors.
Expand Down
39 changes: 36 additions & 3 deletions tests/test_security.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import pytest
from fastapi import HTTPException
from google.oauth2 import id_token

from app.api.security import verify_token
from app.api.security import verify_and_extract_token


def test_missing_client_id_raises_error_when_auth_enabled(
Expand Down Expand Up @@ -40,7 +41,7 @@ def test_missing_client_id_ignored_when_auth_disabled(monkeypatch, test_app):
def test_invalid_token_raises_error(invalid_token):
"""Test that an invalid token raises an error from the verification process."""
with pytest.raises(HTTPException) as exc_info:
verify_token(invalid_token)
verify_and_extract_token(invalid_token)

assert exc_info.value.status_code == 401
assert "Invalid token" in exc_info.value.detail
Expand All @@ -52,7 +53,7 @@ def test_invalid_token_raises_error(invalid_token):
)
def test_query_with_malformed_auth_header_fails(
test_app,
set_mock_verify_token,
set_mock_verify_and_extract_token,
enable_auth,
invalid_auth_header,
monkeypatch,
Expand All @@ -69,3 +70,35 @@ def test_query_with_malformed_auth_header_fails(
)

assert response.status_code == 403


def test_verified_token_returned_without_auth_scheme(monkeypatch, enable_auth):
"""
Test that when a token is valid, verify_token correctly returns the token with the authorization scheme stripped.
"""
mock_valid_token = "Bearer foo"
mock_id_info = {
"iss": "https://accounts.google.com",
"azp": "123abc.apps.googleusercontent.com",
"aud": "123abc.apps.googleusercontent.com",
"sub": "1234567890",
"email": "[email protected]",
"email_verified": True,
"nbf": 1730476622,
"name": "Jane Doe",
"picture": "https://lh3.googleusercontent.com/a/example1234567890",
"given_name": "Jane",
"family_name": "Doe",
"iat": 1730476922,
"exp": 1730480522,
"jti": "123e4567-e89b",
}

def mock_oauth2_verify_token(param, request, client_id, **kwargs):
return mock_id_info

monkeypatch.setattr(
id_token, "verify_oauth2_token", mock_oauth2_verify_token
)

assert verify_and_extract_token(mock_valid_token) == "foo"

0 comments on commit f191115

Please sign in to comment.