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

[enh] Control the login shell when creating users in ynh_system_user_create #455

Open
wants to merge 2 commits into
base: stretch-unstable
from

Conversation

Projects
None yet
3 participants
@JimboJoe
Copy link
Member

JimboJoe commented Apr 29, 2018

The problem

Some packages need to create system users with a valid login shell, but the user creation helper only allows nologin shell.

Solution

Allow this helper to create users using the system default login shell.

PR Status

Finished and tested.

How to test

You can possibly test the Discourse package.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@@ -52,7 +53,12 @@ ynh_system_user_create () {
else
local user_home_dir="--no-create-home"
fi
sudo useradd $user_home_dir --system --user-group $1 --shell /usr/sbin/nologin || ynh_die "Unable to create $1 system account"
if [ $# -ge 3 ]; then # If we want a shell for the user

This comment has been minimized.

@alexAubin

alexAubin Apr 29, 2018

Member

Hm I don't know what's the best design for this 😅 That's a small thing but here you could run ynh_system_user_create foo /bin/false and it would actually use the default login shell because the third argument is taken as a boolean argument. Though it's difficult to design an elegant solution ... eitheir explicitly gives the shell to use (e.g. /bin/bash), or use literal values like True / False or DEFAULT_SHELL / NO_LOGIN ?

Or we can keep it like this, but at least an example of usage would be good to clarify this 😛

(Otherwise that looks good to me though not tested)

This comment has been minimized.

@JimboJoe

JimboJoe Apr 29, 2018

Member

...that's why I was seeking your opinion, because I couldn't find out something completely satisfying myself 😅
Providing the shell as parameter seems overkill and prevents from using the default shell (unless you allow using the default parameter).
Adding an example is definitely a good start!

This comment has been minimized.

@maniackcrudelis

maniackcrudelis May 1, 2018

Contributor

Hum, and here using a specific shell means also you have to specify a home directory.
Sometimes, I'm asking myself if we shouldn't switch from positional arguments to getopts arguments for the helpers.

This comment has been minimized.

@JimboJoe

JimboJoe May 4, 2018

Member

Provided we keep simple enough helpers (which is a good practice!), I recommend to keep positional arguments.

This comment has been minimized.

@alexAubin

alexAubin May 10, 2018

Member

Could we maybe use the same trick as Josue in https://github.com/YunoHost/yunohost/pull/460/files ?

i.e.

VAR=true
if $VAR
then
   ...
fi

That's not perfect, but at least we can say use_shell should be either 'true' or 'false, and at least we don't end up with stuff like ynh_system_user_create foo /home/foo 0 which would trigger the usage of the default shell :P

This comment has been minimized.

@maniackcrudelis

maniackcrudelis May 10, 2018

Contributor

If you let me a little while (I'm quite long because I'm currently on a big contract), I'm sure that I could work on an internal helper that manage getopts arguments to give an easy way to integrate that kind of arguments in our helpers, without the complication of having to reimplemente getopts each time.
Because if I agree that getopts is hard to implemente, using positional parameters is becoming more and more difficult with the options added by the time.
Using getopts would solve that problem definitively

This comment has been minimized.

@maniackcrudelis

maniackcrudelis May 13, 2018

Contributor

I found a few hours today, I made a helper for using getopts easily.
YunoHost-Apps/Experimental_helpers#21

@JimboJoe, what do you think about this helper ? Don't you think that using this helper would permit to use "getopts" arguments without the complexity of getopts

This comment has been minimized.

@JimboJoe

JimboJoe May 20, 2018

Member

In the case of this helper, what would you propose as parameters definition?
Moreover, any idea as how to manage helper API compatibility with existing apps...?

This comment has been minimized.

@maniackcrudelis

maniackcrudelis May 27, 2018

Contributor

Anyway, as the helper should currently be tested before be merged in the core. This PR shouldn't wait for it.
So let's use positional arguments here, and we will make a new PR when the new helper getopts will be ready.

@alexAubin alexAubin changed the title Update ynh_system_user_create helper to allow creating users with login shell [enh] Control the login shell when creating users in ynh_system_user_create May 10, 2018

@alexAubin alexAubin added this to the 3.x milestone Jun 13, 2018

@alexAubin alexAubin changed the base branch from unstable to stretch-unstable Jun 17, 2018

@maniackcrudelis maniackcrudelis referenced this pull request Aug 27, 2018

Merged

Add the internal helper ynh_handle_getopts_args #520

0 of 4 tasks complete
@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Oct 24, 2018

Well, turns out #520 got merged in the meantime ... So we could implement --options right away ? :s

@alexAubin

This comment has been minimized.

Copy link
Member

alexAubin commented Dec 1, 2018

Bumpz ? :s

@alexAubin alexAubin added the inactive label Dec 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment