Use Downloads folder of the user who launched the chroot #3531

ibrado · 2017-11-24T15:57:49Z

For multiple logins, this uses the LastActiveUser in /home/chronos/Local State to determine which ~/Downloads folder should be mapped in the chroot, i.e. that of the user who started the chroot.

Additionally, this creates /etc/crouton/user containing the email address of the ~/Downloads "owner".

See also issue #1410

… the chroot 2. Add /etc/crouton/user with account/email address of said user

googlebot · 2017-11-24T15:57:57Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

ibrado · 2017-11-24T16:06:37Z

I signed it!

googlebot · 2017-11-24T16:06:40Z

CLAs look good, thanks!

DennisLfromGA · 2017-11-24T16:57:52Z

host-bin/enter-chroot

 # Bind-mount the stuff in $CHROOT/etc/crouton/shares, unless NOLOGIN is set
 if [ -z "$NOLOGIN" -a -f "$shares" ]; then
-    localdownloads='/home/chronos/user/Downloads'
+    if [ "$activeuser" != "Default" ]; then
+        localdownloads=$(jq -r ".profile.info_cache | to_entries[] | select(.value.user_name == \"$activeuser\") | \"/home/chronos/\(.key)/Downloads\"" /home/chronos/Local\ State)


Although 'activeuser' is populated and valid, both 'localdownloads' locations appear empty:

chronos@localhost ~ $ localdownloads=$(jq -r ".profile.info_cache | to_entries[] | select(.value.user_name == \"$activeuser\") | \"/home/chronos/\(.key)/Downloads\"" /home/chronos/Local\ State) ls -l $localdownloads /home/chronos/Default/Downloads: total 0 /home/chronos/LockScreenAppsProfile/Downloads: total 0

Yet when I list /home/chronos/user/Downloads it contains all of my files.

I'm not sure this routine will get you the expected results but I may not understand the full intent.

Ah, I'm guessing you haven't logged in to multiple accounts on that Chromebook?

Even so, I expected that the jq -r part would not have run (if [ "$activeuser" != "Default" ]) and /home/chronos/user/Downloads would have been used instead.

Would you mind checking the output of jq -r .LastActiveUser /home/chronos/Local\ State ? That's what activeuser should have been set to at line 449.

BTW, here's what I'm getting (except for the edited email address and u- id):

$ activeuser=$(jq -r .LastActiveUser /home/chronos/Local\ State) $ echo $activeuser [email protected] $ localdownloads=$(jq -r ".profile.info_cache | to_entries[] | select(.value.user_name == \"$activeuser\") | \"/home/chronos/\(.key)/Downloads\"" /home/chronos/Local\ State) $ echo $localdownloads /home/chronos/u-xxxxxxxe64bcd6851c24f81f6566ef0519e781cba/Downloads $ ls $localdownloads ...files of [email protected] here...

You're correct, I only have one account setup on this device.
Here's the output you requested with my email masked:

chronos@localhost ~ $ jq -r .LastActiveUser /home/chronos/Local\ State [email protected]

It could be that when I add another user account I'd get the correct $localdownloads location populated but I'm thinking it might be best not to do that yet so we can test this on devices with only one account. When I run the second part beginning with localdownloads=... I still get the two empty directories.

Let me know if I'm doing something wrong or if you'd like me to test something else.

I ran the modified enter-chroot adding set -x at line #455 and an exit at line #560 to see what's getting set. I posted the output at pastebin here. Hopefully, you'll get something useful out of it.

Cool, that works. 👍

BTW, I ran both commits on a Chromebook with multiple users and they both worked so I guess it was good to check it on a single user device.

Woohoo! Yeah, it was good you had that around... I was seriously considering a powerwash earlier. :-P

I have four Chromebooks going all the way back to a Cr-48 and now a Pixelbook so I have options. ,-)
I like to run the 'chrx' script, 1st phase only and use partiton 7 as a safe place for crouton so I don't have to worry about powerwashing.

Haven't had the guts to try chrx yet, haha! I had 2 Chromebooks before this one, but I lent one and gave the other away...

dnschneid

Thanks for implementing this cleanly!

dnschneid · 2017-12-27T21:20:50Z

host-bin/enter-chroot

+getactiveuser() {
+    local activeuser="Default"
+    if [ -x "/usr/bin/jq" ]; then
+        activeuser=$(jq -r .LastActiveUser /home/chronos/Local\ State)


quote the path instead of using backslash escapes

dnschneid · 2017-12-27T21:20:58Z

host-bin/enter-chroot

+getactiveuser() {
+    local activeuser="Default"
+    if [ -x "/usr/bin/jq" ]; then
+        activeuser=$(jq -r .LastActiveUser /home/chronos/Local\ State)


quote the output of the command

dnschneid · 2017-12-27T21:21:08Z

host-bin/enter-chroot

+    if [ -x "/usr/bin/jq" ]; then
+        activeuser=$(jq -r .LastActiveUser /home/chronos/Local\ State)
+    fi
+    echo $activeuser


quote variable usage

dnschneid · 2017-12-27T21:21:37Z

host-bin/enter-chroot

@@ -436,9 +445,22 @@ elif [ ! -f "$shares" ]; then
 downloads ~/Downloads
 EOF
 fi
+
+activeuser=$(getactiveuser)


I don't think this needs to be a separate function

Ok... Since we're not creating /etc/crouton/user anymore (see next comment), there's no reason for it.

dnschneid · 2017-12-27T21:22:58Z

host-bin/enter-chroot

+if [ -n "$firstrun" ]; then
+    # Store the active user for possible use in other scripts, etc.
+    activeuserfile="$(fixabslinks '/etc/crouton/user')"
+    echo $activeuser > $activeuserfile


I would rather not (easily) expose this information to the chroot. Is there a real use for it?

Just a nice-to-have. Removed...

dnschneid · 2017-12-27T21:23:44Z

host-bin/enter-chroot

 # Bind-mount the stuff in $CHROOT/etc/crouton/shares, unless NOLOGIN is set
 if [ -z "$NOLOGIN" -a -f "$shares" ]; then
-    localdownloads='/home/chronos/user/Downloads'
+    if [ "$activeuser" != "Default" ]; then
+        localdownloads=$(jq -r ".profile.info_cache | to_entries[] | select(.value.user_name == \"$activeuser\") | \"/home/chronos/\(.key)/Downloads\"" /home/chronos/Local\ State)


Keep lines to 80 characters, please

dnschneid · 2017-12-27T21:23:56Z

host-bin/enter-chroot

 # Bind-mount the stuff in $CHROOT/etc/crouton/shares, unless NOLOGIN is set
 if [ -z "$NOLOGIN" -a -f "$shares" ]; then
-    localdownloads='/home/chronos/user/Downloads'
+    if [ "$activeuser" != "Default" ]; then
+        localdownloads=$(jq -r ".profile.info_cache | to_entries[] | select(.value.user_name == \"$activeuser\") | \"/home/chronos/\(.key)/Downloads\"" /home/chronos/Local\ State)


Quote the output of the command

dnschneid · 2017-12-27T21:24:37Z

host-bin/enter-chroot

 # Bind-mount the stuff in $CHROOT/etc/crouton/shares, unless NOLOGIN is set
 if [ -z "$NOLOGIN" -a -f "$shares" ]; then
-    localdownloads='/home/chronos/user/Downloads'
+    if [ "$activeuser" != "Default" ]; then
+        localdownloads=$(jq -r ".profile.info_cache | to_entries[] | select(.value.user_name == \"$activeuser\") | \"/home/chronos/\(.key)/Downloads\"" /home/chronos/Local\ State)


Quote the path instead of escaping spaces

dnschneid · 2017-12-27T21:25:14Z

host-bin/enter-chroot

 # Bind-mount the stuff in $CHROOT/etc/crouton/shares, unless NOLOGIN is set
 if [ -z "$NOLOGIN" -a -f "$shares" ]; then
-    localdownloads='/home/chronos/user/Downloads'
+    if [ "$activeuser" != "Default" ]; then
+        localdownloads=$(jq -r ".profile.info_cache | to_entries[] | select(.value.user_name == \"$activeuser\") | \"/home/chronos/\(.key)/Downloads\"" /home/chronos/Local\ State)


The escape before the parentheses...is that meaningful to jq?

Yes, else you'd get a literal (.key) in the result.

dnschneid · 2017-12-27T21:25:45Z

host-bin/enter-chroot

+# Determines currently-logged in user
+getactiveuser() {
+    local activeuser="Default"
+    if [ -x "/usr/bin/jq" ]; then


I like that you check for the existence of jq (in case it disappears in the future). Is it possible, though, to do this with awk in a reasonable way?

I don't know enough awk to answer this 😊 .. I do think jq will be around for a while since the config file is json. I actually added this check in case it was new...

- Don't create /etc/crouton/user

ibrado · 2017-12-28T03:04:45Z

@DennisLfromGA Hi... Would you mind checking the latest commits on your single-user device? Thanks!

dnschneid · 2017-12-28T05:26:22Z

host-bin/enter-chroot

-    localdownloads='/home/chronos/user/Downloads'
+    activeuser=''
+    if [ -x "/usr/bin/jq" ]; then
+        activeuser="$(jq -r .LastActiveUser '/home/chronos/Local State')"


activeuser="$(sed -n 's/.*"LastActiveUser":\s*"$[^"]*$".*/\1/p' '/home/chronos/Local State')"

dnschneid · 2017-12-28T06:02:57Z

host-bin/enter-chroot

+    if [ -n "$activeuser" ]; then
+        localdownloads="$(jq -r '.profile.info_cache | to_entries[]
+          | select(.value.user_name == "'$activeuser'")
+          | "/home/chronos/\(.key)/Downloads"' '/home/chronos/Local State')"


I'm not convinced this is better (also not certain it is the correct way of doing it), but if we want to get rid of the jq dependency:

localdownloads="$(grep -l '"account_id":\s*"'"$activeuser"'"' /home/chronos/u-*/Preferences | sed 's/Preferences$/Downloads/;q')"

DennisLfromGA · 2018-03-02T23:03:45Z

It looks like something along these lines is being addressed in CrOS:

crbug.com/770766 - Allow running containers with multi-user

EDIT: There is now an environment variable named 'CROS_USER_ID_HASH' that may be an easier approach:

/home/user/${CROS_USER_ID_HASH}/Downloads

It looks like it was added just recently here:
'chromeos: Pass crosh the user hash (Tue Feb 06 16:07:07 2018)'
~~I'm not sure when this variable was implemented but~~ I just found references to it in the crosh 'vmc' and 'c' commands.

-DennisLfromGA

1. For multilogin, use the Downloads folder of user who first started…

232fc0d

… the chroot 2. Add /etc/crouton/user with account/email address of said user

DennisLfromGA reviewed Nov 24, 2017

View reviewed changes

Check for empty localdownloads (single user)

79533de

dnschneid reviewed Dec 27, 2017

View reviewed changes

dnschneid mentioned this pull request Dec 27, 2017

The default shared folder (Downloads) is broken #3578

Closed

ibrado added 3 commits December 28, 2017 10:11

Quote variable usage, command output, and paths

0b8d3b5

Break up long line

63717d2

- Remove getactiveuser function

2e5b690

- Don't create /etc/crouton/user

dnschneid reviewed Dec 28, 2017

View reviewed changes

dnschneid added the stale label Dec 31, 2021

Use Downloads folder of the user who launched the chroot #3531

Are you sure you want to change the base?

Use Downloads folder of the user who launched the chroot #3531

Conversation

ibrado commented Nov 24, 2017 • edited Loading

googlebot commented Nov 24, 2017

ibrado commented Nov 24, 2017

googlebot commented Nov 24, 2017

Choose a reason for hiding this comment

ibrado Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

ibrado Nov 24, 2017 • 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

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

dnschneid left a comment

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

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

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

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

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

ibrado Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

ibrado commented Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DennisLfromGA commented Mar 2, 2018 • edited Loading

ibrado commented Nov 24, 2017 •

edited

Loading

ibrado Nov 24, 2017 •

edited

Loading

ibrado Nov 24, 2017 •

edited

Loading

ibrado Dec 28, 2017 •

edited

Loading

ibrado commented Dec 28, 2017 •

edited

Loading

DennisLfromGA commented Mar 2, 2018 •

edited

Loading