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

Merge MFA into dev #564

Merged
merged 45 commits into from
Oct 26, 2024
Merged

Merge MFA into dev #564

merged 45 commits into from
Oct 26, 2024

Conversation

kozabrada123
Copy link
Member

@kozabrada123 kozabrada123 commented Oct 2, 2024

Continuation of #521

TODOs

Original description

Here is my suggested implementation of MFA #40

Features

  • Adds the required Request/Response structs for the MFA implementation.
  • Adds a new MfaRequired error on ChorusError.
  • Adds complete_mfa_challenge which calls the endpoint to verify MFA.

Refactors

  • Removes struct TotpSchema.
  • Changes ChorusUser struct definition, the user field type is wrapped with Option so there is an exact way to know when it is authenticated. The shell function has also been changed accordingly.
  • Changes current API endpoints according to the changes made to ChorusUser.

Notes

  • The following changes are a foundation for a future full MFA implementation throughout all endpoints that require it, as some endpoints are not implemented yet.
  • For unauthenticated endpoints that require MFA verification, a user of the library (Chorus) will have access to the required functionalities to manually implement such verification.

New changes:

  • Updated mfa branch with the latest dev commits
  • The original Option on the object field in ChorusUser has been removed, to make the api more ergonomic. If we aim to re-do how ratelimited requests work, adding the Option for a few versions and then removing it again at some point is quite annoying from an api pov. shell was (until now, with one unauthenticated mfa endpoint) called only when creating a new (authenticated)ChorusUser, which imo justified its calls to User::default and UserSettings::default as they were overridden soon after. A true solution for this issue would be to allow the Instance to perform unauthenticated yet still ratelimited requests, which likely means a rework of the ratelimiter logic.
  • A new method was added, ChorusUser::update_with_login_data, which replaces the last couple of lines (gateway identifying, fetching user object and user settings) for each login implementation, as that was just duplicated code
  • The mfa login logic has been updated to behave more like the other login functions
  • The AuthenticatorType enum, from the Ready payload has been moved to the mfa types and renamed to MfaAuthenticatorType. Another similar type MfaAuthenticationType was added, which covers authenticator types along with using a password or backup code.
  • A new api was added to MfaToken and ChorusRequest (1927ed8), which allows adding the mfa token to requests that need it (if we have obtained one)
  • As a result of this, MfaToken has been feature locked to the Client feature (825e0ed)
  • Added httptest as a dev-dependency on non wasm targets
  • Added a function to setup httptest's server with common routes needed for testing chorus
  • Added a function to setup TestBundle on an httptest server
  • Updated mfa tests to work with a mocked server
  • Added the new user mfa routes, along with tests for them

xystrive and others added 23 commits July 4, 2024 17:58
…ethod accordingly

This change allows to exactly know when a ChorusUser is authenticated or not
Remove option from the ChorusUser object field, since it makes the api a lot worse.

Tihs means we still call ChorusUser::shell a few times (mostly in logins, but also in one mfa route).

The ratelimiter should be refactored at some point to allow an instance send unauthenticated ratelimited requests
src/instance.rs Fixed Show fixed Hide fixed
Locks entities/mfa_token.rs to the client feature.

This representation is purely client sided, for a server sided
mfa token, you'd likely only store the expiration timestamp and
give the JWT to be handled by the client.
- add Enable and Disable TOTP MFA routes
- add Enable and Disable SMS MFA routes
- Adds the MfaAuthenticator type
- Renames AuthenticatorType to MfaAuthenticationType - these are ways we can authenticate, not types of authenticators
- Adds MfaAuthenticatorType, removes the old ReadyAuthenticatorType which was just this type in the Ready payload
- Adds the get_webauthn_authenticators route
src/types/schema/mfa.rs Fixed Show fixed Hide fixed
kozabrada123 and others added 10 commits October 14, 2024 19:19
changes:
- Removed the weirdness with EnableTotpMfaReturn and EnableTotpMfaResponse. I've realized we have not reason to hide a received token from library users, as they'll probably want to save it somewhere as well.
- For the same reason, made disable_totp_mfa return the new token
- Updated documentation on several mfa types
- Renamed type MfaVerificationSchema to MfaChallenge
- Renamed field mfa on MfaRequiredSchema to mfa_challenge
- Added two aliases for ChorusUser::complete_mfa_challenge, MfaChallenge::complete and MfaVerifySchema::verify_mfa
@kozabrada123 kozabrada123 marked this pull request as ready for review October 16, 2024 10:32
@kozabrada123 kozabrada123 merged commit 55e6235 into dev Oct 26, 2024
9 checks passed
kozabrada123 added a commit that referenced this pull request Nov 24, 2024
Release tracker for v0.18.0 of chorus, set to release on November 24th, 2024.

## Public API changes
- #570: Various entity public api changes
- 644d3be, 85e922b: Add type `OneOrMoreSnowflakes`, allow `GatewayRequestGuildMembers` to request multiple guild and user ids
- f65b9c1: Differentiate `PresenceUpdate` and `GatewayPresenceUpdate`
- 0e5fd86: Temporarily fix `PresenceUpdate` for Spacebar Client by making `user` optional
- 61ac7d1: Updated `LazyRequest` (op 14) to use the `Snowflake` type for ids instead of just `String`

## Additions
- #564: MFA implementation, by @xystrive and @kozabrada123 
- 4ed68ce: Added [Last Messages request](https://docs.discord.sex/topics/gateway-events#request-last-messages) and [response](https://docs.discord.sex/topics/gateway-events#last-messages)
- b23fb68: Add `ReadState` to `GatewayReady`
- #571: Gateway Opcode enum
- #573: Gateway Disconnect Opcode enums

## Bugfixes

- #565: Fix sqlx En-/Decoding of `PremiumType`
- 7460d3f: Fix `GatewayIdentifyConnectionProps` for Spacebar Client by deriving default on all fields, since the client does not send it
-  3d9460f: Derive Default for `MessageReferenceType`, assume default reference_type if none is provided
- 4baecf9: Fixed a deserialization error related to `presences` in `GuildMembersChunk` being an array, not a single value
- 1b20102: Fixed a deserialization error with deserializing `activities` in `PresenceUpdate` as an empty array when they are sent as `null`
- 7feb571: Fixed a deserialization error on discord.com related to experiments (they are not implemented yet, see #578)
- fb94afa: Fixed a deserialization error on discord.com related to `last_viewed` in `ReadState` being a version / counter, not a `DateTime`

## Internal changes
- 40754c5: bump sqlx-pg-uint to v0.8.0
- #575: Refactor of gateway close code handling
- 4ed68ce: Refactored the gateway to fully use the `Opcode` enum instead of constants
- #579

---------

Co-authored-by: bitfl0wer <florian@pro-weber.com>
Co-authored-by: Flori <39242991+bitfl0wer@users.noreply.github.com>
Co-authored-by: xystrive <xys@xystrive.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants