Add set_unix_permissions and syno_user_add_to_legacy_group #3161

Safihre · 2018-02-21T12:29:11Z

Support functions needed to:

Correct permissions on folders during upgrade in Linux mode.
For transfer for download-related packages from 'sc-media' to 'sc-download'

@ymartin59 This separate PR to keep package PR's clean. But all my other PR's kind of depend on it.
After this is merged, I can rebase #3092 #3153 #3053 on master after that so they are package-only.

ymartin59

Definitely that is the right way to do it. And I guess it will probably fix troubles I saw with set_syno_permissions.
Still to confirm it properly works as expected on DSM 5.2

ymartin59 · 2018-02-21T22:21:29Z

mk/spksrc.service.installer

        # Set read/write permissions for GROUP for folder and subfolders
        if [ ! "`synoacltool -get \"${DIRNAME}\"| grep \"group:${GROUP}:allow:rwxpdDaARWcC-:fd--\"`" ]; then
+            # First Unix permissions, but only if it's in Linux mode
+            if [ ! "`synoacltool -get \"${DIRNAME}\"| grep \"Linux mocde\"`" ]; then


Looks like a typo: "mocde" !

ymartin59 · 2018-02-21T22:23:59Z

mk/spksrc.service.installer

+
+    # Check if user in old group
+    MEMBERS="$(synogroup --get $LEGACY_GROUP | grep '^[0-9]' | sed 's/.*\[\([^]]*\)].*/\1/' | tr '\n' ' ')"
+    if [[ $MEMBERS = *"$LEGACY_USER"* &&  $MEMBERS != *"$NEW_USER"* ]]; then


Hum. I would take care about such expression which may only run on bash... so only on DSM 6. I would check against on DSM 5.2 which runs busybox as default shell.

So I came to this, because (at least on DSM61) this if statement in postinst always returns true and the user always gets added even if already present:~~

if ! synogroup --get "$GROUP" | grep '^[0-9]:\[${EFF_USER}\]' &> /dev/null; then

So something is wrong with this syntax, but I couldn't figure out what.~~

I just tested and indeed seems busybox does not support this syntax.. Will come up with a new one!

Aah, my bash/shell-noobness that it took me an hour to figure out. It should be of course:

grep "^[0-9]:\[${EFF_USER}\]"

and not

grep '^[0-9]:\[${EFF_USER}\]'

In both commands.

ymartin59 · 2018-02-21T22:24:27Z

mk/spksrc.service.installer

+    if [[ $MEMBERS = *"$LEGACY_USER"* &&  $MEMBERS != *"$NEW_USER"* ]]; then
+        # Add new user and remove old one
+        echo "Adding '${NEW_USER}' to '${LEGACY_GROUP}' for backwards compatibility" >> ${INST_LOG}
+        MEMBERS=${MEMBERS//$LEGACY_USER}


Here again, probably requires bash too

This one does seem to work:

Support functions needed to: - Correct permissions on folders during upgrade in Linux mode. - For transfer for download-related packages from 'sc-media' to 'sc-download'

Safihre · 2018-02-22T10:42:01Z

Now it is working as expected.
Before it would add svc-download twice and not detect legacy membership correctly, now it does:

Step postinst. USER=nzbdrone GROUP=sc-download SHARE_PATH=
Installing service configuration /var/packages/nzbdrone/conf/nzbdrone.sc
Creating group sc-download
Group Name: [sc-download]
Group Type: [AUTH_LOCAL]
Group ID:   [65537]
Group Members:
0:[svc-nzbdrone]
Invoke service_postinst
Granting 'svc-nzbdrone' unix permissions on /volume1/@appstore/nzbdrone/var/.config
Adding 'svc-nzbdrone' to 'sc-media' for backwards compatibility
Group Name: [sc-media]
Group Type: [AUTH_LOCAL]
Group ID:   [65536]
Group Members:
0:[svc-nzbdrone]
Thr Feb 22 09:56:18 CET 2018

Safihre · 2018-02-22T12:30:32Z

Just found out that grep on DSM 5.2 doesn't support the -P. Have to update all packages to some fancy sed. 😢

So that service_postinst can assume the var directory is available.

Safihre · 2018-02-22T13:35:43Z

Removed Sonarr/Radarr specific things from this PR, creating separate one.

Safihre · 2018-02-22T15:01:49Z

Another small bug: all code assumes EFF_USER is set. But for packages like Python it's not set since they don't need a user.
Introduced checks to all functions to only be performed if there actually is EFF_USER.

ymartin59 · 2018-02-22T18:22:03Z

OK. Do you consider it as ready to merge ?

Safihre · 2018-02-22T18:41:43Z

I would say so yes.
Have been testing all day today on the 3 systems (Xpenology 5.2, Xpenology 6.1 and real 6.1 NAS) and also the "old style" package to "new style" upgrade paths.
That revealed all these problems that I fixed today also in the other PR's.
What a hassle these packages are :|
Never imagined it to be so complex..

Safihre · 2018-02-22T18:43:19Z

If this is merged I'll do a proper rebuild of all the testing packages and test it again on all 3 systems.

m4tt075 · 2018-02-22T19:44:17Z

@ymartin59 @Diaoul Thanks a lot for inviting me to this elect club. More than happy to join, although I will consider myself as a "junior" member for the time being. @Safihre I have been on a business trip the last couple of days and am amazed about the progress you have made in the meantime. This is great, great work. Thanks so much! Looking forward to review "my" packages as well once this is in!

m4tt075 · 2018-02-27T21:51:43Z

mk/spksrc.service.installer

+    if [ -n "${EFF_USER}" ]; then
+        echo "Granting '${EFF_USER}' unix permissions on ${DIRNAME}" >> ${INST_LOG}
+        if [ $SYNOPKG_DSM_VERSION_MAJOR -lt 6 ]; then
+            chown -R ${EFF_USER}:root "${DIRNAME}" >> $INST_LOG 2>&1


@Safihre @ymartin59 I have continued to debug the upgrade problems on DSM5.2 systems that I ran into yesterday (packages did not start anymore after upgrade due to permission issues). I suspect the culprit is that the group is set to root here rather than users for DSM lt 6. Do you agree? If so, we probaby have to re-build all packages which were built since.

Most test packages have only built for DSM 6... Group "users" is used for share folders accessible from other processes. For a package specific folder, group "root" should be far enough as owner matches service user.

Agreed with @ymartin59, but unfortunately in the case of Tvheaded everything is mixed. Package and "user" folders blend together.

But @m4tt075, do you need to use set_syno_permmissions at all in your case? Since everything stays in the Package directory, do you need to call any permissions command at all?

Isn't really impossible to configure TVH download location to a share folder as other applications ? I think that may be the key. If download locations are "hardcoded", it is still possible to create symbolic link from expected/original locations to DSM share folder(s)

OK, thanks for clarifying. @Safihre I agree with you. Unix permissions need to be sufficient for all folders within @appstore. I have been able to implement that approach successfully for DSM6.1 now. I have tested clean installs, upgrading by itself and upgrading from the currently published package. But it does not work on DSM 5.2, when upgrading from the currently published package. In that version, ACL permissions were still set on the target and traversal folders. Is there a way to "remove" those ACL permissions again?

@m4tt075 Why are ACL permissions applied? Only if set_syno_permissions is called this happens, but that shouldn't be needed at all, right?

…nity#3161) * Add set_unix_permissions and syno_user_add_to_legacy_group Support functions needed to: - Correct permissions on folders during upgrade in Linux mode. - For transfer for download-related packages from 'sc-media' to 'sc-download' * Highlight logging of Generic Service to identify steps easier * Create INST_VAR before calling service_postinst So that service_postinst can assume the var directory is available. * Do not assume that EFF_USER exists

Safihre added the framework label Feb 21, 2018

Safihre requested a review from ymartin59 February 21, 2018 12:29

Safihre mentioned this pull request Feb 21, 2018

Service Support: add background command execution #3123

Merged

Safihre force-pushed the generic-service-fixes branch 2 times, most recently from 6253204 to a79e9a2 Compare February 21, 2018 16:33

ymartin59 reviewed Feb 21, 2018

View reviewed changes

Add set_unix_permissions and syno_user_add_to_legacy_group

286d2b9

Support functions needed to: - Correct permissions on folders during upgrade in Linux mode. - For transfer for download-related packages from 'sc-media' to 'sc-download'

Safihre force-pushed the generic-service-fixes branch from a79e9a2 to 323f639 Compare February 22, 2018 10:40

Safihre added 2 commits February 22, 2018 14:34

Create INST_VAR before calling service_postinst

fde0ffe

So that service_postinst can assume the var directory is available.

Highlight logging of Generic Service to identify steps easier

0aea8ee

Safihre force-pushed the generic-service-fixes branch from 250f5af to 0aea8ee Compare February 22, 2018 13:35

Safihre mentioned this pull request Feb 22, 2018

Sonarr and Radarr fixes #3163

Merged

3 tasks

Do not assume that EFF_USER exists

3a9e320

Safihre force-pushed the generic-service-fixes branch from b9f0e30 to 3a9e320 Compare February 22, 2018 15:10

ymartin59 merged commit 005677a into SynoCommunity:master Feb 23, 2018

Safihre deleted the generic-service-fixes branch February 23, 2018 07:31

m4tt075 mentioned this pull request Feb 24, 2018

Update Tvheadend to 4.2.6 (maintenance release) for DSM 5+6 #3050

Merged

4 tasks

m4tt075 reviewed Feb 27, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add set_unix_permissions and syno_user_add_to_legacy_group #3161

Add set_unix_permissions and syno_user_add_to_legacy_group #3161

Safihre commented Feb 21, 2018

ymartin59 left a comment

ymartin59 Feb 21, 2018

ymartin59 Feb 21, 2018

Safihre Feb 22, 2018 •

edited

Loading

Safihre Feb 22, 2018

ymartin59 Feb 21, 2018

Safihre Feb 22, 2018

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

ymartin59 commented Feb 22, 2018

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

m4tt075 commented Feb 22, 2018 •

edited

Loading

m4tt075 Feb 27, 2018

ymartin59 Feb 27, 2018

Safihre Feb 28, 2018 •

edited

Loading

ymartin59 Feb 28, 2018

m4tt075 Feb 28, 2018

Safihre Feb 28, 2018

Add set_unix_permissions and syno_user_add_to_legacy_group #3161

Add set_unix_permissions and syno_user_add_to_legacy_group #3161

Conversation

Safihre commented Feb 21, 2018

ymartin59 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Safihre Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

ymartin59 commented Feb 22, 2018

Safihre commented Feb 22, 2018

Safihre commented Feb 22, 2018

m4tt075 commented Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Safihre Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Safihre Feb 22, 2018 •

edited

Loading

m4tt075 commented Feb 22, 2018 •

edited

Loading

Safihre Feb 28, 2018 •

edited

Loading