02.10.2018 14:30
BugM 24 2100 Источник
image
Все мы часто пишем простые методы в контроллерах работающие через числовые идентификаторы.

    @RequestMapping(value = {"/entityName/{entityId}/get"}, method = RequestMethod.GET)
    @ResponseBody
    public Entity get(@PathVariable(value = "entityId") Integer entityId) {
        //возврат значения сущности по ID
    }

Пришедший ID надо валидировать.Например, не у всех пользователей есть доступ ко всем ID.

Понято что использовать UUID безопаснее и надежнее. Но его надо создавать, хранить, индексировать итд. Для всех сущностей это делать долго и сложно. Во многих случаях проще работать по обычным числовым ID.

Валидацию можно сделать по простому:

    @RequestMapping(value = {"/entityName/{entityId}/get"}, method = RequestMethod.GET)
    @ResponseBody
    public Entity get(@PathVariable(value = "entityId") Integer entityId) {
        if(!@dao.validate(entityId))
             return some_error;

        //возврат значения сущности по ID
    }

Плюс в таком решении только один. Просто и быстро.
Все остальное плохо. Дублирование кода, валидация не совместима с валидацией объектов, нужна отдельная обработка в каждом методе.

Хочется сделать так:

    @RequestMapping(value = {"/entityName/{entityId}/get"}, method = RequestMethod.GET)
    @ResponseBody
    public Entity get(@Validated @PathVariable(value = "entityId") Integer entityId) {
        //возврат значения сущности по ID
    }

Этот простой и логичный вариант не работает. Валидатор просто не вызывается. Валидация PathVariable Спрингом не поддерживается.

Для работоспособности этого варианта надо превратить PathVariable в ModelAttribute:


    @ModelAttribute
    private Integer integerAsModelAttribute(@PathVariable("entityId") Integer id) {
        return id;
}

И получить ошибку при обращении к контроллеру. У врапперов генерик типов нет дефолтного конструктора без параметров и нет сеттера. Обходится это с помощью использования Optional. У него есть и дефолтный конструктор и сеттер принимающий обычные инты.

Превращаем Integer в Optional:


    @ModelAttribute
    private Integer integerAsModelAttribute(@PathVariable("entityId") Optional<Integer> id) {
        return id.orElse(null);
    }

И соответственно сам метод контроллера и объявление валидатора:


    @InitBinder({"entityId"})
    protected void initCommissionIdBinder(WebDataBinder binder) {
        binder.setValidator(validateEntityIdValidator);
        binder.setBindEmptyMultipartFiles(false);
    }

    @RequestMapping(value = {"/entityName/{entityId}/get"}, method = RequestMethod.GET)
    @ResponseBody
    public Entity get(@Validated @ModelAttribute(value = "entityId") Integer entityId) {
       //Можно смело работать с ID. Оно уже валидировано.
    }

Класс валидатора абсолютно обычный:


public class ValidateIntegerValidator implements Validator {
    @Override
    public boolean supports(Class<?> aClass) {
        return Integer.class.equals(aClass);
    }

    @Override
    public void validate(Object o, Errors errors) {

        if(o instanceof Integer) {
            Integer integer = (Integer) o;

            if(!dao.checkId(integer)) {
                errors.reject("-1", "ERROR");
            }
        } else {
            errors.reject("-2","WRONG TYPE");
        }

    }
}

Полный рабочий пример можно взять тут.

Комментарии (24)


  1. voopr
    03.10.2018 00:18
    #19185051

    Пришедший ID надо валидировать.

    Зачем?
    Ваш пример, на мой взгляд, неудачный хотя бы потому что вы по сути пихаете бизнес логику в не предназначенное для нее место.
    А другого примера мне придумать не удалось.


  1. BugM Автор
    03.10.2018 00:25
    #19185073

    В смысле зачем? Сценарий с доступом пользователя не ко всем сущностям одного типа очень стандартный.
    От документооборотов (не все документы доступны) до форумов (не все разделы доступны) везде встречается.

    Разрулить это спринговыми ролями невозможно. Доступ к методу у пользователя должен быть.

    Бизнес логика это действия чтобы собрать и отдать этот объект. Неправильно в ней писать проверку прав пользователя.


    1. voopr
      03.10.2018 12:17
      #19186343

      Я не про сценарий, а про использование валидатора не по назначению.

      Бизнес логика это действия чтобы собрать и отдать этот объект. Неправильно в ней писать проверку прав пользователя.

      Например доступ к документу зависит от его статуса. Что бы узнать статус вы должны получить этот документ из хранилища и если все хорошо, то вызывается метод бизнес логики который опять запрашивает документ. Т.е. уже дважды придется делать одну и ту же работу.


      1. BugM Автор
        03.10.2018 13:02
        #19186597

        Кеширование в помощь.
        Второе обращение к документу не будет стоить практически ничего, по сравнению с первым.


        1. voopr
          03.10.2018 14:08
          #19186961

          А код?
          Вы в двух местах напишите одинаковый код, а потом когда правила поменяются будете его править в двух местах.
          А придет новый человек и поправит только в одном месте.


          1. BugM Автор
            03.10.2018 14:17
            #19187001

            Получения объекта из хранилища это настолько типовая и дублирующаяся в куче мест операция что о ней можно не беспокоиться. Она должна работать, проверяться тестами и кешироваться. Правки затрагивающие ее не должны ломать существующий код.

            Все остальное только в одном месте. Отдельно проверка прав доступа. Отдельно работа с объектом.

            Новый человек должен быть в курсе таких вещей.


      1. ValDubrava
        04.10.2018 19:08
        #19193059

        Мне кажется, что это не энтерпрайз приложение, и тут просто опущен слой сервисов с методами типа getUser(int id), в котором обычно и делается проверка на то, доступна ли сущность. Однако java благодаря spring boot становится доступней =)


    1. lavilav
      03.10.2018 13:00
      #19186583

      так а что проверяет dao метод checkId? ровно тоже самое, что и для всех остальных пользователей.
      традиционно делается поиск типо такого repository.findByUsernameAndId(username, id).orElseThrow { new ItemNotAvailableException }
      построение связности доступа к этому объекту можно решить многий ко многим и убрать непонятный валидатор с непонятным dao.


      1. BugM Автор
        03.10.2018 13:07
        #19186627

        С методами findByUsernameAndId(username, id) проверяюшими права возникает проблема когда надо делать доступ без пользователей. К примеру, автоматические выгрузки и тому подобные сервисные операции.

        Приходится или разрешать username==null и давать доступ ко всему, что черевато дырами при ошибках в контроллерах.
        Или заводить технические учетные записи, что вызывает проблемы управления и поддержки.
        Или писать рядом такие же методы, не проверяющие права. Плохо само по себе.

        Вариант findById(id) не проверяющее никаких прав и проверка прав на уровне контроллеров выглядит гораздо лучше.


    1. GoodGreenTea
      03.10.2018 18:07
      #19188143

      Я б тоже на @Validated отдавал бы только простейшие влидации, который там сейчас поддерживаются javax.validation аннотациями. А более сложные, с участием базы данных, пусть делает сервис и кидает исключение в случае чего.


  1. Apx
    03.10.2018 01:17
    #19185191

    Optional * exception mapper Самый правильный вариант, если хочется для однотипных вызовов иметь предсказуемое и "стандартное" в рамках проекта поведение, но это больше похоже на gist в гите, а не на статью для хабра IMO


    1. BugM Автор
      03.10.2018 13:14
      #19186677

      Меня и так в сложности решения чуть ниже упрекают. Хотя я и упрощал как мог.
      Захотелось на русском написать. А кроме Хабра некуда.


  1. ruomserg
    03.10.2018 10:09
    #19185821

    Вот при всем моем уважении — вариант с явным вызовом валидатора обладает одним преимуществом, которое на длинной дистанции перевешивает все его минусы. Даже для человека, который только вчера начал программировать и увидел проект первый раз в жизни — совершенно очевидно, что в этой точке производится валидация параметра, и ясно — какой класс (объект) за это отвечает. И если что-то пошло не так, то можно поставить точку останова и посмотреть — что, где, и почему… А в случае с аннотациями — вызов валидатора утонет в сгенерированных проксях и магии байткода. И когда это как-то сломается (а ломается рано или поздно все!) — разборки с магией могут длиться часами и днями.

    Мой опыт в программировании для промышленного производства показывает, что код системы следует писать так, чтобы он был очевидным для разработчика квалификации ниже средней. Не потому что разработчики плохие — а потому что если разработчика в два ночи поднять срочным звонком, работоспособность и острота мышления (оказывается!) сильно снижается. А проблему надо решать, и притом прямо сейчас, и с первого раза. Когда код изначально был написан так, чтобы «дураку понятно было» — это очень помогает. Пусть даже ценой некоторого дублирования и общей некрасивости.

    Как это обычно бывает — не догма, и your mileage may vary! :-)


    1. BugM Автор
      03.10.2018 13:13
      #19186665

      Вы мало проектов на Спринге видели. @Validated это понятнее некуда. Самое простое и элегантное решение. Проверка в каждом методе и создание сообщений об ошибках во вью тоже в каждом методе, выходит в конечном итоге сложнее и потенциально содержит больше ошибок.

      И обвязка это самое простое из того что я нашел в залежах «нестандартного Спринга». Все остальное там гораздо сложнее. Проблемы вида совместить windows domain sso и spring form authorization и заставить все это работать работать на виндовом кластере решаются гораздо сложнее. Там такие дебри…


      1. ruomserg
        03.10.2018 13:30
        #19186777

        Ну вот, я же говорю — тут каждый балансирует как может. Мы в свое время наелись магии и чертовщины в Java EE, и теперь очень критично выбираем фичи из фреймворков. Из того же спринга, пользуемся только Transactional, Autowired, RequestMapping и еще парой-тройкой сопутствующих аннотаций. Может быть везет что задачи специфичные — но пока хватает. А может быть возраст и опыт прививают аллергию на сложные решения… В общем, я остаюсь по отношению к внешним компонентам на позиции: пусть или оно незаметно сразу правильно работает «из коробки», или пишем свой функционал и явным образом где нужно вызываем. А сложная магия — ну ее: и порог вхождения для специалиста выше, и при отказе — поди-пойми куда бежать…


        1. BugM Автор
          03.10.2018 14:24
          #19187033
          +1

          Java EE вообще оказалась неудачной концепцией. JSR гораздо удобнее.

          Ну тоже вариант. Мне ближе вариант все что можно повесить на аннотации и по максимуму задействовать функционал уже подключенных библиотек. Иногда это требует доработки напильником. Порог входа повышается, зато мест где можно сделать трудноуловимые ошибки становится гораздо меньше. Хотя и ловить их временами сложнее.


  1. ultrinfaern
    03.10.2018 14:55
    #19187145

    Смешались кони, люди… Зачем смешивать авторизацию и контроль входных данных?
    Кстати спрнг для вашего решения имеет стандарт: @PreAuthorize, @PostAuthorize и даже фильтрации данных @PreFilter и @PostFilter. Для них тоже прекрасно пишутся свои функции контроля. И соответственно в коде отделяется бизнес логика от секюрити логики.


    1. ruomserg
      03.10.2018 15:56
      #19187435

      А нет впечатления, что вот этот подход спринга к безопасности очень похож на ситуацию, когда мы сначала нарисовали приложение — а потом (вдруг!) решили навесить сверху разграничение доступа. То есть для простых приложений — это где-то и удобно: сделали MVP, потом улучшили, потом набросили какую-то безопасность, и оно живет. Но в более-менее сложном и критичном приложении безопасность полагается интегрировать. И чтобы оно при нарушении этой подсистемы, желательно, вообще работать не могло…


      1. ultrinfaern
        03.10.2018 19:23
        #19188517

        Нет. Так как бизнес логика и секюрити это две разные подсистемы и одна о другой должны знать как можно меньше.


        1. ruomserg
          03.10.2018 20:09
          #19188681

          В простых системах, пожалуй да. А если нужно от пользователя через запрос тащить какой-нибудь seciruty-token через всю бизнес-логику, чтобы где-то в конце упихать его в подсистему аудита (а то и в лог БД)? А если с требованием, чтобы если аудит не записался, то и операцию не исполнять? Как тут провести различие между бизнес-логикой и безопасностью. По сути, они в ТЗ делают требования безопасности частью логики бизнес-процессов.


          1. BugM Автор
            03.10.2018 22:49
            #19189175

            Такое даже в банк клиентах не используется, насколько я в курсе.
            2fa и хватит.

            Не поделитесь где такое видели в реальной жизни?


    1. Leffchik
      03.10.2018 16:09
      #19187525

      Абсолютно согласен. Валидатор должен проверять корректность запроса (или его тела), а не доступ к сущности (И вернуть, к примеру HTTP 400). Для проверки секьюрити можно просто воспользоваться @PreAuthorize:

      @PreAuthorize("@AccessService.checkAccess(#entityId)")
@RequestMapping(value = {"/entityName/{entityId}/get"}, method = RequestMethod.GET)
    @ResponseBody
    public Entity get(@PathVariable(value = "entityId") Integer entityId) {
        //возврат значения сущности по ID
    }


      Далее написать секьюрити сервис, который проверит права доступа. В данном случае, при отсутствии доступа вернётся HTTP 403 Forbidden.


      1. BugM Автор
        03.10.2018 16:19
        #19187589

        Проверка прав доступа это просто пример. Самый очевидный, но пример.
        Никто не мешает так валидировать обычные параметры. Когда по каким-то соображениям передается не объект, а пачка переменных.

        Там какой-нибудь: /edit/setExternalLinkTo/anotherEntityId
        Возвращать 400 нехорошо, а проверить существование сущности перед началом обновления объектов хочется.


  1. ultrinfaern
    03.10.2018 19:30
    #19188551

    А вот ещё вопрос: если хочется валидировать параметры, почему бы не воспользоваться всей мощью АОР, да и написать реальный обработчик таких ситуаций. А функцию валидации можно передать через класс в @Validated.