Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SQSERVICES-253] Support provisioning role information with scim PATCH #2855

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion changelog.d/2-features/pr-2851

This file was deleted.

1 change: 1 addition & 0 deletions changelog.d/2-features/pr-2855
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
A team member's role can now be provisioned via SCIM (#2851, #2855)
9 changes: 6 additions & 3 deletions libs/hscim/src/Web/Scim/Schema/User.hs
Original file line number Diff line number Diff line change
Expand Up @@ -315,12 +315,14 @@ applyUserOperation user (Operation Replace (Just (NormalPath (AttrPath _schema a
(\x -> user {externalId = x}) <$> resultToScimError (fromJSON value)
"active" ->
(\x -> user {active = x}) <$> resultToScimError (fromJSON value)
_ -> throwError (badRequest InvalidPath (Just "we only support attributes username, displayname, externalid, active"))
"roles" ->
(\x -> user {roles = x}) <$> resultToScimError (fromJSON value)
_ -> throwError (badRequest InvalidPath (Just "we only support attributes username, displayname, externalid, active, roles"))
applyUserOperation _ (Operation Replace (Just (IntoValuePath _ _)) _) = do
throwError (badRequest InvalidPath (Just "can not lens into multi-valued attributes yet"))
applyUserOperation user (Operation Replace Nothing (Just value)) = do
case value of
Object hm | null ((AttrName . Key.toText <$> KeyMap.keys hm) \\ ["username", "displayname", "externalid", "active"]) -> do
Object hm | null ((AttrName . Key.toText <$> KeyMap.keys hm) \\ ["username", "displayname", "externalid", "active", "roles"]) -> do
(u :: User tag) <- resultToScimError $ fromJSON value
pure $
user
Expand All @@ -329,7 +331,7 @@ applyUserOperation user (Operation Replace Nothing (Just value)) = do
externalId = externalId u,
active = active u
}
_ -> throwError (badRequest InvalidPath (Just "we only support attributes username, displayname, externalid, active"))
_ -> throwError (badRequest InvalidPath (Just "we only support attributes username, displayname, externalid, active, roles"))
battermann marked this conversation as resolved.
Show resolved Hide resolved
applyUserOperation _ (Operation Replace _ Nothing) =
throwError (badRequest InvalidValue (Just "No value was provided"))
applyUserOperation _ (Operation Remove Nothing _) = throwError (badRequest NoTarget Nothing)
Expand All @@ -339,6 +341,7 @@ applyUserOperation user (Operation Remove (Just (NormalPath (AttrPath _schema at
"displayname" -> pure $ user {displayName = Nothing}
"externalid" -> pure $ user {externalId = Nothing}
"active" -> pure $ user {active = Nothing}
"roles" -> pure $ user {roles = []}
_ -> pure user
applyUserOperation _ (Operation Remove (Just (IntoValuePath _ _)) _) = do
throwError (badRequest InvalidPath (Just "can not lens into multi-valued attributes yet"))
Expand Down
1 change: 0 additions & 1 deletion libs/hscim/test/Test/Schema/UserSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ spec = do
("photos", toJSON @[Photo] mempty),
("addresses", toJSON @[Address] mempty),
("entitlements", toJSON @[Text] mempty),
("roles", toJSON @[Text] mempty),
("x509Certificates", toJSON @[Certificate] mempty)
]
$ \(key, upd) -> do
Expand Down
49 changes: 49 additions & 0 deletions services/spar/test-integration/Test/Spar/Scim/UserSpec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ import qualified Web.Scim.Filter as Filter
import qualified Web.Scim.Schema.Common as Scim
import qualified Web.Scim.Schema.ListResponse as Scim
import qualified Web.Scim.Schema.Meta as Scim
import Web.Scim.Schema.PatchOp (Operation)
import qualified Web.Scim.Schema.PatchOp as PatchOp
import qualified Web.Scim.Schema.User as Scim.User
import qualified Wire.API.Team.Export as CsvExport
Expand Down Expand Up @@ -1823,6 +1824,11 @@ specPatchUser = do
PatchOp.Replace
(Just (PatchOp.NormalPath (Filter.topLevelAttrPath name)))
(Just (toJSON value))
let addAttrib name value =
PatchOp.Operation
PatchOp.Add
(Just (PatchOp.NormalPath (Filter.topLevelAttrPath name)))
(Just (toJSON value))
let removeAttrib name =
PatchOp.Operation
PatchOp.Remove
Expand Down Expand Up @@ -1900,6 +1906,8 @@ specPatchUser = do
[replaceAttrib "externalId" externalId]
let user'' = Scim.value . Scim.thing $ storedUser'
liftIO $ Scim.User.externalId user'' `shouldBe` externalId
it "replace role works" $ testPatchRole replaceAttrib
it "add role works" $ testPatchRole addAttrib
it "replacing every supported atttribute at once works" $ do
(tok, _) <- registerIdPAndScimToken
user <- randomScimUser
Expand Down Expand Up @@ -1967,6 +1975,47 @@ specPatchUser = do
let patchOp = PatchOp.PatchOp [removeAttrib "externalId"]
patchUser_ (Just tok) (Just userid) patchOp (env ^. teSpar) !!! const 400 === statusCode

testPatchRole :: (Text -> [Role] -> Operation) -> TestSpar ()
testPatchRole replaceOrAdd = do
env <- ask
let brig = env ^. teBrig
let galley = env ^. teGalley
(owner, tid) <- call $ createUserWithTeam brig galley
tok <- registerScimToken tid Nothing
let testWithInitialRole r = forM_ [minBound .. maxBound] (testPatchRoles brig replaceOrAdd tid owner tok r)
forM_ [minBound .. maxBound] testWithInitialRole
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggested a different way to write this, but this time i have another suggestion :-)

Suggested change
let testWithInitialRole r = forM_ [minBound .. maxBound] (testPatchRoles brig replaceOrAdd tid owner tok r)
forM_ [minBound .. maxBound] testWithInitialRole
let values = [[], ["member", "admin"], ["notarole"]] <> (:[]) <$> [minBound .. maxBound]
testWithInitialRole r = forM_ values (testPatchRoles brig replaceOrAdd tid owner tok r)
forM_ values testWithInitialRole

(this would require to take [Role] instead of Role in initial and target below.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this trick could also be used to simplify the tests for POST, PUT?

Copy link
Contributor Author

@battermann battermann Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggested a different way to write this, but this time i have another suggestion :-)

I know, but that didn't compile, and I tried to come as close as possible.


testPatchRoles :: BrigReq -> (Text -> [Role] -> Operation) -> TeamId -> UserId -> ScimToken -> Role -> Role -> TestSpar ()
testPatchRoles brig replaceOrAdd tid owner tok initialRole targetRole = do
email <- randomEmail
scimUser <-
randomScimUser <&> \u ->
u
{ Scim.User.externalId = Just $ fromEmail email,
Scim.User.roles = [cs $ toByteString initialRole]
}
scimStoredUser <- createUser tok scimUser
let userid = scimUserId scimStoredUser
userName = Name . fromJust . Scim.User.displayName $ scimUser

-- user follows invitation flow
do
inv <- call $ getInvitation brig email
Just inviteeCode <- call $ getInvitationCode brig tid (inInvitation inv)
registerInvitation email userName inviteeCode True
checkTeamMembersRole tid owner userid initialRole

_ <- patchUser tok userid $ PatchOp.PatchOp [replaceOrAdd "roles" [targetRole]]
checkTeamMembersRole tid owner userid targetRole
-- also check if remove works
let removeAttrib name =
PatchOp.Operation
PatchOp.Remove
(Just (PatchOp.NormalPath (Filter.topLevelAttrPath name)))
Nothing
_ <- patchUser tok userid $ PatchOp.PatchOp [removeAttrib "roles"]
checkTeamMembersRole tid owner userid defaultRole

----------------------------------------------------------------------------
-- Deleting users

Expand Down