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

Создание аунтификации #2

Merged
merged 1 commit into from
May 22, 2019

Conversation

Ignutt
Copy link
Contributor

@Ignutt Ignutt commented May 16, 2019

@gurobokum gurobokum self-requested a review May 22, 2019 12:52
Copy link
Contributor

@gurobokum gurobokum left a comment

Choose a reason for hiding this comment

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

Я приму PR, но давай теперь сделаешь фикс того, что я сказал + добавишь тесты, чтобы тебе было легче проверить, как у тебя работает код
Давай ты возьмешь https://github.com/visionmedia/supertest (вот тут пример https://hackernoon.com/api-testing-using-supertest-1f830ce838f1 ) и напишешь код который будет запускать тесты с автоирзацией пользователя
Это называется end-to-end тесты (либо функциональные), ты проверяешь как работает система в общем (вся авторизация)
То есть я хочу, чтобы у тебя было 2 теста, которые бы запускались через npm test, которые бы вызывали /login ручку в 1й раз с верными данными, а второй с неверными и корректно отрабатывали

passport.use(new LocalStrategy(
{ usernameField: 'email' },
(email, password, done) => {
axios.get(`http://localhost:5000/users?email=${email}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Это было бы хорошо вынести в константу
или например в объект

const config = {
     AUTH_ENDPOINT: ....
}

Что меня немного смущает, так это то, что ты здесь запрашииваешь сервер из сервера. Тоесть ты как бы имеешь нужный метод, но идешь к нему в обход через HTTP. Это имеет смысл для микросервисной архитектуры, например у тебя на одном эндпойнте весят сервис авторизации, а на другом бизнес логика. Тут у тебя все вместе, так что такой подход излишен

Ты большой молодец, что пошел своим путем в решении задачи, а не списывал из примера

Смотри, что тут происходит
синтаксис .use - это middleware, прослойка express'a, вот тут подробнее - http://expressjs.com/en/guide/using-middleware.html
Смотри на это как на обычную функцию, которая будет вызвана, когда придет запрос на сервер. В твоем подходе ты получаешь запрос, а потом отсылаешь второй запрос на этот же сервис. Это, в принципе ничего страшного, как я уже говорил, но тут есть нарушение логики. Мы хотим проверить - авторизирован ли пользователь, но ты посылаешь запрос на сервер, который опять же проверит - авторизирован ли пользователь. И так далее. А где сама реализация роута /users/ ?

Смотри, в данном методе у тебя должна быть именно проверка пользователя относительно твоего persistent storage, а это твой json file. То есть тебе нужно сравнить - совпадают ли креды с этим файлом

@@ -0,0 +1 @@
{"cookie":{"originalMaxAge":null,"expires":null,"httpOnly":true,"path":"/"},"__lastAccess":1558013470595}
Copy link
Contributor

Choose a reason for hiding this comment

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

Эти файлы не нужно пушить в репозиторий
Иногда есть файлы временные, которые появились в ходе работы и не должны быть в репозитории (который хранит код и конфигурации). Для этого используй .gitignore - https://uleming.github.io/gitbook/4_%D0%98%D0%B3%D0%BD%D0%BE%D1%80%D0%B8%D1%80%D0%BE%D0%B2%D0%B0%D0%BD%D0%B8%D0%B5_%D1%84%D0%B0%D0%B9%D0%BB%D0%BE%D0%B2.html
так же ты всегда можешь следить за тем, что коммитишь с помощью команды git status

@gurobokum
Copy link
Contributor

Да, и ставь мне плиз в Reviewer, чтобы я видел нотификацию о PR

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.

2 participants