-
Notifications
You must be signed in to change notification settings - Fork 21
PoC: fastapi-users
#363
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
PoC: fastapi-users
#363
Conversation
Add `fastapi-users` package with `beanie` and `oauth` tools for user management. The specific `10.4.0` version has been selected to make it compatible with `fastapi 0.68.1` package version. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
Add `TestUser` model and extend `User` model provided by `fastapi-users`. Add an extra field `username` and create `unique` index on it. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
Also added a method to get database instance `Database._db` to use it for initializing Beanie on API startup. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
`fastapi-users` uses `Beanie` ODM to provide tools to work with MongoDB. We need to initialize Beanie with database and user model on the application startup. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
996d35a
to
86a6b7d
Compare
Below are the example commands for requesting routes provided by the package:
Reponse: null API logs:
Provide the verification token to
API logs:
Reset the password using token:
|
Looks like a great initial investigation. I shall give it a try to get a clearer idea of how this would work and try to provide some feedback. On a side note, I see this uses |
Yes, |
Extend `BaseUserManager` provided by `fastapi-users` to have customized core logic for user management. Add database adapter `get_user_db` to create a link between user model from `fastapi-users` and API database configuration. Inject `UserManager` at a runtime in a database session. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
Create an authentication backend with JWT as a strategy and bearer transport. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
Create an instance of `FastAPIUsers` to bind `UserManager` with authentication backend. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
Add pydantic models for validating schema and serialize response for `fastapi-users` routers. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
Register different routers provided by the package to our application. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>
86a6b7d
to
a51e911
Compare
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.
Looks great to me :) Just a few comments but overall it seems like we're ready to upgrade this PR to a full implementation.
We might need to defer some things such as password reset via email and user moderation into follow-up PRs to discuss a few security issues with the sysadmins first.
@nuclearcat @VinceHillier ^ What do you guys think?
from beanie import Indexed, Document | ||
from fastapi_users.db import BeanieBaseUser | ||
from fastapi_users import schemas | ||
from beanie import PydanticObjectId, Indexed | ||
from typing import Optional |
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 wonder if these should rather be in a separate module to avoid "polluting" the whole api.models
namespace with fastapi_users
imports.
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.
Yes, we should have a separate model file just like we have for pagination related models.
collection.create_index("profile.username", unique=True) | ||
|
||
|
||
class TestUser(BeanieBaseUser, Document): |
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.
Is the class called TestUser
just because this is a PoC?
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.
Yes, exactly.
|
||
|
||
class UserUpdate(schemas.BaseUserUpdate): | ||
username: Optional[Indexed(str, unique=True)] |
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.
Out of interest, why is this one optional whereas it's not in the other models?
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.
That's to allow users to only send fields that are updating. If I remove the Optional
, the endpoint would expect username
field in the request even if the user wants it to be unchanged.
If you look at the definition of the parent class BaseUserUpdate
, you'll see all the fields are optional.
class BaseUserUpdate(CreateUpdateDictModel):
password: Optional[str]
email: Optional[EmailStr]
is_active: Optional[bool]
is_superuser: Optional[bool]
is_verified: Optional[bool]
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.
Right, I think this brings us back to the idea I mentioned a while ago about having a different model for the API and for the database.
await init_beanie( | ||
database=db.db, | ||
document_models=[ | ||
TestUser, | ||
], | ||
) |
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.
Maybe this call could be moved to api.auth
with a method in the Database class to avoid exposing the underlying .db
object? That way only the Database class would follow the object-oriented principle of accessing its own data. We already know we need to import the beanie
modules in api.auth
so it wouldn't be adding extra dependencies there.
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.
The reason I kept this call in main.py
was that init_beanie
needs an actual instantiated database object and not just the class reference. Still, I'll take another look if I can move things around.
async def on_after_forgot_password( | ||
self, user: TestUser, token: str, request: Optional[Request] = None | ||
): | ||
print(f"User {user.id} has forgot their password. Reset token: {token}") |
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.
So I guess this is where an email would be sent with the reset token in a URL, and then the user would then come back via the on_after_request_verify
method?
We could probably test this on the command line using an API token initially although this shouldn't be allowed in production as any leaked token should not enable changing the user's password imho. One issue of course is it's not possible to invalidate a JWT token, it just has a built-in expiry date. I wonder what's the standard way to deal with this in other APIs.
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.
So I guess this is where an email would be sent with the reset token in a URL, and then the user would then come back via the on_after_request_verify method?
Yes, that is true.
Ah also I think this doesn't enable new users to sing up with just an email address, so that's one less security thing to consider here. We'll need to work this out properly for a full production deployment though. |
Completed PoC. Moving forward with the actual integration. |
This is a PoC of
fastapi-users
package for user management tools in API.This is intended for investigation and review only. Improvements are needed for the actual integration.