Move password reset code to AuthenticationSubsystem #4086

battermann · 2024-06-06T15:31:33Z

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

Checklist

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

fisx

glanced through about half of the PR. looks good!

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

pcapriotti

LGTM

libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserKeyStore.hs

…operty

Also implement mock argon2id hash interpreter correctly. Sadly, this makes the tests very slow.

…user-subsystem-add-password-reset-operation-s

This reverts commit 57ef069.

(this change is deferred to separate PR)

git revert --no-commit 70844d2 git revert --no-commit e86ab71 git revert --no-commit 25da81c

fisx · 2024-06-28T09:14:02Z

one more failure (flake? unrelated? serious?) -

      failure - invalid signature:                                                                   FAIL
        Exception: Error executing request: token invalid
        CallStack (from HasCallStack):
          error, called at test/integration/API/OAuth.hs:417:26 in main:API.OAuth
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:101:18 in bilge-0.22.0-6Cxcfrpv0gi69IBgCcDsiP:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-6Cxcfrpv0gi69IBgCcDsiP:Bilge.Assert
          !!!, called at test/integration/API/OAuth.hs:419:82 in main:API.OAuth
        Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&/failure - invalid signature/' to rerun this test only.

fisx · 2024-06-28T09:58:08Z

one more failure (flake? unrelated? serious?) -

      failure - invalid signature:                                                                   FAIL
        Exception: Error executing request: token invalid
        CallStack (from HasCallStack):
          error, called at test/integration/API/OAuth.hs:417:26 in main:API.OAuth
        CallStack (from HasCallStack):
          error, called at src/Bilge/Assert.hs:101:18 in bilge-0.22.0-6Cxcfrpv0gi69IBgCcDsiP:Bilge.Assert
          <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-6Cxcfrpv0gi69IBgCcDsiP:Bilge.Assert
          !!!, called at test/integration/API/OAuth.hs:419:82 in main:API.OAuth
        Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&/failure - invalid signature/' to rerun this test only.

flake!

fisx · 2024-06-28T16:44:36Z

integration tests run times on CI:

base line: 9:26:10-9:49:48 => ~23
this PR, last run: 13:54:41-14:15:31 => ~19

not sure how representative these figures are, but they sure don't provide cause for worry. :-)

services/brig/src/Brig/CanonicalInterpreter.hs

Co-authored-by: Akshay Mankar <akshay@wire.com>

fisx · 2024-07-01T06:22:09Z

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

@@ -94,7 +94,7 @@ newCookie uid cid typ label = do
            cookieSucc = Nothing,
            cookieValue = tok
          }
-  DB.insertCookie uid c Nothing
+  adhocSessionStoreInterpreter $ Store.insertCookie uid (toUnitCookie c) Nothing


couldn't the effect signature have Cookie any instead of Cookie () to make toUnitCookie unnecessary?

anyway i don't want to create another hold-up. just wondering.

It was that, but then writing the mock was a pain. I thought it made more sense anyway to make it () so its clear that the value is not stored.

fisx · 2024-07-01T06:58:04Z

libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserSubsystem.hs

+  interpret \case
+    GetLocalUserAccountByUserKey localUserKey -> case (tUnqualified localUserKey) of
+      UserEmailKey (EmailKey _ email) -> pure $ find (\u -> userEmail u.accountUser == Just email) initialUsers
+      UserPhoneKey _ -> pure Nothing -- Phone stuff is deprecated and soon to be deleted anyway


i would have made this an error, but ok.

…ser-subsystem-add-password-reset-operation-s' into WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s

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

fisx reviewed Jun 12, 2024

View reviewed changes

libs/wire-subsystems/src/Wire/MiniBackend.hs Show resolved Hide resolved

fisx force-pushed the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch 3 times, most recently from e6f4ec2 to 1e1db5c Compare June 17, 2024 11:41

akshaymankar force-pushed the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch from 1e1db5c to e648388 Compare June 17, 2024 12:46

fisx force-pushed the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch 2 times, most recently from f392fc4 to 207284e Compare June 20, 2024 08:58

akshaymankar force-pushed the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch from 18ae8b8 to 50150df Compare June 25, 2024 09:40

akshaymankar changed the title ~~WPB-8890 UserSubsystem Password reset~~ Move password reset code to AuthenticationSubsystem Jun 25, 2024

akshaymankar force-pushed the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch from 42850d1 to 52dfb19 Compare June 26, 2024 08:55

pcapriotti reviewed Jun 26, 2024

View reviewed changes

libs/wire-subsystems/test/unit/Wire/MockInterpreters/UserKeyStore.hs Outdated Show resolved Hide resolved

akshaymankar marked this pull request as ready for review June 26, 2024 09:13

akshaymankar approved these changes Jun 26, 2024

View reviewed changes

akshaymankar force-pushed the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch from 7b7be56 to baf9c13 Compare June 26, 2024 12:56

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

battermann and others added 13 commits June 27, 2024 09:33

wip: moving effects to wire-subsystem

12a919b

Changelog

655e639

Fix merge conflict problem

369dd7e

Unused imports

d189221

No such thing as Local UserKey

d9e49ab

WIP: AuthenticationInterpreterSpec

9adbd10

AuthenticationSubsystem: Make happy path test for password reset a pr…

d373b33

…operty

password store interpreter, fix errors

3f39476

fix undefined usage

3079543

also check allowlists

37d3a83

Move mock interpreters into their own modules

ae85aeb

Also implement mock argon2id hash interpreter correctly. Sadly, this makes the tests very slow.

Reexport all MockInterpreters from one module

a6d85eb

Move MiniBackend to tests and remove unused effects

f68cbcb

akshaymankar added 4 commits June 27, 2024 09:33

Remove unnecessary runErrorUnsafe calls

c9b9777

regen nix

c49767d

Rename static interpreters to "inMemory"

c0df7bc

hlint

9eff1d1

akshaymankar force-pushed the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch from baf9c13 to 9eff1d1 Compare June 27, 2024 07:33

akshaymankar and others added 10 commits June 27, 2024 14:52

Enable service monitors in CI

25da81c

brig: log slow requests for debugging

e86ab71

log duration

70844d2

brig: Log request ids

6953975

Fixup

856b2d6

Merge remote-tracking branch 'origin/develop' into WPB-8890-polysemy-…

1da3aef

…user-subsystem-add-password-reset-operation-s

what do you think CI?

fe58f1c

Revert "what do you think CI?"

df9a365

This reverts commit 57ef069.

Roll back from argon2id to scrypt.

40182c5

(this change is deferred to separate PR)

Revert changes for CI inspection (makes things slow).

0b00505

git revert --no-commit 70844d2 git revert --no-commit e86ab71 git revert --no-commit 25da81c

Update changelog.

56039cf

hi ci

b182427

akshaymankar reviewed Jul 1, 2024

View reviewed changes

services/brig/src/Brig/CanonicalInterpreter.hs Outdated Show resolved Hide resolved

Update services/brig/src/Brig/CanonicalInterpreter.hs

19d26ae

Co-authored-by: Akshay Mankar <akshay@wire.com>

akshaymankar approved these changes Jul 1, 2024

View reviewed changes

fisx approved these changes Jul 1, 2024

View reviewed changes

battermann mentioned this pull request Jul 1, 2024

Introduce EmailSmsSubsystem, use it for a few emails and password reset SMS #4111

Merged

2 tasks

fisx added 2 commits July 1, 2024 10:01

Fix haddocks.

72ce948

Merge remote-tracking branch 'refs/remotes/origin/WPB-8890-polysemy-u…

8a70a25

…ser-subsystem-add-password-reset-operation-s' into WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s

akshaymankar merged commit 6056721 into develop Jul 1, 2024
10 checks passed

akshaymankar deleted the WPB-8890-polysemy-user-subsystem-add-password-reset-operation-s branch July 1, 2024 08:59

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

Move password reset code to AuthenticationSubsystem #4086

Move password reset code to AuthenticationSubsystem #4086

battermann commented Jun 6, 2024 •

edited by akshaymankar

Loading

fisx left a comment

pcapriotti left a comment

fisx commented Jun 28, 2024

fisx commented Jun 28, 2024

fisx commented Jun 28, 2024

fisx Jul 1, 2024

akshaymankar Jul 1, 2024 •

edited

Loading

fisx Jul 1, 2024

Move password reset code to AuthenticationSubsystem #4086

Move password reset code to AuthenticationSubsystem #4086

Conversation

battermann commented Jun 6, 2024 • edited by akshaymankar Loading

Checklist

fisx left a comment

Choose a reason for hiding this comment

pcapriotti left a comment

Choose a reason for hiding this comment

fisx commented Jun 28, 2024

fisx commented Jun 28, 2024

fisx commented Jun 28, 2024

fisx Jul 1, 2024

Choose a reason for hiding this comment

akshaymankar Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

fisx Jul 1, 2024

Choose a reason for hiding this comment

battermann commented Jun 6, 2024 •

edited by akshaymankar

Loading

akshaymankar Jul 1, 2024 •

edited

Loading