Do not perform chown and chmod on /projects unless a user is in sudoers #6416
Conversation
ghost
requested a review
from 
There was a problem hiding this comment.
LGTM and can be merged as it is for me because it has the merit to address some annoying logs that we have since a long time. But clearly this code could be improved: if []; then : else ...chmod and chown... works but is not beautiful.
I would rather:
if is_current_user_sudoer || is_current_user_root; then
... chmod and chown...
fi
| ${SUDO} sh -c "chown -R $(id -u -n) /projects" | ||
| ${SUDO} chmod 755 /projects | ||
| if [ "${CHANGE_PROJECTS_PERMISSIONS}" = false ]; then | ||
| : |
There was a problem hiding this comment.
it's looking strange to have a no-op operation in the if block and the code in the else block
shouldn't it better to change the condition so the logic is in the "if" block ?
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3791/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3800/ |
|
@eivantsov yes but when I read the previous command line it's looking strange as well at first it let me think it will try to use SUDO as well (but the variable is unset) about your PR, does it work if we only check if SUDO variable is set to something nonnull/not empty ? |
|
I think it will work - it's just a different way to check it. And yes, moving mkdir to if clause makes sense. |
| @@ -18,7 +18,11 @@ is_current_user_sudoer() { | |||
| } | |||
|
|
|||
| set_sudo_command() { | |||
| if is_current_user_sudoer && ! is_current_user_root; then SUDO="sudo -E"; else unset SUDO; fi | |||
| if is_current_user_sudoer; then | |||
There was a problem hiding this comment.
Changing this is an error. Function is_current_user_sudoer returns true if the user is root. As a consequence SUDO is set to sudo -E" when the script is run by root user. The correct behavior should be to unset SUDO if user is root.
By the way the goal of your PR is to avoid executing chown and chmod if the user is arbitrary. For this you don't need to change this function (set_sudo_command).
| @@ -55,8 +59,10 @@ MACHINE_TYPE=$(uname -m) | |||
|
|
|||
| mkdir -p ${CHE_DIR} | |||
| ${SUDO} mkdir -p /projects | |||
| ${SUDO} sh -c "chown -R $(id -u -n) /projects" | |||
| ${SUDO} chmod 755 /projects | |||
| if [ "${SUDO}" = "sudo -E" ]; then | |||
There was a problem hiding this comment.
Here you need to check if the user has sudoer privileges. Why don't you just use if is_current_user_sudoer; then. That would be more easy to read.
| ${SUDO} chmod 755 /projects | ||
|
|
||
| if is_current_user_sudoer; then | ||
| ${SUDO} mkdir -p /projects |
There was a problem hiding this comment.
is it ok if /projects is only created for sudoers ?
There was a problem hiding this comment.
I think by the time this script runs /projects is already there since wither Docker or OpenShift creates it when the container starts/is created.
There was a problem hiding this comment.
@benoitf in any case if the user is not a sudoer he cannot create the folder /projects/.
There was a problem hiding this comment.
@l0rd user can be root user
| ${SUDO} mkdir -p /projects | ||
| ${SUDO} sh -c "chown -R $(id -u -n) /projects" | ||
| ${SUDO} chmod 755 /projects | ||
| fi |
There was a problem hiding this comment.
in che-server image we updated only directory permissions
would it work as well if we only change permissions of folder but not the owner ?
(it can make the operation faster)
There was a problem hiding this comment.
There was a problem hiding this comment.
I was thinking replacing 2 instructions chown + chmod with the only find + exec
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3809/ |
|
Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/3811/ |
What does this PR do?
Uses and extends an existing function to avoid failed chown and chmod (when a user is not in sudoers)
What issues does this PR fix or reference?
redhat-developer/rh-che#298