fix: changing owner while creating container for download support

Signed-off-by: Viet Nguyen Duc <[email protected]>

Base/Dockerfile

@@ -104,7 +104,7 @@ RUN mkdir -p ${HOME}/.mozilla ${HOME}/.cache \
 #======================================
 # Add Grid check script
 #======================================
-COPY --chown="${SEL_UID}:${SEL_GID}" check-grid.sh entry_point.sh /opt/bin/
+COPY --chown="${SEL_UID}:${SEL_GID}" check-grid.sh entry_point.sh chown-extra.sh /opt/bin/
 
 #======================================
 # Add Supervisor configuration file
@@ -133,8 +133,7 @@ RUN /tmp/cs fetch --classpath --cache ${EXTERNAL_JARS} \
 RUN rm -fr /root/.cache/*
 
 # Change ownership of directories
-RUN chown -R "${SEL_USER}:${SEL_GID}" ${HOME} ${SEL_DIR} ${SEL_DIR}/assets ${EXTERNAL_JARS} ${SE_DOWNLOAD_DIR} /var/run/supervisor /var/log/supervisor \
-    && fix-permissions ${HOME} ${SEL_DIR} ${SEL_DIR}/assets ${EXTERNAL_JARS} ${SE_DOWNLOAD_DIR} /var/run/supervisor /var/log/supervisor
+RUN fix-permissions ${HOME} ${SEL_DIR} ${SEL_DIR}/assets ${EXTERNAL_JARS} ${SE_DOWNLOAD_DIR} /var/run/supervisor /var/log/supervisor
 
 #==========
 # Relaxing permissions for OpenShift and other non-sudo environments

Base/chown-extra.sh

@@ -0,0 +1,40 @@
+#!/bin/bash
+set -e
+
+_log () {
+    if [[ "$*" == "ERROR:"* ]] || [[ "$*" == "WARNING:"* ]] || [[ "${CONTAINER_LOGS_QUIET}" == "" ]]; then
+        echo "$@"
+    fi
+}
+
+if [ "${SE_DOWNLOAD_DIR}" != "${HOME}/Downloads" ]; then
+    MKDIR_EXTRA=${SE_DOWNLOAD_DIR}","${MKDIR_EXTRA}
+fi
+
+CHOWN_EXTRA=${MKDIR_EXTRA}","${CHOWN_EXTRA}
+
+if [ -n "${MKDIR_EXTRA}" ]; then
+    for extra_dir in $(echo "${MKDIR_EXTRA}" | tr ',' ' '); do
+        _log "Creating directory ${extra_dir} ${MKDIR_EXTRA_OPTS:+(mkdir options: ${MKDIR_EXTRA_OPTS})}"
+        # shellcheck disable=SC2086
+        mkdir ${MKDIR_EXTRA_OPTS:-"-p"} "${extra_dir}"
+    done
+fi
+
+if [ -n "${CHOWN_EXTRA}" ]; then
+    for extra_dir in $(echo "${CHOWN_EXTRA}" | tr ',' ' '); do
+        _log "Changing ${extra_dir} ownership. Ensure ${extra_dir} is owned by ${SEL_USER} ${CHOWN_EXTRA_OPTS:+(chown options: ${CHOWN_EXTRA_OPTS})}"
+        fix-permissions "${extra_dir}"
+    done
+fi
+
+# Raise warning if the user isn't able to write files to extra dirs
+if [ -n "${CHOWN_EXTRA}" ]; then
+    for extra_dir in $(echo "${CHOWN_EXTRA}" | tr ',' ' '); do
+        if [[ ! -w ${extra_dir} ]]; then
+            _log "WARNING: no write access to dir ${extra_dir}. Please correct the permissions manually (or run with --user root) once then start again with non-root user."
+        else
+            _log "Verified dir ${extra_dir} has write access."
+        fi
+    done
+fi

Base/entry_point.sh

@@ -5,46 +5,33 @@ _log () {
     fi
 }
 
-#==============================================
-# OpenShift or non-sudo environments support
-# https://docs.openshift.com/container-platform/3.11/creating_images/guidelines.html#openshift-specific-guidelines
-#==============================================
-
-if ! whoami &> /dev/null; then
-  if [ -w /etc/passwd ]; then
-    echo "${USER_NAME:-${SEL_USER}}:x:$(id -u):0:${USER_NAME:-${SEL_USER}} user:${HOME}:${SE_DOWNLOAD_DIR}:/var:/opt:/sbin/nologin" >> /etc/passwd
-  fi
-fi
-
-MKDIR_EXTRA=${SE_DOWNLOAD_DIR}","${MKDIR_EXTRA}
-CHOWN_EXTRA=${MKDIR_EXTRA}","${CHOWN_EXTRA}
-
-if [ -n "${MKDIR_EXTRA}" ]; then
-    for extra_dir in $(echo "${MKDIR_EXTRA}" | tr ',' ' '); do
-        _log "Creating directory ${extra_dir} ${MKDIR_EXTRA_OPTS:+(mkdir options: ${MKDIR_EXTRA_OPTS})}"
-        # shellcheck disable=SC2086
-        sudo mkdir ${MKDIR_EXTRA_OPTS:-"-p"} "${extra_dir}"
-    done
-fi
-
-if [ -n "${CHOWN_EXTRA}" ]; then
-    for extra_dir in $(echo "${CHOWN_EXTRA}" | tr ',' ' '); do
-        _log "Changing ${extra_dir} ownership. ${extra_dir} is owned by ${SEL_USER} ${CHOWN_EXTRA_OPTS:+(chown options: ${CHOWN_EXTRA_OPTS})}"
-        # shellcheck disable=SC2086
-        sudo chown ${CHOWN_EXTRA_OPTS:-"-R"} "${SEL_UID}:${SEL_GID}" "${extra_dir}"
-        sudo -E fix-permissions "${extra_dir}"
-    done
-fi
-
-# Raise error if the user isn't able to write files to download dir
-if [ -n "${CHOWN_EXTRA}" ]; then
-    for extra_dir in $(echo "${CHOWN_EXTRA}" | tr ',' ' '); do
-        if [[ ! -w ${extra_dir} ]]; then
-            _log "ERROR: no write access to download dir ${SE_DOWNLOAD_DIR}. Please correct the permissions and restart."
+# If the container started as the root user
+if [ "$(id -u)" == 0 ]; then
+    bash /opt/bin/chown-extra.sh
+elif [ "$(id -u)" == "$(id -u ${SEL_USER})" ] && [ "$(id -g)" == "$(id -g ${SEL_USER})" ]; then
+    # Trust SEL_USER is the desired non-root user to execute with sudo
+    sudo -E bash /opt/bin/chown-extra.sh
+else
+    # For non-root user to change ownership
+    # Relaxing permissions for OpenShift and other non-sudo environments
+    # (https://docs.openshift.com/container-platform/latest/openshift_images/create-images.html#use-uid_create-images)
+    _log "There is no entry in /etc/passwd for our UID=$(id -u). Attempting to fix..."
+    if ! whoami &> /dev/null; then
+        if [ -w /etc/passwd ]; then
+            _log "Renaming old ${SEL_USER} user to ${USER_NAME:-default} ($(id -u ${SEL_USER}):$(id -g ${SEL_USER}))"
+            # We cannot use "sed --in-place" since sed tries to create a temp file in
+            # /etc/ and we may not have write access. Apply sed on our own temp file:
+            sed --expression="s/^${SEL_USER}:/${USER_NAME:-default}:/" /etc/passwd > /tmp/passwd
+            echo "${USER_NAME:-default}:x:$(id -u):$(id -g):,,,:${HOME}:/bin/bash" >> /tmp/passwd
+            cat /tmp/passwd > /etc/passwd
+            rm /tmp/passwd
+            _log "Added new ${USER_NAME:-default} user ($(id -u):$(id -g)). Fixed UID!"
         fi
-    done
+    fi
+    bash /opt/bin/chown-extra.sh
 fi
 
+
 /usr/bin/supervisord --configuration /etc/supervisord.conf &
 
 SUPERVISOR_PID=$!

Base/fix-permissions

@@ -1,23 +1,27 @@
 #!/bin/bash
 set -e
-# Run this with USER root only
-for d in "$@"; do
-    find "${d}" \
-        ! \( \
-            -group "${SEL_GID}" \
-            -a -perm -g+rwX \
-        \) \
-        -exec chgrp -R "${SEL_GID}" -- {} \+ \
-        -exec chmod -R g+rwX -- {} \+
-    # setuid, setgid *on directories only*
-    find "${d}" \
-        \( \
-            -type d \
-            -a ! -perm -6000 \
-        \) \
-        -exec chmod -R +6000 -- {} \+
-    # Relaxing permissions for OpenShift and other non-sudo environments
-    chmod -R u+x "${d}"
-    chgrp -R 0 "${d}"
-    chmod -R g=u "${d}"
-done
+
+if [ "$(id -u)" == 0 ]; then
+    # For root user to change ownership
+    for d in "$@"; do
+        # Relaxing permissions for OpenShift and other non-sudo environments
+        chmod -R u+x "${d}"
+        chgrp -R ${CHGRP_EXTRA:-0} "${d}"
+        chmod -R g=u "${d}"
+        find "${d}" \
+            ! \( \
+                -group "${SEL_GID}" \
+                -a -perm -g+rwX \
+            \) \
+            -exec chown -R "${SEL_USER}:${SEL_GID}" -- {} \+ \
+            -exec chgrp -R "${SEL_GID}" -- {} \+ \
+            -exec chmod -R g+rwX -- {} \+
+        # setuid, setgid *on directories only*
+        find "${d}" \
+            \( \
+                -type d \
+                -a ! -perm -6000 \
+            \) \
+            -exec chmod -R +6000 -- {} \+
+    done
+fi

NodeChrome/Dockerfile

@@ -50,7 +50,6 @@ RUN if [ ! -z "$CHROME_DRIVER_VERSION" ]; \
   && unzip /tmp/chromedriver_linux64.zip -d /opt/selenium \
   && rm /tmp/chromedriver_linux64.zip \
   && mv /opt/selenium/chromedriver-linux64/chromedriver /opt/selenium/chromedriver-$CHROME_DRIVER_VERSION \
-  && fix-permissions /opt/selenium/chromedriver-$CHROME_DRIVER_VERSION \
   && ln -fs /opt/selenium/chromedriver-$CHROME_DRIVER_VERSION /usr/bin/chromedriver
 
 USER ${SEL_UID}

NodeEdge/Dockerfile

@@ -43,7 +43,6 @@ RUN if [ -z "$EDGE_DRIVER_VERSION" ]; \
   && unzip /tmp/msedgedriver_linux64.zip -d /opt/selenium \
   && rm /tmp/msedgedriver_linux64.zip \
   && mv /opt/selenium/msedgedriver /opt/selenium/msedgedriver-$EDGE_DRIVER_VERSION \
-  && fix-permissions /opt/selenium/msedgedriver-$EDGE_DRIVER_VERSION \
   && ln -fs /opt/selenium/msedgedriver-$EDGE_DRIVER_VERSION /usr/bin/msedgedriver
 
 USER ${SEL_UID}

README.md

@@ -1302,27 +1302,82 @@ that directory because it is running under the user
 `seluser`. This happens because that is how Docker mounts
 volumes in Linux, more details in this [issue](https://github.com/moby/moby/issues/2259).
 
-There was a fix in this feature [#1947](https://github.com/SeleniumHQ/docker-selenium/issues/1947)
-that changed ownership when staring the container.
+A workaround (to be done manually) for this is to create a directory on the
+host and change its permissions **before mounting the volume**. 
+Depending on your user permissions, you might need to use 
+`sudo` for some of these commands:
 
-You are able to configure browser with another download directory and mount the host with it in container by overriding `SE_DOWNLOAD_DIR`.
+```bash
+mkdir /home/ubuntu/files
+chown 1200:1201 /home/ubuntu/files
+```
+
+After doing this, you should be able to download files
+to the mounted directory.
 
+---
+Another introduced feature [#1947](https://github.com/SeleniumHQ/docker-selenium/issues/1947)
+that take action to change ownership when staring the container.
+
+You are able to configure another default browser download directory and mount the host with it in container by overriding `SE_DOWNLOAD_DIR`.
+
+For example, in test you might be scripting something
 ```groovy
 ChromeOptions options = new ChromeOptions();
 HashMap<String, Object> chromePrefs = new HashMap<String, Object>();
-chromePrefs.put("download.default_directory", "/tmp/downloads");
+chromePrefs.put("download.default_directory", "/path/to/your/downloads");
 options.setExperimentalOption("prefs", chromePrefs);
 options.add_argument('disable-features=DownloadBubble,DownloadBubbleV2')
 WebDriver driver = new ChromeDriver(options);
 ```
 
+When running the container, you set the `SE_DOWNLOAD_DIR` and mount the host with that directory in container.
+```bash
+docker run -d -p 4444:4444 --shm-size="2g" \
+  -e SE_DOWNLOAD_DIR=/path/to/your/downloads \
+  -v /home/ubuntu/files:/path/to/your/downloads \
+  selenium/standalone-chrome:4.16.1-20231212
+```
+
+**Note:** The changing ownership when starting container is not supported well when both overriding `SE_DOWNLOAD_DIR` and running non-root (e.g Podman) or specifying user ids different from `1200` (OpenShift arbitrary user ids).
+
+In this case, you can use above workaround to create and set permissions for the directory on the host before mounting the volume.
+
+You also can run the container with `--user root` once to initialize and change ownership of the directory on the host, then run the container with non-root user again.
+
+For example, the first run with root user:
+```bash
+docker run -d -p 4444:4444 --shm-size="2g" \
+  --user root \
+  -e SE_DOWNLOAD_DIR=/path/to/your/downloads \
+  -v /home/ubuntu/files:/path/to/your/downloads \
+  selenium/standalone-chrome:4.16.1-20231212
+```
+
+Via container logs, you are able to see something like
+```bash
+Creating directory /path/to/your/downloads
+Changing /path/to/your/downloads ownership. Ensure /path/to/your/downloads is owned by seluser
+Verified dir /path/to/your/downloads has write access.
+```
+
+Then stop it, rerun with switching to non-root user:
 ```bash
 docker run -d -p 4444:4444 --shm-size="2g" \
-  -e SE_DOWNLOAD_DIR=/tmp/downloads \
-  -v /home/ubuntu/files:/tmp/downloads \
+  --user 4496 \
+  -e SE_DOWNLOAD_DIR=/path/to/your/downloads \
+  -v /home/ubuntu/files:/path/to/your/downloads \
   selenium/standalone-chrome:4.16.1-20231212
 ```
 
+Via container logs, you are able to see
+```bash
+Added new default user (4496:0). Fixed UID!
+Creating directory /path/to/your/downloads
+Changing /path/to/your/downloads ownership. Ensure /path/to/your/downloads is owned by seluser
+Verified dir /path/to/your/downloads has write access.
+```
+
 ### Change ownership of the volume mount
 
 If you are using Linux and you need to change the ownership of the volume mount, you can set the `CHOWN_EXTRA` and `CHOWN_EXTRA_OPTS` (default is set `-R` - change recursively) environment variables