Skip to content

SQSERVICES-1519 IdP for SAML that was overridden by update cannot be added again #2400

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
battermann merged 6 commits into from
May 19, 2022
Merged

SQSERVICES-1519 IdP for SAML that was overridden by update cannot be added again #2400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
02ed7e0
failing integration test
battermann May 16, 2022
96a5350
remove old issuers when updating IdP
battermann May 17, 2022
1caf1f5
changelog
battermann May 17, 2022
f7b5d9a
Update changelog.d/3-bug-fixes/pr-2400
battermann May 18, 2022
e92473f
delete old issuer after update, added a comment
battermann May 18, 2022
70a664c
hi ci
battermann May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/3-bug-fixes/pr-2400
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
When an IdP issuer (aka entity ID) is updated, the old issuer was still marked as "in use".
5 changes: 4 additions & 1 deletion services/spar/src/Spar/API.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,9 +685,12 @@ idpUpdateXML zusr raw idpmeta idpid = withDebugLog "idpUpdate" (Just . show . (^
GalleyAccess.assertSSOEnabled teamid
IdPRawMetadataStore.store (idp ^. SAML.idpId) raw
Copy link
Contributor

@fisx fisx May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if spar crashes at the right moment, this will cause the old idp to be deleted and the new not to be introduced, which is not fixable over the rest api (let alone the UI).

I don't see a good way around this, though. It would be really nice to have transactions and postgres and everything. but i would like to have the comment below reflect this. (could you maybe also fix my typo there while you're at it ("epected")?)

Copy link
Contributor Author

@battermann battermann May 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved the deletion to the end which leaves a less inconsistent state, in the case of a failure. And I added a comment.

-- (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
Expand Down
2 changes: 2 additions & 0 deletions services/spar/src/Spar/Sem/IdPConfigStore.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ module Spar.Sem.IdPConfigStore
deleteConfig,
setReplacedBy,
clearReplacedBy,
deleteIssuer,
)
where

Expand Down Expand Up @@ -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)

Expand Down
7 changes: 7 additions & 0 deletions services/spar/src/Spar/Sem/IdPConfigStore/Cassandra.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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 = ?"
4 changes: 4 additions & 0 deletions services/spar/src/Spar/Sem/IdPConfigStore/Mem.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand Down Expand Up @@ -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
14 changes: 14 additions & 0 deletions services/spar/test-integration/Test/Spar/APISpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down