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.

Sign up for GitHub

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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
232fc0d
1. For multilogin, use the Downloads folder of user who first started…
ibrado Nov 24, 2017
79533de
Check for empty localdownloads (single user)
ibrado Nov 25, 2017
0b8d3b5
Quote variable usage, command output, and paths
ibrado Dec 28, 2017
63717d2
Break up long line
ibrado Dec 28, 2017
2e5b690
- Remove getactiveuser function
ibrado Dec 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion host-bin/enter-chroot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,15 @@ tmpfsmount() {
mount -i -t tmpfs -o "rw${2:+,}$2" tmpfs "$target"
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Author

@ibrado ibrado Dec 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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...

activeuser=$(jq -r .LastActiveUser /home/chronos/Local\ State)
Copy link
Owner

Choose a reason for hiding this comment

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

quote the path instead of using backslash escapes

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link
Owner

Choose a reason for hiding this comment

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

quote the output of the command

Copy link
Author

Choose a reason for hiding this comment

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

Done!

fi
echo $activeuser
Copy link
Owner

Choose a reason for hiding this comment

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

quote variable usage

Copy link
Author

Choose a reason for hiding this comment

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

Done!

}

# If /var/run isn't mounted, we know the chroot hasn't been started yet.
if mountpoint -q "`fixabslinks '/var/run'`"; then
firstrun=''
Expand Down Expand Up @@ -436,9 +445,21 @@ elif [ ! -f "$shares" ]; then
downloads ~/Downloads
EOF
fi

activeuser=$(getactiveuser)
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

fi

# 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ibrado ibrado Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@ibrado ibrado Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, that works. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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...

Copy link
Owner

Choose a reason for hiding this comment

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

Keep lines to 80 characters, please

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link
Owner

Choose a reason for hiding this comment

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

Quote the output of the command

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link
Owner

Choose a reason for hiding this comment

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

Quote the path instead of escaping spaces

Copy link
Author

Choose a reason for hiding this comment

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

Done!

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

else
localdownloads='/home/chronos/user/Downloads'
fi
if [ ! -d "$localdownloads" ]; then
localdownloads=''
fi
Expand Down