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

Properly Set up login #118

Closed
Jooms opened this issue Apr 11, 2023 · 4 comments · Fixed by #180
Closed

Properly Set up login #118

Jooms opened this issue Apr 11, 2023 · 4 comments · Fixed by #180
Labels
backend concerns backend/API presentation

Comments

@Jooms
Copy link
Contributor

Jooms commented Apr 11, 2023

During MR #101 where the frontend and backend were being connected a couple of things were disabled in the login functionality.

  1. Passwords are compared plain-text. (handled in Connect New FrontEnd To New BackEnd #101)
  2. The backend doesn't return any user information. (Needed)
  3. The backend returns a redirect, the frontend doesn't care. (Task Redirect issue on successful validation - Login Formik #120)

Reasoning:

  1. Our dummy data inserts passwords as plain-text, not hashed. We need to enable the signup functionality to properly hash and salt the password. Then we need to update the login functionality properly bcrypt.compare. This also means the dummy test data in the database needs to be updated to hashed / salted passwords.
  2. Ideally it would return something like session token, or at least a users's names to be shown on the dashboard. Right now it just redirects.
  3. The frontend needs to figure out where the backend wants it to go (login or /), and redirect correctly. Right now it just ignores it.

FYI: @briswells and @prestonmasseyblake

@briswells
Copy link
Contributor

@Jooms Why did you cut out the bvrypt compare? The passwords, minus the ones in the dummy data, were being stored and compared as salted hashes. When I wrote the dummy data it was before updating passport.js and I just haven't updated, but all new users created get hashed. The user_id was also already returning as part of the session cookie, and I believe a session token as well.

@Jooms
Copy link
Contributor Author

Jooms commented Apr 11, 2023

Fair question.
I felt the top priority was to connect the Frontend to the Backend. Secondary is setting up the login and sign up correctly.
This let me test the connectivity, and was the smallest change I could come up with to satisfy my top priority, especially when that top priority is (at least by me) perceived to be a hard-blocker for the rest of the class.
With your explanation I don't believe it would be hard to re-add it appropriately but this was the amount of time I had.

In order to make sure it does get re-added, I created this (and #119) to track fast-follows that are needed after #101 goes it.
With 101 going in as-is, we can do things like:

  • Spin off the Frontend teams to start connecting their pages to the backend apis that already exist
  • Clean up the docker-compose.yml to deploy everything with the write vars (and validate it works for a fresh clone).
  • Fix the login flow
  • Set up the Sign up flow
  • and more.

@hardikpatil hardikpatil added the backend concerns backend/API presentation label Apr 14, 2023
@Jooms
Copy link
Contributor Author

Jooms commented Apr 16, 2023

This task is now tracking the major task

  • The backend doesn't return any user information

A frontend and backend dev are needed for this

@briswells
Copy link
Contributor

Removed redirects for backend login route. Now returns json data including user email. I kept the user data simple I as I unsure what was needed. Any backend routes will be able to pull all user information since we're using sessions. Frontend team will need to specify exactly what user information is needed.

briswells added a commit that referenced this issue Apr 17, 2023
Backend will now return if the login is a success or failure, forward
any authentication error messages, and return user email for on
successful logins. More user info can be added later if needed. Route
also sets a session cookie that can be used to check all other API
routes that require authentication.
@briswells briswells linked a pull request Apr 21, 2023 that will close this issue
AbhinavReddy-Dev pushed a commit that referenced this issue Apr 21, 2023
* Added structure and example for backend API

* Untested commit to change computers

* Updated token based authentication

* Removed debug code

* Fixed rebase issues

* Updated use token to check for valid token

* address login state issues

* Fixed dependany error

* removed black test file

* fixed linter errors

---------

Co-authored-by: Mike Murtey <mike@mikemurtey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend concerns backend/API presentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants