-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implemented logout functionality #60
base: main
Are you sure you want to change the base?
Conversation
hey @khoing0810 ! It looks like you are bringing in commits that were from previous work (and squashed). The way to fix this is to keep your fork main branch absolutely in sync with the one here, and always checkout fresh. Let me know if you want some help to look at this / walk through this process - most projects won't be happy to just ignore (e.g., squash and commit) and will want to see a logical git history, so we should make sure you know how to do that. Happy weekend! |
4949efe
to
80fddf9
Compare
80fddf9
to
dce06e0
Compare
Resurrected this from the dead but I implemented the basic logout button after cleaning up the git workflow. Looking into approach that get it to stop raising the exception and redirected the page to home page |
"request": request, | ||
"data": data, | ||
}, | ||
raise HTTPException( |
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 we still want to give the user a good experience here - the initial intention to redirect somewhere was to do that. Right now this logout function seems to just raise an exception but I'm not sure it does anything?
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 agree. It was just the old code I did last year but unable to get to work on git workflow issue until now. The exception is raised with 401 status code to invalidate current credential, and hence, logging out.
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.
For better user experience, I think we should start looking into another authentication method since Basic Auth is not designed to handle logout functionality (here's the rationale I found: https://stackoverflow.com/a/233551) but I'm open to any suggestion!
This new implementation solves the issue of user logging out without being authenticated prior--when they try to log in again, they're met with 401 status code. The new functionality uses conditional appearance of 'Logout' button only when user is authenticated (logged in) to prevent them performing such action defined in first sentence. |
Thanks @khoing0810 ! I should have time this weekend to review. |
@khoing0810 I did a quick test, and exported my credentials:
And basic auth does not work. It pops back to this screen: and: |
Here is a different approach to try (what I would do). Since we already have a database setup, get rid of the basic auth and needing to cache things in the browser and use JWT tokens, and then you add that to depends for each view. https://medium.com/@chnarsimha986/fastapi-login-logout-changepassword-4c12e92d41e2. If that is too much then I can likely help and get started, but again maybe not immediately because I have a lot on my plate atm! |
user has to hit 'cancel' upon a popup after logging out