Support Card2K into Dotman (close minevn/dotman#54) #63
Conversation
WalkthroughThe pull request introduces a new card provider named "card2k" to the dotman plugin. This involves adding a new case in the Changes
Sequence DiagramsequenceDiagram
participant User
participant CardProvider
participant Card2KCP
participant CardAPI
User->>CardProvider: Initiate card transaction
CardProvider->>Card2KCP: Create with partner credentials
Card2KCP->>Card2KCP: Generate request parameters
Card2KCP->>CardAPI: Send card verification request
CardAPI-->>Card2KCP: Return response
Card2KCP->>Card2KCP: Parse response
Card2KCP->>User: Update transaction status
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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
🧹 Nitpick comments (6)
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt (5)
14-18: Add constructor parameter validation for robustness.Currently, the constructor assumes non-empty, valid strings. If the config is missing or empty, it might lead to unexpected issues or empty credentials. Consider validating or logging warnings if
partnerId/partnerKeyare blank or malformed.class Card2KCP(private val partnerId: String, private val partnerKey: String) : CardProvider() { init { + require(partnerId.isNotEmpty()) { "partnerId must not be empty" } + require(partnerKey.isNotEmpty()) { "partnerKey must not be empty" } statusCards = CardType.entries.filter { it.getTypeId() != null }.associateWith { true } } }
20-21: Consider making API URLs configurable.These strings are currently hardcoded. If they ever change or if you need to target a test environment, you might want to store them in a config file or constants.
Also applies to: 22-23
35-46: Use a logger instead of direct printing for production environments.Line 36 currently prints the raw response. Logging is generally preferable and can be filtered for different log levels, improving observability. Also, ensure not to log sensitive data in production.
66-66: Avoid reliance on enum index ordering.Relying on the position in
CardPrice.entriesmight break if a new price is inserted in the middle. If the API strictly correlates to ordinal values, freeze or carefully govern the enum’s order or store a custom property forpriceId.
91-91: Confirm card status updates handle concurrency.
updateStatus()delegates tocheckWaitingCards(), which iterates waiting cards. If large volumes of card checks are performed, ensure concurrency or performance is not an issue. Consider caching or throttling if needed.dotman-plugin/src/main/java/net/minevn/dotman/providers/CardProvider.kt (1)
52-56: Validate or log absent config values.Though using
!!ensures non-null, ifpartner-idorpartner-keyis missing in the config, the plugin will crash. Consider safer retrieval with fallback or meaningful error messages.- val partnerId = config.getString("partner-id")!! - val partnerKey = config.getString("partner-key") + val partnerId = config.getString("partner-id").orEmpty() + val partnerKey = config.getString("partner-key").orEmpty() + if (partnerId.isBlank() || partnerKey.isBlank()) { + throw IllegalStateException("partner-id or partner-key is missing or empty in configuration") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dotman-plugin/src/main/java/net/minevn/dotman/providers/CardProvider.kt(2 hunks)dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt(1 hunks)dotman-plugin/src/main/java/net/minevn/dotman/utils/Utils.kt(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt (3)
24-33: Ensure secure or unique request IDs if needed.Using only uppercase letters for the
request_idmight be acceptable for simpler integration. However, if the API or integration requires more uniqueness or security, consider adding digits or using a UUID. Also, confirm if the partner’s system requires the same ID for subsequent checks.
48-51: Confirm correct “waiting” state handling.After marking the log as “waiting,” ensure downstream logic genuinely transitions the card to a processed or failed state. Otherwise, waiting logs might remain indefinite if not properly updated afterward.
53-64: Verify correct mapping for multiple card types.Mapping both
VIETTEL,MOBIFONE,VINAPHONE,GATE, andVIETNAMOBILEto"VNMOBI"can be confusing or incorrect if the API expects different codes for each. Ensure the external API indeed treats them all as the same telecommunication type code.dotman-plugin/src/main/java/net/minevn/dotman/utils/Utils.kt (1)
109-111: Avoid MD5 for security-critical operations.MD5 is considered weak for cryptographic purposes. If the external API mandates MD5, that’s fine. Otherwise, consider a more secure alternative (e.g., SHA-256). Also, confirm that the partner’s documentation explicitly requires MD5.
dotman-plugin/src/main/java/net/minevn/dotman/providers/CardProvider.kt (1)
7-7: Import usage looks fine.No concerns regarding the import for
Card2KCP.
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt
Outdated
Show resolved
Hide resolved
tranlong5252
changed the title
tranlong5252
changed the title
tranlong5252
changed the title
tranlong5252
changed the title
tranlong5252
changed the title
dotman-plugin/src/main/java/net/minevn/dotman/providers/CardProvider.kt
Outdated
Show resolved
Hide resolved
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt
Outdated
Show resolved
Hide resolved
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt
Outdated
Show resolved
Hide resolved
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt (1)
37-46:⚠️ Potential issueUnify the response success logic.
The
parseResponsemethod setsisSuccess = (status == "1"), while inisProcessed()you setisSuccess = (status == "success")and treat every non-“wait” status as processed. This discrepancy can lead to confusing or incorrect status handling. Consider consolidating these two checks to ensure consistent success criteria and processed logic.This issue is similar to a previous review comment regarding misaligned
isSuccessand actual return conditions, so marking it as a duplicate.Also applies to: 67-88
🧹 Nitpick comments (4)
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt (4)
19-21: Consider making the API URLs configurable.Currently,
getApiUrl()andgetCardCheckUrl()return hard-coded URLs, which can be problematic if the endpoint changes or you need to support multiple environments (e.g. development vs. production). Adding a configuration property (in a config file or environment variable) to store these URLs would make the code more flexible and maintainable.
23-25: Generate a truly uniquerequest_id.The
request_idlogic uses random uppercase letters. While this might work for testing or small-scale usage, collisions can still occur. Consider using a more robust approach to ensure uniqueness, such asUUID.randomUUID().toString().
34-35: Replaceprintlnwith proper logging.Using
println(response)for logging may be less transparent in a production environment. Consider using a logging framework (e.g., SLF4J or any existing logger in the project) to manage log levels and outputs.
65-66: Avoid relying on ordinal positions inCardPrice.
(CardPrice.entries.indexOf(this) + 1).toString()depends on the index of the enum in the list. If the order of entries changes, the stored or expected price ID can become incorrect. Consider giving each enum a dedicated ID field and returning that instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dotman-plugin/src/main/java/net/minevn/dotman/providers/CardProvider.kt(2 hunks)dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dotman-plugin/src/main/java/net/minevn/dotman/providers/CardProvider.kt
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt
Outdated
Show resolved
Hide resolved
dotman-plugin/src/main/java/net/minevn/dotman/providers/types/Card2KCP.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dotman-plugin/src/main/java/net/minevn/dotman/card/CardWaiting.kt(1 hunks)dotman-plugin/src/main/java/net/minevn/dotman/database/LogDAO.kt(1 hunks)
🔇 Additional comments (2)
dotman-plugin/src/main/java/net/minevn/dotman/card/CardWaiting.kt (2)
7-7: LGTM! Good choice of default value for backward compatibility.The addition of the
pinproperty with a default empty string value ensures backward compatibility with existing code.
15-15: LGTM! Correctly propagates pin value to Card constructor.The
toCard()function now properly passes thepinvalue to theCardconstructor while maintaining the existing functionality.
| @@ -104,6 +104,7 @@ abstract class LogDAO : DataAccess() { | |||
| getInt("id"), | |||
| getString("uuid"), | |||
| getString("seri"), | |||
| getString("pin"), | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify secure handling of sensitive PIN data.
While the implementation correctly retrieves the PIN from the database, ensure that:
- The PIN is stored securely in the database (e.g., hashed or encrypted).
- The PIN is transmitted securely between components.
- The PIN is not logged or exposed in debug/error messages.
Run this script to check for potential PIN exposure in logs:
🏁 Script executed:
#!/bin/bash
# Description: Check for potential PIN exposure in logs or debug statements
# Search for PIN logging
rg -i 'log|debug|info|error|warn' -A 5 -B 5 | rg -i 'pin|card'Length of output: 27277
Security Concern: Ensure Secure Handling of PIN Data
The code retrieves the PIN with getString("pin") (e.g., in dotman-plugin/src/main/java/net/minevn/dotman/database/LogDAO.kt, line 107). Please verify and enforce the following:
- Secure Storage: Confirm that the PIN is not stored in plaintext. It should be hashed or encrypted at rest.
- Secure Transmission: Ensure the PIN is transmitted between components using secure, encrypted channels.
- Avoid Inadvertent Exposure: The
Card.toString()method (indotman-plugin/src/main/java/net/minevn/dotman/card/Card.kt) includes the PIN, which could lead to accidental logging of sensitive data. Consider removing or masking the PIN in any diagnostic or debug outputs.
Summary by CodeRabbit
New Features
Improvements