Fix error in member csv creation (SAML.UserRef decoding error)

.gitignore

@@ -33,7 +33,6 @@ TAGS
 .stack-docker-profile
 .metadata
 *.tix
-*.pem
 .DS_Store
 services/nginz/src
 services/.env
@@ -99,4 +98,4 @@ i.yaml
 b.yaml
 telepresence.log
 
-/.ghci
+/.ghci

changelog.d/3-bug-fixes/pr-1828

@@ -0,0 +1 @@
+SAML columns (Issuer, NameID) in CSV files with team members.

libs/wire-api/package.yaml

@@ -16,6 +16,8 @@ dependencies:
 - servant-swagger-ui
 - case-insensitive
 - hscim
+- saml2-web-sso
+- filepath
 library:
   source-dirs: src
   dependencies:
@@ -60,7 +62,6 @@ library:
   - QuickCheck >=2.14
   - quickcheck-instances >=0.3.16
   - resourcet
-  - saml2-web-sso
   - servant
   - servant-client
   - servant-client-core

libs/wire-api/src/Wire/API/User.hs

@@ -120,7 +120,6 @@ import Data.Schema
 import qualified Data.Swagger as S
 import qualified Data.Swagger.Build.Api as Doc
 import Data.Text.Ascii
-import qualified Data.Text.Lazy as TL
 import Data.UUID (UUID, nil)
 import qualified Data.UUID as UUID
 import Deriving.Swagger
@@ -412,12 +411,12 @@ userSCIMExternalId :: User -> Maybe Text
 userSCIMExternalId usr = userSSOId >=> ssoIdExtId $ usr
   where
     ssoIdExtId :: UserSSOId -> Maybe Text
-    ssoIdExtId (UserSSOId _ nameIdXML) = case userManagedBy usr of
+    ssoIdExtId (UserSSOId (SAML.UserRef _ nameIdXML)) = case userManagedBy usr of
       ManagedByWire -> Nothing
       ManagedByScim ->
-        -- FUTUREWORK: keep the CI value, store the original in the database, but always use
-        -- the CI value for processing.
-        CI.original . SAML.unsafeShowNameID <$> either (const Nothing) pure (SAML.decodeElem (TL.fromStrict nameIdXML))
+        -- FUTUREWORK: this is only ignoring case in the email format, and emails should be
+        -- handled case-insensitively.  https://wearezeta.atlassian.net/browse/SQSERVICES-909
+        Just . CI.original . SAML.unsafeShowNameID $ nameIdXML
     ssoIdExtId (UserScimExternalId extId) = pure extId
 
 connectedProfile :: User -> UserLegalHoldStatus -> UserProfile

libs/wire-api/src/Wire/API/User/Identity.hs

@@ -40,28 +40,43 @@ module Wire.API.User.Identity
 
     -- * UserSSOId
     UserSSOId (..),
-
-    -- * Swagger
+    emailFromSAML,
+    emailToSAML,
+    emailToSAMLNameID,
+    emailFromSAMLNameID,
+    mkSampleUref,
+    mkSimpleSampleUref,
   )
 where
 
 import Control.Applicative (optional)
-import Control.Lens ((.~), (?~))
+import Control.Lens ((.~), (?~), (^.))
 import Data.Aeson (FromJSON (..), ToJSON (..))
 import qualified Data.Aeson as A
+import qualified Data.Aeson.Types as A
 import Data.Attoparsec.Text
-import Data.Bifunctor (first)
+import Data.Bifunctor (first, second)
 import Data.ByteString.Conversion
+import qualified Data.CaseInsensitive as CI
 import Data.Proxy (Proxy (..))
 import Data.Schema
+import Data.String.Conversions (cs)
 import qualified Data.Swagger as S
 import qualified Data.Text as Text
 import Data.Text.Encoding (decodeUtf8', encodeUtf8)
 import Data.Time.Clock
 import Imports
+import SAML2.WebSSO.Test.Arbitrary ()
+import qualified SAML2.WebSSO.Types as SAML
+import qualified SAML2.WebSSO.Types.Email as SAMLEmail
+import qualified SAML2.WebSSO.XML as SAML
+import System.FilePath ((</>))
 import qualified Test.QuickCheck as QC
 import qualified Text.Email.Validate as Email.V
+import qualified URI.ByteString as URI
+import URI.ByteString.QQ (uri)
 import Wire.API.Arbitrary (Arbitrary (arbitrary), GenericUniform (..))
+import Wire.API.User.Profile (fromName, mkName)
 
 --------------------------------------------------------------------------------
 -- UserIdentity
@@ -267,30 +282,27 @@ isValidPhone = either (const False) (const True) . parseOnly e164
 
 -- | User's external identity.
 --
--- Morally this is the same thing as 'SAML.UserRef', but we forget the
--- structure -- i.e. we just store XML-encoded SAML blobs. If the structure
--- of those blobs changes, Brig won't have to deal with it, only Spar will.
+-- 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
+-- xml around it), and the `NameID` to be an email address (=> format "email") or an arbitrary
+-- text (=> format "unspecified").  This is for backwards compatibility and general
+-- robustness.
 --
--- FUTUREWORK: rename the data type to @UserSparId@ (not the two constructors, those are ok).
+-- FUTUREWORK: we should probably drop this entirely and store saml and scim data in separate
+-- database columns.
 data UserSSOId
-  = UserSSOId
-      -- An XML blob pointing to the identity provider that can confirm
-      -- user's identity.
-      Text
-      -- An XML blob specifying the user's ID on the identity provider's side.
-      Text
-  | UserScimExternalId
-      Text
+  = UserSSOId SAML.UserRef
+  | UserScimExternalId Text
   deriving stock (Eq, Show, Generic)
   deriving (Arbitrary) via (GenericUniform UserSSOId)
 
--- FUTUREWORK: This schema should ideally be a choice of either tenant+subject, or scim_external_id
+-- | FUTUREWORK: This schema should ideally be a choice of either tenant+subject, or scim_external_id
 -- but this is currently not possible to derive in swagger2
 -- Maybe this becomes possible with swagger 3?
 instance S.ToSchema UserSSOId where
   declareNamedSchema _ = do
-    tenantSchema <- S.declareSchemaRef (Proxy @Text)
-    subjectSchema <- S.declareSchemaRef (Proxy @Text)
+    tenantSchema <- S.declareSchemaRef (Proxy @Text) -- FUTUREWORK: 'Issuer'
+    subjectSchema <- S.declareSchemaRef (Proxy @Text) -- FUTUREWORK: 'NameID'
     scimSchema <- S.declareSchemaRef (Proxy @Text)
     return $
       S.NamedSchema (Just "UserSSOId") $
@@ -304,16 +316,16 @@ instance S.ToSchema UserSSOId where
 
 instance ToJSON UserSSOId where
   toJSON = \case
-    UserSSOId tenant subject -> A.object ["tenant" A..= tenant, "subject" A..= subject]
+    UserSSOId (SAML.UserRef tenant subject) -> A.object ["tenant" A..= SAML.encodeElem tenant, "subject" A..= SAML.encodeElem subject]
     UserScimExternalId eid -> A.object ["scim_external_id" A..= eid]
 
 instance FromJSON UserSSOId where
   parseJSON = A.withObject "UserSSOId" $ \obj -> do
-    mtenant <- obj A..:? "tenant"
-    msubject <- obj A..:? "subject"
+    mtenant <- lenientlyParseSAMLIssuer =<< (obj A..:? "tenant")
+    msubject <- lenientlyParseSAMLNameID =<< (obj A..:? "subject")
     meid <- obj A..:? "scim_external_id"
     case (mtenant, msubject, meid) of
-      (Just tenant, Just subject, Nothing) -> pure $ UserSSOId tenant subject
+      (Just tenant, Just subject, Nothing) -> pure $ UserSSOId (SAML.UserRef tenant subject)
       (Nothing, Nothing, Just eid) -> pure $ UserScimExternalId eid
       _ -> fail "either need tenant and subject, or scim_external_id, but not both"
 
@@ -331,3 +343,78 @@ instance FromJSON PhoneBudgetTimeout where
 
 instance ToJSON PhoneBudgetTimeout where
   toJSON (PhoneBudgetTimeout t) = A.object ["expires_in" A..= t]
+
+lenientlyParseSAMLIssuer :: Maybe LText -> A.Parser (Maybe SAML.Issuer)
+lenientlyParseSAMLIssuer mbtxt = forM mbtxt $ \txt -> do
+  let asxml :: Either String SAML.Issuer
+      asxml = SAML.decodeElem txt
+
+      asurl :: Either String SAML.Issuer
+      asurl =
+        first show
+          . second SAML.Issuer
+          $ URI.parseURI URI.laxURIParserOptions (cs txt)
+
+      err :: String
+      err = "lenientlyParseSAMLIssuer: " <> show (asxml, asurl, mbtxt)
+
+  either (const $ fail err) pure $ asxml <|> asurl
+
+lenientlyParseSAMLNameID :: Maybe LText -> A.Parser (Maybe SAML.NameID)
+lenientlyParseSAMLNameID Nothing = pure Nothing
+lenientlyParseSAMLNameID (Just txt) = do
+  let asxml :: Either String SAML.NameID
+      asxml = SAML.decodeElem txt
+
+      asemail :: Either String SAML.NameID
+      asemail =
+        maybe
+          (Left "not an email")
+          (fmap emailToSAMLNameID . validateEmail)
+          (parseEmail (cs txt))
+
+      astxt :: Either String SAML.NameID
+      astxt = do
+        nm <- mkName (cs txt)
+        SAML.mkNameID (SAML.mkUNameIDUnspecified (fromName nm)) Nothing Nothing Nothing
+
+      err :: String
+      err = "lenientlyParseSAMLNameID: " <> show (asxml, asemail, astxt, txt)
+
+  either
+    (const $ fail err)
+    (pure . Just)
+    (asxml <|> asemail <|> astxt)
+
+emailFromSAML :: HasCallStack => SAMLEmail.Email -> Email
+emailFromSAML = fromJust . parseEmail . SAMLEmail.render
+
+emailToSAML :: HasCallStack => Email -> SAMLEmail.Email
+emailToSAML = CI.original . fromRight (error "emailToSAML") . SAMLEmail.validate . toByteString
+
+-- | FUTUREWORK(fisx): if saml2-web-sso exported the 'NameID' constructor, we could make this
+-- function total without all that praying and hoping.
+emailToSAMLNameID :: HasCallStack => Email -> SAML.NameID
+emailToSAMLNameID = fromRight (error "impossible") . SAML.emailNameID . fromEmail
+
+emailFromSAMLNameID :: HasCallStack => SAML.NameID -> Maybe Email
+emailFromSAMLNameID nid = case nid ^. SAML.nameID of
+  SAML.UNameIDEmail email -> Just . emailFromSAML . CI.original $ email
+  _ -> Nothing
+
+-- | For testing.  Create a sample 'SAML.UserRef' value with random seeds to make 'Issuer' and
+-- 'NameID' unique.  FUTUREWORK: move to saml2-web-sso.
+mkSampleUref :: Text -> Text -> SAML.UserRef
+mkSampleUref iseed nseed = SAML.UserRef issuer nameid
+  where
+    issuer :: SAML.Issuer
+    issuer = SAML.Issuer ([uri|http://example.com/|] & URI.pathL .~ cs ("/" </> cs iseed))
+
+    nameid :: SAML.NameID
+    nameid = fromRight (error "impossible") $ do
+      unqualified <- SAML.mkUNameIDEmail $ "me" <> nseed <> "@example.com"
+      SAML.mkNameID unqualified Nothing Nothing Nothing
+
+-- | @mkSampleUref "" ""@
+mkSimpleSampleUref :: SAML.UserRef
+mkSimpleSampleUref = mkSampleUref "" ""

libs/wire-api/test/golden/testObject_ActivationResponse_user_1.json

@@ -2,7 +2,7 @@
     "email": "𨠞\rZ\u0007\u001b@p𠋁",
     "first": false,
     "sso_id": {
-        "subject": "\u001e",
-        "tenant": ""
+        "subject": "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">me@example.com</NameID>",
+        "tenant": "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">http://example.com/</Issuer>"
     }
 }

libs/wire-api/test/golden/testObject_ActivationResponse_user_19.json

@@ -2,7 +2,7 @@
     "email": "R@K",
     "first": false,
     "sso_id": {
-        "subject": "",
-        "tenant": ""
+        "subject": "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">me@example.com</NameID>",
+        "tenant": "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">http://example.com/</Issuer>"
     }
 }

libs/wire-api/test/golden/testObject_NewUser_user_6.json

@@ -2,8 +2,8 @@
     "assets": [],
     "name": "test name",
     "sso_id": {
-        "subject": "thing",
-        "tenant": "some"
+        "subject": "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">me@example.com</NameID>",
+        "tenant": "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">http://example.com/</Issuer>"
     },
     "team_id": "00007b0e-0000-3489-0000-075c00005be7"
 }

libs/wire-api/test/golden/testObject_UserIdentity_user_16.json

@@ -2,7 +2,7 @@
     "email": "%x\u0013􀔑\u0004.@G빯t.6",
     "phone": "+298116118047",
     "sso_id": {
-        "subject": "\u0013\u001c",
-        "tenant": "a\u001c"
+        "subject": "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">me@example.com</NameID>",
+        "tenant": "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">http://example.com</Issuer>"
     }
 }

libs/wire-api/test/golden/testObject_UserIdentity_user_5.json

@@ -2,7 +2,7 @@
     "email": null,
     "phone": "+49198172826",
     "sso_id": {
-        "subject": "󴤰",
-        "tenant": ">􋲗􎚆󾪂"
+        "subject": "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">me@example.com</NameID>",
+        "tenant": "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">http://example.com</Issuer>"
     }
 }

libs/wire-api/test/golden/testObject_UserIdentity_user_8.json

@@ -2,7 +2,7 @@
     "email": null,
     "phone": "+149548802116267",
     "sso_id": {
-        "subject": "",
-        "tenant": ""
+        "subject": "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">me@example.com</NameID>",
+        "tenant": "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">http://example.com</Issuer>"
     }
 }

libs/wire-api/test/golden/testObject_UserSSOId_user_2.json

@@ -1,3 +1,4 @@
 {
-    "scim_external_id": "퀶\u001a\u0002\u000bf\u0008-󿰣qA􄚨\u0005 >jJ"
+    "subject": "<NameID xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" Format=\"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">me@example.com</NameID>",
+    "tenant": "<Issuer xmlns:samlp=\"urn:oasis:names:tc:SAML:2.0:protocol\" xmlns:samla=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:samlm=\"urn:oasis:names:tc:SAML:2.0:metadata\" xmlns:ds=\"http://www.w3.org/2000/09/xmldsig#\" xmlns=\"urn:oasis:names:tc:SAML:2.0:assertion\">http://example.com/</Issuer>"
 }

libs/wire-api/test/unit/Test/Wire/API/Golden/FromJSON.hs

@@ -94,25 +94,13 @@ tests =
             testFromJSONFailureWithMsg @NewUser
               (Just "all team users must set a password on creation")
               "testObject_NewUser_user_5-2.json",
-          testCase "testObject_NewUser_user_6-2.json" $
-            testFromJSONFailureWithMsg @NewUser
-              (Just "sso_id, team_id must be either both present or both absent.")
-              "testObject_NewUser_user_6-2.json",
           testCase "testObject_NewUser_user_6-3.json" $
             testFromJSONFailureWithMsg @NewUser
               (Just "sso_id, team_id must be either both present or both absent.")
-              "testObject_NewUser_user_6-3.json",
-          testCase "testObject_NewUser_user_6-4.json" $
-            testFromJSONFailureWithMsg @NewUser
-              (Just "team_code, team, invitation_code, sso_id, and the pair (sso_id, team_id) are mutually exclusive")
-              "testObject_NewUser_user_6-4.json"
+              "testObject_NewUser_user_6-3.json"
         ],
       testGroup "NewUserPublic: failure" $
-        [ testCase "testObject_NewUserPublic_user_1-1.json" $
-            testFromJSONFailureWithMsg @NewUserPublic
-              (Just "SSO-managed users are not allowed here.")
-              "testObject_NewUserPublic_user_1-1.json",
-          testCase "testObject_NewUserPublic_user_1-2.json" $
+        [ testCase "testObject_NewUserPublic_user_1-2.json" $
             testFromJSONFailureWithMsg @NewUserPublic
               (Just "it is not allowed to provide a UUID for the users here.")
               "testObject_NewUserPublic_user_1-2.json",