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

Ported user routes to axum #50

Merged
merged 22 commits into from
Oct 15, 2023
Merged

Conversation

ItsEthra
Copy link
Contributor

@ItsEthra ItsEthra commented Oct 2, 2023

I ported /user handles to axum without breaking frontend code. There is some room for improvements though, mainly
1. Handles should return Result<_, E> where E is an IntoResponse type which logs important server errors, makes it easier to debug.
2. Didn't port any tests yet

So for now I'll leave this pr as draft

@secana
Copy link
Contributor

secana commented Oct 3, 2023

Hi @ItsEthra thanks alot for the work. Escpecially seeing how the authentication is done in axum really helps.

I've created an issue to track the migration initiative: #51

@secana secana added the implementation Change an implementation details without adding new functionality label Oct 3, 2023
crates/web_ui/src/session.rs Outdated Show resolved Hide resolved
crates/web_ui/src/session.rs Outdated Show resolved Hide resolved
@ItsEthra
Copy link
Contributor Author

ItsEthra commented Oct 3, 2023

Also I believe handles should use PrivateCookieJar, but it needs a key. afair rocket by itself manages cookie signing/encryption key, but axum requires key to be declared manually, so my question is how should user set it? Perhaps just an environment variable should be fine?

EDIT: I just realized there are settings, so maybe it's a better place for a key?

@secana
Copy link
Contributor

secana commented Oct 3, 2023

Also I believe handles should use PrivateCookieJar, but it needs a key. afair rocket by itself manages cookie signing/encryption key, but axum requires key to be declared manually, so my question is how should user set it? Perhaps just an environment variable should be fine?

EDIT: I just realized there are settings, so maybe it's a better place for a key?

With rocket, a random key is generated on every startup for Kellnr. To keep it easy, we can take the same approach for axum. If Kellnr is restarted, the users need to login again, but that is perfectly fine as the session expires anyways.

There is the common::util::generate_rand_string that can be used to get a random string.

@ItsEthra
Copy link
Contributor Author

ItsEthra commented Oct 3, 2023

Alright then, I'll implement it so the key is generated at startup and stored in the AppState.

crates/web_ui/src/user.rs Outdated Show resolved Hide resolved
@ItsEthra
Copy link
Contributor Author

ItsEthra commented Oct 4, 2023

@jplatte Do you have any pointers on how to do testing? I don't have experience with writing tests for web apps.

@jplatte
Copy link
Contributor

jplatte commented Oct 4, 2023

Does this help?

@ItsEthra
Copy link
Contributor Author

ItsEthra commented Oct 4, 2023

Yes, thanks

@ItsEthra
Copy link
Contributor Author

ItsEthra commented Oct 4, 2023

@secana I'm trying to port tests at the moment and I'm a bit confused about this test, does it ensure that admin cannot access pages meant for normal users? If so shouldn't admin be able access anything? The test expects not found code and It's just strange to me.

EDIT: Similar question to the test any_auth_user_but_no_cookie_in_store, shouldn't anyone be allowed even if the cookie is missing?

@secana
Copy link
Contributor

secana commented Oct 5, 2023

@secana I'm trying to port tests at the moment and I'm a bit confused about this test, does it ensure that admin cannot access pages meant for normal users? If so shouldn't admin be able access anything? The test expects not found code and It's just strange to me.

EDIT: Similar question to the test any_auth_user_but_no_cookie_in_store, shouldn't anyone be allowed even if the cookie is missing?

There are three types of users in Kellnr.

  1. The "normal" user. That's a user that is logged in but not an admin
  2. The "admin" user. Logged in user with more rights
  3. The "any user" that is a "normal" or "admin" user but still logged in.

There are pages that are only accessible to either one of them. For example the normal user sees a different settings page than the admin user. Some pages are the same for normal and admin users. That's where the "any user" is used.

The normal_auth_user_is_admin checks is, if a route which is for a normal user is requested by an admin user, a 404 is returned. Concrete example: The admin user should not get the normal user settings.

The any_auth_user_but_no_cookie_in_store tests what a logged out user should see. If any user tries to authenticate with a session token, but the token is not known by Kellnr (because it expired or the db was reset), "unauthorized" has to be returned, such that the user can login again.

I hope that helps.

@ItsEthra
Copy link
Contributor Author

ItsEthra commented Oct 5, 2023

I see, thanks, that helps.

@ItsEthra
Copy link
Contributor Author

ItsEthra commented Oct 6, 2023

Alright, I believe I ported most tests and all routes under /user path. I'm still not very happy about the way I set cookies in tests but I haven't found a better way to do so.

@ItsEthra ItsEthra marked this pull request as ready for review October 6, 2023 10:17
@secana
Copy link
Contributor

secana commented Oct 8, 2023

I'm on a business trip for the next few days. I'll review and merge when I'm back. So far: thank you for the incredible work.

@secana secana merged commit 0e1cac1 into kellnr:migrate-to-axum Oct 15, 2023
@ItsEthra ItsEthra deleted the user-axum-routes branch October 15, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implementation Change an implementation details without adding new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants