Add pve realm support #170
Conversation
WalkthroughTwo module files introduce a new helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes follow a consistent, repetitive pattern across both files. The new helper function is straightforward (realm extraction logic). Primary review focus should verify that:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
modules/addons/pvewhmcs/pvewhmcs.php (1)
177-203: LGTM! Clean implementation of realm resolution logic.The helper function correctly parses realm-suffixed usernames, defaults to 'pam' for backward compatibility, and validates realms using a whitelist approach (only 'pam' or 'pve' accepted), which prevents injection attacks. The logic handles edge cases reasonably:
- Multiple '@' symbols: correctly takes last segment as realm
- Invalid realms: safely default to 'pam'
- Empty username after '@': preserved and will fail gracefully at API authentication
Optional: Add PHPDoc comment block for clarity.
Consider adding a documentation block to explain the function's behavior, especially edge cases:
+/** + * Resolve username and realm from possibly realm-suffixed input. + * + * Parses username in format "user[@realm]" where realm defaults to 'pam'. + * Only 'pam' and 'pve' realms are accepted; invalid realms default to 'pam'. + * + * @param string $rawUsername Username with optional @realm suffix + * @return array [username, realm] tuple + */ function pvewhmcs_resolve_user_realm($rawUsername) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
modules/addons/pvewhmcs/pvewhmcs.php(4 hunks)modules/servers/pvewhmcs/pvewhmcs.php(14 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lsthompson
Repo: The-Network-Crew/Proxmox-VE-for-WHMCS PR: 135
File: modules/addons/pvewhmcs/pvewhmcs.php:87-111
Timestamp: 2025-07-31T03:17:41.077Z
Learning: In the Proxmox VE for WHMCS project, lsthompson prefers to follow WHMCS conventions and documentation patterns over adding additional error handling or logging in upgrade functions, deferring such enhancements for future improvements.
📚 Learning: 2025-07-31T03:17:41.077Z
Learnt from: lsthompson
Repo: The-Network-Crew/Proxmox-VE-for-WHMCS PR: 135
File: modules/addons/pvewhmcs/pvewhmcs.php:87-111
Timestamp: 2025-07-31T03:17:41.077Z
Learning: In the Proxmox VE for WHMCS project, lsthompson prefers to follow WHMCS conventions and documentation patterns over adding additional error handling or logging in upgrade functions, deferring such enhancements for future improvements.
Applied to files:
modules/addons/pvewhmcs/pvewhmcs.phpmodules/servers/pvewhmcs/pvewhmcs.php
🧬 Code graph analysis (2)
modules/addons/pvewhmcs/pvewhmcs.php (2)
modules/servers/pvewhmcs/pvewhmcs.php (1)
pvewhmcs_resolve_user_realm(872-895)modules/addons/pvewhmcs/proxmox.php (1)
PVE2_API(31-599)
modules/servers/pvewhmcs/pvewhmcs.php (2)
modules/addons/pvewhmcs/pvewhmcs.php (1)
pvewhmcs_resolve_user_realm(179-202)modules/addons/pvewhmcs/proxmox.php (1)
PVE2_API(31-599)
🔇 Additional comments (9)
modules/addons/pvewhmcs/pvewhmcs.php (2)
293-294: LGTM! Correct application of realm resolution.The realm is properly resolved from the server username before API instantiation. This allows administrators to configure servers with explicit realm suffixes (e.g., "admin@pve") while maintaining backward compatibility for existing "admin" entries (which default to "pam").
637-638: LGTM! Consistent realm resolution pattern.The realm resolution is correctly applied in the LOGS tab section, matching the pattern used in the NODES tab. This ensures consistent authentication behavior across all admin module sections.
modules/servers/pvewhmcs/pvewhmcs.php (7)
870-896: LGTM! Helper function implementation matches addon module.The function is implemented identically to the addon module version, ensuring consistent realm resolution behavior across both provisioning and addon modules. The
if (!function_exists(...))guard correctly prevents redefinition errors if both modules are loaded.
139-139: LGTM! Realm resolution correctly positioned at function entry.The realm is resolved once at the beginning of
pvewhmcs_CreateAccount()and the resolved credentials are used consistently in both the KVM Template cloning path (line 155) and the regular VM/CT creation path (line 372). This ensures all creation flows respect the configured realm.
529-530: LGTM! Test connection now supports realm-suffixed credentials.The realm resolution is correctly applied in the test connection function, allowing administrators to validate server credentials that include explicit realm suffixes. This is important for the initial server configuration workflow.
562-564: LGTM! Lifecycle functions consistently apply realm resolution.The suspend, unsuspend, and terminate functions all correctly resolve the realm before API authentication. The consistent pattern across these critical lifecycle operations ensures reliable guest management regardless of the configured authentication realm.
Also applies to: 600-602, 638-640
940-942: LGTM! Client area correctly resolves realm for guest information retrieval.The realm resolution is properly applied in the client area function, ensuring end users can view their guest information regardless of which authentication realm is configured for the Proxmox server. This is critical for the client-facing functionality.
1219-1220: LGTM! All VM control functions consistently apply realm resolution.The start, reboot, shutdown, and stop functions all follow the same pattern: retrieve server credentials from the database, resolve the realm, and authenticate using the resolved values. This consistency ensures reliable guest control operations for end users regardless of the configured authentication realm.
Also applies to: 1262-1263, 1315-1316, 1359-1360
1145-1145: LGTM! VNC functions correctly retain hardcoded 'pve' realm.The VNC and SPICE console functions intentionally continue using the hardcoded "pve" realm for the special 'vnc' user, as noted in the PR description. This is correct because the VNC proxy user must authenticate to the 'pve' realm specifically, independent of the realm configured for the main server credentials.
Also applies to: 1182-1182
|
@louis-berruyer many thanks for the PR! Have you tested this? Can you please show screenshots? |
|
I think there is AI involved in this without sufficient intervention. Please clean-up and tag me here so we can re-open. :-)
|
2 participants
Summary
Details
Summary by CodeRabbit