Error while signing on using Discord if no avatar #1649 #1650
Conversation
WalkthroughThe changes focus predominantly on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Discord
participant SignOnController
User->>Browser: Accesses Sign-On Page
Browser->>Discord: Initiates OAuth Login
Discord-->>Browser: Redirects with Callback (OAuth Code)
Browser->>SignOnController: Sends Callback with OAuth Code
SignOnController->>Discord: Gets User Info with OAuth Code
Discord-->>SignOnController: Returns User Info (JSON)
SignOnController->>SignOnController: Processes Avatar Field
Note right of SignOnController: Checks if `avatar` is null or empty
SignOnController-->>Browser: Returns Sign-On Response
Browser->>User: Displays Sign-On Result
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Additional context usedLearnings (1)
Tip Early access features: enabledWe are currently testing the following features in early access:
Note:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/main/java/ai/elimu/web/SignOnControllerDiscord.java (1)
Line range hint
58-58: Remove hardcoded API key.The API key should not be hardcoded in the source code. Use environment variables or a secure configuration management system to store and retrieve the API key.
// Example of using environment variables String clientSecret = System.getenv("DISCORD_API_SECRET"); if (clientSecret == null) { throw new IllegalStateException("DISCORD_API_SECRET environment variable not set"); }
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1650 +/- ##
=========================================
Coverage 13.78% 13.78%
Complexity 454 454
=========================================
Files 257 257
Lines 8503 8503
Branches 881 881
=========================================
Hits 1172 1172
Misses 7279 7279
Partials 52 52 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/main/java/ai/elimu/web/SignOnControllerDiscord.java (1)
Line range hint
58-58: Externalize client secrets to configuration files or environment variables.Hardcoding client secrets is a security risk. Consider moving them to configuration files or environment variables.
- String clientSecret = "Lf0O65Zk0Qv9cXyC7igPgljIjSJJBZbv"; + String clientSecret = ConfigHelper.getProperty("discord.api.secret.dev");
There was a problem hiding this comment.
@venkatesh2k3 It should be possible to fix this bug with one or two lines of code, to keep the amount of code to a minimum.
Do you think it would work to replace jsonObject.has("avatar") with something like jsonObject.has("avatar") && !StringUtils.isBlank(jsonObject.get("avatar"))?
I haven't seen a sample of what the JSON response looks like when the bug occurs, but maybe this additional check would work?
Or maybe jsonObject.has("avatar") && (jsonObject.get("avatar") instanceof String)?
Yeah it will optimize the code @jo-elimu |
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/main/java/ai/elimu/web/SignOnControllerDiscord.java (1)
Line range hint
58-58: Remove hardcoded API keys.Detected a Generic API Key, potentially exposing access to various services and sensitive operations. Store sensitive data in environment variables or a secure vault.
- String clientSecret = "Lf0O65Zk0Qv9cXyC7igPgljIjSJJBZbv"; + String clientSecret = ConfigHelper.getProperty("discord.api.secret");Tools
GitHub Check: codecov/patch
[warning] 137-141: src/main/java/ai/elimu/web/SignOnControllerDiscord.java#L137-L141
Added lines #L137 - L141 were not covered by tests
[warning] 143-143: src/main/java/ai/elimu/web/SignOnControllerDiscord.java#L143
Added line #L143 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
src/main/java/ai/elimu/web/SignOnControllerDiscord.java (1)
Line range hint
58-58: Remove or secure the exposed API key.A generic API key is detected, which could potentially expose access to various services and sensitive operations. Ensure this key is secured.
- String clientSecret = "Lf0O65Zk0Qv9cXyC7igPgljIjSJJBZbv"; + String clientSecret = ConfigHelper.getProperty("discord.api.secret");
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/main/java/ai/elimu/web/SignOnControllerDiscord.java (1)
Line range hint
58-58: Avoid hardcoding API keys.Detected a Generic API Key, potentially exposing access to various services and sensitive operations. Store the API key securely using environment variables or a secrets management service.
- String clientSecret = "Lf0O65Zk0Qv9cXyC7igPgljIjSJJBZbv"; + String clientSecret = System.getenv("DISCORD_API_SECRET");
| if (!jsonObject.isNull("avatar")) { | ||
| String uriAvatar = "https://cdn.discordapp.com/avatars/" + jsonObject.getLong("id") + "/" + jsonObject.getString("avatar") + ".png"; | ||
| logger.info(uriAvatar); | ||
| logger.info("Avatar URL: " + uriAvatar); | ||
| contributor.setImageUrl(uriAvatar); | ||
| } | ||
|
|
There was a problem hiding this comment.
Enhance validation for the avatar field and extract the default avatar URL into a constant.
The current implementation only checks if the avatar field is not null. It should also check if the field is a non-empty string to ensure robustness. Additionally, extract the default avatar URL into a constant to avoid duplication and improve maintainability.
private static final String DEFAULT_AVATAR_URL = "https://e7.pngegg.com/pngimages/84/165/png-clipart-united-states-avatar-organization-information-user-avatar-service-computer-wallpaper-thumbnail.png";
// Usage in the method
if (jsonObject.has("avatar") && jsonObject.get("avatar") instanceof String && !StringUtils.isBlank(jsonObject.getString("avatar"))) {
String avatar = jsonObject.getString("avatar");
String uriAvatar = "https://cdn.discordapp.com/avatars/" + jsonObject.getLong("id") + "/" + avatar + ".png";
logger.info("Avatar URL: " + uriAvatar);
contributor.setImageUrl(uriAvatar);
} else {
logger.info("Avatar field is missing, not a string, or is blank.");
contributor.setImageUrl(DEFAULT_AVATAR_URL);
}Tools
GitHub Check: codecov/patch
[warning] 139-139: src/main/java/ai/elimu/web/SignOnControllerDiscord.java#L139
Added line #L139 was not covered by tests
|
|
||
| if (!jsonObject.isNull("avatar")) { | ||
| String uriAvatar = "https://cdn.discordapp.com/avatars/" + jsonObject.getLong("id") + "/" + jsonObject.getString("avatar") + ".png"; | ||
| logger.info("Avatar URL: " + uriAvatar); | ||
| contributor.setImageUrl(uriAvatar); | ||
| } |
There was a problem hiding this comment.
Remove redundant code.
The logic to handle the avatar field is duplicated and should be removed.
- if (!jsonObject.isNull("avatar")) {
- String uriAvatar = "https://cdn.discordapp.com/avatars/" + jsonObject.getLong("id") + "/" + jsonObject.getString("avatar") + ".png";
- logger.info("Avatar URL: " + uriAvatar);
- contributor.setImageUrl(uriAvatar);
- }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!jsonObject.isNull("avatar")) { | |
| String uriAvatar = "https://cdn.discordapp.com/avatars/" + jsonObject.getLong("id") + "/" + jsonObject.getString("avatar") + ".png"; | |
| logger.info("Avatar URL: " + uriAvatar); | |
| contributor.setImageUrl(uriAvatar); | |
| } |
Tools
GitHub Check: codecov/patch
[warning] 145-147: src/main/java/ai/elimu/web/SignOnControllerDiscord.java#L145-L147
Added lines #L145 - L147 were not covered by tests
venkatesh2k3
force-pushed
the
main
branch
2 times, most recently
from
45befbe to
674a67b
Compare
There was a problem hiding this comment.
There seems to be a compilation error: https://github.com/elimu-ai/webapp/actions/runs/9758324950/job/26932626216?pr=1650
Description
Changes Made:
Enhanced handling of the avatar field in SignOnControllerDiscord.java to ensure robust behavior when the avatar is missing or not a string.
Details:
I added validation to check for the avatar field in the JSON response, implemented logic to handle empty or non-string avatar fields, and introduced a default avatar URL fallback mechanism.
Impact:
This update ensures the application handles sign-on scenarios without Discord avatars gracefully and consistently, providing a stable user experience.
Testing:
Tested locally to verify functionality across different scenarios (avatar present, empty avatar).
Note:
The default avatar URL (https://e7.pngegg.com/pngimages/84/165/png-clipart-united-states-avatar-organization-information-user-avatar-service-computer-wallpaper-thumbnail.png) used in this implementation can be replaced with an actual URL pointing to a suitable default image for the application.