diff --git a/changelog.d/3-bug-fixes/pr-2400 b/changelog.d/3-bug-fixes/pr-2400 new file mode 100644 index 0000000000..7e8054bf95 --- /dev/null +++ b/changelog.d/3-bug-fixes/pr-2400 @@ -0,0 +1 @@ +When an IdP issuer (aka entity ID) is updated, the old issuer was still marked as "in use". diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index d1c81bbf58..66200ec4d1 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -685,9 +685,12 @@ idpUpdateXML zusr raw idpmeta idpid = withDebugLog "idpUpdate" (Just . show . (^ GalleyAccess.assertSSOEnabled teamid IdPRawMetadataStore.store (idp ^. SAML.idpId) raw -- (if raw metadata is stored and then spar goes out, raw metadata won't match the - -- structured idp config. since this will lead to a 5xx response, the client is epected to + -- structured idp config. since this will lead to a 5xx response, the client is expected to -- try again, which would clean up cassandra state.) storeIdPConfig idp + -- if the IdP issuer is updated, the old issuer must be removed explicitly. + -- if this step is ommitted (due to a crash) resending the update request should fix the inconsistent state. + forM_ (idp ^. SAML.idpExtraInfo . wiOldIssuers) IdPConfigStore.deleteIssuer pure idp -- | Check that: idp id is valid; calling user is admin in that idp's home team; team id in diff --git a/services/spar/src/Spar/Sem/IdPConfigStore.hs b/services/spar/src/Spar/Sem/IdPConfigStore.hs index 68e2f29924..630c71d01e 100644 --- a/services/spar/src/Spar/Sem/IdPConfigStore.hs +++ b/services/spar/src/Spar/Sem/IdPConfigStore.hs @@ -30,6 +30,7 @@ module Spar.Sem.IdPConfigStore deleteConfig, setReplacedBy, clearReplacedBy, + deleteIssuer, ) where @@ -70,6 +71,7 @@ data IdPConfigStore m a where -- affects _wiReplacedBy in GetConfig SetReplacedBy :: Replaced -> Replacing -> IdPConfigStore m () ClearReplacedBy :: Replaced -> IdPConfigStore m () + DeleteIssuer :: SAML.Issuer -> IdPConfigStore m () deriving stock instance Show (IdPConfigStore m a) diff --git a/services/spar/src/Spar/Sem/IdPConfigStore/Cassandra.hs b/services/spar/src/Spar/Sem/IdPConfigStore/Cassandra.hs index 15623a7627..65f4819f17 100644 --- a/services/spar/src/Spar/Sem/IdPConfigStore/Cassandra.hs +++ b/services/spar/src/Spar/Sem/IdPConfigStore/Cassandra.hs @@ -56,6 +56,7 @@ idPToCassandra = in deleteIdPConfig idpid issuer team SetReplacedBy r r11 -> setReplacedBy r r11 ClearReplacedBy r -> clearReplacedBy r + DeleteIssuer i -> deleteIssuer i type IdPConfigRow = (SAML.IdPId, SAML.Issuer, URI, SignedCertificate, [SignedCertificate], TeamId, Maybe WireIdPAPIVersion, [SAML.Issuer], Maybe SAML.IdPId) @@ -231,3 +232,9 @@ clearReplacedBy (Replaced old) = do where ins :: PrepQuery W (Identity SAML.IdPId) () ins = "UPDATE idp SET replaced_by = null WHERE idp = ?" + +deleteIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> m () +deleteIssuer issuer = retry x5 $ write del (params LocalQuorum (Identity issuer)) + where + del :: PrepQuery W (Identity SAML.Issuer) () + del = "DELETE FROM issuer_idp_v2 WHERE issuer = ?" diff --git a/services/spar/src/Spar/Sem/IdPConfigStore/Mem.hs b/services/spar/src/Spar/Sem/IdPConfigStore/Mem.hs index 9007580131..708c380ab4 100644 --- a/services/spar/src/Spar/Sem/IdPConfigStore/Mem.hs +++ b/services/spar/src/Spar/Sem/IdPConfigStore/Mem.hs @@ -58,6 +58,7 @@ idPToMem = evState . evEff modify' (updateReplacedBy (Just replacing) replaced <$>) ClearReplacedBy (Replaced replaced) -> modify' (updateReplacedBy Nothing replaced <$>) + DeleteIssuer issuer -> modify' (deleteIssuer issuer) storeConfig :: IP.IdP -> TypedState -> TypedState storeConfig iw = @@ -114,3 +115,6 @@ updateReplacedBy mbReplacing replaced idp = & if idp ^. SAML.idpId == replaced then SAML.idpExtraInfo . IP.wiReplacedBy .~ mbReplacing else id + +deleteIssuer :: SAML.Issuer -> TypedState -> TypedState +deleteIssuer = const id diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index d1d68a8b4e..9ed5d732a6 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -908,6 +908,20 @@ specCRUDIdentityProvider = do `shouldRespondWith` ((== 200) . statusCode) callIdpGet (env ^. teSpar) (Just owner) idpid `shouldRespondWith` ((== idpmeta') . view idpMetadata) + it "updates IdP metadata and creates a new IdP with the first metadata" $ do + env <- ask + (owner, _) <- call $ createUserWithTeam (env ^. teBrig) (env ^. teGalley) + -- create new idp + (SampleIdP metadata1 _ _ _) <- makeSampleIdPMetadata + idp1 <- call $ callIdpCreate (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner) metadata1 + -- update the idp metadata + (SampleIdP metadata2 _ _ _) <- makeSampleIdPMetadata + callIdpUpdate' (env ^. teSpar) (Just owner) (idp1 ^. idpId) (IdPMetadataValue (cs $ SAML.encode metadata2) undefined) + `shouldRespondWith` ((== 200) . statusCode) + -- create a new idp with the first metadata (should succeed) + callIdpCreate' (env ^. teWireIdPAPIVersion) (env ^. teSpar) (Just owner) metadata1 + `shouldRespondWith` ((== 201) . statusCode) + context "invalid body" $ do it "rejects" $ do env <- ask