Integrate `fastapi-users` for user management #377

JenySadadia · 2023-10-06T12:39:38Z

Use fastapi-users for user management logic in the API.

gctucker · 2023-10-24T10:28:57Z

@JenySadadia It looks like the scope of the unit tests isn't completely clear, so sometimes it uses the methods from api.main to get users and sometimes it seems to need a mock instead. So maybe just always use a mock everywhere like before, and potentially add some tests to specifically cover the methods to get users in api.main?

On a side note, one thing I didn't figure out while looking at the code yesterday was how the event_loop() fixture was being used. Is it a magic name or something that pytest can use? At the end of the unit tests there seems to be some deadlock, I wonder if it's related and maybe that's why the GitHub checks are taking quite a long time to complete.

JenySadadia · 2023-10-24T10:47:31Z

Fixed end-to-end tests.

JenySadadia · 2023-10-25T11:42:20Z

@JenySadadia It looks like the scope of the unit tests isn't completely clear, so sometimes it uses the methods from api.main to get users and sometimes it seems to need a mock instead. So maybe just always use a mock everywhere like before, and potentially add some tests to specifically cover the methods to get users in api.main?

We are not using methods from api.main but mocking them instead in conftest.py. The mock functions mock_get_current_user and mock_get_current_admin_user are used for getting regular user and admin user respectively.

On a side note, one thing I didn't figure out while looking at the code yesterday was how the event_loop() fixture was being used. Is it a magic name or something that pytest can use? At the end of the unit tests there seems to be some deadlock, I wonder if it's related and maybe that's why the GitHub checks are taking quite a long time to complete.

Usually pytest-asyncio uses default event_loop to run the tests. In case of async tests, the issue was default event loop was closing before all the test runs were completed. That's why event_loop fixture was implemented. Please see 732d6b9

At the end of the unit tests there seems to be some deadlock, I wonder if it's related and maybe that's why the GitHub checks are taking quite a long time to complete.

I think that's because of PubSub._keep_alive. Need to investigate what happens there.

Task was destroyed but it is pending!
task: <Task pending name='Task-74' coro=<PubSub._keep_alive() running at /home/jeny/kernelCI/kernelci-api_env/src/kernelci-api/api/pubsub.py:81> wait_for=<Future pending cb=[_chain_future.<locals>._call_check_cancel() at /usr/lib/python3.10/asyncio/futures.py:385, Task.task_wakeup()]> cb=[_chain_future.<locals>._call_set_state() at /usr/lib/python3.10/asyncio/futures.py:392]>

gctucker · 2023-10-25T15:48:32Z

We are not using methods from api.main but mocking them instead in conftest.py. The mock functions mock_get_current_user and mock_get_current_admin_user are used for getting regular user and admin user respectively.

Ah yes sorry, I mean here:

    app.dependency_overrides[get_current_user] = mock_get_current_user
    app.dependency_overrides[get_current_superuser] = mock_get_current_admin_user

The mock_* methods aren't actual fixtures any more, but some tests still expect them to be fixtures. Maybe we should have these methods and actual mock fixtures to wrap around them? That way tests that just need a test client will get them via the snippet above and those that require a mock method explicitly will get the actual mock.

Since the `User` schema has been changed after `fastapi-users` integration, add a migration to update all the existing documents accordingly. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>

Implement an endpoint to update a user password. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>

JenySadadia · 2023-11-04T11:48:00Z

Added /user/update-password endpoint to update password for a user account.

gctucker · 2023-11-04T12:02:03Z

OK thanks. Now I think it's all in place I'll test and review it on Monday.

today at 11:49:2811/06/2023 06:19:28 AM UTC [ERROR] 1 validation error for Settings today at 11:49:28email_sender today at 11:49:28 value is not a valid email address (type=value_error.email) Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>

gctucker · 2023-11-07T09:20:11Z

The API service failed to start, I guess the migration didn't get applied automatically:

pymongo.errors.DuplicateKeyError: Index build failed: 1d9b688a-2f09-4192-955d-0ad38df3905d: Collection kernelci.user ( 8ce1d873-8042-4e69-8062-7f6e42423136 ) :: caused by :: E11000 duplicate key error collection: kernelci.user index: username_1 dup key: { username: null }, full error: {'ok': 0.0, 'errmsg': 'Index build failed: 1d9b688a-2f09-4192-955d-0ad38df3905d: Collection kernelci.user ( 8ce1d873-8042-4e69-8062-7f6e42423136 ) :: caused by :: E11000 duplicate key error collection: kernelci.user index: username_1 dup key: { username: null }', 'code': 11000, 'codeName': 'DuplicateKey', 'keyPattern': {'username': 1}, 'keyValue': {'username': None}}

gctucker · 2023-11-07T09:22:13Z

migrations/20231102101356_user.py

The commit message prefix is migration: but it should be migrations:. I can fix this after merging with a force-push.

gctucker · 2023-11-07T09:23:08Z

migrations/20231102101356_user.py

+def downgrade(db: "pymongo.database.Database"):
+    pass


So fastapi-users is a point of no return :) I guess that's fine as we're not in production yet. It would be good to test a rollback at some point with the Early Access deployment though.

We can deal with that different way (with mongodb snapshots and loss of some data that was submitted after upgrade), often it might be cheaper than implement complex downgrade (which might be also not always possible).

The downgrade has the advantage of being usable with new data.

gctucker · 2023-11-07T09:24:29Z

The API service failed to start, I guess the migration didn't get applied automatically:

pymongo.errors.DuplicateKeyError: Index build failed: 1d9b688a-2f09-4192-955d-0ad38df3905d: Collection kernelci.user ( 8ce1d873-8042-4e69-8062-7f6e42423136 ) :: caused by :: E11000 duplicate key error collection: kernelci.user index: username_1 dup key: { username: null }, full error: {'ok': 0.0, 'errmsg': 'Index build failed: 1d9b688a-2f09-4192-955d-0ad38df3905d: Collection kernelci.user ( 8ce1d873-8042-4e69-8062-7f6e42423136 ) :: caused by :: E11000 duplicate key error collection: kernelci.user index: username_1 dup key: { username: null }', 'code': 11000, 'codeName': 'DuplicateKey', 'keyPattern': {'username': 1}, 'keyValue': {'username': None}}

Ah I see migrations need to be initiated manually:

docker-compose exec api /bin/sh -c 'pymongo-migrate migrate -u mongodb://db/kernelci -m migrations -v'

I was expecting them to run automatically when the API service starts, assuming it can keep track of migrations already applied. I'll create a GitHub issue for this.

gctucker · 2023-11-07T09:32:24Z

Actually this should be a docker-compose run api ... command as it can't be run with a running container since the container needs the migrations to have been applied in order to start. Will also fix the docs :)

nuclearcat · 2023-11-07T09:43:38Z

I prefer to avoid any exec, because with kubernetes it will make migration a bit non-trivial and too much manual. Imho on startup some script should check mongodb schema version and execute migration scripts accordingly.

gctucker · 2023-11-07T09:44:44Z

No I mean, it should just be run on startup (see my previous comment). But for running manually, it should be docker-compose run rather than docker-compose exec.

gctucker · 2023-11-07T09:46:03Z

@nuclearcat Feel free to work on enabling the migrations to run during startup btw, I'll look just at this PR for now. I've prepared an update in the docs for running the migrations manually but that's orthogonal I guess.

nuclearcat · 2023-11-07T09:48:51Z

Ok, i will try to do that after will complete initial deployment scripts, on second revision it should be part of deployment

nuclearcat · 2023-11-07T11:46:03Z

@JenySadadia we need to have a way (config option, environment variable, daemon flag, anything) to disable mandatory email verification, it is a bit difficult to implement this on staging for now.

gctucker · 2023-11-07T14:42:16Z

api/admin.py


 async def main(args):
    db = Database(args.mongo, args.database)
+    await db.initialize_beanie()


I don't think we need Beanie here. I'll dig a bit deeper, as far as I know we don't actually use Beanie anywhere in the API code at the moment, just fastapi-users uses it internally.

We might however decide to rely more on Beanie going forward to simplify the code, it could basically be a replacement for the Database class somehow. See also the documentation:
https://beanie-odm.dev/

We are accessing DB to create an admin user with db.create. Also, access DB for finding existing users with the provided username in db.find_one_by_attributes.
For these methods to work, we need to initialize Beanie first as they are accessing the collection user created by Beanie.

gctucker · 2023-11-07T14:54:25Z

api/main.py

The commit summary should be in the present tense i.e. rename rather than renamed. Also something that can be fixed after merging this PR.

.github/workflows/main.yml

gctucker · 2023-11-07T14:59:01Z

migrations/20231102101356_user.py

+# Author: Jeny Sadadia <jeny.sadadia@collabora.com>
+
+"""
+Migration for User schema


Here as well, it might be better to mention fastapi-users.

gctucker · 2023-11-07T15:01:28Z

api/email_sender.py

The commit message for fixing the staging bug doesn't explain what this does. It looks like it's temporary so should it really be merged? This looks like a staging configuration issue, or maybe the API code should be able to deal with such cases and not try to send emails when the sender is not valid.

gctucker

OK LGTM overall. As it's a large PR there are a few things that could be improved. I just used it locally and migrated my users fine, and could authenticate with the API.

Next I'll try to create / update / find users with kci user to test this completely. If that works then I think it's good to go, with a few things to clean-up once it's merged.

gctucker

Tested OK with local instance. A few things need to be tweaked, I'll send some follow-up PRs for that.

JenySadadia force-pushed the fastapi-users-integration branch 3 times, most recently from 2787c5c to f6a6e0b Compare October 9, 2023 12:52

JenySadadia added the staging-skip Don't test automatically on staging.kernelci.org label Oct 9, 2023

JenySadadia force-pushed the fastapi-users-integration branch 6 times, most recently from 5fd0256 to ce659c8 Compare October 17, 2023 11:03

JenySadadia marked this pull request as ready for review October 17, 2023 11:22

JenySadadia requested a review from gctucker October 17, 2023 11:23

JenySadadia force-pushed the fastapi-users-integration branch 3 times, most recently from 7b2b4de to e17304c Compare October 19, 2023 11:54

JenySadadia mentioned this pull request Oct 20, 2023

Remove /hash endpoint #386

Closed

3 tasks

JenySadadia linked an issue Oct 20, 2023 that may be closed by this pull request

Remove /hash endpoint #386

Closed

3 tasks

JenySadadia force-pushed the fastapi-users-integration branch 2 times, most recently from c0f0aa3 to af9af53 Compare October 23, 2023 11:35

JenySadadia mentioned this pull request Oct 24, 2023

src/timeout: update logic to get username of current user kernelci/kernelci-pipeline#335

Merged

JenySadadia force-pushed the fastapi-users-integration branch from 289e400 to 81b5266 Compare October 24, 2023 10:26

JenySadadia force-pushed the fastapi-users-integration branch from 81b5266 to 2587977 Compare October 24, 2023 10:43

JenySadadia mentioned this pull request Oct 25, 2023

Enable find users with boolean query filters #390

Closed

2 tasks

JenySadadia force-pushed the fastapi-users-integration branch from 2587977 to d139e08 Compare October 25, 2023 10:48

gctucker mentioned this pull request Oct 25, 2023

unit tests hacks #391

Closed

JenySadadia force-pushed the fastapi-users-integration branch from d139e08 to 16ef4db Compare October 26, 2023 06:07

Jeny Sadadia added 2 commits November 4, 2023 12:29

migration: add migration for user documents

46557c5

Since the `User` schema has been changed after `fastapi-users` integration, add a migration to update all the existing documents accordingly. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>

api.main: add POST /user/update-password endpoint

c9dde29

Implement an endpoint to update a user password. Signed-off-by: Jeny Sadadia <jeny.sadadia@collabora.com>

JenySadadia force-pushed the fastapi-users-integration branch from aba6d60 to c9dde29 Compare November 4, 2023 07:00

nuclearcat mentioned this pull request Nov 7, 2023

api/: Remove profile. from user collection #398

Closed

gctucker reviewed Nov 7, 2023

View reviewed changes

.github/workflows/main.yml Show resolved Hide resolved

gctucker reviewed Nov 7, 2023

View reviewed changes

gctucker approved these changes Nov 10, 2023

View reviewed changes

gctucker added this pull request to the merge queue Nov 10, 2023

Merged via the queue into kernelci:main with commit dd2c625 Nov 10, 2023

JenySadadia deleted the fastapi-users-integration branch November 15, 2023 06:46

JenySadadia mentioned this pull request Nov 30, 2023

Review user accounts implementation #351

Closed

3 tasks

JenySadadia linked an issue Dec 7, 2023 that may be closed by this pull request

User account management #304

Closed

5 tasks

Integrate fastapi-users for user management #377

Integrate fastapi-users for user management #377

Uh oh!

Conversation

JenySadadia commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gctucker commented Oct 24, 2023

Uh oh!

JenySadadia commented Oct 24, 2023

Uh oh!

JenySadadia commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gctucker commented Oct 25, 2023

Uh oh!

JenySadadia commented Nov 4, 2023

Uh oh!

gctucker commented Nov 4, 2023

Uh oh!

gctucker commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gctucker commented Nov 7, 2023

Uh oh!

gctucker commented Nov 7, 2023

Uh oh!

nuclearcat commented Nov 7, 2023

Uh oh!

gctucker commented Nov 7, 2023

Uh oh!

gctucker commented Nov 7, 2023

Uh oh!

nuclearcat commented Nov 7, 2023

Uh oh!

nuclearcat commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gctucker left a comment

Choose a reason for hiding this comment

Uh oh!

gctucker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Integrate `fastapi-users` for user management #377

Integrate `fastapi-users` for user management #377

JenySadadia commented Oct 6, 2023 •

edited

Loading

JenySadadia commented Oct 25, 2023 •

edited

Loading