Multi Subscription Single Connection

[ErnestKz]

Current State of Draft

Added 2 additional modules to separate new behaviour from old:

Icepeak.Server.WebsocketServer.HandleClientSingleSubscription
Icepeak.Server.WebsocketServer.HandleClientMultiSubscription

Current draft implements the ability for the client thread to :

• Client thread to case split on payloads, and sub and un-sub from the subscription tree accordingly.
• Client thread to receive and read from multiple paths in the subscription tree, and send to socket.
• Client thread to disconnect, and clean up the subscription tree after itself.

Left to do:

• Payload parsing.
• Authenticate the token and path.
• Handle the un-authorised case.
• Handle the unrecognised payload case.
• Dispatch old vs new behaviour based on request.
• Other protocol specific details.

[crtschin]

Nice progress already! Left some preliminary comments. Curious to see the rest.

[fatho]

Good start already! I have a few remarks, but overall it looks like it should work

[ErnestKz]

Will begin implementing protocol specific details, then circle around and re-visit some of the comments as things begin to take more shape.

[ErnestKz]

Currently authorisation is built for:

• Accepting incoming websocket connection:
  • It inspects the request headers for the token
  • It checks path that it is trying to subscribe to

Most relevant functions are:

• isRequestAuthorized
  |- getRequestClaim
  |- isAuthorizedByClaim

Can pretty much re-use isAuthorisedByClaim

isAuthorizedByClaim :: IcepeakClaim -> Path -> AccessMode -> Bool

Within getRequestClaim there is:

case maybeSecret of
       Nothing     ->
         -- authorization is enabled, but no secret provided, accept all tokens
         getTokenBytes >>= extractClaimUnverified
       Just secret -> getTokenBytes >>= extractClaim now secret

Will perhaps be re-using:

extractClaim :: POSIXTime -> JWT.VerifySigner -> SBS.ByteString -> Either TokenError IcepeakClaim

The SBS.ByteString is the token.

Can get POSIXTime from:

now <- Clock.getPOSIXTime

Can get the JWT.VerifySigner from the config:

  -- | The secret used for verifying the JWT signatures. If no secret is
  -- specified even though JWT authorization is enabled, tokens will still be
  -- used, but not be verified.
  , configJwtSecret :: Maybe JWT.VerifySigner

---

New protocol will need to verify a list of paths using the token provided by the subscribe payload.
So it will be an extractClaim followed by an isAuthorizedByClaim. And the Maybe JWT.VerifySigner will be passed into the context of the function.

Presumably the configEnableJwtAuth :: Bool option only makes sense upon connection initalisation? What are the varying degrees of authorisation configurability?

Perhaps we will have to make re-use of extractClaimUnverified?

-- | Extract the icepeak claim from the token without verifying it.
extractClaimUnverified :: SBS.ByteString -> Either TokenError IcepeakClaim

[ErnestKz]

So it feels like the core functionality should be there. Steps to see through to the refinement process:

• Collaborate with frontend to see how the service functions
• See what kind of tests would be appropriate
• Split code up further, stick to the single style, address PR comments
• Request feedback on functionality, style, and code structure

[ErnestKz]

@crtschin How does it sound? Also, any suggestions for the direction on code structure? Particularly the chunky onPayload

[crtschin]

Discussed offline, splitting up the onPayload and adding tests are the next steps.

[ErnestKz]

This PR should be ready for review! Overall since last time, made big refactors, added tests, adjusted endpoint entrypoint to accept re-usable connections.

[crtschin]

Nice, I have some notes and suggestions. Might be good to setup your editor to work with fourmolu. The codestyle is quite inconsistent, and there quite some trailing whitespace strewn about.

[ErnestKz]

Made a first pass over the PR comments, and pushed commits for them, the outstanding things yet to be addressed are:
General:

• Metrics for the new protocol

Implementation:

• Change implementation to not require initial connection auth and use timeout, update docs:
  • Multi Subscription Single Connection #119 (comment)
• Clean up ServerState in Icepeak.Server.Core
  • Multi Subscription Single Connection #119 (comment)

Tests:

• Use context and use it for each shouldBe for invalid payload
  • Multi Subscription Single Connection #119 (comment)
• Same for successfulSubscribe
  • Multi Subscription Single Connection #119 (comment)
• Presumably the same pattern for the rest of the tests.
• Test if subscribing to non-existent path also updates the client.

[ErnestKz]

I've added a subscription deadline timeout mechanism, and added more descriptive tests.

I think we can begin gradually rolling this out.

We can write a list of items that we may want to be wary of and come back to and implement if we see that they are needed as we roll out.

@crtschin

[fatho]

Good work! I have a few smaller remarks, but overall looking good 👍