Refactor strategy instantiation for more extensitiliby (#1254)

* Instantiate strategies via abstract Strategy base class

A generalized Strategy abstract base class provides generalized getter methods
that instantiate strategy subclasses (implementations).
These methods rely on the builtin __subclasses__() method to identify Strategy subclasses,
which allows for more dynamic and extensible strategy implementation, removing the need
for a hardcoded enumeration of supported Strategy implementations.
Abstract strategy types inherit from this new abstract base class,
and strategy subclasses (implementations) must provide `name` and `configuration_model` attributes
that are leveraged by new instantiation mechanism in the abstract base class.

* Update get_description() to be a class rather than static method

This allows the method to leverage the new `name` class variable rather than
relying on a static constant variable.

* Remove strategy factories and update references

Strategy factories are no longer needed with refactored Strategy getters.
Update the uses (references) of strategy factories throughout the codebase
to now rely on the new Strategy getters.
Strategy subclasses (implementations) now need to be imported explicitly
in __init__.py's because they used to be imported in factory modules.
Also remove the old MaskingStrategy registration/factory mechanisms.

* Remove strategy name constants

Now that the abstract Strategy base class enforces implementation subclasses
to have a `name` class attribute, this attribute should be relied upon rather than
the arbitrary name constants declared previously.
The get_strategy_name() abstract method is also superfluous, as the `name`
class attribute can be used as a standardized way to retrieve the strategy name.

* Remove get_configuration_model() abstract method

The generalized strategy getter now relies upon the `configuration_model`
class variable that's on each Strategy. Therefore we no longer need the
get_configuration_model() getter on each Strategy subclass.

* Update MaskingStrategy docs with new Strategy functionality

* Update changelog

* Improve recursion in _find_all_strategy_subclasses

* Fix recursion bug when finding all strategies

Update associated tests to make sure the recursion is properly tested

* Tweak conditional for falsy check

* Make get_strategies endpoint test more robust

* Fix typo in documentation

Co-authored-by: Adam Sachs <[email protected]>

CHANGELOG.md

@@ -24,6 +24,7 @@ The types of changes are:
 * Created a docker image for the privacy center [#1165](https://github.com/ethyca/fidesops/pull/1165)
 * Adds email scopes to postman collection [#1241](https://github.com/ethyca/fidesops/pull/1241)
 * Clean up docker build [#1252](https://github.com/ethyca/fidesops/pull/1252)
+* Add `Strategy` abstract base class for more extensible strategy development [1254](https://github.com/ethyca/fidesops/pull/1254)
 
 
 ### Added

docs/fidesops/docs/guides/masking_strategies.md

@@ -169,28 +169,31 @@ strategies available, along with their configuration options.
 ## Extensibility
 
 In fidesops, masking strategies are all built on top of an abstract base class - `MaskingStrategy`.
-`MaskingStrategy` has five methods - `mask`, `secrets_required`, `get_configuration_model`, `get_description`, and `data_type_supported`. For more detail on these
+`MaskingStrategy` has four methods - `mask`, `secrets_required`, `get_description`, and `data_type_supported`. For more detail on these
 methods, visit the class in the fidesops repository. For now, we will focus on the implementation of
 `RandomStringRewriteMaskingStrategy` below:
 
 ```python
 import string
-from typing import Optional
 from secrets import choice
+from typing import List, Optional, Type
 
-from fidesops.ops.schemas.masking.masking_configuration import RandomStringMaskingConfiguration, MaskingConfiguration
-from fidesops.ops.schemas.masking.masking_strategy_description import MaskingStrategyDescription
+from fidesops.ops.schemas.masking.masking_configuration import (
+    RandomStringMaskingConfiguration,
+)
+from fidesops.ops.schemas.masking.masking_strategy_description import (
+    MaskingStrategyConfigurationDescription,
+    MaskingStrategyDescription,
+)
 from fidesops.ops.service.masking.strategy.format_preservation import FormatPreservation
 from fidesops.ops.service.masking.strategy.masking_strategy import MaskingStrategy
-from fidesops.ops.service.masking.strategy.masking_strategy_factory import (
-    MaskingStrategyFactory,
-)
 
-RANDOM_STRING_REWRITE_STRATEGY_NAME = "random_string_rewrite"
 
-@MaskingStrategyFactory.register(RANDOM_STRING_REWRITE_STRATEGY_NAME)
 class RandomStringRewriteMaskingStrategy(MaskingStrategy):
-    """Masks a value with a random string of the length specified in the configuration."""
+    """Masks each provied value with a random string of the length specified in the configuration."""
+
+    name = "random_string_rewrite"
+    configuration_model = RandomStringMaskingConfiguration
 
     def __init__(
         self,
@@ -199,7 +202,9 @@ class RandomStringRewriteMaskingStrategy(MaskingStrategy):
         self.length = configuration.length
         self.format_preservation = configuration.format_preservation
 
-    def mask(self, values: Optional[List[str]], privacy_request_id: Optional[str]) -> Optional[List[str]]:
+    def mask(
+        self, values: Optional[List[str]], request_id: Optional[str]
+    ) -> Optional[List[str]]:
         """Replaces the value with a random lowercase string of the configured length"""
         if values is None:
             return None
@@ -217,12 +222,11 @@ class RandomStringRewriteMaskingStrategy(MaskingStrategy):
             masked_values.append(masked)
         return masked_values
 
-    @staticmethod
-    def get_configuration_model() -> MaskingConfiguration:
+    def secrets_required(self) -> bool:
         """Not covered in this example"""
 
-    @staticmethod
-    def get_description() -> MaskingStrategyDescription:
+    @classmethod
+    def get_description(cls: Type[MaskingStrategy]) -> MaskingStrategyDescription:
         """Not covered in this example"""
 
     @staticmethod
@@ -241,6 +245,4 @@ any defaults that should be applied in their absence. All configuration classes
 
 ### Integrate the masking strategy factory
 
-In order to leverage an implemented masking strategy, the `MaskingStrategy` subclass must be registered with the `MaskingStrategyFactory`. To register a new `MaskingStrategy`, use the `register` decorator on the `MaskingStrategy` subclass definition, as shown in the above example.
-
-The value passed as the argument to the decorator must be the registered name of the `MaskingStrategy` subclass. This is the same value defined by [callers](#using-fidesops-as-a-masking-service) in the `"masking_strategy"."strategy"` field.
+In order to leverage an implemented masking strategy, the `MaskingStrategy` subclass must be imported into the application runtime. Also, the `MaskingStrategy` class must define two class variables: `name`, which is the unique, registered name that [callers](#using-fidesops-as-a-masking-service) will use in their `"masking_strategy"."strategy"` field to invoke the strategy; and `configuration_model`, which references the configuration class used to parameterize the strategy.

src/fidesops/ops/api/v1/endpoints/masking_endpoints.py

@@ -5,18 +5,15 @@
 from starlette.status import HTTP_400_BAD_REQUEST, HTTP_404_NOT_FOUND
 
 from fidesops.ops.api.v1.urn_registry import MASKING, MASKING_STRATEGY, V1_URL_PREFIX
-from fidesops.ops.common_exceptions import ValidationError
+from fidesops.ops.common_exceptions import NoSuchStrategyException, ValidationError
 from fidesops.ops.schemas.masking.masking_api import (
     MaskingAPIRequest,
     MaskingAPIResponse,
 )
 from fidesops.ops.schemas.masking.masking_strategy_description import (
     MaskingStrategyDescription,
 )
-from fidesops.ops.service.masking.strategy.masking_strategy_factory import (
-    MaskingStrategyFactory,
-    NoSuchStrategyException,
-)
+from fidesops.ops.service.masking.strategy.masking_strategy import MaskingStrategy
 from fidesops.ops.util.api_router import APIRouter
 
 router = APIRouter(tags=["Masking"], prefix=V1_URL_PREFIX)
@@ -30,7 +27,7 @@ def mask_value(request: MaskingAPIRequest) -> MaskingAPIResponse:
     try:
         values = request.values
         masking_strategy = request.masking_strategy
-        strategy = MaskingStrategyFactory.get_strategy(
+        strategy = MaskingStrategy.get_strategy(
             masking_strategy.strategy, masking_strategy.configuration
         )
         logger.info(
@@ -52,7 +49,4 @@ def mask_value(request: MaskingAPIRequest) -> MaskingAPIResponse:
 def list_masking_strategies() -> List[MaskingStrategyDescription]:
     """Lists available masking strategies with instructions on how to use them"""
     logger.info("Getting available masking strategies")
-    return [
-        strategy.get_description()
-        for strategy in MaskingStrategyFactory.get_strategies()
-    ]
+    return [strategy.get_description() for strategy in MaskingStrategy.get_strategies()]

src/fidesops/ops/api/v1/endpoints/oauth_endpoints.py

@@ -42,8 +42,8 @@
 from fidesops.ops.models.authentication_request import AuthenticationRequest
 from fidesops.ops.models.connectionconfig import ConnectionConfig
 from fidesops.ops.schemas.client import ClientCreatedResponse
-from fidesops.ops.service.authentication.authentication_strategy_factory import (
-    get_strategy,
+from fidesops.ops.service.authentication.authentication_strategy import (
+    AuthenticationStrategy,
 )
 from fidesops.ops.service.authentication.authentication_strategy_oauth2_authorization_code import (
     OAuth2AuthorizationCodeAuthenticationStrategy,
@@ -214,7 +214,7 @@ def oauth_callback(code: str, state: str, db: Session = Depends(get_db)) -> None
         authentication = (
             connection_config.get_saas_config().client_config.authentication  # type: ignore
         )
-        auth_strategy: OAuth2AuthorizationCodeAuthenticationStrategy = get_strategy(  # type: ignore
+        auth_strategy: OAuth2AuthorizationCodeAuthenticationStrategy = AuthenticationStrategy.get_strategy(  # type: ignore
             authentication.strategy, authentication.configuration  # type: ignore
         )
         connection_config.secrets = {**connection_config.secrets, "code": code}  # type: ignore

src/fidesops/ops/api/v1/endpoints/saas_config_endpoints.py

@@ -44,8 +44,8 @@
     ValidateSaaSConfigResponse,
 )
 from fidesops.ops.schemas.shared_schemas import FidesOpsKey
-from fidesops.ops.service.authentication.authentication_strategy_factory import (
-    get_strategy,
+from fidesops.ops.service.authentication.authentication_strategy import (
+    AuthenticationStrategy,
 )
 from fidesops.ops.service.authentication.authentication_strategy_oauth2_authorization_code import (
     OAuth2AuthorizationCodeAuthenticationStrategy,
@@ -113,10 +113,7 @@ def verify_oauth_connection_config(
             detail="The connection config does not contain an authentication configuration.",
         )
 
-    if (
-        authentication.strategy
-        != OAuth2AuthorizationCodeAuthenticationStrategy.strategy_name
-    ):
+    if authentication.strategy != OAuth2AuthorizationCodeAuthenticationStrategy.name:
         raise HTTPException(
             status_code=HTTP_422_UNPROCESSABLE_ENTITY,
             detail="The connection config does not use OAuth2 Authorization Code authentication.",
@@ -262,7 +259,7 @@ def authorize_connection(
     authentication = connection_config.get_saas_config().client_config.authentication  # type: ignore
 
     try:
-        auth_strategy: OAuth2AuthorizationCodeAuthenticationStrategy = get_strategy(
+        auth_strategy: OAuth2AuthorizationCodeAuthenticationStrategy = AuthenticationStrategy.get_strategy(
             authentication.strategy, authentication.configuration  # type: ignore
         )
         return auth_strategy.get_authorization_url(db, connection_config)

src/fidesops/ops/migrations/versions/55d61eb8ed12_add_default_policies.py

@@ -30,9 +30,6 @@
 from fidesops.ops.db.base_class import JSONTypeOverride
 from fidesops.ops.models.policy import ActionType, DrpAction
 from fidesops.ops.schemas.storage.storage import StorageType
-from fidesops.ops.service.masking.strategy.masking_strategy_string_rewrite import (
-    STRING_REWRITE_STRATEGY_NAME,
-)
 from fidesops.ops.util.data_category import DataCategory
 
 logging.basicConfig()
@@ -48,6 +45,7 @@
 FIDESOPS_AUTOGENERATED_STORAGE_KEY = "fidesops_autogenerated_storage_destination"
 AUTOGENERATED_ACCESS_KEY = "download"
 AUTOGENERATED_ERASURE_KEY = "delete"
+STRING_REWRITE_STRATEGY_NAME = "string_rewrite"
 
 client_select_query: TextClause = text(
     """SELECT client.id FROM client WHERE fides_key = :fides_key"""

src/fidesops/ops/schemas/saas/saas_config.py

@@ -116,13 +116,13 @@ def validate_request_for_pagination(cls, values: Dict[str, Any]) -> Dict[str, An
         """
 
         # delay import to avoid cyclic-dependency error - We still ignore the pylint error
-        from fidesops.ops.service.pagination.pagination_strategy_factory import (  # pylint: disable=R0401
-            get_strategy,
+        from fidesops.ops.service.pagination.pagination_strategy import (  # pylint: disable=R0401
+            PaginationStrategy,
         )
 
         pagination = values.get("pagination")
         if pagination is not None:
-            pagination_strategy = get_strategy(
+            pagination_strategy = PaginationStrategy.get_strategy(
                 pagination.get("strategy"), pagination.get("configuration")
             )
             pagination_strategy.validate_request(values)

src/fidesops/ops/service/authentication/__init__.py

@@ -0,0 +1,7 @@
+from fidesops.ops.service.authentication import (
+    authentication_strategy_basic,
+    authentication_strategy_bearer,
+    authentication_strategy_oauth2_authorization_code,
+    authentication_strategy_oauth2_client_credentials,
+    authentication_strategy_query_param,
+)

src/fidesops/ops/service/authentication/authentication_strategy.py

@@ -1,21 +1,16 @@
-from abc import ABC, abstractmethod
+from abc import abstractmethod
 
 from requests import PreparedRequest
 
 from fidesops.ops.models.connectionconfig import ConnectionConfig
-from fidesops.ops.schemas.saas.strategy_configuration import StrategyConfiguration
+from fidesops.ops.service.strategy import Strategy
 
 
-class AuthenticationStrategy(ABC):
+class AuthenticationStrategy(Strategy):
     """Abstract base class for SaaS authentication strategies"""
 
     @abstractmethod
     def add_authentication(
         self, request: PreparedRequest, connection_config: ConnectionConfig
     ) -> PreparedRequest:
         """Add authentication to the request"""
-
-    @staticmethod
-    @abstractmethod
-    def get_configuration_model() -> StrategyConfiguration:
-        """Used to get the configuration model to configure the strategy"""

src/fidesops/ops/service/authentication/authentication_strategy_basic.py

@@ -3,7 +3,6 @@
 from fidesops.ops.models.connectionconfig import ConnectionConfig
 from fidesops.ops.schemas.saas.strategy_configuration import (
     BasicAuthenticationConfiguration,
-    StrategyConfiguration,
 )
 from fidesops.ops.service.authentication.authentication_strategy import (
     AuthenticationStrategy,
@@ -17,7 +16,8 @@ class BasicAuthenticationStrategy(AuthenticationStrategy):
     and uses them to add a basic authentication header to the incoming request.
     """
 
-    strategy_name = "basic"
+    name = "basic"
+    configuration_model = BasicAuthenticationConfiguration
 
     def __init__(self, configuration: BasicAuthenticationConfiguration):
         self.username = configuration.username
@@ -36,7 +36,3 @@ def add_authentication(
             )
         )
         return request
-
-    @staticmethod
-    def get_configuration_model() -> StrategyConfiguration:
-        return BasicAuthenticationConfiguration  # type: ignore

src/fidesops/ops/service/authentication/authentication_strategy_bearer.py

@@ -3,7 +3,6 @@
 from fidesops.ops.models.connectionconfig import ConnectionConfig
 from fidesops.ops.schemas.saas.strategy_configuration import (
     BearerAuthenticationConfiguration,
-    StrategyConfiguration,
 )
 from fidesops.ops.service.authentication.authentication_strategy import (
     AuthenticationStrategy,
@@ -17,7 +16,8 @@ class BearerAuthenticationStrategy(AuthenticationStrategy):
     and uses it to add a bearer authentication header to the incoming request.
     """
 
-    strategy_name = "bearer"
+    name = "bearer"
+    configuration_model = BearerAuthenticationConfiguration
 
     def __init__(self, configuration: BearerAuthenticationConfiguration):
         self.token = configuration.token
@@ -30,7 +30,3 @@ def add_authentication(
             self.token, connection_config.secrets  # type: ignore
         )
         return request
-
-    @staticmethod
-    def get_configuration_model() -> StrategyConfiguration:
-        return BearerAuthenticationConfiguration  # type: ignore