search

sevrn73 / Auth_sprint_2

Спринт 7
0 stars 1 forks source link

Code review #11

Open BigDeepBlue opened 1 year ago

BigDeepBlue commented 1 year ago

Очень хорошо 👍 Все учтено, код читается, а значит поддерживаться будет легко.

  1. Много лишних и опасных портов выставлено наружу в docker-compose.yml. Пользователю достаточно будет оставить только порт nginx. Для разработки https://habr.com/ru/company/otus/blog/337688/
  2. Давайте трейсер занесем под if settings.enable_tracer, чтобы была возможность его отключить в тех же тестах
  3. Вот сюда рекомендую добавить параметр resource, чтобы сервис был не unknown_service в интерфейсе JaegerUI 
      resource = Resource(attributes={
       SERVICE_NAME: 'auth-service'
  })
  provider = TracerProvider(resource=resource)
  4. Имя хоста и порт нужно отправить в настройки и переменную окружения.
  5. Везде в моделях БД, где вы используете поле типа String, нужно указывать максимальную длину поля. Это позволяет оптимизировать расход ресурсов и защищает от случаев, когда в такие поля пытаются вставить огромные тексты. Если тебе точно нужно не ограничивать длину строки, то лучше использовать поле Text. https://flask-sqlalchemy.palletsprojects.com/en/2.x/models/
  6. Нужно озаботиться тем, что если пользователь будет удален, то и все его записи OAuthAccount и LoginHistory были удалены автоматически. https://docs.sqlalchemy.org/en/20/orm/cascades.html#using-foreign-key-on-delete-cascade-with-orm-relationships. Или тут с примерами https://esmithy.net/2020/06/20/sqlalchemy-cascade-delete/
  7. nit: привильно пишется account
  8. Имена провайдеров (google, yandex и т.д.) лучше поместить в enum. Работа с литералами - всегда не очень хорошая практика. Далее с развитием проекта может возникнуть необходимость их использовать в коде. Работа с enum, как минимум, избавит нас от опасности опечатки.
  9. Лишние скобки, можно без них return 'google$' + me['id'], me.get('email').split('@')[0], me.get('email')
  10. Уже больше 10 лет можно не наследоваться от object
  11. Не забывайте указывать типы принимаемых параметров и возвращаемых значений в методах
  12. Одним из заданий было: Создайте интеграцию Auth-сервиса и AsyncAPI-сервиса, используя контракт, который вы сделали в прошлом задании. Т.е. в сервисе выдачи, который у нас на FastAPI добавить middleware, например, которая будет разбирать токен или ходить с ним в сервис авторизации и в зависимости от прав давать тот или иной контент. Например, авторизованным - все, гостям - только списком фильмы без подробностей. Посмотрите очень подробный и интересный пример реализации https://testdriven.io/blog/fastapi-jwt-auth/ , самое интересное в разделе Securing Routes
sevrn73 commented 1 year ago
  1. У нас уже реализована зависимость через JWTBearer для ручек, в которых необходима авторизация (все кроме поиска фильма и поиска кинопроизведений по страницам).
BigDeepBlue commented 1 year ago
  1. У нас уже реализована зависимость через JWTBearer для ручек, в которых необходима авторизация (все кроме поиска фильма и поиска кинопроизведений по страницам).

Ого, как это я просмотрел 😀

BigDeepBlue commented 1 year ago

LGTM