-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't add remote user to setfacl command, if it doesn't exist as an os user #2822
Don't add remote user to setfacl command, if it doesn't exist as an os user #2822
Conversation
Only add the the remote user to setfacl command if he exists as an os user.
run() usually returns a string, so the static analyzer throws an error, when using is_int() on its return value.
@antonmedv is there anything I need to do in order to get approval for the workflows? |
Sorry for the delay. Will check tomorrow |
The whole check is to verify dirs will be writable to remote user. Task should fail in remote user not found. |
@antonmedv Why does the remote user have to exist an an acl user? With my hoster that is not the case and this leads to the error described in #2787 |
Ah, yes. I confused it with www-data user. |
Use test() instead of run to check whether the remote user also exists as an os user.
@@ -108,8 +108,8 @@ | |||
} elseif (commandExist('setfacl')) { | |||
$setFaclUsers = "-m u:\"$httpUser\":rwX"; | |||
// Check if remote user exists, before adding it to setfacl | |||
$remoteUserId = run("id -u $remoteUser &>/dev/null 2>&1 || exit 0"); | |||
if (!empty($remoteUserId)) { | |||
$remoteUserExists = test("id -u $remoteUser &>/dev/null 2>&1 || exit 0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also delete exit 0
and use
run('...', ['no_throw' => true]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antonmedv Does that mean I shall replace test()
with run()
again, as test()
doesn't have any additional options?
If I just remove || exit 0
and use it with test()
, it keeps working. I still get true
if the user exists and false
if it doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, I see, sorry for misleading. Let me think a bit. Maybe test() should also have options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antonmedv
I am not sure if that's necessary. I tried all variants to see how it works:
If the user does exist
run("id -u $remoteUser &>/dev/null 2>&1")
: Returns empty string -> the deployment continuesrun("id -u $remoteUser &>/dev/null 2>&1", ['no_throw' => true])
: Returns empty string -> the deployment continuestest("id -u $remoteUser &>/dev/null 2>&1")
: Returns true -> the deployment continues
If the user doesn't exist
run("id -u $remoteUser &>/dev/null 2>&1")
: Throws exception -> the deployment is abortet with exit code 1.run("id -u $remoteUser &>/dev/null 2>&1", ['no_throw' => true])
: Returns empty string -> the deployment continuestest("id -u $remoteUser &>/dev/null 2>&1")
: Returns false -> the deployment continues
So i'd say test() works as expected, but I am not aware of side effects you might see.
* upstream/master: (35 commits) Fix grammar [automatic] Update docs with bin/docgen Update php.php (deployphp#2916) Update getting-started.md (deployphp#2915) Fix Magento version detection, fixes deployphp#2905 (deployphp#2906) Use -L on chgrp (deployphp#2834) Added the Doctrine mapping files validation (with --skip option) as a (possible) deploy task. (deployphp#2901) Don't add remote user to setfacl command, if it doesn't exist as an os user (deployphp#2822) Update writable.php Set version 7.0.0-master Docs style Use npm ci Small fixes Remove unused use. [automatic] Update docs with bin/docgen Refactor deploy:cleanup Command (deployphp#2788) Use StrictHostKeyChecking=accept-new by default Use StrictHostKeyChecking=accept-new by default Move git related options to update_code.php composer update ...
Only add the the remote user to setfacl command if it exists as an os user.
Based on this discussion:
#2787
Bug fix #…?
New feature?
BC breaks?
Tests added?
Docs added?