Wip: SS Central - whitelist, discord link #946
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request implements two major features: linking Discord accounts via OAuth and integrating a new whitelist system using SS Central. In addition, it refactors HTTP request logging, updates donation level logic, and removes legacy code related to the old whitelist and donation systems. Class Diagram for SScentral SubsystemclassDiagram
class SScentral {
+list discord_links
+load_whitelist()
+load_whitelist_callback(response)
+get_player_discord_async(player)
+get_player_discord_callback(player, response)
+is_player_discord_linked(player)
+is_player_whitelisted(ckey)
+add_to_whitelist(ckey, added_by, duration_days)
+whitelist_ban_player(ckey, admin, duration_days, reason)
+verify_in_discord(player)
+verify_in_discord_callback(player, response)
+update_player_donate_tier_async(player)
+update_player_donate_tier_callback(player, response)
+get_player_donate_tier_blocking(player)
}
note for SScentral "Handles Discord link, whitelist, and donation tier updates via HTTP requests"
class SSHttp {
+create_async_request(method, endpoint, body, headers, callback)
+make_blocking_request(method, endpoint, body, headers)
+log_request(req)
+log_response(res, id)
}
SScentral --> SSHttp : uses
class GLOB {
<<singleton>>
+whitelist: list
}
SScentral ..> GLOB : updates whitelist entries
%% End of SScentral class structure
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Furrior - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The 'req' variable is undefined in this scope. This will cause runtime errors when logging responses. (link)
Overall Comments:
- The
make_blocking_request()implementation using UNTIL is dangerous and could cause server hangs/deadlocks. Consider making this fully asynchronous instead.
Here's what I looked at during the review
- 🔴 General issues: 1 blocking issue, 1 other issue
- 🟢 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 and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @Furrior - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hardcoded SS_CENTRAL_TOKEN found. (link)
Overall Comments:
- The blocking HTTP request using UNTIL is dangerous and could freeze the game if the request hangs. Consider using async requests consistently throughout the codebase.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Hey @Furrior - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Hardcoded SS_CENTRAL_TOKEN found. (link)
Overall Comments:
- The
make_blocking_request()method using UNTIL is dangerous and could cause deadlocks. Consider making this fully async or adding additional safeguards if blocking behavior is absolutely required.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Gaxeer
reviewed
Jan 1, 2025
ss220app
bot
added
the
📜 CL невалиден
AyIong
reviewed
Feb 2, 2025
|
Надо добавить поддержк донатов и чтоб тесты не падали, когда централ отключен |
|
Бля, ну, вроде, работает |
Gaxeer
reviewed
Feb 7, 2025
| var/list/data = json_decode(response.body) | ||
| var/login_endpoint = "[CONFIG_GET(string/ss_central_url)]/player/login?token=[data]" | ||
|
|
||
| to_chat(player, boxed_message("Авторизуйтесь в открывшемся окне и ожидайте 30 секунд.<br/>Если окно не открывается, можете открыть ссылку в браузере самостоятельно:<br/><a href='[login_endpoint]'>[login_endpoint]</a>.")) |
There was a problem hiding this comment.
Suggested change
| to_chat(player, boxed_message("Авторизуйтесь в открывшемся окне и ожидайте 30 секунд.<br/>Если окно не открывается, можете открыть ссылку в браузере самостоятельно:<br/><a href='[login_endpoint]'>[login_endpoint]</a>.")) | |
| to_chat(player, boxed_message("Авторизуйтесь в открывшемся окне и ожидайте 30 секунд.<br/>Если окно не открывается, можете открыть ссылку в браузере самостоятельно:<br/><a href='[login_endpoint]'>Привязать дискорд.</a>.")) |
Gaxeer
reviewed
Feb 7, 2025
Gaxeer
reviewed
Feb 7, 2025
|
|
||
| /// WARNING: only semi async - UNTIL based | ||
| /datum/controller/subsystem/central/proc/is_player_whitelisted(ckey) | ||
| . = (ckey in GLOB.whitelist) |
There was a problem hiding this comment.
не совсем понял зачем это, если ты всё равно делаешь запрос в любом случае, и если запрос зафейлен, то возвращаешь FALSE.
Gaxeer
reviewed
Feb 7, 2025
Gaxeer
approved these changes
Feb 7, 2025
Co-authored-by: Gaxeer <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Что этот PR делает
fixes #191 #65 #66
Почему это хорошо для игры
Изображения изменений
Тестирование
Changelog
🆑
add: Привязка дискорда через OAuth
tweak: Новая система белого списка
/:cl:
Summary by Sourcery
Implement Discord link via OAuth and a new whitelist system that uses SS Central.
New Features:
Tests:
Summary by Sourcery
Update the whitelist system to use SS Central, add Discord account linking via OAuth, and implement a blocking HTTP request method.
New Features:
Tests: