search

simenshteyn / Auth_sprint_1

Спринт 6
0 stars 0 forks source link

Code Review #38

Open BigDeepBlue opened 3 years ago

BigDeepBlue commented 3 years ago

Контейнер внедрения зависимостей - это совершенно верное и отличное решение. Тесты проходят, покрытие хорошее. Рекомендации по коду не существенные:

  1. Сервис мне разрешил зарегистрировать пользователя и этому пользователю разрешил просматривать и редактировать роли, эти задачи должны быть доступны только администратору (или какой-то другой предустановленной роли) можно рассмотреть такой вариант
  2. Не нашел endpoint проверки прав пользователя, это та самая "ручка", к которой будут обращаться другие сервисы для получения информации о пользователе по его токену (или если у вас другое видение этого вопроса - опишите). Кстати это будет самым высоконагруженным местом и его нужно будет кешировать.
  3. Мелочь конечно, но все же: проект на завелся, ругался при старте на:
auth_app  | cache_time
auth_app  |   none is not an allowed value (type=type_error.none.not_allowed)

все потому-что вот в этом месте Space inside allowed only for quoted values. Заменил на change_me - завелось.

  1. Вот в этом месте правильнее будет параметру model указать другой тип: model: Type[BaseModel] и сразу IDE перестанет ругаться что model is not callable - вот тут обсуждают этот момент https://stackoverflow.com/questions/41417679/how-to-annotate-a-type-thats-a-class-object-instead-of-a-class-instance
  2. Документация очень подробная и хорошо организованная, жаль что не подключена к локальной реализации swagger или хотя бы была секция servers для тестирования в https://editor.swagger.io/
servers:
  - url: http://localhost:8000/api/v1
  1. Я бы рекомендовал все сообщения, отдаваемые пользователю ('Permission for Role with that UUID not found' и т.д.) вынести в отдельное место, для упрощения их дальнейшей поддержки, а это постоянно требуется.
  2. Полезная библиотека isort(https://github.com/PyCQA/isort) приводит импорты в порядок, например вот тут импорты набросаны как попало.
simenshteyn commented 2 years ago

Спасибо за оперативное и развернутое ревью.

  1. Была реализована проверка наличия роли суперпользователя для всех маршрутов по JWT токену, тестами покрыто. Функция проверки полномочий роли суперюзера для удобства включена в метод BaseService, который является предком каждого инжектируемого сервиса.
  2. Система полномочий у нас устроена следующим образом: пользователь имеет одну или несколько ролей, роль имеет одно или несколько полномочий (создавать, читать и т.п.). Права пользователя на совершение определенного действия определяются по полномочиям, проверка на ручке /user/UUID/permissions/UUID доступна для всех, возвращает объект с булевым значением по полю. Кэширование реализовано, время кэширования вынесено в настройки.
  3. Поправили.
  4. Поправили.
  5. Реализована локальная реализация SwaggerUI в докер в профиле testing, доступно на порту 8080, документация обновлена.
  6. Сообщения вынесены в свойства базового класса сервисов, для удобства использованы namedtuple.
  7. Библиотека добавлена.
BigDeepBlue commented 2 years ago

Все отлично. По п. 1. можно элегантнее: у вас есть удобный декоратор authenticate(), можно было бы ему в качестве необязательного параметра передавать роли и там же проводить проверку, и если роли переданы, а у пользователя их нет... не пускать.