From bb063db9e26738e510dd09b6a275fe8975658be2 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Mon, 1 Aug 2022 14:22:37 -0700 Subject: [PATCH] Update OAuth strategy to be able to perform local testing (#962) --- .../api/v1/endpoints/oauth_endpoints.py | 2 +- src/fidesops/core/config.py | 1 + .../authentication_strategy_oauth2.py | 21 ++++++++++++++++++- .../api/v1/endpoints/test_oauth_endpoints.py | 6 +++--- .../test_authentication_strategy_oauth2.py | 8 ++++--- 5 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/fidesops/api/v1/endpoints/oauth_endpoints.py b/src/fidesops/api/v1/endpoints/oauth_endpoints.py index 06c8128699..1f5a096395 100644 --- a/src/fidesops/api/v1/endpoints/oauth_endpoints.py +++ b/src/fidesops/api/v1/endpoints/oauth_endpoints.py @@ -186,7 +186,7 @@ def read_scopes() -> List[str]: return SCOPE_REGISTRY -@router.post(OAUTH_CALLBACK, response_model=None) +@router.get(OAUTH_CALLBACK, response_model=None) def oauth_callback(code: str, state: str, db: Session = Depends(get_db)) -> None: """ Uses the passed in code to generate the token access request diff --git a/src/fidesops/core/config.py b/src/fidesops/core/config.py index 90568c4982..078a518742 100644 --- a/src/fidesops/core/config.py +++ b/src/fidesops/core/config.py @@ -159,6 +159,7 @@ class FidesopsConfig(FidesSettings): is_test_mode: bool = os.getenv("TESTING", "").lower() == "true" hot_reloading: bool = os.getenv("FIDESOPS__HOT_RELOAD", "").lower() == "true" dev_mode: bool = os.getenv("FIDESOPS__DEV_MODE", "").lower() == "true" + oauth_instance: Optional[str] = os.getenv("FIDESOPS__OAUTH_INSTANCE") class Config: # pylint: disable=C0115 case_sensitive = True diff --git a/src/fidesops/service/authentication/authentication_strategy_oauth2.py b/src/fidesops/service/authentication/authentication_strategy_oauth2.py index 5fb968a7ee..710eb99601 100644 --- a/src/fidesops/service/authentication/authentication_strategy_oauth2.py +++ b/src/fidesops/service/authentication/authentication_strategy_oauth2.py @@ -8,6 +8,7 @@ from sqlalchemy.orm import Session from fidesops.common_exceptions import FidesopsException, OAuth2TokenException +from fidesops.core.config import config from fidesops.models.authentication_request import AuthenticationRequest from fidesops.models.connectionconfig import ConnectionConfig from fidesops.schemas.saas.saas_config import ClientConfig, SaaSRequest @@ -76,6 +77,24 @@ def add_authentication( request.headers["Authorization"] = "Bearer " + access_token return request + @staticmethod + def _generate_state() -> str: + """ + Generates a string value to associate the authentication request + with an eventual OAuth2 callback response. + + If an oauth_instance name is defined then the name is added as a + prefix to the generated state. The state prefix can be used by a + proxy server to route the callback response to a specific Fidesops + instance. This functionality is not part of the OAuth2 specification + but it can be used for local testing of OAuth2 workflows. + """ + + state = str(uuid4()) + if config.oauth_instance: + state = f"{config.oauth_instance}-{state}" + return state + @staticmethod def _close_to_expiration( expires_at: int, connection_config: ConnectionConfig @@ -223,7 +242,7 @@ def get_authorization_url( self._check_required_secrets(connection_config) # generate the state that will be used to link this authorization request to this connector - state = str(uuid4()) + state = self._generate_state() AuthenticationRequest.create_or_update( db, data={"connection_key": connection_config.key, "state": state} ) diff --git a/tests/ops/api/v1/endpoints/test_oauth_endpoints.py b/tests/ops/api/v1/endpoints/test_oauth_endpoints.py index 2a3180c19a..5ecd0cc452 100644 --- a/tests/ops/api/v1/endpoints/test_oauth_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_oauth_endpoints.py @@ -449,7 +449,7 @@ def callback_url(self) -> str: return V1_URL_PREFIX + OAUTH_CALLBACK def test_callback_for_missing_state(self, db, api_client: TestClient, callback_url): - response = api_client.post( + response = api_client.get( callback_url, params={"code": "abc", "state": "not_found"} ) assert response.status_code == 404 @@ -476,7 +476,7 @@ def test_callback_for_valid_state( "state": "new_request", }, ) - response = api_client.post( + response = api_client.get( callback_url, params={"code": "abc", "state": "new_request"} ) assert response.ok @@ -505,7 +505,7 @@ def test_callback_for_valid_state_with_token_error( "state": "new_request", }, ) - response = api_client.post( + response = api_client.get( callback_url, params={"code": "abc", "state": "new_request"} ) assert response.status_code == 400 diff --git a/tests/ops/service/authentication/test_authentication_strategy_oauth2.py b/tests/ops/service/authentication/test_authentication_strategy_oauth2.py index 5368baa612..47a4fd092a 100644 --- a/tests/ops/service/authentication/test_authentication_strategy_oauth2.py +++ b/tests/ops/service/authentication/test_authentication_strategy_oauth2.py @@ -166,20 +166,22 @@ def test_oauth2_authentication_failed_refresh( class TestAuthorizationUrl: - @mock.patch("fidesops.service.authentication.authentication_strategy_oauth2.uuid4") + @mock.patch( + "fidesops.service.authentication.authentication_strategy_oauth2.OAuth2AuthenticationStrategy._generate_state" + ) @mock.patch( "fidesops.models.authentication_request.AuthenticationRequest.create_or_update" ) def test_get_authorization_url( self, mock_create_or_update: Mock, - mock_uuid: Mock, + mock_state: Mock, db: Session, oauth2_connection_config, oauth2_configuration, ): state = "unique_value" - mock_uuid.return_value = state + mock_state.return_value = state auth_strategy: OAuth2AuthenticationStrategy = get_strategy( "oauth2", oauth2_configuration )