-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
WIP: refactor ktra to a modular design #57
base: develop
Are you sure you want to change the base?
Conversation
fMeow
commented
Jan 22, 2023
•
edited
Loading
edited
- refactor ktra to be both a lib and bin. This design can help user to customize and add some private functionalities (e.g., new database backend, new token authorization workflow)
- refactor ktra to organize by functionalities/features instead of HTTP methods
- refactor DbManager trait to separate registries related and user managerment related logics
Different database backend might require quite different data and struct to create a manager. It causes more harm than good to hard code a struct used to create a db manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
As you can see activity kind of slowed down here. Yuya has always been a little busy since the .5 release I think, and since then I was able to take over a small part to push .7 to crates.io and finish a few tasks that I needed from the repo. Now I'm also lacking time a bit for happy personal reasons, so I'll try to follow the work being done here since I mostly think this refactoring is a good idea.
The main "big" task I want to add to ktra rather than adding https support or the sparse index RFC implementation (those are great features though!!) is the ability to add a rust-docs feature that handles serving a docs.rs clone just for the crates that ktra is hosting (in its index); the separation between library and binary is definitely a good first step towards that.
Simplifying the code and features is also nice to have.
@@ -18,6 +18,7 @@ pub fn apis( | |||
new_user(db_manager.clone()) | |||
.or(login(db_manager.clone())) | |||
.or(change_password(db_manager)) | |||
.or(me()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this route should be gated by the openid feature: we want to deactivate this route when openid is activated (and all password-based authentication, really) because when I implemented openid support I had to keep inserting tokens in the user database, but they are all bogus
// With openid enabled, the `/me` route is handled in src/openid.rs | ||
#[cfg(not(feature = "openid"))] | ||
let routes = routes.or(me()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the part that changed the handling of the route
@@ -0,0 +1,11 @@ | |||
[package] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably an extra testing folder?
@@ -0,0 +1,23 @@ | |||
use warp::Filter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably an extra testing folder?