Accept any query string for externalId (#1330)

Co-authored-by: fisx <[email protected]>

Author: smatting

services/spar/src/Spar/Scim/User.hs

@@ -47,7 +47,8 @@ import Brig.Types.User (ManagedBy (..), Name (..), User (..))
 import qualified Brig.Types.User as BT
 import qualified Control.Applicative as Applicative (empty)
 import Control.Lens (view, (^.))
-import Control.Monad.Except (MonadError, throwError)
+import Control.Monad.Error.Class (MonadError)
+import Control.Monad.Except (runExceptT, throwError)
 import Control.Monad.Trans.Except (mapExceptT)
 import Control.Monad.Trans.Maybe (MaybeT (MaybeT), runMaybeT)
 import Crypto.Hash (Digest, SHA256, hashlazy)
@@ -765,7 +766,13 @@ scimFindUserByHandle mIdpConfig stiTeam hndl = do
 -- successful authentication with their SAML credentials.
 scimFindUserByEmail :: Maybe IdP -> TeamId -> Text -> MaybeT (Scim.ScimHandler Spar) (Scim.StoredUser ST.SparTag)
 scimFindUserByEmail mIdpConfig stiTeam email = do
-  veid <- mkValidExternalId mIdpConfig (pure email)
+  -- Azure has been observed to search for externalIds that are not emails, even if the
+  -- mapping is set up like it should be.  This is a problem: if there is no SAML IdP, 'mkValidExternalId'
+  -- only supports external IDs that are emails.  This is a missing feature / bug in spar tracked in
+  -- https://wearezeta.atlassian.net/browse/SQSERVICES-157; once it is fixed, we should go back to
+  -- throwing errors returned by 'mkValidExternalId' here, but *not* throw an error if the externalId is
+  -- a UUID, or any other text that is valid according to SCIM.
+  veid <- MaybeT (either (const Nothing) Just <$> runExceptT (mkValidExternalId mIdpConfig (pure email)))
   uid <- MaybeT . lift $ ST.runValidExternalId withUref withEmailOnly veid
   brigUser <- MaybeT . lift . Brig.getBrigUserAccount Brig.WithPendingInvitations $ uid
   guard $ userTeam (accountUser brigUser) == Just stiTeam

services/spar/test-integration/Test/Spar/Scim/UserSpec.hs

@@ -1565,12 +1565,26 @@ specDeleteUser = do
 
 -- | Azure sends a request for an unknown user to test out whether your API is online However;
 -- it sends a userName that is not a valid wire handle. So we should treat 'invalid' as 'not
--- found'.
+-- found'. If we treat it as invalid Azure will put the scim provisioning into quarantine mode,
+-- which requires manual intervention by the customer.
 specAzureQuirks :: SpecWith TestEnv
 specAzureQuirks = do
   describe "Assert that we implement all azure quirks" $ do
-    it "GET /Users?filter=randomField eq <invalid value> should return empty list; not error out" $ do
-      (tok, (_, _, _)) <- registerIdPAndScimToken
+    context "with SAML IDP" $
+      it "GET /Users?filter=randomField eq <invalid value> should return empty list; not error out" $ do
+        (tok, (_, _, _)) <- registerIdPAndScimToken
+        testUUIds tok
+
+    context "without SAML IdP" $
+      it "GET /Users?filter=randomField eq <invalid value> should return empty list; not error out" $ do
+        env <- ask
+        let brig = env ^. teBrig
+            galley = env ^. teGalley
+        (_owner, tid) <- call $ createUserWithTeam brig galley
+        tok <- registerScimToken tid Nothing
+        testUUIds tok
+  where
+    testUUIds tok = do
       users <- listUsers tok (Just (filterBy "userName" "f52dcb88-9fa1-4ec7-984f-7bc2d4046a9c"))
       liftIO $ users `shouldBe` []
       users' <- listUsers tok (Just (filterBy "externalId" "f52dcb88-9fa1-4ec7-984f-7bc2d4046a9c"))