VadimVolkovsky / practicum_middle_33_auth

0 stars 0 forks source link

Code review #31

Closed BigDeepBlue closed 3 months ago

BigDeepBlue commented 3 months ago

Хорошая работа! Рекомендации и предложения:

  1. Мы должны заботиться о безопасности приложения. В этой связи рекомендую убрать проброс портов Redis и PostgreSQL из docker-compose.yaml
  2. Вот это все нужно обернуть в settings.is_debug, дабы случайно в прод не отправить :)
  3. Вот такой способ инициализации и обновления таблиц БД не самый лучший, а в проде - недопустимый. Только именованные миграции. Попробуйте https://alembic.sqlalchemy.org/en/latest/.
  4. Значение параметра echo лучше вынести в настройки и переменную окружения, она отвечает за логирование запросов в базу данных. А это не всегда нужно.
  5. Можно немного усовершенствовать этот блок, если нужен откат транзакции при возникшей проблеме.
      async def get_session() -> AsyncSession:
          async with async_session() as session:
              try:
                  yield session
              except Exception:
                  await session.rollback()
                  raise
              finally:
                  await session.close()
  6. Предлагаю продумать следующее, что если пользователь будет удален, то и все его записи истории UserLoginHistory были удалены автоматически. 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. Я бы рекомендовал отказаться от авторизации по имени пользователя и использовать адрес электронной почты. Наша задача сделать максимально простую регистрацию для пользователя. Почты у всех уникальные, а придумывать имя пользователя - то еще приключение. Вспомните когда от вас требовалось при регистрации на коком-либо ресурсе придумывать имя пользователя. Наверняка это было давно и редко, и сколько раз приходилось изменять имя пользователя, т.к. оно уже занято, и сколько раз вы хотели все бросить и уйти с этого ресурса.
  8. При логауте нужно инвалидировать оба токена. В вашем случае access еще будет валиден. А если его у нас украли, и я нажал Выйти, специально, чтобы злоумышленника выкинуло. Для этого в payload одного токена можно положить jti другого.
  9. Выдача истории входов пользователя должна быть с пагинацией, там через 2-3 месяца будут выдаваться сотни записей. https://uriyyo-fastapi-pagination.netlify.app/
  10. [можно лучше] Для проверки прав пользователя можно создать декоратор, который будет доставать токен из заголовка запроса и принимать решение о допуске пользователя к эндпоинту. Вот тут может быть что-то покажется интересным в этой связи https://gist.github.com/RomanAVolodin/e2731b303ef8b3c29d1052e82d5a7b36
  11. Не нашел каким образом у вас появляется суперпользователь. Оптимальным будет создание консольной команды https://typer.tiangolo.com/
BigDeepBlue commented 3 months ago

LGTM