Better way of adding the user to the group

[XPlantefeve]

Rather than hacking /etc/group with sed in app_permissions.sh, the official way of adding an user to a group in Synology can be used:

synogroup --member log Log_Analysis

[toafez]

Hi!

Thank you for your feedback.

As far as I know, when using synogroup --member, not only the new user must be specified, but also all users already entered in this group. This may still work for the group log, since only the user log is usually entered here. However, I also use the app-permissions.sh for the group administrators or system, for example, and then it becomes difficult. You would first have to extract the known users in order to be able to give them to the --member option. I have not yet found out how to do this.

Tommes

[XPlantefeve]

You are right indeed. You can get the current list with a bit of regexp and add Log_Analysis to the current members:

> LOGUSERS=`synogroup --get log | grep -E '^[0-9]*:' | sed -e 's/^[0-9]*:\[\(.*\)\]/\1/'`
> synogroup --member log $LOGUSERS Log_Analysis

[toafez]

Nice code snippet. I just tried it out and it works very well. Thank you very much for that. I'll see what I can do with it.

But now you didn't hack the /etc/group with sed, but you hacked the synogroup command with sed. A little joke ;-)

Just a note in case anyone else is reading this. Log_Analysis was the old package name to keep the packages for DSM6 and DSM7 apart. LogAnalysis (without underscore) is the current package name and should be used in the above code snippet.

[XPlantefeve]

I find it better to use synogroup because I surmise its content is shadowed in a synology only file. The logic can be reused with something like that:

function synogroupadduser {
    GROUPUSERS=`synogroup --get $1 | grep -E '^[0-9]*:' | sed -e 's/^[0-9]*:\[\(.*\)\]/\1/' | tr '\n' ' '`
    synogroup --member $1 $GROUPUSERS $2
}

> synogroupadduser log whateveruseryouwanttoadd

(I added TR at the end of the pipeline to avoid side effects with groups that already have several members)

[toafez]

That definitely sounds plausible, better to use the synogroup here. I will test this out and probably implement it. Maybe I'll build a function to remove a user from the group again. That can sometimes be quite useful.

Thanks again for this great tip.

[geimist]

I find it better to use synogroup because I surmise its content is shadowed in a synology only file. The logic can be reused with something like that:

function synogroupadduser {
    GROUPUSERS=`synogroup --get $1 | grep -E '^[0-9]*:' | sed -e 's/^[0-9]*:\[\(.*\)\]/\1/' | tr '\n' ' '`
    synogroup --member $1 $GROUPUSERS $2
}

> synogroupadduser log whateveruseryouwanttoadd

(I added TR at the end of the pipeline to avoid side effects with groups that already have several members)

Nice solution, but users should be quoted, as spaces are also allowed in usernames.

function synogroupadduser {
    GROUPUSERS=$(synogroup --get "$1" | grep -E '^[0-9]*:' | sed -e 's/^[0-9]*:\[\(.*\)\]/\1/;s/^/"/;s/$/"/' | tr '\n' ' ')
    synogroup --member "$1" $GROUPUSERS "$2"
}

synogroupadduser log whateveruseryouwanttoadd

Same to delete user:

function synogroupdeluser {
    GROUPUSERS=$(synogroup --get "$1" | grep -E '^[0-9]*:' | sed -e 's/^[0-9]*:\[\(.*\)\]/\1/;s/^/"/;s/$/"/' | grep -v "$2" | tr '\n' ' ')
    synogroup --member "$1" $GROUPUSERS
}

synogroupdeluser log whateveruseryouwanttodel

[XPlantefeve]

Better than quotes: an array.

function synogroupadduser {
    # We make sure that the field separator does not include space, it would break
    # on usernames including one (non POSIX, but synology allows them). We will restore
    # the current value at the end.
    OLDIFS=$IFS
    IFS=$'\n'
    
    GROUP=$1
    NEWUSER=$2
    USERLIST=()
    # We read the group info, grep the lines that list users, and
    # remove any extra characters with sed.
    CURRENTUSERS=$(synogroup --get $GROUP | grep -E '^[0-9]*:'| sed -e 's/^[0-9]*:\[\(.*\)\]/\1\n/')
    
    # We use a string array, as we'll be able to pass that as a parameters
    # for synogroup, and it will preserve spaces in usernames.
    for USER in $CURRENTUSERS; do
        USERLIST+=($USER)
    done
    USERLIST+=($NEWUSER)
    
    synogroup --member $GROUP ${USERLIST[@]}
    
    IFS=$OLDIFS
}

But I admit it starts to get noticeably longer.

[toafez]

But I admit it starts to get noticeably longer.

In return, it is also getting better and better. I'm going to get some popcorn and watch you perfect the code. It would have taken me days to implement it. Really great and thanks for helping

[toafez]

I tested your script this morning and it works really well. When calling the function, you should at least put the user in quotes, especially if it contains a space. As far as I know, group names should not contain spaces, so the quotes can be omitted here.

I have rewritten your function so that you can also delete a user again. It would then look like this...

function synogroupdeluser {
    # We make sure that the field separator does not include space, it would break
    # on usernames including one (non POSIX, but synology allows them). We will restore
    # the current value at the end.
    OLDIFS=$IFS
    IFS=$'\n'

    GROUP=$1
    DELUSER=$2
    USERLIST=()
    # We read the group info, grep the lines that list users, and
    # remove any extra characters with sed.
    CURRENTUSERS=$(synogroup --get $GROUP | grep -E '^[0-9]*:'| sed -e 's/^[0-9]*:\[\(.*\)\]/\1\n/')

    # We use a string array, as we'll be able to pass that as a parameters
    # for synogroup, and it will preserve spaces in usernames.
    for USER in $CURRENTUSERS; do
        if [ "$USER" != "$DELUSER" ]; then
                USERLIST+=($USER)
        fi
    done

    synogroup --member $GROUP ${USERLIST[@]}

    IFS=$OLDIFS
}

The function call would then look like this...

synogroupdeluser log "user name"

I like this very much and will incorporate it into LogAnalysis (and BasicBackup) in the next few days and test it further. Until then, if you see further potential for improvement, let me know.

[XPlantefeve]

I have almost no change for your synogroupdeluser, kudos for that. Just that small detail: "\n" is not needed for sed:

CURRENTUSERS=$(synogroup --get $GROUP | grep -E '^[0-9]*:'| sed -e 's/^[0-9]*:\[\(.*\)\]/\1/')

I noticed that my synogroupadduser could be a bit shorter, though:

function synogroupadduser {
    # We make sure that the field separator does not include space, it would break
    # on usernames including one (non POSIX, but synology allows them). We will restore
    # the current value at the end.
    OLDIFS=$IFS
    IFS=$'\n'
    
    GROUP=$1
    NEWUSER=$2
    # We read the group info, grep the lines that list users, and
    # remove any extra characters with sed. The end result is an array
    # we'll be able to add the new user to, and use as a parameter for synogroup
    USERLIST=($(synogroup --get $GROUP | grep -E '^[0-9]*:'| sed -e 's/^[0-9]*:\[\(.*\)\]/\1\n/'))
    USERLIST+=($NEWUSER)
    
    synogroup --member $GROUP ${USERLIST[@]}
    
    IFS=$OLDIFS
}

Let it be kept in mind that I am by no means a Bash specialist. I script a lot, but in other languages, and I tend to see Bash as a collection of hacks. A true bash aficionado would surely do it better.

[toafez]

I have revised the functions for adding and deleting a user again and secured them a little. It was possible to add a user to a group more than once. Also, when deleting a user, the synogroup command was executed even if no matching user was found.

• When adding a user, it is first checked whether a user with the same name already exists in the specified group. If yes, the script ends with an exit 1, otherwise the user is added to the specified group and the whole thing is confirmed with an exit 0.

function synogroupadduser {
	# Function call: synogroupadduser GROUPNAME "USERNAME"

	# Ensure that IFS does not contain a space, as it would not work for usernames that contain a space (not POS>
	oldIFS=${IFS}
	IFS=$'\n'

	# Transfer position parameters ($1 and $2) to variables
	group=${1}
	adduser=${2}

	# Read the group info, grep the lines that list users, and remove any extra characters with sed.
	userlist=$(synogroup --get ${group} | grep -E '^[0-9]*:'| sed -e 's/^[0-9]*:\[\(.*\)\]/\1\n/')

	# Create an array to update the userlist
	updatelist=()

	# Reading out the user list
	for user in ${userlist}; do
		# Add all read out users to the array
		[[ "${user}" != "${adduser}" ]] && updatelist+=(${user})
		# If the user to be added already exists
		[[ "${user}" == "${adduser}" ]] && userexists="true"
	done

	# Add the new user to the array
	updatelist+=(${adduser})

	# Adding all users to the group
	[ -z "${userexists}" ] && synogroup --member ${group} ${updatelist[@]}

	# Restore the original IFS value
	IFS=${oldIFS}

	# If the user to be added already exists
	[ -n "${userexists}" ] && exit 1 || exit 0
}

• When deleting a user, it is first checked whether the user name already exists in the specified group. If yes, the script ends with an exit 1, otherwise the user is deleted from the specified group and the whole thing is confirmed with an exit 0.

function synogroupdeluser {
	# Function call: synogroupdeluser GROUPNAME "USERNAME"

	# Ensure that IFS does not contain a space, as it would not work for usernames that contain >
	oldIFS=${IFS}
	IFS=$'\n'

	# Transfer position parameters ($1 and $2) to variables
	group=${1}
	deluser=${2}

	# Read the group info, grep the lines that list users, and remove any extra characters with sed.
	userlist=$(synogroup --get ${group} | grep -E '^[0-9]*:'| sed -e 's/^[0-9]*:\[\(.*\)\]/\1/')

	# Create an array to update the userlist
	updatelist=()

	# Reading out the user list
	for user in ${userlist}; do
		# Add all users to the array except the user to be deleted
		[[ "${user}" != "${deluser}" ]] && updatelist+=(${user})
		# If the user to be deleted exists
		[[ "${user}" == "${deluser}" ]] && userexists="true"
	done

	# Delete the user from the group
	[ -n "${userexists}" ] && synogroup --member ${group} ${updatelist[@]}

	# Restore the original IFS value
	IFS=${oldIFS}

	# If the user to be deleted was not found
	[ -z "${userexists}" ] && exit 1 || exit 0
}

@XPlantefeve
Since I'm not very familiar with sed, a question. I have now only omitted the \n in the synogroupdeluser function. Is that correct, or can it also be omitted in the synogroupadduser function? By the way... I'm not a bash specialist either, but we complement each other. In the end, what counts is the result and that we all have a good feeling about it.

[XPlantefeve]

Yes, there's no reason to get the userlist differently. Adding that \n will just add empty lines that will be ignored later. There's no need for it.