diff --git a/changelog.d/3-bug-fixes/pr-2519 b/changelog.d/3-bug-fixes/pr-2519 new file mode 100644 index 00000000000..b887542ec64 --- /dev/null +++ b/changelog.d/3-bug-fixes/pr-2519 @@ -0,0 +1 @@ +A user now cannot delete an identity provider that they are authenticated with any more diff --git a/libs/wire-api/src/Wire/API/User.hs b/libs/wire-api/src/Wire/API/User.hs index d3052191cb4..1760ca872f4 100644 --- a/libs/wire-api/src/Wire/API/User.hs +++ b/libs/wire-api/src/Wire/API/User.hs @@ -31,6 +31,7 @@ module Wire.API.User userEmail, userPhone, userSSOId, + userIssuer, userSCIMExternalId, scimExternalId, ssoIssuerAndNameId, @@ -396,6 +397,13 @@ ssoIssuerAndNameId (UserSSOId (SAML.UserRef (SAML.Issuer uri) nameIdXML)) = Just fromNameId = CI.original . SAML.unsafeShowNameID ssoIssuerAndNameId (UserScimExternalId _) = Nothing +userIssuer :: User -> Maybe SAML.Issuer +userIssuer user = userSSOId user >>= fromSSOId + where + fromSSOId :: UserSSOId -> Maybe SAML.Issuer + fromSSOId (UserSSOId (SAML.UserRef issuer _)) = Just issuer + fromSSOId _ = Nothing + connectedProfile :: User -> UserLegalHoldStatus -> UserProfile connectedProfile u legalHoldStatus = UserProfile diff --git a/services/spar/src/Spar/API.hs b/services/spar/src/Spar/API.hs index 6723ff3a069..8cf4bd33acf 100644 --- a/services/spar/src/Spar/API.hs +++ b/services/spar/src/Spar/API.hs @@ -44,6 +44,7 @@ module Spar.API ) where +import Brig.Types.User (HavePendingInvitations (NoPendingInvitations)) import Control.Lens import Control.Monad.Except import qualified Data.ByteString as SBS @@ -94,6 +95,7 @@ import qualified Spar.Sem.VerdictFormatStore as VerdictFormatStore import System.Logger (Msg) import qualified URI.ByteString as URI import Wire.API.Routes.Public.Spar +import Wire.API.User (userIssuer) import Wire.API.User.IdentityProvider import Wire.API.User.Saml import Wire.Sem.Logger (Logger) @@ -412,7 +414,6 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons let issuer = idp ^. SAML.idpMetadata . SAML.edIssuer team = idp ^. SAML.idpExtraInfo . wiTeam -- if idp is not empty: fail or purge - idpIsEmpty <- isNothing <$> SAMLUserStore.getAnyByIssuer issuer let doPurge :: Sem r () doPurge = do some <- SAMLUserStore.getSomeByIssuer issuer @@ -420,6 +421,8 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons BrigAccess.delete uid SAMLUserStore.delete uid uref unless (null some) doPurge + idpIsEmpty <- isNothing <$> SAMLUserStore.getAnyByIssuer issuer + whenM (maybe (pure False) (idpDoesAuthSelf idp) zusr) $ throwSparSem SparIdPCannotDeleteOwnIdp unless idpIsEmpty $ if purge then doPurge @@ -444,7 +447,7 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons -- to be deleted in its old issuers list, but it's tricky to avoid race conditions, and -- there is little to be gained here: we only use old issuers to find users that have not -- been migrated yet, and if an old user points to a deleted idp, it just means that we - -- won't find any users to migrate. still, doesn't hurt mucht to look either. so we + -- won't find any users to migrate. still, doesn't hurt much to look either. so we -- leave old issuers dangling for now. updateReplacingIdP :: IdP -> Sem r () @@ -456,6 +459,12 @@ idpDelete zusr idpid (fromMaybe False -> purge) = withDebugLog "idpDelete" (cons GetIdPNonUnique _ -> pure () GetIdPWrongTeam _ -> pure () + idpDoesAuthSelf :: IdP -> UserId -> Sem r Bool + idpDoesAuthSelf idp uid = do + let idpIssuer = idp ^. SAML.idpMetadata . SAML.edIssuer + mUserIssuer <- (>>= userIssuer) <$> Brig.getBrigUser NoPendingInvitations uid + pure $ mUserIssuer == Just idpIssuer + -- | This handler only does the json parsing, and leaves all authorization checks and -- application logic to 'idpCreateXML'. idpCreate :: diff --git a/services/spar/src/Spar/Error.hs b/services/spar/src/Spar/Error.hs index a9360555129..7fb7db18f7f 100644 --- a/services/spar/src/Spar/Error.hs +++ b/services/spar/src/Spar/Error.hs @@ -94,6 +94,7 @@ data SparCustomError | SparNewIdPWantHttps LT | SparIdPHasBoundUsers | SparIdPIssuerInUse + | SparIdPCannotDeleteOwnIdp | SparProvisioningMoreThanOneIdP LT | SparProvisioningTokenLimitReached | -- | FUTUREWORK(fisx): This constructor is used in exactly one place (see @@ -172,6 +173,7 @@ renderSparError (SAML.CustomError (SparNewIdPAlreadyInUse msg)) = Right $ Wai.mk renderSparError (SAML.CustomError (SparNewIdPWantHttps msg)) = Right $ Wai.mkError status400 "idp-must-be-https" ("an idp request uri must be https, not http or other: " <> msg) renderSparError (SAML.CustomError SparIdPHasBoundUsers) = Right $ Wai.mkError status412 "idp-has-bound-users" "an idp can only be deleted if it is empty" renderSparError (SAML.CustomError SparIdPIssuerInUse) = Right $ Wai.mkError status400 "idp-issuer-in-use" "The issuer of your IdP is already in use. Remove the entry in the team that uses it, or construct a new IdP issuer." +renderSparError (SAML.CustomError SparIdPCannotDeleteOwnIdp) = Right $ Wai.mkError status409 "cannot-delete-own-idp" "You cannot delete the IdP used to login with your own account." -- Errors related to provisioning renderSparError (SAML.CustomError (SparProvisioningMoreThanOneIdP msg)) = Right $ Wai.mkError status400 "more-than-one-idp" ("Team can have at most one IdP configured: " <> msg) renderSparError (SAML.CustomError SparProvisioningTokenLimitReached) = Right $ Wai.mkError status403 "token-limit-reached" "The limit of provisioning tokens per team has been reached" diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index 63d5bcb8caf..047205dd5f2 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -675,17 +675,17 @@ specCRUDIdentityProvider = do it "responds with 412 and does not remove IdP" $ do env <- ask (firstOwner, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta - ssoOwner <- mkSsoOwner firstOwner tid idp privcreds - callIdpDelete' (env ^. teSpar) (Just ssoOwner) (idp ^. idpId) + _ <- mkSsoOwner firstOwner tid idp privcreds + callIdpDelete' (env ^. teSpar) (Just firstOwner) (idp ^. idpId) `shouldRespondWith` checkErrHspec 412 "idp-has-bound-users" - callIdpGet' (env ^. teSpar) (Just ssoOwner) (idp ^. idpId) + callIdpGet' (env ^. teSpar) (Just firstOwner) (idp ^. idpId) `shouldRespondWith` \resp -> statusCode resp < 300 context "with email, idp non-empty, purge=true" $ do it "responds with 2xx and removes IdP and users *synchronously*" $ do env <- ask (firstOwner, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta ssoOwner <- mkSsoOwner firstOwner tid idp privcreds - callIdpDeletePurge' (env ^. teSpar) (Just ssoOwner) (idp ^. idpId) + callIdpDeletePurge' (env ^. teSpar) (Just firstOwner) (idp ^. idpId) `shouldRespondWith` \resp -> statusCode resp < 300 _ <- aFewTimes (getUserBrig ssoOwner) isNothing ssoOwner' <- userId <$$> getUserBrig ssoOwner @@ -695,6 +695,13 @@ specCRUDIdentityProvider = do firstOwner' `shouldBe` Just firstOwner callIdpGet' (env ^. teSpar) (Just firstOwner) (idp ^. idpId) `shouldRespondWith` checkErrHspec 404 "not-found" + context "with email, user who tries to delete is authenticated by the IdP, purge=true" $ do + it "responds with 409 'cannot-delete-own-idp'" $ do + env <- ask + (firstOwner, tid, idp, (_, privcreds)) <- registerTestIdPWithMeta + ssoOwner <- mkSsoOwner firstOwner tid idp privcreds + callIdpDeletePurge' (env ^. teSpar) (Just ssoOwner) (idp ^. idpId) + `shouldRespondWith` checkErrHspec 409 "cannot-delete-own-idp" describe "PUT /identity-providers/:idp" $ do testGetPutDelete callIdpUpdate' context "known IdP, client is team owner" $ do