WPB-26291 prevent SCIM user change name on registration

changelog.d/1-api-changes/WPB-26291

@@ -0,0 +1,2 @@
+- In API version v17 `POST /register` may now return a new `403 managed-by-scim` error when a SCIM-managed pending user changes the display name during registration
+- The `GET /teams/invitations/info?code=...` response may now include the optional `managed_by` field

changelog.d/3-bug-fixes/WPB-26291

@@ -0,0 +1 @@
+Do not allow changing the profile name during registration for SCIM users

integration/test/API/Brig.hs

@@ -914,13 +914,16 @@ revokeApplicationAccess user cid password = do
   submit "DELETE" $ req & addJSONObject ["password" .= password]
 
 registerUser :: (HasCallStack, MakesValue domain) => domain -> String -> String -> App Response
-registerUser domain email inviteeCode = do
+registerUser domain email inviteeCode = registerUserWith domain email inviteeCode "Alice"
+
+registerUserWith :: (HasCallStack, MakesValue domain) => domain -> String -> String -> String -> App Response
+registerUserWith domain email inviteeCode name = do
   req <- baseRequest domain Brig Versioned "register"
   submit "POST" $
     req
       & addClientIP
       & addJSONObject
-        [ "name" .= "Alice",
+        [ "name" .= name,
           "email" .= email,
           "password" .= defPassword,
           "team_code" .= inviteeCode

integration/test/SetupHelpers.hs

@@ -745,14 +745,9 @@ activateEmail domain email = do
 
 registerInvitedUser :: (HasCallStack, MakesValue domain) => domain -> String -> String -> App ()
 registerInvitedUser domain tid email = do
-  getInvitationByEmail domain email
-    >>= getJSON 200
-    >>= getInvitationCodeForTeam domain tid
-    >>= getJSON 200
-    >>= (%. "code")
-    >>= asString
-    >>= registerUser domain email
-    >>= assertSuccess
+  code <- getInvitationByEmail domain email >>= getJSON 200 >>= getInvitationCodeForTeam domain tid >>= getJSON 200 >>= (%. "code") >>= asString
+  name <- getInvitationByCode domain code >>= getJSON 200 >>= flip lookupField "name" >>= maybe (pure "Alice") asString
+  registerUserWith domain email code name >>= assertSuccess
 
 getMetrics :: (HasCallStack, MakesValue domain) => domain -> Service -> App Response
 getMetrics domain service = do

integration/test/Test/Spar.hs

@@ -1424,3 +1424,45 @@ testNoPasswordResetForSAMLUser = do
   getPasswordResetCode OwnDomain email `bindResponse` \resp -> do
     resp.status `shouldMatchInt` 400
     resp.json %. "label" `shouldMatch` "invalid-key"
+
+testScimUserIsNotAllowedToChangeName :: (HasCallStack) => App ()
+testScimUserIsNotAllowedToChangeName = do
+  (owner, tid, _) <- createTeam OwnDomain 1
+  tok <- createScimToken owner def >>= getJSON 200 >>= (%. "token") >>= asString
+  scimUser <- randomScimUser
+  email <- scimUser %. "emails" >>= asList >>= assertOne >>= (%. "value") >>= asString
+  void $ createScimUser OwnDomain tok scimUser >>= assertSuccess
+  registerInvitedUser OwnDomain tid email
+  user <- getUsersByEmail OwnDomain [email] >>= getJSON 200 >>= asList >>= assertOne
+  putHandle user "foo" `bindResponse` \resp -> do
+    resp.status `shouldMatchInt` 403
+    resp.json %. "label" `shouldMatch` "managed-by-scim"
+  putSelf user def {name = Just "foo"} `bindResponse` \resp -> do
+    resp.status `shouldMatchInt` 403
+    resp.json %. "label" `shouldMatch` "managed-by-scim"
+
+testScimUserIsNotAllowedToChangeNameOnRegistering :: (HasCallStack) => App ()
+testScimUserIsNotAllowedToChangeNameOnRegistering = do
+  (owner, tid, _) <- createTeam OwnDomain 1
+  tok <- createScimToken owner def >>= getJSON 200 >>= (%. "token") >>= asString
+  scimUser <- randomScimUser
+  scimUserDisplayName <- scimUser %. "displayName"
+  email <- scimUser %. "emails" >>= asList >>= assertOne >>= (%. "value") >>= asString
+  scimUserId <- createScimUser OwnDomain tok scimUser >>= getJSON 201 >>= (%. "id") >>= asString
+  code <-
+    getInvitationByEmail OwnDomain email
+      >>= getJSON 200
+      >>= getInvitationCodeForTeam OwnDomain tid
+      >>= getJSON 200
+      >>= (%. "code")
+      >>= asString
+  getInvitationByCode OwnDomain code `bindResponse` \resp -> do
+    resp.status `shouldMatchInt` 200
+    resp.json %. "id" `shouldMatch` scimUserId
+    resp.json %. "managed_by" `shouldMatch` "scim"
+    resp.json %. "name" `shouldMatch` scimUserDisplayName
+
+  let newProfilename = "Takemiya Masaki"
+  registerUserWith OwnDomain email code newProfilename `bindResponse` \resp -> do
+    resp.status `shouldMatchInt` 403
+    resp.json %. "label" `shouldMatch` "managed-by-scim"

integration/test/Test/Teams.hs

@@ -500,3 +500,17 @@ testListUsersEmailVisibility = do
       returnedUsers <- resp.json %. "found" >>= asList
       returnedEmails <- for returnedUsers ((%. "email") >=> asString)
       returnedEmails `shouldMatchSet` memEmails
+
+testGetTeamsInvitationInfo :: (HasCallStack) => App ()
+testGetTeamsInvitationInfo = do
+  (owner, tid, _) <- createTeam OwnDomain 1
+  email <- randomEmail
+  inv <- postInvitation owner (PostInvitation (Just email) Nothing) >>= getJSON 201
+  code <- I.getInvitationCode owner inv >>= getJSON 200 >>= (%. "code") & asString
+  bindResponse (getInvitationByCode owner code) $ \resp -> do
+    resp.status `shouldMatchInt` 200
+    resp.json %. "email" `shouldMatch` email
+    resp.json %. "team" `shouldMatch` tid
+    resp.json %. "role" `shouldMatch` "member"
+    resp.json %. "id" `shouldMatch` (inv %. "id")
+    lookupField resp.json "managed_by" `shouldMatch` (Nothing :: Maybe Value)

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

@@ -667,8 +667,9 @@ type AccountAPI =
     -- - UserActivated event to created user, if it is a team invitation or user has an SSO ID
     -- - UserIdentityUpdated event to created user, if email code or phone code is provided
     Named
-      "register"
+      "register@v16"
       ( Summary "Register a new user."
+          :> Until 'V17
           :> Description
                "If the environment where the registration takes \
                \place is private and a registered email address \
@@ -678,6 +679,19 @@ type AccountAPI =
           :> ReqBody '[JSON] NewUserPublic
           :> MultiVerb 'POST '[JSON] RegisterResponses (Either RegisterError RegisterSuccess)
       )
+    :<|> Named
+           "register"
+           ( Summary "Register a new user."
+               :> From 'V17
+               :> Description
+                    "If the environment where the registration takes \
+                    \place is private and a registered email address \
+                    \is not whitelisted, a 403 error is returned."
+               :> "register"
+               :> Header' '[Required, Strict] "X-Forwarded-For" IpAddr
+               :> ReqBody '[JSON] NewUserPublic
+               :> MultiVerb 'POST '[JSON] RegisterResponses (Either RegisterError RegisterSuccess)
+           )
     -- This endpoint can lead to the following events being sent:
     -- UserDeleted event to contacts of deleted user
     -- MemberLeave event to members for all conversations the user was in (via galley)

libs/wire-api/src/Wire/API/Team/Invitation.hs

@@ -214,7 +214,8 @@ instance ToSchema AcceptTeamInvitation where
 
 data InvitationUserView = InvitationUserView
   { invitation :: Invitation,
-    inviterEmail :: Maybe EmailAddress
+    inviterEmail :: Maybe EmailAddress,
+    managedBy :: Maybe ManagedBy
   }
   deriving stock (Eq, Show, Generic)
   deriving (Arbitrary) via (GenericUniform InvitationUserView)
@@ -226,3 +227,4 @@ instance ToSchema InvitationUserView where
       InvitationUserView
         <$> invitation .= invitationObjectSchema
         <*> inviterEmail .= maybe_ (optField "created_by_email" schema)
+        <*> managedBy .= maybe_ (optField "managed_by" schema)

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

@@ -904,6 +904,7 @@ data RegisterError
   | RegisterErrorTooManyTeamMembers
   | RegisterErrorUserCreationRestricted
   | RegisterErrorEphemeralUserCreationDisabled
+  | RegisterErrorScimDisplayNameMismatch
   deriving (Show, Generic)
   deriving (AsUnion RegisterErrorResponses) via GenericAsUnion RegisterErrorResponses RegisterError
 
@@ -921,7 +922,8 @@ type RegisterErrorResponses =
      ErrorResponse 'BlacklistedEmail,
      ErrorResponse 'TooManyTeamMembers,
      ErrorResponse 'UserCreationRestricted,
-     ErrorResponse 'EphemeralUserCreationDisabled
+     ErrorResponse 'EphemeralUserCreationDisabled,
+     ErrorResponse 'NameManagedByScim
    ]
 
 type RegisterResponses =

libs/wire-api/test/golden/Test/Wire/API/Golden/Manual/InvitationUserView.hs

@@ -24,7 +24,7 @@ import Imports
 import Wire.API.Team.Invitation
 import Wire.API.Team.Role
 import Wire.API.User.Identity
-import Wire.API.User.Profile (Name (Name, fromName))
+import Wire.API.User.Profile
 
 testObject_InvitationUserView_team_1 :: InvitationUserView
 testObject_InvitationUserView_team_1 =
@@ -40,7 +40,8 @@ testObject_InvitationUserView_team_1 =
             inviteeName = Nothing,
             inviteeUrl = Nothing
           },
-      inviterEmail = Just $ unsafeEmailAddress "some" "example"
+      inviterEmail = Just $ unsafeEmailAddress "some" "example",
+      managedBy = Just ManagedByWire
     }
 
 testObject_InvitationUserView_team_2 :: InvitationUserView
@@ -57,5 +58,6 @@ testObject_InvitationUserView_team_2 =
             inviteeName = Just (Name {fromName = "\1067847} 2pGEW+\rT\171609p\174643\157218&\146145v0\b"}),
             inviteeUrl = Nothing
           },
-      inviterEmail = Nothing
+      inviterEmail = Nothing,
+      managedBy = Just ManagedByScim
     }

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

@@ -4,6 +4,7 @@
     "created_by_email": "some@example",
     "email": "some@example",
     "id": "00000002-0000-0001-0000-000200000000",
+    "managed_by": "wire",
     "name": null,
     "role": "admin",
     "team": "00000002-0000-0001-0000-000200000002",

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

@@ -3,6 +3,7 @@
     "created_by": "00000002-0000-0001-0000-000200000001",
     "email": "some@example",
     "id": "00000002-0000-0001-0000-000100000002",
+    "managed_by": "scim",
     "name": "􄭇} 2pGEW+\rT𩹙p𪨳𦘢&𣫡v0\u0008",
     "role": "partner",
     "team": "00000000-0000-0001-0000-000000000000",

services/brig/src/Brig/API/Public.hs

@@ -498,6 +498,7 @@ servantSitemap =
     accountAPI :: ServerT AccountAPI (Handler r)
     accountAPI =
       Named @"upgrade-personal-to-team" upgradePersonalToTeam
+        :<|> Named @"register@v16" createUserV16
         :<|> Named @"register" createUser
         :<|> Named @"verify-delete" verifyDeleteUser
         :<|> Named @"get-activate" activate
@@ -932,12 +933,43 @@ createUser ::
   IpAddr ->
   Public.NewUserPublic ->
   Handler r (Either Public.RegisterError Public.RegisterSuccess)
-createUser ip (Public.NewUserPublic new) = lift . runExceptT $ do
+createUser = createUserWith API.createUser
+
+createUserV16 ::
+  ( Member BlockListStore r,
+    Member GalleyAPIAccess r,
+    Member InvitationStore r,
+    Member (UserPendingActivationStore p) r,
+    Member (Input (Local ())) r,
+    Member TinyLog r,
+    Member UserKeyStore r,
+    Member UserStore r,
+    Member EmailSubsystem r,
+    Member Events r,
+    Member UserSubsystem r,
+    Member PasswordResetCodeStore r,
+    Member HashPassword r,
+    Member ActivationCodeStore r,
+    Member RateLimit r,
+    Member AuthenticationSubsystem r
+  ) =>
+  IpAddr ->
+  Public.NewUserPublic ->
+  Handler r (Either Public.RegisterError Public.RegisterSuccess)
+createUserV16 = createUserWith API.createUserV16
+
+createUserWith ::
+  (Member EmailSubsystem r, Member AuthenticationSubsystem r) =>
+  (RateLimitKey -> Public.NewUser PlainTextPassword8 -> ExceptT RegisterError (AppT r) CreateUserResult) ->
+  IpAddr ->
+  Public.NewUserPublic ->
+  Handler r (Either Public.RegisterError Public.RegisterSuccess)
+createUserWith createUserImpl ip (Public.NewUserPublic new) = lift . runExceptT $ do
   API.checkRestrictedUserCreation new
   for_ (Public.newUserEmail new) $
     mapExceptT wrapHttp . checkAllowlistWithError RegisterErrorAllowlistError
 
-  result <- API.createUser (RateLimitIp ip) new
+  result <- createUserImpl (RateLimitIp ip) new
   let acc = createdAccount result
 
   let eac = createdEmailActivation result

services/brig/src/Brig/API/User.hs

@@ -22,6 +22,7 @@ module Brig.API.User
   ( -- * User Accounts / Profiles
     upgradePersonalToTeam,
     createUser,
+    createUserV16,
     createUserSpar,
     createUserInviteViaScim,
     checkRestrictedUserCreation,
@@ -175,6 +176,21 @@ identityErrorToBrigError = \case
   IdentityErrorBlacklistedEmail -> StdError $ errorToWai @'E.BlacklistedEmail
   IdentityErrorUserKeyExists -> StdError $ errorToWai @'E.UserKeyExists
 
+-- SCIM invites carry a canonical display name end-to-end. The invitation info
+-- endpoint exposes it to the client, and when it is present the client locks the
+-- field and reuses that same value in the register request. Brig stores the same
+-- value in the pending SCIM user account, so in the SCIM case it is correct to
+-- validate the incoming request against `userDisplayName` rather than treating
+-- the request as user-chosen input.
+guardUserDisplayname ::
+  NewUser a ->
+  User ->
+  ExceptT RegisterError (AppT r) ()
+guardUserDisplayname new user =
+  when (user.userManagedBy == ManagedByScim) $
+    unless (new.newUserDisplayName == user.userDisplayName) $
+      throwE RegisterErrorScimDisplayNameMismatch
+
 verifyUniquenessAndCheckBlacklist ::
   ( Member BlockListStore r,
     Member UserKeyStore r,
@@ -356,7 +372,52 @@ createUser ::
   RateLimitKey ->
   NewUser PlainTextPassword8 ->
   ExceptT RegisterError (AppT r) CreateUserResult
-createUser rateLimitKey new = do
+createUser = createUserWith (flip guardUserDisplayname)
+
+createUserV16 ::
+  forall r p.
+  ( Member BlockListStore r,
+    Member GalleyAPIAccess r,
+    Member (UserPendingActivationStore p) r,
+    Member UserKeyStore r,
+    Member UserStore r,
+    Member UserSubsystem r,
+    Member TinyLog r,
+    Member Events r,
+    Member (Input (Local ())) r,
+    Member PasswordResetCodeStore r,
+    Member HashPassword r,
+    Member InvitationStore r,
+    Member ActivationCodeStore r,
+    Member RateLimit r
+  ) =>
+  RateLimitKey ->
+  NewUser PlainTextPassword8 ->
+  ExceptT RegisterError (AppT r) CreateUserResult
+createUserV16 = createUserWith (\_ _ -> pure ())
+
+createUserWith ::
+  forall r p.
+  ( Member BlockListStore r,
+    Member GalleyAPIAccess r,
+    Member (UserPendingActivationStore p) r,
+    Member UserKeyStore r,
+    Member UserStore r,
+    Member UserSubsystem r,
+    Member TinyLog r,
+    Member Events r,
+    Member (Input (Local ())) r,
+    Member PasswordResetCodeStore r,
+    Member HashPassword r,
+    Member InvitationStore r,
+    Member ActivationCodeStore r,
+    Member RateLimit r
+  ) =>
+  (User -> NewUser PlainTextPassword8 -> ExceptT RegisterError (AppT r) ()) ->
+  RateLimitKey ->
+  NewUser PlainTextPassword8 ->
+  ExceptT RegisterError (AppT r) CreateUserResult
+createUserWith checkScimDisplayName rateLimitKey new = do
   email <- fetchAndValidateEmail new
 
   -- get invitation and existing account
@@ -384,6 +445,9 @@ createUser rateLimitKey new = do
             luid :: Local UserId <- qualifyLocal' (coerce invid)
             User.getLocalAccountBy WithPendingInvitations luid
 
+  for_ mbExistingAccount $ \existingAccount ->
+    checkScimDisplayName existingAccount new
+
   let (new', mbHandle) = case mbExistingAccount of
         Nothing ->
           ( new {newUserIdentity = newIdentity email (newUserSSOId new)},

services/brig/src/Brig/Data/User.hs

@@ -22,6 +22,7 @@ module Brig.Data.User
   ( -- * Creation
     newStoredUser,
     newStoredUserViaScim,
+    invitationIdToUserId,
   )
 where
 
@@ -40,6 +41,10 @@ import Wire.API.User
 import Wire.AuthenticationSubsystem.Config
 import Wire.StoredUser
 
+-- | Pending invitation users reuse the invitation UUID as the user UUID.
+invitationIdToUserId :: InvitationId -> UserId
+invitationIdToUserId = Id . toUUID
+
 -- | Preconditions:
 --
 -- 1. @newUserUUID u == Just inv || isNothing (newUserUUID u)@.