Skip to content

Check the logged user instead of $USER #3330

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

Merged
merged 2 commits into from
May 11, 2021
Merged

Conversation

videlanicolas
Copy link
Contributor

Given that sudo usermod --login "$DOCKER_USER" coder and sudo groupmod -n "$DOCKER_USER" coder modify the container's disk it'll persist across restarts, but environment variables will be reset to whatever state they had at the end of Dockerfile. In this case, $USER is set to coder, so this branch will always be true.

By checking with the output of whoami, which gets it's information from /etc/passwd, we make sure to get the real logged user and not the one defined by $USER.

We also move USER="$DOCKER_USER" out of the branch, since we always want this to happen at entry-point. If we don't do this assignment, $USER will contain coder upon restart.

Fixes #2767.

Given that `sudo usermod --login "$DOCKER_USER" coder` and `sudo groupmod -n "$DOCKER_USER" coder` modify the container's disk it'll persist across restarts, but environment variables will be reset to whatever state they had at the end of `Dockerfile`. In this case, `$USER` is set to `coder`, so this branch will always be true.

By checking with the output of `whoami`, which gets it's information from `/etc/passwd`, we make sure to get the real logged user and not the one defined by $USER.

We also move `USER="$DOCKER_USER"` out of the branch, since we always want this to happen at entry-point. If we don't do this assignment, $USER will contain `coder` upon restart.
@videlanicolas videlanicolas requested a review from a team as a code owner May 9, 2021 15:43
@code-asher code-asher self-assigned this May 10, 2021
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!! Your PR description was fantastic too.

@jsjoeio jsjoeio added enhancement Some improvement that isn't a feature new contributor For PRs by first-time contributor labels May 10, 2021
@jsjoeio jsjoeio added this to the v3.11.0 milestone May 10, 2021
@repo-ranger
Copy link
Contributor

repo-ranger bot commented May 10, 2021

Thanks for making your first contribution! 🙂

@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #3330 (b491e06) into main (02a0e05) will not change coverage.
The diff coverage is n/a.

❗ Current head b491e06 differs from pull request most recent head 1aed545. Consider uploading reports for the commit 1aed545 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3330   +/-   ##
=======================================
  Coverage   58.95%   58.95%           
=======================================
  Files          35       35           
  Lines        1703     1703           
  Branches      374      374           
=======================================
  Hits         1004     1004           
  Misses        561      561           
  Partials      138      138           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02a0e05...1aed545. Read the comment docs.

@jsjoeio
Copy link
Contributor

jsjoeio commented May 10, 2021

You can ignore the trivy-scan failure. We're aware of that and working on a fix. As for the audit vulnerabilities, we're also aware of a postcss CVE that needs to be fixed. Everything else looks good!

Check `$DOCKER_USER` was defined before copying it to `$USER`.
@videlanicolas videlanicolas requested a review from code-asher May 11, 2021 00:07
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

🎉 🎉 🎉

@code-asher code-asher merged commit 3df771f into coder:main May 11, 2021
@jsjoeio jsjoeio mentioned this pull request May 11, 2021
xfl12345 added a commit to xfl12345/code-server that referenced this pull request Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Some improvement that isn't a feature new contributor For PRs by first-time contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot start stopped docker container, user 'coder' does not exist
3 participants