MFA implementation - Githubissues

polyphony-chat / chorus

A Rust library for interacting with multiple Polyphony- and Spacebar-Compatible instances at once.

https://crates.io/crates/chorus

Mozilla Public License 2.0

16 stars 7 forks source link

MFA implementation #521

Closed xystrive closed 2 months ago

xystrive commented 3 months ago

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.

xystrive commented 3 months ago

New changes made according to what was pointed out by @kozabrada123.

Also taking into account this discussion https://github.com/polyphony-chat/chorus/pull/521#discussion_r1666921209, before going forward with a merge I would want to include the API endpoint for verifying MFA for login requests.

bitfl0wer commented 2 months ago

One more question before merging, @xystrive: Would it be possible for you to add integration tests for this, at this time? If you need help/guidance with doing that, I'd be happy to assist. A "no" is fine too, by the way! The crate isn't stable yet anyhow, and tests can be added a little later.

xystrive commented 2 months ago

One more question before merging, @xystrive: Would it be possible for you to add integration tests for this, at this time? If you need help/guidance with doing that, I'd be happy to assist. A "no" is fine too, by the way! The crate isn't stable yet anyhow, and tests can be added a little later.

@bitfl0wer Yes for sure!

xystrive commented 2 months ago

@bitfl0wer I have added the tests you have asked me, however they are just a skeleton implementation and won't succeed since I haven't found a proper way to test against an endpoint that requires MFA.

bitfl0wer commented 2 months ago

Thank you so much for being so cooperative and for opening this PR! It's extremely appreciated :3 Edit: I have merged into a branch called mfa for now, since as you said, the tests you added currently fail. I will mock the missing MFA endpoint using the httptest crate, then merge into dev

xystrive commented 2 months ago

Thank you so much for being so cooperative and for opening this PR! It's extremely appreciated :3 Edit: I have merged into a branch called mfa for now, since as you said, the tests you added currently fail. I will mock the missing MFA endpoint using the httptest crate, then merge into dev

Gotcha, thank you for the care and guidance as well :slightly_smiling_face: