Notify Spar when a team is deleted #519

neongreen · 2018-11-22T13:38:54Z

Delete tokens when the team is deleted
~~Delete tokens when the issuer is deleted?~~
PRs to hegemony and cailleach about adding Spar to galley.yaml

neongreen · 2018-12-06T15:32:41Z

@fisx What other things should I do in Spar when a team is deleted? Should I delete relevant entries from team_idp?

fisx · 2018-12-06T18:56:14Z

@fisx What other things should I do in Spar when a team is deleted? Should I delete relevant entries from team_idp?

tables to be cleaned up from looking at services/spar/schema/src/V*.hs: user, idp, issuer_idp, team_idp, team_provisioning_by_team, team_provisioning_by_token.

it should be straight-forward to decide which rows are dangling and need to removed, no?

the other tables should be harmless: bind cookies, authn_requests, assertions, verdict formats are all only needed during or after an authentication response has been validated, which won't be successful without the issuer mapping to an idp id.

not sure this answers your question?

neongreen · 2018-12-10T09:25:28Z

@fisx Please look at the last commit.

fisx · 2018-12-10T10:40:40Z

@fisx Please look at the last commit.

looks good! i'll review the rest now.

fisx

(Let me know if you don't want to write the test yourself.)

fisx · 2018-12-10T10:55:02Z

services/galley/src/Galley/Intra/Spar.hs

+
+-- | Notify Spar that a team is being deleted.
+deleteTeam :: TeamId -> Galley ()
+deleteTeam tid = do


Can't find a test. I would add it to spar integration tests and make it create a team, owner, idp, two sso users create via a scim token, and then delete it all and look at the scim, saml data in spar, user data on brig, and team data on galley. Helpers of interest:

wire-server/services/spar/test-integration/Util/SCIM.hs

Lines 46 to 49 in ce105f4

registerIdPAndSCIMToken :: HasCallStack => TestSpar (ScimToken, (UserId, TeamId, IdP))

registerIdPAndSCIMToken = do

team@(_owner, teamid, idp) <- registerTestIdP

(, team) <$> registerSCIMToken teamid (Just (idp ^. idpId))

wire-server/services/spar/test-integration/Util/Core.hs

Lines 659 to 676 in ce105f4

runSparCass

:: (HasCallStack, m ~ Client, MonadIO m', MonadReader TestEnv m')

=> m a -> m' a

runSparCass action = do

env <- ask

liftIO $ runClient (env ^. teCql) action

runSparCassWithEnv

:: ( HasCallStack

, m ~ ReaderT Data.Env (ExceptT TTLError Cas.Client)

, MonadIO m', MonadReader TestEnv m'

)

=> m a -> m' a

runSparCassWithEnv action = do

env <- ask

denv <- Data.mkEnv <$> (pure $ env ^. teOpts) <*> liftIO getCurrentTime

val <- runSparCass (runExceptT (action `runReaderT` denv))

either (liftIO . throwIO . ErrorCall . show) pure val

you can write helpers to connect to brig, galley cassandras in analogy to runSparCass.

neongreen · 2018-12-12T17:18:15Z

@fisx The test has been written by me and Tiago

neongreen · 2018-12-12T17:18:25Z

Me and Tiago wrote the test

fisx

Some comments, but if you are confident that the two tests that you deleted serve no purpose then I'm happy with this.

fisx · 2018-12-12T19:25:45Z

services/spar/src/Spar/API.hs

-          issuer = idp ^. SAML.idpMetadata . SAML.edIssuer
-      Data.deleteIdPConfig idpid issuer team
-      Data.deleteUsersByIssuer issuer
+  wrapMonadClient $ Data.deleteTeam team


fisx · 2018-12-12T19:27:06Z

services/spar/test-integration/Test/Spar/SCIMSpec.hs

 import Bilge
 import Bilge.Assert
 import Control.Lens
 import Data.ByteString.Conversion
-import Imports


sorry... :-/

fisx · 2018-12-12T19:29:28Z

services/spar/test-integration/Test/Spar/DataSpec.hs

+    --
+    -- The token from 'team_provisioning_by_token':
+    do tokenInfo <- runSparCass $ Data.lookupScimToken tok
+       liftIO $ tokenInfo `shouldBe` Nothing


[mumbling to myself] there is shouldReturn, which would make this more concise and arguably more readable, but it's in IO and not in MonadIO, so I'm not sure how to rewrite this. i really should write mutants of all the should* functions in MonadIO...

should functions should absolutely be in MonadIO and I've also been mumbling to myself about this for quite some time.

We can use http://hackage.haskell.org/package/hspec-expectations-lifted-0.10.0/docs/Test-Hspec-Expectations-Lifted.html but I'd rather have a homegrown library which would also contain some other common expectations that I end up reimplementing from time to time.

fisx · 2018-12-12T19:30:36Z

services/spar/test-integration/Test/Spar/DataSpec.hs

+    do let issuer = idp ^. SAML.idpMetadata . SAML.edIssuer
+       mbIdp <- runSparCass $ Data.getIdPIdByIssuer issuer
+       liftIO $ mbIdp `shouldBe` Nothing
+    do idps <- runSparCass $ Data.getIdPConfigsByTeam tid


this one doesn't have a comment line. :-)

fisx · 2018-12-12T19:33:30Z

services/spar/test-integration/Test/Spar/DataSpec.hs

@@ -203,20 +210,44 @@ testSPStoreID store unstore isalive = do
        isit <- runSparCassWithEnv $ isalive xid
        liftIO $ isit `shouldBe` True

-    context "after TTL" $ do


why did these two tests go away? can i have them back?

I have no idea :/ I'll get them back.

neongreen · 2018-12-12T19:42:29Z

Fixed everything. Sorry for the force-push though

tiago-loureiro

FWIW 👍

neongreen mentioned this pull request Nov 22, 2018

SCIM authentication #508

Merged

neongreen force-pushed the scim-new-auth branch 2 times, most recently from bfa7302 to 75c5fcb Compare November 26, 2018 23:04

neongreen force-pushed the scim-new-auth branch from eb0153d to 18ef172 Compare November 30, 2018 14:43

neongreen force-pushed the spar-delete-team branch from bd1108e to 42cd509 Compare December 6, 2018 15:11

neongreen changed the base branch from scim-new-auth to develop December 6, 2018 15:11

neongreen force-pushed the spar-delete-team branch from 42cd509 to cfe7489 Compare December 6, 2018 15:27

neongreen changed the title ~~[WIP] Notify Spar when a team is deleted~~ Notify Spar when a team is deleted Dec 10, 2018

neongreen requested a review from fisx December 10, 2018 09:25

fisx requested changes Dec 10, 2018

View reviewed changes

neongreen force-pushed the spar-delete-team branch from 9e1240f to e12c968 Compare December 12, 2018 17:16

Artyom Kazak added 2 commits December 12, 2018 18:22

Notify Spar when a team is deleted

7fbb847

Clean up the other tables, too

65aab30

neongreen force-pushed the spar-delete-team branch from e12c968 to 320fa15 Compare December 12, 2018 17:23

fisx approved these changes Dec 12, 2018

View reviewed changes

Write a test

d9aad9a

neongreen force-pushed the spar-delete-team branch from 320fa15 to d9aad9a Compare December 12, 2018 19:42

neongreen merged commit 27bf3e0 into develop Dec 12, 2018

neongreen deleted the spar-delete-team branch December 12, 2018 20:54

tiago-loureiro reviewed Dec 18, 2018

View reviewed changes

jschaul mentioned this pull request Jan 10, 2019

Release/2019 01 10 #567

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notify Spar when a team is deleted #519

Notify Spar when a team is deleted #519

neongreen commented Nov 22, 2018 •

edited

neongreen commented Dec 6, 2018

fisx commented Dec 6, 2018

neongreen commented Dec 10, 2018

fisx commented Dec 10, 2018

fisx left a comment

fisx Dec 10, 2018

neongreen commented Dec 12, 2018

neongreen commented Dec 12, 2018

fisx left a comment

fisx Dec 12, 2018

fisx Dec 12, 2018

fisx Dec 12, 2018

neongreen Dec 12, 2018

fisx Dec 12, 2018

fisx Dec 12, 2018

neongreen Dec 12, 2018

neongreen commented Dec 12, 2018

tiago-loureiro left a comment

	registerIdPAndSCIMToken :: HasCallStack => TestSpar (ScimToken, (UserId, TeamId, IdP))
	registerIdPAndSCIMToken = do
	team@(_owner, teamid, idp) <- registerTestIdP
	(, team) <$> registerSCIMToken teamid (Just (idp ^. idpId))

	runSparCass
	:: (HasCallStack, m ~ Client, MonadIO m', MonadReader TestEnv m')
	=> m a -> m' a
	runSparCass action = do
	env <- ask
	liftIO $ runClient (env ^. teCql) action

	runSparCassWithEnv
	:: ( HasCallStack
	, m ~ ReaderT Data.Env (ExceptT TTLError Cas.Client)
	, MonadIO m', MonadReader TestEnv m'
	)
	=> m a -> m' a
	runSparCassWithEnv action = do
	env <- ask
	denv <- Data.mkEnv <$> (pure $ env ^. teOpts) <*> liftIO getCurrentTime
	val <- runSparCass (runExceptT (action `runReaderT` denv))
	either (liftIO . throwIO . ErrorCall . show) pure val

Notify Spar when a team is deleted #519

Notify Spar when a team is deleted #519

Conversation

neongreen commented Nov 22, 2018 • edited

neongreen commented Dec 6, 2018

fisx commented Dec 6, 2018

neongreen commented Dec 10, 2018

fisx commented Dec 10, 2018

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen commented Dec 12, 2018

neongreen commented Dec 12, 2018

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neongreen commented Dec 12, 2018

tiago-loureiro left a comment

Choose a reason for hiding this comment

neongreen commented Nov 22, 2018 •

edited