Clean up SCIM-invited users with expired invitation

[smatting]

Implements the clean up job discussed in #1238

https://wearezeta.atlassian.net/browse/SQSERVICES-26

[fisx]

I restarted the CI: https://bridge-ie-concourse.zinfra.io/teams/main/pipelines/wire-server-pr/jobs/wire-server-integration/builds/1683

[fisx]

There are no tests whatsoever. Not sure how to test this, and where, perhaps we can think about this together.

Sorry, lots of ideas how to change this. Do you want to go through them together?

[fisx]

I think this is duplicating existing code. Can we move the old function somewhere from where we can call it here as well? Or is it awkward to generalize? (Not that important.)

[smatting]

It's already duplicated 3 times. I created a separate issue for this https://wearezeta.atlassian.net/browse/SQSERVICES-146

[fisx]

Yeah, we should keep an eye on this.

I suggest we:

• add a function deleteUsersNoVerify that deletes many users
• we can implement it for now using mapM_
• but we should call it only once per page of PendingActivationExpiration
• and keep a metric that tells us how often this is called
• ideally, we should also have a metric that tells us how much time & space deletion takes, but that may be too much work for now.

[smatting]

To avoid scheduling too many deletions too fast, I added a throttling delay, which will schedule at most 2 items per second (170k per day).

Implemented suggestions 1-4

[jschaul]

one more thing: what happens if a user gets deleted by means other than expiring invitation? it will still have an entry in the new table, right? will the gc loop choke on that entry? solution may be to modify the async delete-user routine to also handle the new table.

I think use deletions are idempotent; so you can attempt to delete the same user over and over again with the same result. It might be nice to (if not already done, I didn't look at the code, just replying to the previous comment), upon regular user deletions, as part of the deletion onEvent flow also delete the entry in this new expiry-tracking table (which will be a no-op if no entry under that key exists)

[smatting]

If the GC loop cannot find a user then isPendingInvitation will be False:

isPendingInvitation <- (Just PendingInvitation ==) <$> API.lookupStatus uid

The user then skips deletion and the entry is removed from the expiry table.

Currently there are no calls to remove an entry from the expiry table except for the loop. The GC loop removes entries from the expiry table by itself. I have slight prefrence for not adding it as a hook to the user deletion, because this creates two call sites that remove entries from the table, one of which is not even strictly necessary.

I have another clarification question for @fisx : Is it possible that spar creates a second invitation for a user after the first invitation is expired? If so then I need to implement this: "every user has at most on entry in the expiry table, which will be overwritten with every invitation".

[jschaul]

then I need to implement this: "every user has at most on entry in the expiry table, which will be overwritten with every invitation".

If needed, this implementation can be achieved easily with the following:

        (
          user            uuid
        , expires_at      timestamp
-        , primary key (user, expires_at)
+        , primary key (user)
        )       

I believe at this point you don't need to have multiple entries for the same user, so you don't need a composite primary key.

[smatting]

If needed, this implementation can be achieved easily with the following:

        (
          user            uuid
        , expires_at      timestamp
-        , primary key (user, expires_at)
+        , primary key (user)
        )       

Thanks! Applied.

[fisx]

2a64d28

[smatting]

ah, nice. Didn't see that.

[fisx]

I've committed one more change, pls check whether this makes sense, and if you agree