[#319] Add yandex authorisation#323
Conversation
|
привет, ПР готов, можно проверять |
|
deploy: https://hexlet-correction-cnd1.onrender.com path for registration/login in this deploy: https://hexlet-correction-cnd1.onrender.com/oauth2/authorization/yandex |
|
@fey привет, собрал сюда яндекс авторизацию и гитхаб яндекс авторизация до сих пор не стабильно работает, я мучался 5 дней, я без понятия что с этим делать, это уже другая реализация авторизации и та же проблема :\ я перепробовал наверное уже все, менял куки, cors, отключал csrf, фильтры писал, не получилось только базу Redis для хранения данных сессии поднять.. но зачем она проекту, по сути она только для render и нужна была бы.. Сейчас это работает так ну сейчас я думаю что во всем render виноват)) ну или его бесплатная версия все таки логика авторизации работает с github, и у yandex она сейчас аналогичная |
|
вот деплой: https://hexlet-correction-2n6d.onrender.com |
|
@Sanapol потыкал кнопки, выглядит ок. В итоге тут получается две задачи в одной? Если да, то проси ребят в команде поревьювить (важно чтобы вы код друг у друга ревьювили, был в курсе изменений, предлагали их) + попроси тестировщика кейсы накинуть, потестить тоже. И на будущее, было бы здорово, чтобы у пользователя в кабинете была кнопка привязки соц. сетей к аккаунту. Например как на Хекслете щас. Просто у меня например почта на яндексе одна, а в гитхабе другая. В итоге через яндекс создается новый аккаунт |
|
Сделал |
|
мб коммент не отправился. А для чего нужен custom service? |
|
не понял вопроса, ты про CustomOAuth2UserService? |
| public String getEmail() { | ||
| var email = attributes.get("email"); | ||
| if (email == null) { | ||
| WebClient webClient = WebClient.builder() |
There was a problem hiding this comment.
Здесь выполняются запросы сетевые, при этом, в яндексе этого нет. Может стоит вынести получение емейла в сервис?
There was a problem hiding this comment.
Привет! А в Яндексе это нужно? Тут это применяется потому, что с Гитхаба может почта не приходить, если она там не указана как основная, поэтому нужно ее получать принудительно.
There was a problem hiding this comment.
в яндексе это не нужно, у меня все приходит в ответе
| import java.util.Map; | ||
|
|
||
| @Getter | ||
| public class CustomOAuth2User extends DefaultOAuth2User { |
There was a problem hiding this comment.
мне не нравится, что тут не совсем custom. Кастом предполагает переопределение поведение и тд. Но тут оно не совсем переопределяется. Это скорее обертка (это и service тоже).
При чем тут обертка сработает только для двух соц. сетей - для гитхаба и вероятно яндекса. Но разные провайдеры могут предоставлять разные поля для пользователей в качестве юзернейма idи так далее. Если появится VK, то как с ним это будет работать?
Т.е. для каждого провайдера/способа входа способ обработки и нормализации данных, метод там или класс
There was a problem hiding this comment.
Как будто бы в CustomOAuth2User нет смысла. YandexOAuth2UserInfo и гитхабовский возвращают username getUsername() уже.
@Override
public String getUsername() {
return attributes.get("login").toString();
}
может рассихрон произошел из-за того, что код параллельно писали.
There was a problem hiding this comment.
Да, YandexOAuth2UserInfo и GithubOAuth2UserInfo возвращают username, но это лишь представление информации о пользователе в зависимости от сайта, изначальная возвращаемая сущность для всех реализаций DefaultOAuth2User, а у него изначально есть только 1 возвращаемое поле getName(), для того, чтобы на фронт отдать информацию о email и username был создан CustomOAuth2User, в который по сути было добавлено поле nickname, в остальном он такой же как DefaultOAuth2User
There was a problem hiding this comment.
его убирать никак нельзя
|
Итого
|
|
принято, а на счет тестов, еще пишем) |
|
и того:
|
|
@fey я написал тесты... finally.... |
|
А для гитхаба напишите тесты? |
|
@ean3ena, на сколько я понял ты сам хочешь тесты для гитхаба написать или мне написать? |
|
Мы с Николаем обсуждали архитектуру, в итоге я ее переделал и сейчас пишу тесты. Потом новым ПР-ом отправлю изменения. |
|
@fey Может тогда мержим пр? |
3 participants
No description provided.