Merge pull request #370 from NHSDigital/10291-logout-cis2--II

10291 CIS2 refactors and improvements

Author: malcolmbaig

Dockerfile

@@ -20,6 +20,8 @@ ARG poetry_version
 
 WORKDIR /app
 
+RUN apk add --no-cache libgcc libstdc++ build-base
+
 # Set environment variables
 ENV PYTHONDONTWRITEBYTECODE=1
 ENV PYTHONUNBUFFERED=1

docs/cis2_auth.md

@@ -40,8 +40,8 @@ The following settings in `manage_breast_screening/config/settings.py` provide t
 
 - **`CIS2_SERVER_METADATA_URL`** - URL to CIS2's OpenID Connect discovery document
 - **`CIS2_CLIENT_ID`** - Client identifier registered with CIS2
-- **`CIS2_PRIVATE_KEY`** - RSA private key in PEM format for JWT signing (supports `\n` escaped newlines)
-- **`CIS2_PUBLIC_KEY`** - Corresponding RSA public key in PEM format (supports `\n` escaped newlines)
+- **`CIS2_CLIENT_PRIVATE_KEY`** - RSA private key in PEM format for JWT signing (supports `\n` escaped newlines)
+- **`CIS2_CLIENT_PUBLIC_KEY`** - Corresponding RSA public key in PEM format (supports `\n` escaped newlines)
 - **`CIS2_SCOPES`** - OAuth scopes requested
 
 The private and public keys form a keypair used for the `private_key_jwt` client authentication method.

manage_breast_screening/auth/oauth.py

@@ -15,22 +15,27 @@ def get_cis2_client():
         name
         for name in [
             "CIS2_CLIENT_ID",
-            "CIS2_PRIVATE_KEY",
-            "CIS2_PUBLIC_KEY",
+            "CIS2_CLIENT_PRIVATE_KEY",
+            "CIS2_CLIENT_PUBLIC_KEY",
             "CIS2_SERVER_METADATA_URL",
         ]
         if not getattr(settings, name, None)
     ]
     if missing:
         raise ValueError(f"Missing required CIS2 OAuth settings: {', '.join(missing)}")
 
+    # Return existing client if already registered
+    client = oauth._clients.get("cis2")
+    if client:
+        return client
+
     jwk = jwk_from_public_key()
     kid = jwk.thumbprint()
 
     client = oauth.register(
         "cis2",
         client_id=settings.CIS2_CLIENT_ID,
-        client_secret=settings.CIS2_PRIVATE_KEY,
+        client_secret=settings.CIS2_CLIENT_PRIVATE_KEY,
         server_metadata_url=settings.CIS2_SERVER_METADATA_URL,
         client_kwargs={
             "scope": settings.CIS2_SCOPES,
@@ -68,7 +73,7 @@ def jwk_from_public_key():
     Returns:
         JsonWebKey | None: Public JWK or None on failure.
     """
-    jwk = JsonWebKey.import_key(settings.CIS2_PUBLIC_KEY, {"kty": "RSA"})
+    jwk = JsonWebKey.import_key(settings.CIS2_CLIENT_PUBLIC_KEY, {"kty": "RSA"})
     return jwk
 
 

manage_breast_screening/auth/services.py

@@ -8,64 +8,52 @@
     InvalidTokenError,
     MissingClaimError,
 )
+from django.conf import settings
 
 logger = logging.getLogger(__name__)
 
 
 class InvalidLogoutToken(Exception):
     """Raised when a CIS2 back-channel logout token is invalid."""
 
-    def __init__(self, cause=None):
-        """
-        Initialize with optional cause.
 
-        Args:
-            cause: The original exception that caused this error
-        """
-        self.cause = cause
-        super().__init__(str(cause) if cause else "Invalid logout token")
-
-
-class DecodeLogoutToken:
-    def call(
-        self,
-        metadata: Dict[str, Any],
-        logout_token: str,
-        client_id: str,
-        key_loader,
-    ):
-        """
-        Decode and validate a CIS2 back-channel logout token.
-
-        Returns the decoded claims on success and raises InvalidLogoutToken on
-        any underlying decoding/validation error.
-        """
-        try:
-            verification_rules = {
-                "iss": {"values": [metadata["issuer"]], "essential": True},
-                "aud": {"values": [client_id], "essential": True},
-                "exp": {"essential": True},
-                "iat": {"essential": True},
-                "events": {
-                    "essential": True,
-                    "validate": lambda claim, value: isinstance(value, dict)
-                    and "http://schemas.openid.net/event/backchannel-logout" in value,
-                },
-                "nonce": {"validate": lambda claim, value: value is None},
-            }
-            claims = jwt.decode(
-                logout_token,
-                key=key_loader,
-                claims_options=verification_rules,
-            )
-            claims.validate(leeway=60)
-            return claims
-        except (
-            ExpiredTokenError,
-            InvalidClaimError,
-            InvalidTokenError,
-            JoseError,
-            MissingClaimError,
-        ) as e:
-            logger.exception("Invalid logout token")
-            raise InvalidLogoutToken(cause=e) from e
+def decode_logout_token(
+    issuer: str,
+    key_loader,
+    logout_token: str,
+) -> Dict[str, Any]:
+    """
+    Decode and validate a CIS2 back-channel logout token.
+
+    Returns the decoded claims on success and raises InvalidLogoutToken on
+    any underlying decoding/validation error.
+    """
+    try:
+        verification_rules = {
+            "iss": {"values": [issuer], "essential": True},
+            "aud": {"values": [settings.CIS2_CLIENT_ID], "essential": True},
+            "exp": {"essential": True},
+            "iat": {"essential": True},
+            "events": {
+                "essential": True,
+                "validate": lambda claim, value: isinstance(value, dict)
+                and "http://schemas.openid.net/event/backchannel-logout" in value,
+            },
+            "nonce": {"validate": lambda claim, value: value is None},
+        }
+        claims = jwt.decode(
+            logout_token,
+            key=key_loader,
+            claims_options=verification_rules,
+        )
+        claims.validate(leeway=60)
+        return claims
+    except (
+        ExpiredTokenError,
+        InvalidClaimError,
+        InvalidTokenError,
+        JoseError,
+        MissingClaimError,
+    ) as e:
+        logger.exception("Invalid logout token")
+        raise InvalidLogoutToken() from e

manage_breast_screening/auth/tests/test_services.py

@@ -8,10 +8,11 @@
     JoseError,
     MissingClaimError,
 )
+from django.conf import settings
 
 from manage_breast_screening.auth.services import (
-    DecodeLogoutToken,
     InvalidLogoutToken,
+    decode_logout_token,
 )
 
 
@@ -30,13 +31,12 @@ def _make_token(
         private_jwk: dict,
         kid: str,
         issuer: str,
-        client_id: str,
         overrides: dict | None = None,
     ) -> str:
         now = int(time.time())
         payload = {
             "iss": issuer,
-            "aud": client_id,
+            "aud": settings.CIS2_CLIENT_ID,
             "iat": now,
             "exp": now + 300,
             "events": {"http://schemas.openid.net/event/backchannel-logout": {}},
@@ -57,28 +57,24 @@ def _make_token(
 
     @staticmethod
     def _key_loader(public_jwk: dict):
-        def loader(headers, payload):
+        def loader(_headers, _payload):
             return public_jwk
 
         return loader
 
     def test_valid_token_returns_claims(self):
         kid = "k1"
-        issuer = "test-issuer"
-        client_id = "client-1"
         private_jwk, public_jwk = self._make_keys(kid)
-        token = self._make_token(private_jwk, kid, issuer, client_id)
-
-        service = DecodeLogoutToken()
-        claims = service.call(
-            metadata={"issuer": issuer},
-            logout_token=token,
-            client_id=client_id,
-            key_loader=self._key_loader(public_jwk),
+        token = self._make_token(private_jwk, kid, "test-issuer")
+
+        claims = decode_logout_token(
+            "test-issuer",
+            self._key_loader(public_jwk),
+            token,
         )
 
-        assert claims["iss"] == issuer
-        assert claims["aud"] == client_id
+        assert claims["iss"] == "test-issuer"
+        assert claims["aud"] == settings.CIS2_CLIENT_ID
         assert claims["sub"] == "user-123"
         assert "http://schemas.openid.net/event/backchannel-logout" in claims["events"]
 
@@ -108,95 +104,71 @@ def test_invalid_claims_raise_error(
         self, overrides, expected_error_type, expected_error_text
     ):
         kid = "k1"
-        issuer = "test-issuer"
-        client_id = "client-1"
         private_jwk, public_jwk = self._make_keys(kid)
-        token = self._make_token(
-            private_jwk, kid, issuer, client_id, overrides=overrides
-        )
+        token = self._make_token(private_jwk, kid, "test-issuer", overrides=overrides)
 
-        service = DecodeLogoutToken()
         with pytest.raises(InvalidLogoutToken) as excinfo:
-            service.call(
-                metadata={"issuer": issuer},
-                logout_token=token,
-                client_id=client_id,
-                key_loader=self._key_loader(public_jwk),
+            decode_logout_token(
+                "test-issuer",
+                self._key_loader(public_jwk),
+                token,
             )
 
-        # Assert on the cause type and error message content
-        assert isinstance(excinfo.value.cause, expected_error_type)
-        assert expected_error_text in str(excinfo.value.cause)
+        assert isinstance(excinfo.value.__cause__, expected_error_type)
+        assert expected_error_text in str(excinfo.value.__cause__)
 
     def test_invalid_signature_raises_error(self):
         kid = "k1"
-        issuer = "test-issuer"
-        client_id = "client-1"
         # Create two different key pairs
         private_jwk_1, public_jwk_1 = self._make_keys(kid)
         private_jwk_2, _public_jwk_2 = self._make_keys(kid)
         # Sign with private_jwk_2 but verify with public_jwk_1 -> invalid signature
-        token = self._make_token(private_jwk_2, kid, issuer, client_id)
+        token = self._make_token(private_jwk_2, kid, "test-issuer")
 
-        service = DecodeLogoutToken()
         with pytest.raises(InvalidLogoutToken) as excinfo:
-            service.call(
-                metadata={"issuer": issuer},
-                logout_token=token,
-                client_id=client_id,
-                key_loader=self._key_loader(public_jwk_1),
+            decode_logout_token(
+                "test-issuer",
+                self._key_loader(public_jwk_1),
+                token,
             )
 
-        # Invalid signature should raise a JoseError
-        assert isinstance(excinfo.value.cause, JoseError)
-        assert "signature" in str(excinfo.value.cause).lower()
+        assert isinstance(excinfo.value.__cause__, JoseError)
+        assert "signature" in str(excinfo.value.__cause__)
 
     def test_expired_token_raises_error(self):
         kid = "k1"
-        issuer = "test-issuer"
-        client_id = "client-1"
         private_jwk, public_jwk = self._make_keys(kid)
         now = int(time.time())
         token = self._make_token(
             private_jwk,
             kid,
-            issuer,
-            client_id,
+            "test-issuer",
             overrides={"exp": now - 120},  # already expired beyond leeway
         )
 
-        service = DecodeLogoutToken()
         with pytest.raises(InvalidLogoutToken) as excinfo:
-            service.call(
-                metadata={"issuer": issuer},
-                logout_token=token,
-                client_id=client_id,
-                key_loader=self._key_loader(public_jwk),
+            decode_logout_token(
+                "test-issuer",
+                self._key_loader(public_jwk),
+                token,
             )
-
-        # Expired token should raise an ExpiredTokenError
-        assert isinstance(excinfo.value.cause, ExpiredTokenError)
-        assert "expired" in str(excinfo.value.cause).lower()
+        assert isinstance(excinfo.value.__cause__, ExpiredTokenError)
+        assert "expired" in str(excinfo.value.__cause__)
 
     def test_missing_iat_raises_error(self):
         kid = "k1"
-        issuer = "test-issuer"
-        client_id = "client-1"
         private_jwk, public_jwk = self._make_keys(kid)
         # Use overrides to remove the iat claim
         token = self._make_token(
-            private_jwk, kid, issuer, client_id, overrides={"iat": None}
+            private_jwk, kid, "test-issuer", overrides={"iat": None}
         )
 
-        service = DecodeLogoutToken()
         with pytest.raises(InvalidLogoutToken) as excinfo:
-            service.call(
-                metadata={"issuer": issuer},
-                logout_token=token,
-                client_id=client_id,
-                key_loader=self._key_loader(public_jwk),
+            decode_logout_token(
+                "test-issuer",
+                self._key_loader(public_jwk),
+                token,
             )
 
-        # Missing iat should raise a MissingClaimError
-        assert isinstance(excinfo.value.cause, MissingClaimError)
-        assert "iat" in str(excinfo.value.cause).lower()
+        assert isinstance(excinfo.value.__cause__, MissingClaimError)
+        assert "iat" in str(excinfo.value.__cause__)