diff --git a/changelog.d/3-bug-fixes/WPB-21744 b/changelog.d/3-bug-fixes/WPB-21744 new file mode 100644 index 0000000000..db6664b392 --- /dev/null +++ b/changelog.d/3-bug-fixes/WPB-21744 @@ -0,0 +1 @@ +Fix email verification resend for SCIM users. diff --git a/integration/test/Test/User.hs b/integration/test/Test/User.hs index 0c8ff60eba..0106d216d2 100644 --- a/integration/test/Test/User.hs +++ b/integration/test/Test/User.hs @@ -418,3 +418,94 @@ testEphemeralUserCreation (TaggedBool enabled) = do where registerEphemeralUser domain = addUser domain def registerUserWithEmail domain = addUser domain def {email = Just ("user@" <> domain)} + +testBrigSCIMResendVerificationEmail :: (HasCallStack) => App () +testBrigSCIMResendVerificationEmail = do + -- 1. Create team (owner) + (owner, _tid, _) <- createTeam OwnDomain 1 + + -- 2. Create SCIM token + tok <- bindResponse (Spar.createScimTokenV6 owner def) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "token" >>= asString + + -- 3. Create SCIM User + scimEmail <- randomEmail + scimExtId <- randomEmail + scimUser <- randomScimUserWithEmail scimExtId scimEmail + + userId <- bindResponse (Spar.createScimUser OwnDomain tok scimUser) $ \resp -> do + resp.status `shouldMatchInt` 201 + resp.json %. "id" >>= asString + + -- 4. Re-send verification email + putUserEmail owner (object ["id" .= userId, "domain" .= ("example.com" :: String)]) scimEmail `bindResponse` \resp -> do + resp.status `shouldMatchInt` 200 + +testBrigSCIMPromotedResendVerificationEmail :: (HasCallStack) => App () +testBrigSCIMPromotedResendVerificationEmail = do + -- 1. Create team (owner) and a normal user (alice) + (owner, tid, _) <- createTeam OwnDomain 1 + + aliceEmail <- randomEmail + alice <- createUser OwnDomain def {email = Just aliceEmail, password = Just defPassword, activate = True} >>= getJSON 201 + aliceId <- asString $ alice %. "id" + + -- Add alice to team + bindResponse (postInvitation owner def {email = Just aliceEmail}) $ \resp -> do + resp.status `shouldMatchInt` 201 + + inv <- getInvitationByEmail OwnDomain aliceEmail >>= getJSON 200 + invCodeResp <- I.getInvitationCodeForTeam OwnDomain tid inv >>= getJSON 200 + code <- invCodeResp %. "code" & asString + + bindResponse (acceptTeamInvitation alice code (Just defPassword)) $ \resp -> do + resp.status `shouldMatchInt` 200 + + -- 2. Login as alice + (cookie, token) <- bindResponse (login OwnDomain aliceEmail defPassword) $ \resp -> do + resp.status `shouldMatchInt` 200 + token <- resp.json %. "access_token" & asString + let cookie = fromJust $ getCookie "zuid" resp + pure ("zuid=" <> cookie, token) + + -- 3. Alice requests email change to B (pending state) + emailDomain <- randomDomain + newEmail <- randomName <&> (<> "@" <> emailDomain) + updateEmail alice newEmail cookie token >>= assertSuccess + + -- 4. Make alice managed by SCIM (Adopt via PUT) + scimToken <- bindResponse (Spar.createScimToken owner def) $ \resp -> do + resp.status `shouldMatchInt` 200 + resp.json %. "token" & asString + + let aliceUserName = map (\c -> if c == '@' then '_' else c) aliceEmail + let scimUser = + object + [ "schemas" .= ["urn:ietf:params:scim:schemas:core:2.0:User"], + "userName" .= aliceUserName, + "emails" .= [object ["value" .= aliceEmail, "type" .= "work", "primary" .= True]], + "externalId" .= aliceId, + "name" .= object ["familyName" .= ("User" :: String), "givenName" .= ("Alice" :: String)] + ] + + -- Try to update user via SCIM. This should set managed_by=scim. (`GET .../Users` would have the same effect.) + bindResponse (Spar.updateScimUser owner scimToken aliceId scimUser) $ \resp -> do + resp.status `shouldMatchOneOf` [Number 200, Number 201, Number 204] + + -- 7. Alice tries to resend verification for B (client side) + -- This should now succeed (202) despite being managed-by-scim, because it is a resend of pending email + updateEmail alice newEmail cookie token `bindResponse` \resp -> do + resp.status `shouldMatchInt` 403 -- or something; also check error label. + updateEmail alice aliceEmail cookie token `bindResponse` \resp -> do + resp.status `shouldMatchInt` 204 + +-- (TODO: there may be a refresh problem in team-management here: if +-- scim updates the user, team-management may not notice.) + +-- relevant end-points that we may want to test: +-- https://staging-nginz-https.zinfra.io/v14/api/swagger-ui/#/default/accept-team-invitation (should not work any more, or be a noop, not sure) +-- https://staging-nginz-https.zinfra.io/v14/api/swagger-ui/#/default/head-team-invitations +-- https://staging-nginz-https.zinfra.io/v14/api/swagger-ui/#/default/get-team-invitation-info +-- https://staging-nginz-https.zinfra.io/v14/api/swagger-ui/#/default/browse-team +-- https://staging-nginz-https.zinfra.io/v14/api/swagger-ui/#/default/get-team-member diff --git a/libs/wire-api/src/Wire/API/User/Identity.hs b/libs/wire-api/src/Wire/API/User/Identity.hs index 97a3c503e5..c550d3a2c3 100644 --- a/libs/wire-api/src/Wire/API/User/Identity.hs +++ b/libs/wire-api/src/Wire/API/User/Identity.hs @@ -135,7 +135,7 @@ ssoIdentity _ = Nothing -------------------------------------------------------------------------------- -- UserSSOId --- | User's external identity. +-- | User's external identity (both SAML idpid, nameid, and SCIM externalId) -- -- NB: this type is serialized to the full xml encoding of the `SAML.UserRef` components, but -- deserialiation is more lenient: it also allows for the `Issuer` to be a plain URL (without diff --git a/services/brig/src/Brig/Data/User.hs b/services/brig/src/Brig/Data/User.hs index 0116e5e8e2..19e94efbcf 100644 --- a/services/brig/src/Brig/Data/User.hs +++ b/services/brig/src/Brig/Data/User.hs @@ -185,7 +185,7 @@ updateSSOId u ssoid = do mteamid <- lookupUserTeam u case mteamid of Just _ -> do - retry x5 $ write userSSOIdUpdate (params LocalQuorum (ssoid, u)) + retry x5 $ write userSSOIdUpdate (params LocalQuorum (ssoid, u)) -- TODO: why is this not asking team member for address verification, even with feature flag enabled? pure True Nothing -> pure False diff --git a/services/spar/src/Spar/Scim/User.hs b/services/spar/src/Spar/Scim/User.hs index aaf4d56d41..ac40615f56 100644 --- a/services/spar/src/Spar/Scim/User.hs +++ b/services/spar/src/Spar/Scim/User.hs @@ -735,7 +735,7 @@ updateVsuUref team uid old new = do ScimExternalIdStore.insert team new.validScimIdExternal uid for_ (justThere new.validScimIdAuthInfo) (`SAMLUserStore.insert` uid) - BrigAccess.setSSOId uid $ veidToUserSSOId new + BrigAccess.setSSOId uid $ veidToUserSSOId new -- TODO: theory: this doesn't remove the old email update verification token. toScimStoredUser :: (HasCallStack) => @@ -1186,11 +1186,13 @@ scimFindUserByExternalId mIdpConfig stiTeam eid = do -- there are a few ways to find a user. this should all be redundant, especially the where -- we lookup a user from brig by email, throw it away and only keep the uid, and then use -- the uid to lookup the account again. but cassandra, and also reasons. + -- TODO: BUG! this is handled by `listActivatedAccountsH` in brig, but we also want *inactive* accounts here! mViaEmail :: Maybe UserId <- join <$> (for (justHere veid.validScimIdAuthInfo) ((userId <$$>) . BrigAccess.getByEmail)) mViaUref :: Maybe UserId <- join <$> (for (justThere veid.validScimIdAuthInfo) SAMLUserStore.get) pure $ mViaEmail <|> mViaUref Just uid -> pure uid acc <- MaybeT . lift . BrigAccess.getAccount Brig.WithPendingInvitations $ uid + -- TODO: how can userId acc /= uid here? do we even need to call getAccount? getUserById mIdpConfig stiTeam (userId acc) logFilter :: Filter -> (Msg -> Msg)