Skip to content

AuthenticationSubsystem: Move newCookie into it #4479

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

Merged
merged 45 commits into from
Apr 8, 2025

Conversation

akshaymankar
Copy link
Member

@akshaymankar akshaymankar commented Mar 4, 2025

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

Checklist

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

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 4, 2025
@akshaymankar akshaymankar force-pushed the user-subsystem-create-user branch 6 times, most recently from e5c5529 to bdf3ff7 Compare March 10, 2025 08:50
@akshaymankar akshaymankar force-pushed the user-subsystem-create-user branch 5 times, most recently from 6707f3f to 80c4577 Compare March 13, 2025 07:54
@akshaymankar akshaymankar changed the title AuthenticationSubsystem: Move code for issuing tokens AuthenticationSubsystem: Move newCookie into it Mar 13, 2025
@fisx fisx force-pushed the user-subsystem-create-user branch from 35f5e92 to 3fbad60 Compare March 17, 2025 14:48
@fisx fisx marked this pull request as ready for review March 17, 2025 16:18
@fisx fisx requested a review from a team as a code owner March 17, 2025 16:18
@fisx fisx requested a review from a team as a code owner March 18, 2025 12:50
This needs more changes to the zauth library before it can be integrated in the
the AuthenticationSubsystem.
The 'newAccessToken' function is moved to the 'AccessTokenLike' typeclass
returns an 'AccessToken' type which contains the serialized token, removing the
need for polymorphism.

Similarly 'renewAccessToken' function in the 'UserTokenLike' typeclass returns
an 'AccessToken'.

We might be able to get rid of all of this polymophism as things are not _so_
complicated. This is just the first step.
It was only used in integration tests and only for one type of access token
…ntom type paramter

These are essentially same things and we have invented type classes to treat
them so, but if they contain the same data structures inside and have same
serialisation/de-serialisation logic, there is little point in having new type
wrappers for LegalHoldUser and LegalHoldAccess.

The phantom type parameter is of type `UserTokenType` with values `ActualUser`
and `LHUser`. These names don't completely make any sense because there is no
such thing as a `LegalHoldUser` different from `ActualUser`. It _should_ rather
be called something like `TokenType`, but that name is taken by something else.
I suspect this type will go away soon, so while transitioning its probably fine
to keep it as it.

Doing all this makes it easier for The typeclasses `AccessTokenLike` and
`UserTokenLike` to be merged. The actual use of those type classes is to
determine which ttls to apply to these the `UserTokenType`, which can now be
made clearer in the code.
These help de-dupe bit more code.
The wrong type was applied couple of commits ago.
Delete bunch of unused code.
akshaymankar and others added 8 commits April 3, 2025 09:36
The old code is good enough, and singletons may add more complexity
than they abstract away.
This would require to take the type apart in the individual actions
again, and doing so without handing the context for the entire effect
to every action seems to get too boilerplaty.
@akshaymankar akshaymankar force-pushed the user-subsystem-create-user branch from 0c1b720 to 83f96f1 Compare April 3, 2025 07:36
This reverts commit 49a4f1f.
Brig updates its time every second. If we create many cookies very quickly all
of them get assigned the same cookie creation time, this makes it impossible to
guess which cookie will get evicted when we have too many cookies. So waiting a
second between cookie creation gets around this problem.
@akshaymankar akshaymankar requested a review from a team as a code owner April 3, 2025 10:22
The MonadThrow was being caught at the time of hosting the servant application,
this throwing error in the subsystem is much better.
Move the required settings to AuthenticationSubsystemConfig.
@akshaymankar akshaymankar force-pushed the user-subsystem-create-user branch from 71193d3 to 95ed333 Compare April 3, 2025 14:57
Copy link
Contributor

@battermann battermann left a comment

Choose a reason for hiding this comment

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

LGTM only very minor comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

4 participants