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

User endpoint #24

Merged
merged 8 commits into from
May 15, 2023
Merged

User endpoint #24

merged 8 commits into from
May 15, 2023

Conversation

MehmedGIT
Copy link
Collaborator

This PR contains a basic user authentication/registration mechanism.

@MehmedGIT MehmedGIT requested a review from joschrew May 4, 2023 13:38
@MehmedGIT MehmedGIT requested review from kba and tdoan2010 May 4, 2023 13:41
Copy link
Member

@kba kba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Is HttpBasic via HTTPS secure?

ocrd_webapi/authentication.py Outdated Show resolved Hide resolved
ocrd_webapi/main.py Outdated Show resolved Hide resolved
ocrd_webapi/routers/user.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@joschrew joschrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the other comments I made, when I tested the authentication I found this "problems":

  • When I try to upload a workspace with a non existing user, I get 500 instead of 403.
  • When I try to upload a workspace with correct user but wrong password I get 500 instead of 403

ocrd_webapi/authentication.py Outdated Show resolved Hide resolved
ocrd_webapi/authentication.py Show resolved Hide resolved
ocrd_webapi/main.py Outdated Show resolved Hide resolved
@MehmedGIT
Copy link
Collaborator Author

MehmedGIT commented May 5, 2023

In addition to the other comments I made, when I tested the authentication I found this "problems":

* When I try to upload a workspace with a non existing user, I get 500 instead of 403.

* When I try to upload a workspace with correct user but wrong password I get 500 instead of 403

You're right. I was using the wrong method. I have added 2 extra tests for unauthorized usage. It's fixed now and it returns 401 Unauthorized. That's the default of FastAPI for wrong credentials anyway. Let me know if you still think 403 is better to be returned and why.

@MehmedGIT
Copy link
Collaborator Author

Is HttpBasic via HTTPS secure?

@kba, unfortunately, no. There is no HTTPS replacement for HTTP and that's bad. Potentially we should improve with a better protocol among the supported ones: https://fastapi.tiangolo.com/tutorial/security/

@MehmedGIT MehmedGIT requested a review from joschrew May 5, 2023 11:29
ocrd_webapi/routers/user.py Outdated Show resolved Hide resolved
ocrd_webapi/routers/user.py Outdated Show resolved Hide resolved
ocrd_webapi/routers/user.py Outdated Show resolved Hide resolved
ocrd_webapi/database.py Show resolved Hide resolved
ocrd_webapi/database.py Show resolved Hide resolved
ocrd_webapi/authentication.py Outdated Show resolved Hide resolved
tests/test_workflow_api.py Outdated Show resolved Hide resolved
@joschrew
Copy link
Collaborator

joschrew commented May 5, 2023

I'd say HttpBasic Auth via HTTPS is secure enough for our purpose for now, at least for my current usage. I do not protect any data but "only" misuse of the infrastructure (An isolated VM). Could/should be updated later, but not in this PR I think.
Regarding 401 vs 403, I think you are right I confused it.

Copy link
Collaborator

@joschrew joschrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, tested and worked as expected. 👍

@MehmedGIT MehmedGIT merged commit f5260c3 into main May 15, 2023
@MehmedGIT MehmedGIT deleted the user_management branch May 15, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants