fix: address code review findings for DPoP implementation

- Fix redundant get_dpop_error_message() call in oauth.py
- Fix env var coercion for dpopEnabled/dpopKeyRotationInterval in config_validator
- Fix test isolation bug: reset _access_token_dpop_warned in setUp
- Fix request_executor timeout returning raw string instead of Exception
- Use LOGGER_NAME constant consistently across cache, jwt modules
- Narrow except clauses to specific exception types
- Improve type hints, docstrings, and variable

Author: BinoyOza-okta

okta/api_client.py

@@ -95,7 +95,11 @@ def __init__(
             "authorization_mode": configuration["client"].get("authorizationMode", "SSWS"),
         }
 
-        # Add DPoP parameters if enabled
+        # Store DPoP parameters in Configuration for completeness.
+        # NOTE: DPoP proof generation and header injection are handled
+        # entirely by RequestExecutor → OAuth → DPoPProofGenerator.
+        # The Configuration object stores these values but they are NOT
+        # consumed by the synchronous urllib3/RESTClientObject path.
         if configuration["client"].get("dpopEnabled", False):
             config_params.update({
                 "dpop_enabled": True,

okta/cache/no_op_cache.py

@@ -17,15 +17,17 @@ class NoOpCache(Cache):
     in the cache.
     Implementing the okta.cache.cache.Cache abstract class.
 
-    .. warning::
-        **DPoP Performance Impact**: When using DPoP (Demonstrating Proof-of-Possession)
-        authentication with NoOpCache, OAuth tokens will be regenerated on every request
-        instead of being cached. This may significantly impact performance and could
-        trigger rate limits. Consider using OktaCache instead for production DPoP usage.
+    .. note::
+        **DPoP with NoOpCache**: When using NoOpCache, the ``OKTA_ACCESS_TOKEN``
+        cache key is not persisted between requests.  However, the OAuth class
+        maintains its own internal token state, so tokens are still reused
+        across requests without hitting the authorization server each time.
+        For production use with DPoP, enabling OktaCache provides an additional
+        layer of caching that avoids redundant ``get_oauth_token()`` calls.
     """
 
     def __init__(self):
-        super()
+        super().__init__()
 
     def get(self, key):
         """

okta/cache/okta_cache.py

@@ -12,8 +12,9 @@
 import time
 
 from okta.cache.cache import Cache
+from okta.constants import LOGGER_NAME
 
-logger = logging.getLogger("okta-sdk-python")
+logger = logging.getLogger(LOGGER_NAME)
 
 
 class OktaCache(Cache):
@@ -46,7 +47,7 @@ def __init__(self, ttl, tti):
             ttl {float} -- Time to Live: for cache entries
             tti {float} -- Time to Idle: for cache entries
         """
-        super()  # Inherit from parent class
+        super().__init__()  # Inherit from parent class
         self._store = {}  # key -> {value, TTI, TTL}
         self._time_to_live = ttl
         self._time_to_idle = tti
@@ -71,8 +72,7 @@ def get(self, key):
             entry["tti"] = now + self._time_to_idle
             # Return desired value and update cache
             self._clean_cache()
-            logger.info(f'Got value from cache for key "{key}".')
-            logger.debug(f'Cached value for key {key}: {entry["value"]}')
+            logger.info('Got value from cache for key "%s".', key)
             return entry["value"]
 
         # Return None if key isn't in cache and update cache
@@ -91,13 +91,14 @@ def contains(self, key):
         """
         return key in self._store and self._is_valid_entry(self._store[key])
 
-    def add(self, key: str, value: tuple):
+    def add(self, key: str, value):
         """
         Adds a key-value pair to the cache.
 
         Arguments:
             key {str} -- Key in pair
-            value {tuple} -- Tuple of response and response body
+            value -- Value to cache (e.g., tuple of response and body,
+                     or tuple of access_token and token_type)
         """
         if isinstance(key, str) and (
             not isinstance(value, list) or not isinstance(value[1], list)
@@ -111,8 +112,7 @@ def add(self, key: str, value: tuple):
                 "tti": now + self._time_to_idle,
                 "ttl": now + self._time_to_live,
             }
-            logger.info(f'Added to cache value for key "{key}".')
-            logger.debug(f"Cached value for key {key}: {value}.")
+            logger.info('Added to cache value for key "%s".', key)
         # Update cache
         self._clean_cache()
 
@@ -127,7 +127,7 @@ def delete(self, key):
         if key in self._store:
             # Delete entry
             del self._store[key]
-            logger.info(f'Removed value from cache for key "{key}".')
+            logger.info('Removed value from cache for key "%s".', key)
 
     def clear(self):
         """

okta/config/config_setter.py

@@ -41,6 +41,8 @@ class ConfigSetter:
             "proxy": {"port": "", "host": "", "username": "", "password": ""},
             "rateLimit": {"maxRetries": ""},
             "oauthTokenRenewalOffset": "",
+            "dpopEnabled": "",
+            "dpopKeyRotationInterval": "",
         },
         "testing": {"testingDisableHttpsCheck": ""},
     }

okta/config/config_validator.py

@@ -10,7 +10,10 @@
 
 import logging
 
-from okta.constants import FINDING_OKTA_DOMAIN, REPO_URL, MIN_DPOP_KEY_ROTATION_SECONDS, MAX_DPOP_KEY_ROTATION_SECONDS
+from okta.constants import (
+    FINDING_OKTA_DOMAIN, LOGGER_NAME, REPO_URL,
+    MIN_DPOP_KEY_ROTATION_SECONDS, MAX_DPOP_KEY_ROTATION_SECONDS,
+)
 from okta.error_messages import (
     ERROR_MESSAGE_ORG_URL_MISSING,
     ERROR_MESSAGE_API_TOKEN_DEFAULT,
@@ -28,7 +31,7 @@
     ERROR_MESSAGE_PROXY_INVALID_PORT,
 )
 
-logger = logging.getLogger("okta-sdk-python")
+logger = logging.getLogger(LOGGER_NAME)
 
 
 class ConfigValidator:
@@ -55,7 +58,6 @@ def validate_config(self):
         errors = []
         # Defensive: default to empty dict if sections are missing
         client = self._config.get("client", {})
-        _ = self._config.get("testing", {})
 
         # check org url
         errors += self._validate_org_url(client.get("orgUrl", ""))
@@ -65,6 +67,14 @@ def validate_config(self):
         # check API details based on authorization mode
         if client.get("authorizationMode") in ("SSWS", "Bearer"):
             errors += self._validate_token(client.get("token", ""))
+            # Warn if DPoP is enabled with a non-OAuth auth mode (it has no effect)
+            if client.get("dpopEnabled", False):
+                logger.warning(
+                    "dpopEnabled is True but authorizationMode is '%s'. "
+                    "DPoP only applies to 'PrivateKey' (OAuth) mode and will "
+                    "be ignored. Set authorizationMode to 'PrivateKey' to use DPoP.",
+                    client.get("authorizationMode"),
+                )
         elif client.get("authorizationMode") == "PrivateKey":
             client_fields = [
                 "clientId",
@@ -237,49 +247,89 @@ def _validate_dpop_config(self, client):
         """
         Validate DPoP-specific configuration.
 
+        Coerces string values from environment variables to their expected
+        types (bool for ``dpopEnabled``, int for ``dpopKeyRotationInterval``)
+        before validation.  The coerced values are written back into *client*
+        so that downstream code receives the correct Python types.
+
         Note: This method is only called when authorizationMode is 'PrivateKey',
         so no need to re-check the auth mode here.
 
         Args:
-            client: Client configuration dict
+            client (dict): Client configuration dict (mutated in-place for
+                type coercion of string values from environment variables).
 
         Returns:
             list: List of error messages (empty if valid)
         """
 
         errors = []
 
-        if not client.get('dpopEnabled'):
+        dpop_enabled = client.get('dpopEnabled', False)
+
+        # Coerce string from env vars to bool (e.g. "true" → True)
+        if isinstance(dpop_enabled, str):
+            dpop_enabled = dpop_enabled.strip().lower() == 'true'
+            client['dpopEnabled'] = dpop_enabled
+
+        # Validate dpopEnabled is a boolean
+        if not isinstance(dpop_enabled, bool):
+            errors.append(
+                "dpopEnabled must be a boolean (True/False), "
+                f"but got {type(dpop_enabled).__name__}: {dpop_enabled!r}"
+            )
+            return errors  # Cannot validate further if type is wrong
+
+        if not dpop_enabled:
             return errors  # DPoP not enabled, nothing to validate
 
         # Validate key rotation interval
         rotation_interval = client.get('dpopKeyRotationInterval', 86400)
 
+        # Coerce string from env vars to int (e.g. "86400" → 86400)
+        if isinstance(rotation_interval, str):
+            try:
+                rotation_interval = int(rotation_interval)
+                client['dpopKeyRotationInterval'] = rotation_interval
+            except ValueError:
+                errors.append(
+                    "dpopKeyRotationInterval must be a valid integer "
+                    f"(seconds), but got non-numeric string: "
+                    f"{rotation_interval!r}"
+                )
+                return errors
+
         if not isinstance(rotation_interval, int):
             errors.append(
-                f"dpopKeyRotationInterval must be an integer (seconds), "
+                "dpopKeyRotationInterval must be an integer (seconds), "
                 f"but got {type(rotation_interval).__name__}"
             )
-        elif rotation_interval < MIN_DPOP_KEY_ROTATION_SECONDS:  # Minimum 1 hour
+        elif rotation_interval < MIN_DPOP_KEY_ROTATION_SECONDS:
             errors.append(
-                f"dpopKeyRotationInterval must be at least {MIN_DPOP_KEY_ROTATION_SECONDS} seconds (1 hour), "
+                "dpopKeyRotationInterval must be at least "
+                f"{MIN_DPOP_KEY_ROTATION_SECONDS} seconds (1 hour), "
                 f"but got {rotation_interval} seconds. "
                 "Shorter intervals may cause performance issues."
             )
-        elif rotation_interval > MAX_DPOP_KEY_ROTATION_SECONDS:  # Maximum 90 days
+        elif rotation_interval > MAX_DPOP_KEY_ROTATION_SECONDS:
+            max_days = MAX_DPOP_KEY_ROTATION_SECONDS // 86400
+            given_days = rotation_interval // 86400
             errors.append(
-                f"dpopKeyRotationInterval must be at most {MAX_DPOP_KEY_ROTATION_SECONDS} seconds "
-                f"({MAX_DPOP_KEY_ROTATION_SECONDS // 86400} days), "
-                f"but got {rotation_interval} seconds ({rotation_interval // 86400} days). "
-                "Excessive rotation intervals defeat the security purpose of DPoP. "
+                "dpopKeyRotationInterval must be at most "
+                f"{MAX_DPOP_KEY_ROTATION_SECONDS} seconds "
+                f"({max_days} days), but got {rotation_interval} "
+                f"seconds ({given_days} days). "
+                "Excessive rotation intervals defeat the security "
+                "purpose of DPoP. "
                 "Recommended: 24-48 hours for production use."
             )
         elif rotation_interval > 7 * 24 * 3600:  # Warning for > 7 days
             # This is a warning, not an error
             logger.warning(
-                f"dpopKeyRotationInterval is very long ({rotation_interval} seconds, "
-                f"{rotation_interval // 86400} days). "
-                "Consider shorter intervals (24-48 hours) for better security."
+                "dpopKeyRotationInterval is very long (%d seconds, "
+                "%d days). Consider shorter intervals (24-48 hours) "
+                "for better security.",
+                rotation_interval, rotation_interval // 86400,
             )
 
         return errors

okta/constants.py

@@ -35,3 +35,6 @@
 MAX_DPOP_NONCE_RETRIES = 2
 MAX_DPOP_BACKOFF_DELAY = 1.0  # Maximum backoff delay in seconds for nonce retries
 DPOP_USER_AGENT_EXTENSION = "isDPoP:true"
+
+# SDK-wide logger name — single source of truth for all hand-written modules
+LOGGER_NAME = "okta-sdk-python"