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

[WPB-5990] Introduce user subsystem #3977

Merged
merged 107 commits into from
May 2, 2024
Merged

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Apr 2, 2024

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

Checklist

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

@akshaymankar akshaymankar force-pushed the wpb-5990/user-subsystem branch from acefa82 to 911774d Compare April 2, 2024 15:04
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Apr 2, 2024
@pcapriotti pcapriotti force-pushed the wpb-5990/user-subsystem branch from 911774d to ec34fee Compare April 3, 2024 09:33
@fisx fisx force-pushed the wpb-5990/user-subsystem branch from 1dc3942 to 53a3f99 Compare April 15, 2024 12:01
@fisx fisx removed the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Apr 15, 2024
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Apr 15, 2024
@fisx
Copy link
Contributor

fisx commented Apr 16, 2024

i started to pull this PR apart, more PRs with trivial, but noisy changes coming up!

Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

I have a general organisation question: why do we introduce these two effects: both Wire.UserStore and Wire.UserSubsystem? Shouldn't they be one effect instead?

libs/wire-subsystems/src/Wire/MiniBackend.hs Show resolved Hide resolved
libs/wire-subsystems/src/Wire/MiniBackend.hs Outdated Show resolved Hide resolved
libs/wire-subsystems/src/Wire/MiniBackend.hs Outdated Show resolved Hide resolved
@fisx
Copy link
Contributor

fisx commented Apr 30, 2024

      refresh /access
        invalid-cookie:                                                                      FAIL
          Exception: Assertions failed:
           2: Just "expired" not in Just "{\"code\":403,\"label\":\"invalid-credentials\",\"message\":\"Invalid token\"}"
          
          Response was:
          
          Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Tue, 30 Apr 2024 10:54:15 GMT"),("Server","Warp/3.3.30"),("Content-Encoding","gzip"),("Content-Type","application/json"),("Vary","Accept-Encoding")], responseBody = Just "{\"code\":403,\"label\":\"invalid-credentials\",\"message\":\"Invalid token\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose, responseOriginalRequest = Request {
            host                 = "brig.test-k0zibhkkw1ey.svc.cluster.local"
            port                 = 8080
            secure               = False
            requestHeaders       = [("Cookie","zuid=u-26cLooGfPnoln43d-hSgLn5mo4AfM2kKl-kJ_nsqcGLfH2gLrsZ5wYQGRgNLLyIxvGT3m4y2HVtcitNzcJDA==.v=1.k=1.d=1714474455.t=u.l=.u=e7b3aacd-4b2e-4a4b-8810-d46dc3cd9804.r=620db778")]
            path                 = "/access"
            queryString          = ""
            method               = "POST"
            proxy                = Nothing
            rawBody              = False
            redirectCount        = 10
            responseTimeout      = ResponseTimeoutDefault
            requestVersion       = HTTP/1.1
            proxySecureMode      = ProxySecureWithConnect
          }
          , responseEarlyHints = []}
          CallStack (from HasCallStack):
            error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-5UDVUBJ4yLy2nT3LW1wv9Q:Bilge.Assert
            <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-5UDVUBJ4yLy2nT3LW1wv9Q:Bilge.Assert
            !!!, called at test/integration/API/User/Auth.hs:673:64 in main:API.User.Auth
          Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&$0=="Brig API Integration.user.auth.refresh /access.invalid-cookie"' to rerun this test only.

this looks like an actual change in behavior, so we may want to fix this in the prod code, not in the tests. or not. maybe something about deleting expired users faster now than before?

@fisx
Copy link
Contributor

fisx commented Apr 30, 2024

I have a general organisation question: why do we introduce these two effects: both Wire.UserStore and Wire.UserSubsystem? Shouldn't they be one effect instead?

UserStore is a low-level persistence effect implemented in cassandra. UserSubsystem is a top-level effect that is implemented in terms of more fine-grained polysemy effects, one of them being UserStore.

if we replace the UserStore implementation with something MVar-based, we can test UserSubsystem without the need for cassandra. if UserStore would happen internally in UserSubsystem.Interpreter, it would be unclear how to replace the cassandra interpreter in a robust way for the tests.

Copy link
Contributor

@MangoIV MangoIV left a comment

Choose a reason for hiding this comment

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

just some nitpicks, I guess, feel free to ignore :)

Comment on lines +20 to +24
forall (c :: Component) f a m x fedM.
(KnownComponent c, Foldable f) =>
f (Remote x) ->
(Remote x -> fedM c a) ->
FederationAPIAccess fedM m [Either (Remote x, FederationError) (Remote a)]
Copy link
Contributor

Choose a reason for hiding this comment

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

@pcapriotti now that we're porting this, have we thought about putting a concrete type here (i.e. [])? passing dictionaries is expensive and Foldable basically says "this is isomorphic to []"

libs/wire-subsystems/src/Wire/MiniBackend.hs Show resolved Hide resolved
libs/wire-subsystems/src/Wire/ParseException.hs Outdated Show resolved Hide resolved
libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs Outdated Show resolved Hide resolved
libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs Outdated Show resolved Hide resolved
services/brig/src/Brig/App.hs Show resolved Hide resolved
Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

There are a few things I believe have to be addressed. Comments are inlined.

'[ UserSubsystem,
DeleteQueue,
Error Wire.API.Federation.Error.FederationError,
Wire.FederationAPIAccess.FederationAPIAccess Wire.API.Federation.Client.FederatorClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Wire.FederationAPIAccess.FederationAPIAccess Wire.API.Federation.Client.FederatorClient,
FederationAPIAccess FederatorClient,

I suppose we can import these things such that we can write the much shorter version.

services/brig/test/integration/API/User/Account.hs Outdated Show resolved Hide resolved
fisx and others added 7 commits April 30, 2024 14:31
Co-authored-by: Mango The Fourth <40720523+MangoIV@users.noreply.github.com>
Co-authored-by: Mango The Fourth <40720523+MangoIV@users.noreply.github.com>
Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>
This reverts commit c354f07.
@fisx
Copy link
Contributor

fisx commented Apr 30, 2024

      refresh /access
        invalid-cookie:                                                                      FAIL
          Exception: Assertions failed:
           2: Just "expired" not in Just "{\"code\":403,\"label\":\"invalid-credentials\",\"message\":\"Invalid token\"}"
          
          Response was:
          
          Response {responseStatus = Status {statusCode = 403, statusMessage = "Forbidden"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Tue, 30 Apr 2024 10:54:15 GMT"),("Server","Warp/3.3.30"),("Content-Encoding","gzip"),("Content-Type","application/json"),("Vary","Accept-Encoding")], responseBody = Just "{\"code\":403,\"label\":\"invalid-credentials\",\"message\":\"Invalid token\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose, responseOriginalRequest = Request {
            host                 = "brig.test-k0zibhkkw1ey.svc.cluster.local"
            port                 = 8080
            secure               = False
            requestHeaders       = [("Cookie","zuid=u-26cLooGfPnoln43d-hSgLn5mo4AfM2kKl-kJ_nsqcGLfH2gLrsZ5wYQGRgNLLyIxvGT3m4y2HVtcitNzcJDA==.v=1.k=1.d=1714474455.t=u.l=.u=e7b3aacd-4b2e-4a4b-8810-d46dc3cd9804.r=620db778")]
            path                 = "/access"
            queryString          = ""
            method               = "POST"
            proxy                = Nothing
            rawBody              = False
            redirectCount        = 10
            responseTimeout      = ResponseTimeoutDefault
            requestVersion       = HTTP/1.1
            proxySecureMode      = ProxySecureWithConnect
          }
          , responseEarlyHints = []}
          CallStack (from HasCallStack):
            error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-5UDVUBJ4yLy2nT3LW1wv9Q:Bilge.Assert
            <!!, called at src/Bilge/Assert.hs:109:19 in bilge-0.22.0-5UDVUBJ4yLy2nT3LW1wv9Q:Bilge.Assert
            !!!, called at test/integration/API/User/Auth.hs:673:64 in main:API.User.Auth
          Use -p '(!/turn/&&!/user.auth.cookies.limit/)&&$0=="Brig API Integration.user.auth.refresh /access.invalid-cookie"' to rerun this test only.

this looks like an actual change in behavior, so we may want to fix this in the prod code, not in the tests. or not. maybe something about deleting expired users faster now than before?

never mind, this is actually happening in other branches, too.

@mdimjasevic mdimjasevic merged commit 083263f into develop May 2, 2024
8 checks passed
@mdimjasevic mdimjasevic deleted the wpb-5990/user-subsystem branch May 2, 2024 14:30
@MangoIV
Copy link
Contributor

MangoIV commented May 2, 2024

woooooooooooohooooooooooooo 🎉

@echoes-hq 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: technical-roadmap/technical-debt More specific category, to highlight Technical Debt being tackled. echoes: technical-roadmap/throughput More specific category, to highlight task aiming at improving the development velocity and effici... ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants