Skip to content
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 run composer as root #5689

Merged
merged 2 commits into from
Jun 21, 2018
Merged

Don't run composer as root #5689

merged 2 commits into from
Jun 21, 2018

Conversation

tiagom62
Copy link
Contributor

@tiagom62 tiagom62 commented Jun 9, 2018

Still needs to be thoroughly tested. Please don't merge yet.

@tiagom62 tiagom62 requested a review from snipe as a code owner June 9, 2018 22:56
@snipe
Copy link
Owner

snipe commented Jun 10, 2018

Awesome, thank you! I've been wanting to handle this issue for ages and didn't get around to it. Ping me back when it's ready!

@snipe snipe changed the title Don't run composer as root WIP: Don't run composer as root Jun 10, 2018
@tiagom62 tiagom62 force-pushed the dont_run_as_root branch 4 times, most recently from 669f696 to 477c609 Compare June 10, 2018 18:53
@EarlRamirez
Copy link
Contributor

EarlRamirez commented Jun 11, 2018

@snipe do you think it will be good to add COMPOSER_PROCESS_TIMEOUT=2000 or any different number for users that have a slow internet connection?

I have the following on the customised OS
Added composer_process_timeout variable for slow internet connections
COMPOSER_PROCESS_TIMEOUT=6000 php composer.phar install --no-dev --prefer-source

@tiagom62 tiagom62 force-pushed the dont_run_as_root branch 13 times, most recently from 3953e6b to 9bda831 Compare June 16, 2018 21:30
@dmeltzer
Copy link
Contributor

As you're poking at the script, you should be able to use the Ubuntu 16.04 section for 18.04 as well, I made the edit locally and it seems to be behaving.

@tiagom62
Copy link
Contributor Author

@dmeltzer I added 18.04 support last week. PR #5687.

@tiagom62 tiagom62 force-pushed the dont_run_as_root branch 2 times, most recently from 19ac69e to b57f219 Compare June 17, 2018 16:58
@tiagom62
Copy link
Contributor Author

@snipe I think i'm done with the changes this round. Tested against the boxes in Vagrantfile.

Composer is installed globally using snipeitapp user. The app is now owned by snipeitapp user as well.

Let me know what you think and if you want any changes.

snipeit.sh Outdated
usermod -a -G "$apache_group" "$APP_USER"
}

run_as () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might rename this run_as_webuser() or similar. it's confusing when reading through the code to know that "run_as php -r blah blah blah" does not mean to run the command a user named php.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, I’ll make the change later tonight.

@tiagom62 tiagom62 changed the title WIP: Don't run composer as root Don't run composer as root Jun 18, 2018
@EarlRamirez
Copy link
Contributor

EarlRamirez commented Jun 21, 2018

@tiagom62 saw that it has not merged with the master; therefore, don't know if you still can update the script and modify line 208 SELinux label httpd_sys_script_rw_t, the label should be httpd_sys_rw_content_t at least for the storage and public directory.

@snipe
Copy link
Owner

snipe commented Jun 21, 2018

@tiagom62 I was waiting until it looked like you were done - is this ready for review/merge?

@tiagom62
Copy link
Contributor Author

@EarlRamirez Why is this change needed? That should probably be a separate PR.

@snipe Yup, it is ready for review/merge.

@EarlRamirez
Copy link
Contributor

@tiagom62 the current label is invalid; therefore, those with SELinux enable will get permission errors for the logs and when uploading any files to public/upload. I will try and do a separate pull request

@snipe snipe merged commit 3bbd49d into snipe:develop Jun 21, 2018
@tiagom62 tiagom62 deleted the dont_run_as_root branch June 21, 2018 03:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants