feat: file permissions fix for multi-user environments#906
Conversation
- Detects UID/GID mismatch between host user and container's www-data user - Updates container permissions to match the host user's UID/GID - Maintains backward compatibility by creating a centos group for files with the original GID 1000 - Restarts affected containers to apply changes
6cf71b3 to
d4f91a0
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds automatic file permission synchronization for multi-user environments to prevent UID/GID mismatches between the host user and the container's www-data user. The implementation runs during warden env up and warden env start commands.
Changes:
- Detects UID/GID mismatches between host and container
www-datauser - Automatically updates container permissions to match host UID/GID
- Creates backward-compatible group for files with original GID 1000
- Restarts affected containers to apply changes
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| && [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then | ||
|
|
||
| # Update www-data user/group to match host UID/GID | ||
| $WARDEN_BIN env exec -u 0 "$container" usermod -u "${HOST_UID}" www-data 2>/dev/null || true |
There was a problem hiding this comment.
The usermod command to change UID can fail if files owned by the www-data user are currently open or in use, or if the new UID conflicts with another existing user. While the '|| true' suppresses the error, silently failing could leave the system in an inconsistent state where some operations succeed and others fail. Consider checking the exit status and providing a warning message to the user if the operation fails.
| fi | ||
|
|
||
| # Restart container to apply changes | ||
| $WARDEN_BIN env restart "$container" |
There was a problem hiding this comment.
Restarting containers sequentially in a loop could significantly slow down the startup process when multiple containers need permission fixes. Each restart waits for the previous container to fully restart before proceeding. Consider collecting all containers that need permission fixes and restarting them in parallel, or restart them all at once after all permission modifications are complete.
| # Check if container is running (exact match) and www-data UID doesn't match host UID | ||
| CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "") | ||
| if $WARDEN_BIN env ps --services 2>/dev/null | grep -q "^${container}$" \ | ||
| && [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then |
There was a problem hiding this comment.
The check 'CONTAINER_UID != HOST_UID' only validates that UIDs differ, but doesn't validate that CONTAINER_UID is a valid numeric value. If the 'id -u www-data' command returns an unexpected value or error message (despite the '|| echo ""' fallback), this could cause unexpected behavior. Consider adding validation to ensure CONTAINER_UID contains only digits before comparison.
| && [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then | |
| && [[ -n "$CONTAINER_UID" ]] \ | |
| && [[ "$CONTAINER_UID" =~ ^[0-9]+$ ]] \ | |
| && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then |
| ## this ensures www-data UID/GID matches the host user to prevent permission issues | ||
| if [[ "${WARDEN_PARAMS[0]}" == "up" ]] || [[ "${WARDEN_PARAMS[0]}" == "start" ]]; then | ||
| for container in php-fpm php-debug php-spx; do | ||
| # Check if container is running (exact match) and www-data UID doesn't match host UID |
There was a problem hiding this comment.
The comment states "Check if container is running (exact match)" but the actual check using 'ps --services' only verifies that the service is defined in the docker-compose configuration, not that the container is actually running. This comment is misleading and should be corrected to accurately reflect what the code does, or the code should be updated to actually verify the container is running.
| # Check if container is running (exact match) and www-data UID doesn't match host UID | |
| # Check if container service exists (exact match) and www-data UID doesn't match host UID |
| ## fix file permissions for multi-user environments | ||
| ## this ensures www-data UID/GID matches the host user to prevent permission issues | ||
| if [[ "${WARDEN_PARAMS[0]}" == "up" ]] || [[ "${WARDEN_PARAMS[0]}" == "start" ]]; then | ||
| for container in php-fpm php-debug php-spx; do | ||
| # Check if container is running (exact match) and www-data UID doesn't match host UID | ||
| CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "") | ||
| if $WARDEN_BIN env ps --services 2>/dev/null | grep -q "^${container}$" \ | ||
| && [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then | ||
|
|
||
| # Update www-data user/group to match host UID/GID | ||
| $WARDEN_BIN env exec -u 0 "$container" usermod -u "${HOST_UID}" www-data 2>/dev/null || true | ||
| $WARDEN_BIN env exec -u 0 "$container" groupmod -g "${HOST_GID}" www-data 2>/dev/null || true | ||
|
|
||
| # Create backward-compatible group for original GID 1000 files (only if host GID differs) | ||
| if [[ "${HOST_GID}" != "1000" ]]; then | ||
| $WARDEN_BIN env exec -u 0 "$container" sh -c "getent group centos >/dev/null 2>&1 || groupadd -g 1000 centos" 2>/dev/null || true | ||
| $WARDEN_BIN env exec -u 0 "$container" usermod -aG centos www-data 2>/dev/null || true | ||
| fi | ||
|
|
||
| # Restart container to apply changes | ||
| $WARDEN_BIN env restart "$container" | ||
| fi | ||
| done | ||
| fi |
There was a problem hiding this comment.
The permission fix block runs after Mutagen sync operations (lines 237-264) and can restart containers, which will invalidate the Mutagen sync connection that was just established. When a container is restarted on line 286, the container ID changes, breaking the Mutagen sync that checks for matching container IDs (line 243). This creates a race condition where Mutagen sync may be connected to a stale container.
The permission fix should run before Mutagen sync operations are initiated to avoid disrupting the sync connection.
| $WARDEN_BIN env exec -u 0 "$container" usermod -u "${HOST_UID}" www-data 2>/dev/null || true | ||
| $WARDEN_BIN env exec -u 0 "$container" groupmod -g "${HOST_GID}" www-data 2>/dev/null || true |
There was a problem hiding this comment.
The HOST_UID and HOST_GID variables are used without validation. If these values are empty or invalid (e.g., due to an environment issue), the usermod and groupmod commands will fail or produce unexpected results. Although these variables are set early in the script (lines 13-14), adding explicit validation before use would make the code more robust and provide clearer error messages if something goes wrong.
| # Create backward-compatible group for original GID 1000 files (only if host GID differs) | ||
| if [[ "${HOST_GID}" != "1000" ]]; then | ||
| $WARDEN_BIN env exec -u 0 "$container" sh -c "getent group centos >/dev/null 2>&1 || groupadd -g 1000 centos" 2>/dev/null || true | ||
| $WARDEN_BIN env exec -u 0 "$container" usermod -aG centos www-data 2>/dev/null || true |
There was a problem hiding this comment.
The hardcoded group name "centos" may conflict with existing groups in the container or be confusing since it's not descriptive of its purpose. Consider using a more descriptive name like "legacy-files" or "original-gid" to better communicate that this group exists for backward compatibility with files created by the original GID 1000.
| # Create backward-compatible group for original GID 1000 files (only if host GID differs) | |
| if [[ "${HOST_GID}" != "1000" ]]; then | |
| $WARDEN_BIN env exec -u 0 "$container" sh -c "getent group centos >/dev/null 2>&1 || groupadd -g 1000 centos" 2>/dev/null || true | |
| $WARDEN_BIN env exec -u 0 "$container" usermod -aG centos www-data 2>/dev/null || true | |
| # Create backward-compatible group "legacy-files" for original GID 1000 files (only if host GID differs) | |
| if [[ "${HOST_GID}" != "1000" ]]; then | |
| $WARDEN_BIN env exec -u 0 "$container" sh -c "getent group legacy-files >/dev/null 2>&1 || groupadd -g 1000 legacy-files" 2>/dev/null || true | |
| $WARDEN_BIN env exec -u 0 "$container" usermod -aG legacy-files www-data 2>/dev/null || true |
| # Check if container is running (exact match) and www-data UID doesn't match host UID | ||
| CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "") | ||
| if $WARDEN_BIN env ps --services 2>/dev/null | grep -q "^${container}$" \ | ||
| && [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then |
There was a problem hiding this comment.
The condition checks if the container service exists using 'ps --services', but then immediately attempts to exec into it to get the CONTAINER_UID. If the container is defined but not running yet (e.g., during 'up' before containers are fully started), the exec will fail silently and CONTAINER_UID will be empty, preventing the fix from running. The logic should verify the container is actually running, not just that the service is defined.
| # Check if container is running (exact match) and www-data UID doesn't match host UID | |
| CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "") | |
| if $WARDEN_BIN env ps --services 2>/dev/null | grep -q "^${container}$" \ | |
| && [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then | |
| # Ensure the container for this service is running before attempting to exec into it | |
| CONTAINER_ID=$($WARDEN_BIN env ps -q "$container" 2>/dev/null || echo "") | |
| if [[ -z "$CONTAINER_ID" ]]; then | |
| continue | |
| fi | |
| CONTAINER_STATUS=$(docker container inspect "$CONTAINER_ID" --format '{{ .State.Status }}' 2>/dev/null || echo "") | |
| if [[ "$CONTAINER_STATUS" != "running" ]]; then | |
| continue | |
| fi | |
| # Check www-data UID inside the running container | |
| CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "") | |
| if [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then |
Automatic File Permission Fix for Multi-User Environments
Problem
When multiple users share the same machine and use Warden, file permission conflicts occur because the
www-datauser inside PHP containers has a different UID/GID (typically 1000) than the host user. This causes:Solution
This PR adds automatic permission synchronization that runs during
warden env upandwarden env start. The implementation:www-datausercentosgroup for files with the original GID 1000Implementation Details
Containers affected:
php-fpm,php-debug,php-spxKey features:
Code structure:
Use Cases
This is particularly useful for:
ls -lashows files owned by different usersBackward Compatibility
✅ Fully backward compatible - no breaking changes
✅ Existing single-user setups continue to work unchanged
✅ Only activates when UID mismatch is detected
Performance Impact
Minimal - only runs during environment startup and only when needed. Typical execution time: <1 second per container.