fix: changing owner while creating container for download support (Se…

…leniumHQ#2064)

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

.github/workflows/helm-chart-test.yml

@@ -65,12 +65,12 @@ jobs:
         if: always()
         uses: actions/upload-artifact@v4
         with:
-          name: ${{ env.CHART_FILE_NAME }}
+          name: ${{ matrix.test-strategy }}_${{ env.CHART_FILE_NAME }}
           path: ${{ env.CHART_PACKAGE_PATH }}
       - name: Upload Helm chart template rendered
         if: always()
         uses: actions/upload-artifact@v4
         with:
-          name: chart_template_rendered.yaml
+          name: ${{ matrix.test-strategy }}_chart_template_rendered.yaml
           path: ./tests/tests/output_deployment.yaml
           if-no-files-found: ignore

Base/Dockerfile

@@ -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
@@ -145,6 +144,7 @@ RUN chmod g=u /etc/passwd
 # Run the following commands as non-privileged user
 #===================================================
 USER ${SEL_UID}:${SEL_GID}
+VOLUME ${SE_DOWNLOAD_DIR}
 
 # Boolean value, maps "--bind-host"
 ENV SE_BIND_HOST false

Base/entry_point.sh

@@ -5,44 +5,30 @@ _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
+    fix-permissions "${SE_DOWNLOAD_DIR}"
+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 fix-permissions "${SE_DOWNLOAD_DIR}"
+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)
+    if ! whoami &> /dev/null; then
+        _log "There is no entry in /etc/passwd for our UID=$(id -u). Attempting to fix..."
+        if [ -w /etc/passwd ]; then
+            _log "Renaming user to ${USER_NAME:-default} ($(id -u):$(id -g)"
+            # 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):${USER_NAME:-default} user:${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
+    fix-permissions "${SE_DOWNLOAD_DIR}"
 fi
 
 /usr/bin/supervisord --configuration /etc/supervisord.conf &

Base/fix-permissions

@@ -1,23 +1,46 @@
 #!/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 [[ "${SE_DOWNLOAD_DIR}" != "${HOME}/Downloads" ]] && [[ -d "${SE_DOWNLOAD_DIR}" ]]; then
+    mkdir -p "${SE_DOWNLOAD_DIR}"
+fi
+
+if [ "$(id -u)" == 0 ]; then
+    # For root user to change ownership
+    for d in "$@"; do
+        find "${d}" \
+            ! \( \
+                -group "${SEL_GID}" \
+                -a -perm -g+rwX \
+            \) \
+            -exec chown -R "${SEL_UID}:${SEL_GID}" -- {} \+ \
+            -exec chgrp -R "${SEL_GID}" -- {} \+ \
+            -exec chmod -R g+rwX -- {} \+
+        find "${d}" \
+            \( \
+                -type d \
+                -a ! -perm -775 \
+            \) \
+            -exec chmod -R 775 -- {} \+
+        # Relaxing permissions for OpenShift and other non-sudo environments
+        chmod -R u+x "${d}"
+        chgrp -R 0 "${d}"
+        chmod -R g=u "${d}"
+    done
+
+    # Only give write access for app data in case running of read-only filesystem
+    dirs=(
+        "${HOME}"
+        "${SEL_DIR}"
+        "${SE_DOWNLOAD_DIR}"
+        "/var/run/supervisor"
+        "/var/log/supervisor"
+        "/etc/passwd"
+        "/tmp/.X11-unix"
+    )
+    for d in "${dirs[@]}"; do
+        if [[ -d $d ]]; then
+            chmod 777 -R "${d}"
+        fi
+    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,46 +1302,72 @@ 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=/tmp/downloads \
-  -v /home/ubuntu/files:/tmp/downloads \
+  -e SE_DOWNLOAD_DIR=/path/to/your/downloads \
+  -v /home/ubuntu/files:/path/to/your/downloads \
   selenium/standalone-chrome:4.16.1-20231212
 ```
 
-### Change ownership of the volume mount
+**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.
 
-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
+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" \
-  -v /home/ubuntu/my-certs:/etc/certs \
-  -e CHOWN_EXTRA=/etc/certs \
+  --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
 ```
 
-If you want a  new volume mount directory to be created and set ownership, you can set the `MKDIR_EXTRA` and `MKDIR_EXTRA_OPTS` (default is set `-p` - create a directory hierarchy) environment variables.
-
+Then stop it, rerun with switching to non-root user:
 ```bash
 docker run -d -p 4444:4444 --shm-size="2g" \
-  -v /home/ubuntu/my-nssdb:/home/seluser/.pki/nssdb \
-  -e MKDIR_EXTRA=/home/seluser/.pki/nssdb \
+  --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
 ```
+Summarize the supported use case for changing ownership when starting container:
 
-Both `CHOWN_EXTRA` and `MKDIR_EXTRA` can be set to multiple directories by separating them with a `space` or `comma`. For example: `CHOWN_EXTRA=<some-dir>,<some-other-dir>`
+| User (uid)           | Mount `SE_DOWNLOAD_DIR` in container | Auto changing |
+|----------------------|--------------------------------------|---------------|
+| seluser (uid `1200`) | default `/home/seluser/Downloads`    | Yes           |
+| seluser (uid `1200`) | any `/path/to/downloads`             | Yes           |
+| any (uid != `1200`)  | default `/home/seluser/Downloads`    | Yes           |
+| any (uid != `1200`)  | any `/path/to/downloads`             | No            |
 

tests/test.py

@@ -174,7 +174,7 @@ def standalone_browser_container_matches(container):
 
     use_random_user_id = USE_RANDOM_USER_ID == 'true'
     run_in_docker_compose = RUN_IN_DOCKER_COMPOSE == 'true'
-    random_user_id = random.randint(2000, 65000)
+    random_user_id = "%s:%s" % (random.randint(2000, 65000), random.randint(2001, 65001))
 
     if use_random_user_id:
         logger.info("Running tests with a random user ID -> %s" % random_user_id)