feat: server install deploy keys - #75
Conversation
- Validate server names contain only letters, numbers, hyphens, and underscores - Improve user experience with clear validation error messages
- Require deployer user to already exist in demo-site.sh - Remove user creation logic from demo site playbook - Update output to reflect 'existing' deployer user status - Better separation of installation phases
- Generate SSH deploy keys automatically during server installation - Setup deployer user with proper permissions and group membership - Configure PHP-FPM and Caddy integration with deployer user - Return deploy public key for Git provider configuration - Add comprehensive user and directory permission setup
- Show generated deploy public key for Git provider configuration - Pass server name to installation playbook for SSH key comments - Improve user experience with clear next steps after installation
- Consolidate server-install.sh by removing duplicate code between Ubuntu/Debian branches - Remove DEPLOYER_FAMILY environment variable and related helper functions - Update playbook documentation to clarify Ubuntu/Debian-only support - Hardcode debian family values (www-data user, php8.4-fpm service) - Remove distribution family branching from demo-site.sh - Update PlaybooksTrait documentation
Add validateServerPermissions() method to ServersTrait and integrate it into getServerInfo() to ensure servers have root or sudo permissions before proceeding with operations that require elevated privileges.
Remove duplicate distribution and permissions validation code from ServerInstallCommand. Now uses getServerInfo() which handles both validations centrally, reducing code duplication and improving maintainability.
Update comments across server commands to mention that getServerInfo() validates both distribution and permissions, not just distribution.
WalkthroughRefactors server installation and demo-site playbooks to require/validate an existing deployer user, add server permission validation, introduce deploy key generation/capture and DEPLOYER_SERVER_NAME, and modularize server-install steps; several CLI command comments/docblocks were updated to reflect permission validation. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as ServerInstallCommand
participant Playbook as server-install.sh
participant Server as Remote Server
CLI->>Playbook: run with DEPLOYER_SERVER_NAME, DEPLOYER_DISTRO, DEPLOYER_PERMS
Playbook->>Playbook: setup_repositories()
Playbook->>Playbook: ensure_deployer_user()
Playbook->>Playbook: setup_deploy_user()
Playbook->>Playbook: configure_deployer_groups()
Playbook->>Playbook: setup_deploy_directories()
Playbook->>Playbook: setup_deploy_key()
Playbook->>Server: install packages & configure services
rect rgb(200,220,255)
Note over Playbook,Server: Deploy key generation
Playbook->>Server: write /home/deployer/.ssh/id_rsa.pub
Server-->>Playbook: return deploy_public_key
end
Playbook-->>CLI: YAML output (includes deploy_public_key)
CLI->>CLI: extract deploy_public_key
CLI->>CLI: verifyInstallation(url, deployKey?)
CLI-->>CLI: produce Next steps including deploy key block
sequenceDiagram
participant CLI as DemoSiteCommand
participant Playbook as demo-site.sh
participant Server as Remote Server
rect rgb(255,220,220)
Note over Playbook: Old (removed) flow
Playbook->>Playbook: create_deployer_user() -- removed
Playbook->>Server: manage PHP-FPM users/services -- removed
end
rect rgb(220,255,220)
Note over Playbook: New flow (validation)
CLI->>Playbook: run demo-site
Playbook->>Server: require_deployer_user() (verify exists)
alt user exists
Playbook->>Playbook: continue demo setup
else user missing
Playbook-->>CLI: error -> instruct to run server:install
end
end
Playbook-->>CLI: YAML output (deployer_user: existing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (3)
playbooks/server-install.sh (2)
45-51: Consider extracting shared helper to a common library.The
run_cmd()function is duplicated in bothplaybooks/server-install.shandplaybooks/demo-site.sh. This violates the DRY principle.Consider extracting common playbook helpers to a shared library file that both playbooks can source, e.g.,
playbooks/common.sh:#!/usr/bin/env bash # playbooks/common.sh run_cmd() { if [[ $DEPLOYER_PERMS == 'root' ]]; then "$@" else sudo -n "$@" fi }Then source it in both playbooks:
# Source common helpers source "$(dirname "${BASH_SOURCE[0]}")/common.sh"As per coding guidelines
186-285: Consider splitting this large function.The
install_all_packages()function handles multiple distinct responsibilities: prerequisites, repositories, system utilities, main packages, and PHP configuration. While functional, this could be split into smaller, focused functions for better maintainability.Example structure:
install_all_packages() { install_prerequisites setup_repositories update_package_lists install_system_utilities install_main_packages install_and_configure_php }app/Traits/ServersTrait.php (1)
62-70: Consider short-circuiting on first validation failure.The current implementation calls both validation methods even if the first fails, potentially displaying multiple error messages to the user.
Apply this diff to short-circuit on first failure:
// Validate server distribution and permissions $distroResult = $this->validateServerDistribution($info); + if (is_int($distroResult)) { + return Command::FAILURE; + } + $permissionsResult = $this->validateServerPermissions($info); - - if (is_int($distroResult) || is_int($permissionsResult)) { + if (is_int($permissionsResult)) { return Command::FAILURE; } return $info;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/Console/Server/ServerAddCommand.php(1 hunks)app/Console/Server/ServerInfoCommand.php(1 hunks)app/Console/Server/ServerInstallCommand.php(7 hunks)app/Console/Server/ServerLogsCommand.php(1 hunks)app/Console/Server/ServerProvisionDigitalOceanCommand.php(1 hunks)app/Traits/PlaybooksTrait.php(1 hunks)app/Traits/ServersTrait.php(4 hunks)playbooks/demo-site.sh(4 hunks)playbooks/server-install.sh(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Server/ServerProvisionDigitalOceanCommand.phpapp/Console/Server/ServerLogsCommand.phpapp/Traits/ServersTrait.phpapp/Traits/PlaybooksTrait.phpapp/Console/Server/ServerInfoCommand.phpapp/Console/Server/ServerAddCommand.phpapp/Console/Server/ServerInstallCommand.php
🧬 Code graph analysis (4)
app/Traits/ServersTrait.php (2)
app/Services/IOService.php (1)
info(474-477)app/Contracts/BaseCommand.php (1)
nay(184-188)
playbooks/demo-site.sh (1)
playbooks/server-install.sh (1)
run_cmd(45-51)
playbooks/server-install.sh (1)
playbooks/demo-site.sh (1)
run_cmd(36-42)
app/Console/Server/ServerInstallCommand.php (2)
app/Services/IOService.php (2)
info(474-477)promptSpin(428-441)app/Traits/PlaybooksTrait.php (1)
executePlaybook(45-180)
🪛 GitHub Check: phpstan
app/Console/Server/ServerInstallCommand.php
[failure] 83-83:
Parameter #4 $playbookVars of method Bigpixelrocket\DeployerPHP\Console\Server\ServerInstallCommand::executePlaybook() expects array<string, string>, array<string, mixed> given.
🔇 Additional comments (15)
app/Console/Server/ServerProvisionDigitalOceanCommand.php (1)
167-167: LGTM!The comment accurately reflects the expanded validation scope that now includes permissions alongside distribution validation.
app/Console/Server/ServerLogsCommand.php (1)
65-65: LGTM!The comment update correctly reflects the enhanced validation logic.
app/Traits/PlaybooksTrait.php (1)
37-37: LGTM!The documentation now explicitly lists the supported distributions, improving clarity.
playbooks/server-install.sh (6)
136-184: LGTM!The modular repository setup function properly handles both Ubuntu and Debian distributions with appropriate idempotency checks.
302-313: LGTM!The deployer user creation logic is properly idempotent and uses appropriate flags.
315-355: LGTM!Proper group configuration with necessary service restarts to apply membership changes. Good handling of the www-data user potentially not existing.
388-441: LGTM!The SSH key generation follows security best practices:
- Uses modern ed25519 algorithm
- Sets correct permissions (700/.ssh, 600/private, 644/public)
- Properly idempotent with existence checks
- Empty passphrase is appropriate for automated deployment scenarios
443-481: LGTM!The directory setup properly enforces ownership and permissions, with 750 (rwxr-x---) allowing the deployer user and group members (caddy/www-data) appropriate access.
509-550: LGTM!The main execution flow correctly sequences the installation steps and captures the deployment key for downstream verification.
playbooks/demo-site.sh (2)
47-57: LGTM!The precondition validation properly checks for the deployer user and provides clear guidance to run
server:installfirst.
112-113: LGTM!The hardcoded socket path is correct for both Ubuntu and Debian distributions, simplifying the code appropriately.
app/Console/Server/ServerAddCommand.php (1)
87-87: LGTM!The comment accurately reflects the expanded validation.
app/Console/Server/ServerInfoCommand.php (1)
59-59: LGTM!The comment correctly describes the enhanced validation behavior.
app/Traits/ServersTrait.php (2)
102-119: LGTM!The permission validation correctly enforces root or sudo access requirements with appropriate error handling.
350-353: LGTM!The format validation prevents potentially problematic characters in server names, improving security and reliability.
| [ | ||
| 'distro' => $distro, | ||
| 'permissions' => $permissions, | ||
| ] = $info; | ||
|
|
||
| // | ||
| // Execute installation playbook | ||
| // --- | ||
| // ---- | ||
|
|
||
| $result = $this->executePlaybook( | ||
| $server, | ||
| 'server-install', | ||
| 'Installing server...', | ||
| [ | ||
| 'DEPLOYER_DISTRO' => $distro, | ||
| 'DEPLOYER_FAMILY' => $family, | ||
| 'DEPLOYER_PERMS' => $permissions, | ||
| 'DEPLOYER_SERVER_NAME' => $server->name, | ||
| ], |
There was a problem hiding this comment.
Serialize permissions before passing to playbook
executePlaybook() builds env vars with escapeshellarg((string) $value). When $permissions is the structured array returned by getServerInfo(), this cast produces the literal string "Array", so the playbook never receives the actual permission data and phpstan flags the type mismatch. Normalize the permissions to a string (e.g., JSON) before calling the playbook:
[
'distro' => $distro,
'permissions' => $permissions,
] = $info;
+ if (!is_string($permissions)) {
+ try {
+ $permissions = json_encode($permissions, JSON_THROW_ON_ERROR);
+ } catch (\JsonException $e) {
+ $this->io->error('Failed to encode server permissions: ' . $e->getMessage());
+
+ return Command::FAILURE;
+ }
+ }
...
- 'DEPLOYER_PERMS' => $permissions,
+ 'DEPLOYER_PERMS' => $permissions,This keeps the playbook contract intact and resolves the phpstan error.
🧰 Tools
🪛 GitHub Check: phpstan
[failure] 83-83:
Parameter #4 $playbookVars of method Bigpixelrocket\DeployerPHP\Console\Server\ServerInstallCommand::executePlaybook() expects array<string, string>, array<string, mixed> given.
🤖 Prompt for AI Agents
In app/Console/Server/ServerInstallCommand.php around lines 70 to 87, the code
passes $permissions (an array) directly into the playbook env which casts to the
literal "Array"; serialize the permissions to a string before passing them (for
example: $permissions = json_encode($permissions, JSON_THROW_ON_ERROR) or
fallback json_encode($permissions) if exceptions are not desired) and then pass
that serialized string as DEPLOYER_PERMS in the executePlaybook env array so the
playbook receives the real data and phpstan type mismatch is resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/Console/Server/ServerInstallCommand.php (1)
70-76: Verify permissions is serialized as a string (duplicate concern from past review)The past review identified that
executePlaybookexpectsarray<string, string>but$permissionsextracted from$infomay be an array structure, which would be cast to the literal string"Array"when passed to the playbook.The docblock at line 76 asserts
@var string $permissions, but this doesn't guarantee the actual type fromgetServerInfo(). If the server-info playbook now returns permissions as a JSON string or if validation converts it, this is resolved. Otherwise, the phpstan error and functional issue remain.Run the following script to verify how permissions are returned from getServerInfo and server-info playbook:
#!/bin/bash # Check how permissions are structured in getServerInfo return and server-info playbook output # Find server-info playbook to see what it outputs for permissions fd -t f "server-info.sh" --exec cat {} # Check getServerInfo implementation to see if it transforms permissions ast-grep --pattern $'function getServerInfo($$$) { $$$ }' # Check for any JSON encoding or string conversion of permissions rg -nP --type=php 'permissions.*json_encode|json_encode.*permissions' -A 3 -B 3Also applies to: 88-88, 112-112
🧹 Nitpick comments (1)
app/Console/Server/ServerInstallCommand.php (1)
130-132: Consider trimming whitespace from deploy key (optional)The deploy key extraction logic properly validates that
deploy_public_keyexists, is a string, and is not the placeholder'unknown'. However, it doesn't handle potential leading/trailing whitespace that might come from the playbook output.Consider applying this diff to trim whitespace:
- $deployKey = isset($result['deploy_public_key']) && is_string($result['deploy_public_key']) && $result['deploy_public_key'] !== 'unknown' - ? $result['deploy_public_key'] + $deployKey = isset($result['deploy_public_key']) && is_string($result['deploy_public_key']) && trim($result['deploy_public_key']) !== '' && $result['deploy_public_key'] !== 'unknown' + ? trim($result['deploy_public_key']) : null;This also guards against empty strings while removing any accidental whitespace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Console/Server/ServerInstallCommand.php(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
📄 CodeRabbit inference engine (.cursor/rules/00-main.mdc)
**/*.php: Eliminate single-use methods: inline if a method is called only once
Cache computed values: initialize expensive calculations in the constructor
Avoid method call overhead: prefer direct property access when appropriate
Files:
app/Console/Server/ServerInstallCommand.php
🧬 Code graph analysis (1)
app/Console/Server/ServerInstallCommand.php (3)
app/Services/IOService.php (2)
info(474-477)promptSpin(428-441)app/Traits/PlaybooksTrait.php (1)
executePlaybook(45-180)app/Traits/ServersTrait.php (2)
validateServerDistribution(72-93)ServersTrait(26-386)
🔇 Additional comments (3)
app/Console/Server/ServerInstallCommand.php (3)
89-89: LGTM - DEPLOYER_SERVER_NAME additionThe addition of
DEPLOYER_SERVER_NAMEproperly passes the server name to the playbook. The value from$server->nameis correctly typed and aligns with the AI summary indicating DEPLOYER_FAMILY was replaced with DEPLOYER_SERVER_NAME.
169-169: LGTM - Method signature enhancementThe addition of the optional
$deployKeyparameter toverifyInstallationis well-typed with?stringand maintains backward compatibility while supporting the new deploy key feature.
197-214: LGTM - Dynamic next steps outputThe refactoring of the verification result from static to dynamic next steps is well-executed. The code:
- Builds a base set of instructions that always apply
- Conditionally includes the deploy key with clear user guidance when available
- Uses appropriate formatting with color tags for readability
- Maintains the existing return structure while making it more flexible
1 participant
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Refactor
Documentation