Skip to content

Commit

Permalink
SQSERVICES-1628-dont-allow-deleting-id-ps-if-the-deleting-user-is-aut…
Browse files Browse the repository at this point in the history
…henticated-by-it (#2519)
  • Loading branch information
battermann authored Jun 29, 2022
1 parent bbc9ffa commit f244af0
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 6 deletions.
1 change: 1 addition & 0 deletions changelog.d/3-bug-fixes/pr-2519
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
A user now cannot delete an identity provider that they are authenticated with any more
8 changes: 8 additions & 0 deletions libs/wire-api/src/Wire/API/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ module Wire.API.User
userEmail,
userPhone,
userSSOId,
userIssuer,
userSCIMExternalId,
scimExternalId,
ssoIssuerAndNameId,
Expand Down Expand Up @@ -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
Expand Down
13 changes: 11 additions & 2 deletions services/spar/src/Spar/API.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -412,14 +414,15 @@ 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
forM_ some $ \(uref, uid) -> do
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
Expand All @@ -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 ()
Expand All @@ -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 ::
Expand Down
2 changes: 2 additions & 0 deletions services/spar/src/Spar/Error.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
15 changes: 11 additions & 4 deletions services/spar/test-integration/Test/Spar/APISpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit f244af0

Please sign in to comment.