-
-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Авторизация по OAuth #243
Авторизация по OAuth #243
Conversation
Reviewer's Guide by SourceryThis pull request introduces an File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @MSITETOP - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Potential mutation of params dictionary. (link)
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 3 other issues
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
1,2 видел. насколько я понял это решение было примерно 5 лет назад. с тех пор уже много всего изменилось. более того я не вижу смысла в вашу библиотеку пихать механизм рефреша токена, так как он за собой влечет и хранение этих токенов, а это какая-то БД и тут опять не хотелось бы привязываться к конкретной БД и ее структуре. |
Кажется, с точки зрения OAuth-авторизации не изменилось ничего.
не вижу логики -- зачем хранить токены в БД? |
в целом я не против варианта реализации предложенного в Авторизация по OAuth #149
а где их еще хранить? наши приложения ставят на разные порталы у каждого портала свой домен и остальные креды. нам нужно по каждому порталу хранить домен, рефреш токен, access_token (чтобы не рефрешить его каждый раз), а так же данные специфичные для нашего приложения |
кажется я понял о чем ты "зачем делать реализацию хранения токена в fast_bitrix24, если для получения токена используется своя произвольная функция получения токена?" согласен, в этой реализации хранение токенов внутри fast_bitrix24 не требуется |
Это можно поправить. Ключевое, что мне хотелось бы видеть в релизе -- это какой-то механизм обновления токена. У меня в #149 это сделано через вызов пользовательской функции, которая будет возвращать свежий токен. То есть, удобным способом отдано на откуп пользователю. Возможно, есть еще какие-то механизмы? У меня предложение -- можете попробовать у себя покрутить #149 ? Если хотите, добавить туда упаковку auth в тело POST-запроса. Либо в свой PR добавьте обновление токена. |
ок. тоесть мне в этом pr сделать реализацию с передачей функции, а не токена напрямую? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ок. тоесть мне в этом pr сделать реализацию с передачей функции, а не токена напрямую?
Я добавил обновление токена. Осталось:
- протестировать работоспособность
- написать автотесты
- обновить документацию
я чем сейчас могу помочь? протестировать работоспособность? |
Да. У меня нет под рукой аккаунта, где требуется авторизация, и провести интеграционное тестирование я сам не смогу. Ну и в отношении автотестов и документации помощь тоже приветствуется. :) |
ок. в течении дня протестирую и отпишусь |
Мне также нужно будет получить от вас образец ответа сервера, который отправляется при необходимости обновления токена. Его можно получить, например, включив логирование. Сможете прислать? |
да. все отправлю после тестов. в том числе (видимо в личку в телеге) стенд где можно проверить и креды приложения тестового. |
код ответа при протухании токена 401, но с механизмом рефреша, что то не так. нужна будет твоя помощь, чтобы это исправить наверное |
Добавить авторизацию по токену.
При инициализации объекта Bitrix пользователь добавляет параметр token_func, содержащий ссылку на асинхронную функцию, которая вызывается библиотекой при необходимости получения или обновления токена.
Эта функция должна возвращать токен авторизации.
Если сервер возвращает ошибку о том, что токен устарел, все прочие запросы, идущие в параллели, приостанавливаются, пока не будет обновлен токен.