evolix/evoadmin-web
22
0
Fork 1
Code Issues 31 Pull requests 4 Releases 1 Wiki Activity

sendmail_path in LXC, and better op_del #85

Merged
whirigoyen merged 10 commits from unstable into master 2023-11-30 15:53:09 +01:00
Conversation 5 Commits 10 Files changed 2 +22 -9
whirigoyen commented 2023-11-09 17:13:37 +01:00
Owner
Copy link
  • Fix missing ITK admin link for multi PHP
  • Prevent op_del to fail and able to remove web account when part of it is already removed
  • Add sendmail_path and open_basedir in LXC PHP pool configs

Tested on test-www01

* Fix missing ITK admin link for multi PHP * Prevent op_del to fail and able to remove web account when part of it is already removed * Add sendmail_path and open_basedir in LXC PHP pool configs Tested on test-www01
🎉 1
whirigoyen added 4 commits 2023-11-09 17:13:39 +01:00
whirigoyen changed title from unstable to ITK 4 all, sendmail_path in LXC, and better op_del 2023-11-09 17:16:40 +01:00
whirigoyen added 1 commit 2023-11-09 17:21:40 +01:00
lpoujol commented 2023-11-09 18:28:22 +01:00
Owner
Copy link

Fix missing ITK admin link for multi PHP

This was intentional.

Curently, this (via web-add.sh enable-user-itk & disable-user-itk) will only toggle the AssignUserID value in the Apache vhost file of the account (adding or removing the www- prefix

While in mono-PHP setup this is enough, in multi-PHP setup it's only missleading because the user of execution of the PHP code will the be the one defined in the PHP-FPM pool config. Toggling it in that case will only create confusion.

So 2fd65724f7 (+ c385c102c5 ) should be reverted as long as web-add.sh does not do the change to FPM Pool config

> Fix missing ITK admin link for multi PHP This was intentional. Curently, this (via web-add.sh enable-user-itk & disable-user-itk) will only toggle the `AssignUserID` value in the Apache vhost file of the account (adding or removing the `www-` prefix While in mono-PHP setup this is enough, in multi-PHP setup it's only missleading because the user of execution of the PHP code will the be the one defined in the PHP-FPM pool config. Toggling it in that case will only create confusion. So 2fd65724f7f316c79b9d376af5ebcb06956963e9 (+ c385c102c5fc7de0fe0799b4744803e929ac13fe ) should be reverted as long as web-add.sh does not do the change to FPM Pool config
lpoujol commented 2023-11-09 18:30:49 +01:00
Owner
Copy link

Prevent op_del to fail and able to remove web account when part of it is already removed
Add sendmail_path and open_basedir in LXC PHP pool configs

Sounds good, looking forward to test this :)

>Prevent op_del to fail and able to remove web account when part of it is already removed > Add sendmail_path and open_basedir in LXC PHP pool configs Sounds good, looking forward to test this :)
mtrossevin requested changes 2023-11-10 10:03:31 +01:00
tpl/webadmin.tpl.php Outdated
@ -109,4 +109,2 @@
if(is_multiphp()) {
printf('<a href="/webadmin/%s/php/">PHP</a> - ', $vhost_info['owner']);
} else {
printf('<a href="/webadmin/%s/itk/">ITK</a> - ', $vhost_info['owner']);
mtrossevin commented 2023-11-10 10:00:54 +01:00
Owner
Copy link

From @lpoujol :

Fix missing ITK admin link for multi PHP

This was intentional.

Curently, this (via web-add.sh enable-user-itk & disable-user-itk) will only toggle the AssignUserID value in the Apache vhost file of the account (adding or removing the www- prefix

While in mono-PHP setup this is enough, in multi-PHP setup it's only missleading because the user of execution of the PHP code will the be the one defined in the PHP-FPM pool config. Toggling it in that case will only create confusion.

So 2fd65724f7 (+ c385c102c5 ) should be reverted as long as web-add.sh does not do the change to FPM Pool config

From @lpoujol : > > Fix missing ITK admin link for multi PHP > > This was intentional. > > Curently, this (via web-add.sh enable-user-itk & disable-user-itk) will only toggle the `AssignUserID` value in the Apache vhost file of the account (adding or removing the `www-` prefix > > While in mono-PHP setup this is enough, in multi-PHP setup it's only missleading because the user of execution of the PHP code will the be the one defined in the PHP-FPM pool config. Toggling it in that case will only create confusion. > > So 2fd65724f7f316c79b9d376af5ebcb06956963e9 (+ c385c102c5fc7de0fe0799b4744803e929ac13fe ) should be reverted as long as web-add.sh does not do the change to FPM Pool config
mtrossevin marked this conversation as resolved
mtrossevin requested changes 2023-11-10 10:19:16 +01:00
mtrossevin left a comment
Owner
Copy link

While I do agree with @lpoujol that it would be better not to fail if the web-account was already (partially) removed, the implementation doesn't seems to be correct.

While I do agree with @lpoujol that it would be better not to fail if the web-account was already (partially) removed, the implementation doesn't seems to be correct.
scripts/web-add.sh Outdated
@ -807,3 +810,3 @@
if [ "$WEB_SERVER" == "apache" ]; then
if id www-"$login" &> /dev/null; then
userdel -f www-"$login"
userdel -f www-"$login" || true
mtrossevin commented 2023-11-10 10:10:56 +01:00
Owner
Copy link

Considering the if id www-"$login" means that www-"$login" do exist for the system root, failure on this userdel seems to be a real problem (it is already bypassed if the user doesn't exists).

Considering the `if id www-"$login"` means that `www-"$login"` do exist for the system root, failure on this userdel seems to be a real problem (it is already bypassed if the user doesn't exists).
mtrossevin marked this conversation as resolved
scripts/web-add.sh Outdated
@ -812,3 +815,3 @@
for php_version in "${PHP_VERSIONS[@]}"; do
if lxc-attach -n php"${php_version}" -- id www-"$login" &> /dev/null; then
lxc-attach -n php"${php_version}" -- userdel -f www-"$login"
lxc-attach -n php"${php_version}" -- userdel -f www-"$login" || true
mtrossevin commented 2023-11-10 10:14:57 +01:00
Owner
Copy link

Wouldn't it be better to check for the user existence in the container instead of silencing all errors ?

I would do something like this :

lxc-attach -n php"${php_version}" -- sh -c "getent passwd 'www-$login' && userdel -f 'www-$login'"

or

lxc-attach -n php"${php_version}" -- getent passwd www-"$login" && lxc-attach -n php"${php_version}" -- userdel -f www-"$login"
Wouldn't it be better to check for the user existence in the container instead of silencing all errors ? I would do something like this : ```sh lxc-attach -n php"${php_version}" -- sh -c "getent passwd 'www-$login' && userdel -f 'www-$login'" ``` or ```sh lxc-attach -n php"${php_version}" -- getent passwd www-"$login" && lxc-attach -n php"${php_version}" -- userdel -f www-"$login" ```
mtrossevin marked this conversation as resolved
scripts/web-add.sh Outdated
@ -815,2 +817,3 @@
lxc-attach -n php"${php_version}" -- userdel -f www-"$login" || true
fi
lxc-attach -n php"${php_version}" -- userdel -f "$login"
lxc-attach -n php"${php_version}" -- userdel -f "$login" || true
mtrossevin commented 2023-11-10 10:16:13 +01:00
Owner
Copy link

Wouldn't it be better to check for the user existence in the container instead of silencing all errors ?

I would do something like this :

lxc-attach -n php"${php_version}" -- sh -c "getent passwd '$login' && userdel -f '$login'"

or

lxc-attach -n php"${php_version}" -- getent passwd "$login" && lxc-attach -n php"${php_version}" -- userdel -f "$login"
Wouldn't it be better to check for the user existence in the container instead of silencing all errors ? I would do something like this : ```sh lxc-attach -n php"${php_version}" -- sh -c "getent passwd '$login' && userdel -f '$login'" ``` or ```sh lxc-attach -n php"${php_version}" -- getent passwd "$login" && lxc-attach -n php"${php_version}" -- userdel -f "$login" ```
mtrossevin marked this conversation as resolved
scripts/web-add.sh Outdated
@ -818,3 +821,3 @@
fi
userdel -f "$login"
userdel -f "$login" || true
mtrossevin commented 2023-11-10 10:17:19 +01:00
Owner
Copy link

Wouldn't it be better to check for the user existence in the container instead of silencing all errors ?

I would do something like this :

getent passwd "$login" && userdel -f "$login"

(or use an if block)

Wouldn't it be better to check for the user existence in the container instead of silencing all errors ? I would do something like this : ```sh getent passwd "$login" && userdel -f "$login" ``` (or use an if block)
mtrossevin marked this conversation as resolved
mtrossevin approved these changes 2023-11-10 10:24:09 +01:00
mtrossevin left a comment
Owner
Copy link

On sendmail_path and open_basedir, it looks good.

On `sendmail_path` and `open_basedir`, it looks good.
CHANGELOG.md Outdated
@ -17,2 +21,4 @@
### Fixed
* Fix sendmail_path hostname (missing domain / FQDN)
mtrossevin commented 2023-11-10 10:23:13 +01:00
Owner
Copy link

Since sendmail_path wasn't defined before this PR, should we really have this in the changelog ?

Since `sendmail_path` wasn't defined before this PR, should we really have this in the changelog ?
mtrossevin commented 2023-11-10 15:25:49 +01:00
Owner
Copy link

@mtrossevin : For sendmail_path, it is already defined in the vhost template for php mod, but was missing for FPM in the containers.

Perhaps, but the change seems to only affect what was added in this PR. At least according to the diffs.

> @mtrossevin : For `sendmail_path`, it is already defined in the vhost template for php mod, but was missing for FPM in the containers. Perhaps, but the change seems to only affect what was added in this PR. At least according to the diffs.
whirigoyen commented 2023-11-13 14:29:34 +01:00
Author
Owner
Copy link

Your right, it was a commit that fixed my previous commit, but this should'nt be in the changlog.

Your right, it was a commit that fixed my previous commit, but this should'nt be in the changlog.
mtrossevin marked this conversation as resolved
scripts/web-add.sh
@ -457,2 +458,4 @@
php_admin_value[error_log] = /home/${in_login}/log/php.log
php_admin_value[sendmail_path] = "/usr/sbin/sendmail -t -i -f www-${in_login}@${HOST}"
php_admin_value[open_basedir] = "/usr/share/php:/home/${in_login}:/tmp"
mtrossevin commented 2023-11-10 10:22:07 +01:00
Owner
Copy link

Seems good.

Seems good.
mtrossevin marked this conversation as resolved
mtrossevin requested changes 2023-11-10 10:25:06 +01:00
mtrossevin left a comment
Owner
Copy link

Oops, should have just left a comment, see above

Oops, should have just left a comment, see above
whirigoyen commented 2023-11-10 10:46:53 +01:00
Author
Owner
Copy link

@lpoujol @mtrossevin : Thanks for your remarks.

  • I'll implement ITK for multiPHP in another PR (shouldn't be too complicated, with sed in the containers). I revert the present change.

  • @mtrossevin : For getent passwd <LOGIN>, I'd rather use getent passwd <LOGIN> > /dev/null, don't you think ?

  • @mtrossevin : For sendmail_path, it is already defined in the vhost template for php mod, but was missing for FPM in the containers.

@lpoujol @mtrossevin : Thanks for your remarks. * I'll implement ITK for multiPHP in another PR (shouldn't be too complicated, with sed in the containers). I revert the present change. * @mtrossevin : For `getent passwd <LOGIN>`, I'd rather use `getent passwd <LOGIN> > /dev/null`, don't you think ? * @mtrossevin : For `sendmail_path`, it is already defined in the vhost template for php mod, but was missing for FPM in the containers.
whirigoyen added 2 commits 2023-11-10 10:54:45 +01:00
whirigoyen added 1 commit 2023-11-10 11:12:00 +01:00
whirigoyen commented 2023-11-10 11:12:15 +01:00
Author
Owner
Copy link

Done !
(and account removal tested on test-www01)

Done ! (and account removal tested on test-www01)
mtrossevin approved these changes 2023-11-10 12:02:02 +01:00
mtrossevin left a comment
Owner
Copy link

All good for me.

All good for me.
whirigoyen added 1 commit 2023-11-13 14:30:04 +01:00
whirigoyen commented 2023-11-13 14:30:40 +01:00
Author
Owner
Copy link

@mtrossevin ^

@mtrossevin ^
mtrossevin approved these changes 2023-11-13 15:05:16 +01:00
mtrossevin left a comment
Owner
Copy link

lgtm

lgtm
whirigoyen requested review from mtrossevin 2023-11-13 15:08:12 +01:00
mtrossevin approved these changes 2023-11-13 15:25:33 +01:00
mtrossevin changed title from ITK 4 all, sendmail_path in LXC, and better op_del to sendmail_path in LXC, and better op_del 2023-11-13 16:09:19 +01:00
mtrossevin added 1 commit 2023-11-16 11:04:33 +01:00
whirigoyen merged commit 93c9e450ff into master 2023-11-30 15:53:09 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
mtrossevin
Labels
Clear labels   
Bug

Fixing or reporting bugs

  
Doc

Things that needs to be documented (use and/or dev)

  
Feature

Adding new features

  
Forge

Issues imported from https://forge.evolix.org/projects/evoadmin-web/issues

Archived

  
Mode
Cluster

  
Mode
MultiPHP

  
Script

Code that can be run both on the command line or by evoadmin

  
Server

Server configuration

  
Web

Code that is run on or by the http server.

  
wontfix
No labels
Bug
Doc
Feature
Forge
Mode
Cluster
Mode
MultiPHP
Script
Server
Web
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: evolix/evoadmin-web#85
No description provided.
Delete branch "unstable"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?