AlexanderZharyuk/async-auth

Code review

Closed this issue · 1 comments

BigDeepBlue commented

Отличная работа! Немного с правами доступа не доработали, сейчас все могут все:

  1. В моделях базы данных (в src/v1/users/models.py) везде где используется ForeignKey необходимо продумать следующее: что если пользователь будет удален, то и все его записи UserLogin и UserRefreshTokens были удалены автоматически. 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/
  2. Пустой файл src/v1/services.py
  3. А сделайте dev версию файла docker-compose.yml, коллеги будут благодарны.
  4. Тут для корректного запуска контейнера с базой данных должна быть переменная окружения POSTGRES_PASSWORD без этого контейнер не стартует и ругается.
    Вообще все заработало только после того, как привел к виду: 
    LISTEN_PORT=8000
LISTEN_ADDR=0.0.0.0
POSTGRES_HOST=db
POSTGRES_PORT=5432
POSTGRES_DB=auth_db
POSTGRES_USER=app
POSTGRES_PWD=123qwe
POSTGRES_PASSWORD=123qwe
REDIS_HOST=redis
  5. Зарегистрировался, все отлично. Залогинился - тоже очень хорошо. Но почему-то смог создать роли, просмотреть всякое, что не должен видеть обычный пользователь. Нужно предусмотреть разграничение доступа. А для проверки прав пользователя можно создать декоратор, который будет доставать токен из заголовка запроса и принимать решение о допуске пользователя к эндпоинту. Может будет интересно, у меня тут набросок: https://gist.github.com/RomanAVolodin/e2731b303ef8b3c29d1052e82d5a7b36
  6. Пункт 6 относится и к просмотру пользователей, истории входов чужой и так далее. Это все только админу должно быть доступно.
  7. Пользователю при регистрации нужно выдавать какую-нибудь роль по умолчанию, а то сейчас список ролей пустой. Это не очень правильно.
  8. Залогинился пользователем 20 раз, а в истории входов одна запись. Кажется, вы зря просто обновляете время в модели. Нужно хранить всю историю логинов.
  9. Только в связи с тем, что вы сильнейшая команда: попробуйте при случае (не обязательно сейчас, но вообще возьмите на заметку) разделять по разным слоям сервисы и репозитории. Вы же помните спринт про SOLID. Посмотрите как у меня тут в демо проекте https://github.com/RomanAVolodin/FastAPI_DI/tree/feature/DI. Вдруг будет и это интересно :)
BigDeepBlue commented

LGTM