User subsystem: add profile update operations #4046

pcapriotti · 2024-05-10T12:08:29Z

https://wearezeta.atlassian.net/browse/WPB-8880

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

Some of the functions in brig now have a UserStore constraint. This is only temporary until all the user-related functionality has been migrated to subsystems.

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

fisx · 2024-06-03T07:20:57Z

libs/wire-subsystems/src/Wire/UserStore/Cassandra.hs

+    -- PERFORMANCE(fisx): if a user changes 4 attributes with one request, the database will
+    -- be hit with 4 requests (one for each attribute).  this is probably fine as this
+    -- operation is not heavily used.  (also, the four operations are batched, which may or
+    -- may not help.)


anybody care to give a second opinion?

fisx · 2024-06-03T08:45:19Z

libs/wire-subsystems/src/Wire/UserStore.hs

@@ -1,13 +1,72 @@
 {-# LANGUAGE TemplateHaskell #-}
+{-# OPTIONS_GHC -Wno-ambiguous-fields #-}


What do you mean? Is this scheduled to become an error in ghc? What's the replacement?

fisx · 2024-06-03T08:47:38Z

libs/wire-subsystems/src/Wire/UserStore/Cassandra.hs

+      retry x5 $ write handleDelete (params LocalQuorum (Identity h))
+      let key = "@" <> fromHandle h
+      deleteClaim uid key (30 # Minute)
+    _ -> pure () -- this shouldn't happen, the call side should always check that `h` and `uid` belong to the same account.


this is very old code. let's not touch it in this PR?

fisx · 2024-06-03T08:49:20Z

libs/wire-subsystems/src/Wire/UserSubsystem.hs

+  -- | Simple updates (as opposed to, eg., handle, where we need to manage locks).  Empty fields are ignored (not deleted).
+  UpdateUserProfile :: Local UserId -> Maybe ConnId -> UpdateOriginType -> UserProfileUpdate -> UserSubsystem m ()
+  -- | parse and lookup a handle, return what the operation has found
+  CheckHandle :: Text {- use Handle here? -} -> UserSubsystem m CheckHandleResp
+  -- | checks a number of 'Handle's for availability and returns at most 'Word' amount of them
+  CheckHandles :: [Handle] -> Word -> UserSubsystem m [Handle]
+  -- | parses a handle, this may fail so it's effectful
+  UpdateHandle :: Local UserId -> Maybe ConnId -> UpdateOriginType -> Text {- use Handle here? -} -> UserSubsystem m ()


not decided yet, that's why we left the question. :)

fisx · 2024-06-03T08:49:49Z

libs/wire-subsystems/src/Wire/UserSubsystem.hs

+  -- | Simple updates (as opposed to, eg., handle, where we need to manage locks).  Empty fields are ignored (not deleted).
+  UpdateUserProfile :: Local UserId -> Maybe ConnId -> UpdateOriginType -> UserProfileUpdate -> UserSubsystem m ()
+  -- | parse and lookup a handle, return what the operation has found
+  CheckHandle :: Text {- use Handle here? -} -> UserSubsystem m CheckHandleResp
+  -- | checks a number of 'Handle's for availability and returns at most 'Word' amount of them
+  CheckHandles :: [Handle] -> Word -> UserSubsystem m [Handle]
+  -- | parses a handle, this may fail so it's effectful
+  UpdateHandle :: Local UserId -> Maybe ConnId -> UpdateOriginType -> Text {- use Handle here? -} -> UserSubsystem m ()


i think it's better to keep the question rather than making the impression to future readers that nobody has ever thought about this.

fisx · 2024-06-03T09:09:04Z

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

@@ -544,17 +561,18 @@ listActivatedAccountsH
      u4 <- (\phone -> API.lookupAccountsByIdentity (Right phone) includePendingInvitations) `mapM` phones
      pure $ u1 <> u2 <> join u3 <> join u4

+-- FUTUREWORK: this should use UserStore only through UserSubsystem.


maybe resolve that now?

nah, next PR!

services/brig/test/integration/API/UserPendingActivation.hs

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

fisx · 2024-06-03T11:28:48Z

libs/wire-subsystems/src/Wire/MiniBackend.hs

@@ -52,28 +75,41 @@ newtype NotPendingStoredUser = NotPendingStoredUser StoredUser

 instance Arbitrary NotPendingStoredUser where
  arbitrary = do
-    user <- arbitrary
+    user <- arbitrary `suchThat` \user -> isJust user.identity


because it's not trivial (nothing is, not even this conversation) and serves no practical purpose? sorry :). let's discuss in person if that doesn't convince you.

fisx · 2024-06-03T11:30:47Z

libs/wire-subsystems/src/Wire/MiniBackend.hs

@@ -52,28 +75,41 @@ newtype NotPendingStoredUser = NotPendingStoredUser StoredUser

 instance Arbitrary NotPendingStoredUser where
  arbitrary = do
-    user <- arbitrary
+    user <- arbitrary `suchThat` \user -> isJust user.identity


(i would prefer to think / talk about the question how to reduce the amount of assumptions that goes into this type. like, is an arbitrary user consistent if we merely change the status?)

This reverts commit 8c66230. (maybe this is used elsewhere? also the removal wasn't complete.)

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

fisx · 2024-06-04T09:07:06Z

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

@@ -544,17 +561,18 @@ listActivatedAccountsH
      u4 <- (\phone -> API.lookupAccountsByIdentity (Right phone) includePendingInvitations) `mapM` phones
      pure $ u1 <> u2 <> join u3 <> join u4

+-- FUTUREWORK: this should use UserStore only through UserSubsystem.


nah, next PR!

…o update-profile

This reverts commit 4059bf9.

This reverts commit c40670e.

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label May 10, 2024

MangoIV force-pushed the update-profile branch from aa96349 to b23985e Compare May 13, 2024 12:42

pcapriotti force-pushed the update-profile branch from 3896b3c to c250c5f Compare May 14, 2024 14:33

pcapriotti force-pushed the update-profile branch from 0211289 to 8cf862d Compare May 22, 2024 08:08

pcapriotti and others added 22 commits May 22, 2024 17:27

UpdateUser operation; work on Error sub-effects.

8c4ec63

Fix build

2b044a8

Rename onUserEvent → generateUserEvent

1e17a86

Use state effect for local users in mini backends

a1352a4

Implement user update in mini backends

0466072

Add user update property test

478693d

[feat] rethrow errors as wai errors

7d70e79

[feat] test behaviour if user is managed by scim

6d6695c

[feat] set galley api access

1ed40db

[feat] test all of the update record members

17b5155

[feat] interpret user events

619d032

Use MiniBackend state effect in mini-backend stack

b34debf

Add fake event interpreter

e58521e

Add Arbitrary instance for AllowSCIMUpdates

9ee4efe

Replace UserUpdate with a new type

2bbb388

Use update functionality in brig

dc57f85

Add locale update to user subsystem

898b917

Move allowScim argument to update structure

696342a

Add handle update functionality to UserSubsystem

11dc928

Some of the functions in brig now have a UserStore constraint. This is only temporary until all the user-related functionality has been migrated to subsystems.

Make sure NotPending users have an identity

e38f08b

Check claimed handles

8c79463

Implement handle lookup in mini backend

3f3ac72

pcapriotti force-pushed the update-profile branch from 8cf862d to 3f3ac72 Compare May 22, 2024 15:32

pcapriotti and others added 3 commits May 23, 2024 11:06

Add DeleteUser action to UserStore

8f211d8

Add some TODOs

9617c93

Lint and format

1ea9657

fisx added 5 commits June 3, 2024 13:14

Rename local function.

07d53e5

Fix test case.

948b889

Merge remote-tracking branch 'origin/develop' into update-profile

49c0cf6

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

remove obsolete changelog entry (this has been fixed in WPB-9488).

555161e

Rm dead code from rest api.

8c66230

fisx approved these changes Jun 3, 2024

View reviewed changes

fisx added 3 commits June 3, 2024 13:51

Revert "Rm dead code from rest api."

e039e95

This reverts commit 8c66230. (maybe this is used elsewhere? also the removal wasn't complete.)

Update services/brig/test/integration/API/UserPendingActivation.hs

79fe0e5

Merge remote-tracking branch 'origin/develop' into update-profile

874d7ba

fisx approved these changes Jun 4, 2024

View reviewed changes

fisx added 14 commits June 4, 2024 11:07

Update services/brig/src/Brig/User/Auth.hs

149854d

More guards in unit tests against invalid arbitrary values.

88b0190

Merge remote-tracking branch 'refs/remotes/origin/update-profile' int…

7fa4efa

…o update-profile

Fixup

93ee32d

Fix test case.

4059bf9

Improve error message for *ManagedByScim.

385a29a

Revert "Fix test case."

298c406

This reverts commit 4059bf9.

Fix application logic around blocking updates because scim or e2eid.

829c88a

Merge remote-tracking branch 'origin/develop' into update-profile

b9067a7

hlint.

5f50929

failed attempt to port a galley test to /integration

c40670e

Revert "failed attempt to port a galley test to /integration"

66a9abc

This reverts commit c40670e.

I think I found the problem with this test! (fix coming up)

f927d98

Small fix for legacy integration test.

4ac5de7

fisx mentioned this pull request Jun 6, 2024

Forbid locale updates for scim-managed users. #4078

Closed

2 tasks

fisx merged commit 5d37f6c into develop Jun 6, 2024
9 checks passed

fisx deleted the update-profile branch June 6, 2024 09:19

echoes-hq bot added echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. labels Jun 27, 2024

zebot mentioned this pull request Jul 8, 2024

Release 2024-07-08 - (expected chart version 5.4.0) #4126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User subsystem: add profile update operations #4046

User subsystem: add profile update operations #4046

pcapriotti commented May 10, 2024

fisx Jun 3, 2024

fisx Jun 3, 2024

fisx Jun 3, 2024

fisx Jun 3, 2024

fisx Jun 3, 2024

fisx Jun 3, 2024

fisx Jun 4, 2024

fisx Jun 3, 2024

fisx Jun 3, 2024

fisx Jun 4, 2024

		@@ -1,13 +1,72 @@
		{-# LANGUAGE TemplateHaskell #-}
		{-# OPTIONS_GHC -Wno-ambiguous-fields #-}

User subsystem: add profile update operations #4046

User subsystem: add profile update operations #4046

Conversation

pcapriotti commented May 10, 2024

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment