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

admin auth: server and client #126

Merged
merged 2 commits into from Sep 24, 2020
Merged

Conversation

jmensch1
Copy link
Collaborator

@jmensch1 jmensch1 commented Sep 22, 2020

This does two main things:

Token-based auth

The backend now has a users table along with routes for creating users and for logging in. The users table stores first name, last name, email, and password hash. There are routes for user registration and login.

An admin clent

I took food-oasis's frontend and removed everything except the auth components. So you can register, login, and logout. No email notifications yet, and no forgot password feature.

Other things

  • created a controllers folder in the backend so that routes can be really simple
  • switched the DB to use camelCase instead of snake_case (as discussed on slack)

// res.send({"Error": "Error occurred"})
// });
// });
router.get('/', control.list)
Copy link
Member

Choose a reason for hiding this comment

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

This is a neat pattern

client/web-admin/.env.production Outdated Show resolved Hide resolved
email: {
type: Sequelize.STRING,
},
password_hash: {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need camel case here passwordHash?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

so the files in _misc aren't being used right now, I'm just saving them for when I make the migrations real. I'll def fix that when I do

const { email, password } = req.body

const user = await req.db.User.findOne({ where: { email } })
if (!user) return res.json({ emailNotFound: true })
Copy link
Member

Choose a reason for hiding this comment

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

what Http response code gets returned out here?
We should return HTTP 401 here or further up stream on failure

Copy link
Collaborator Author

@jmensch1 jmensch1 Sep 23, 2020

Choose a reason for hiding this comment

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

The /login route returns a 200 if email is not found. But we return a 401 if the user ever attempts to access a route requiring authentication, and their request doesn't include a valid, unexpired token (see @middleware/authenticate).

My thinking is that we should return a 200 in the email-not-found scenario because because the /login route itself doesn't require a token, and it would confuse things a bit to return a 401 for a route not requiring a token.

Copy link
Member

Choose a reason for hiding this comment

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

Cool this isn't a show stopper at all and better to get this in. but will put into an issue so we can catch later because we should follow the http spec.
401 for failed login (no email or wrong password)
403 for forbidden (you are authenticed but do not have permission to view the resource)

@aNullValue aNullValue merged commit f5e1356 into hackforla:master Sep 24, 2020
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.

None yet

4 participants