Skip to content

snapenv: add environment variable with UID value#12565

Merged
mvo5 merged 11 commits intocanonical:masterfrom
sergio-costas:add_user_id_environment_variable
Mar 28, 2023
Merged

snapenv: add environment variable with UID value#12565
mvo5 merged 11 commits intocanonical:masterfrom
sergio-costas:add_user_id_environment_variable

Conversation

@sergio-costas
Copy link
Copy Markdown
Contributor

@sergio-costas sergio-costas commented Feb 14, 2023

Some ENVs require the user ID, but the only way of getting it is using a little shell script that exports the ENV after using "id -u" to get the value. Other uses tricks, like taking advantage of a similar path (like firefox, which sets PULSE_SERVER to "$XDG_RUNTIME_DIR/../pulse/native".

This patch adds the SNAP_UID environment variable, which contains the effective user id. With that, it's possible to define paths containing it directly in the YAML file.

@sergio-costas sergio-costas changed the title Add environment variable with UID value snapenv: add environment variable with UID value Feb 14, 2023
@sergio-costas sergio-costas force-pushed the add_user_id_environment_variable branch from 169f491 to 69b62fe Compare February 14, 2023 12:11
ernestl
ernestl previously approved these changes Feb 16, 2023
Comment thread snap/snapenv/snapenv_test.go Outdated
@mvo5 mvo5 requested a review from pedronis February 21, 2023 14:52
@pedronis
Copy link
Copy Markdown
Contributor

pedronis commented Feb 23, 2023

This needs to define a matching assume (snap-uid perhaps)?, see o/snapstate/check_snap.go, as a snap using this, especially directly from the snap.yaml running on a old snapd would usually fail without ways to fallback so we need to way to prevent it installing early

@pedronis pedronis added the Needs Samuele review Needs a review from Samuele before it can land label Feb 24, 2023
@sergio-costas sergio-costas force-pushed the add_user_id_environment_variable branch from 8516bfd to a8e4767 Compare February 24, 2023 14:31
@sergio-costas
Copy link
Copy Markdown
Contributor Author

@pedronis Good point! Added.

Copy link
Copy Markdown
Contributor

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

question

Comment thread snap/snapenv/snapenv.go Outdated
Comment thread overlord/snapstate/check_snap.go Outdated
@pedronis pedronis requested review from jhenstridge and pedronis March 2, 2023 16:27
@pedronis pedronis added Needs Documentation 📖 Needs security review Can only be merged once security gave a :+1: labels Mar 2, 2023
Copy link
Copy Markdown
Contributor

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

this looks alright in itself, I asked though security to review to see if they can chime in about a proper comment on the usage of euid

@sergio-costas
Copy link
Copy Markdown
Contributor Author

About the "needs documentation" tag... where should I send a patch to include that?

@Meulengracht
Copy link
Copy Markdown
Member

About the "needs documentation" tag... where should I send a patch to include that?

https://forum.snapcraft.io/t/environment-variables/7983

@sergio-costas sergio-costas force-pushed the add_user_id_environment_variable branch 2 times, most recently from bb2d3a7 to 6cd271b Compare March 6, 2023 11:53
@mvo5 mvo5 added the Squash-merge Please squash this PR when merging. label Mar 7, 2023
@sergio-costas sergio-costas force-pushed the add_user_id_environment_variable branch 2 times, most recently from 53c053c to b816223 Compare March 7, 2023 11:56
@sergio-costas sergio-costas requested review from pedronis and removed request for jhenstridge March 14, 2023 12:57
Copy link
Copy Markdown
Contributor

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

couple small comments

Comment thread tests/main/snap-env/task.yaml Outdated
Comment thread overlord/snapstate/check_snap.go Outdated
Comment thread snap/snapenv/snapenv.go Outdated
@pedronis pedronis added this to the 2.60 milestone Mar 15, 2023
Some ENVs require the user ID, but the only way of getting it
is using a little shell script that exports the ENV after using
"id -u" to get the value. Other uses tricks, like taking advantage
of a similar path (like firefox, which sets PULSE_SERVER to
"$XDG_RUNTIME_DIR/../pulse/native".

This patch adds the SNAP_UID environment variable, which contains
the effective user id. With that, it's possible to define paths
containing it directly in the YAML file.
@sergio-costas sergio-costas force-pushed the add_user_id_environment_variable branch from 9ae6c40 to 5df0079 Compare March 15, 2023 13:26
Copy link
Copy Markdown
Member

@Meulengracht Meulengracht left a comment

Choose a reason for hiding this comment

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

LGTM

@pedronis pedronis self-requested a review March 20, 2023 09:26
Copy link
Copy Markdown
Contributor

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

+1

Comment thread overlord/snapstate/check_snap.go Outdated
@mvo5 mvo5 merged commit 6d90d61 into canonical:master Mar 28, 2023
@sergio-costas
Copy link
Copy Markdown
Contributor Author

Commented in the documentation thread the new variables. https://forum.snapcraft.io/t/environment-variables/7983/28?u=sergiocostas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Documentation 📖 Needs Samuele review Needs a review from Samuele before it can land Needs security review Can only be merged once security gave a :+1: Squash-merge Please squash this PR when merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants