Refactor flatpak doctor symlink check for consistency with setup; update doctor to warn that it will invoke sudo if using a system flatpak - #55
Conversation
| if [ "$install_mode" = "system" ]; then | ||
| info " Checking system Flatpak permissions (may prompt for sudo)..." | ||
| fi | ||
| if flatpak_can_access_devtools_dir; then |
There was a problem hiding this comment.
So this permissions check occurs right before a read-only operation that justifies it. That puts users who have no issue at risk of an unnecessary escalation. Could do the reads, then if necessary do the permission check using the reads to inform the decision.
There was a problem hiding this comment.
Ok, can refactor (not quite sure how, but I'll figure it out)
There was a problem hiding this comment.
Is sudo necessary here? I feel like that's wild for flatpak's security model. We should probably just remove sudo from flatpak info --show-permissions
There was a problem hiding this comment.
Ok, can refactor (not quite sure how, but I'll figure it out)
Yeah sketch of what I meant:
local -a linked=()
for name in bar chobby engine; do
[ -L "${link_map[$name]}" ] && linked+=("$name")
done
if [ "${#linked[@]}" -eq 0 ]; then
echo ""
return
fi
# only now the permission check
if flatpak_can_access_devtools_dir; then
echo ""
return
fi
# report ${linked[@]} as beforeBut I think the escalation itself is unnecessary (second comment)? We're not writing anything there. I could see us using flatpak to offer to straight up install bar for people on linux, if we don't find it, though -- that'd be cool, and sidestep all of our WHERE IS YOUR APPIMAGE weirdness. But even then we'd use flatpak install --user and bypass sudo. Does flatpak info --show-permissions work on your flake?
There was a problem hiding this comment.
Is sudo necessary here? I feel like that's wild for flatpak's security model. We should probably just remove sudo from
flatpak info --show-permissions
I tried, it literally reports different fs permissions between run-as-user and run-as-sudo. I did not try anything farther beyond that though...
norty@norty-rog:~/hacknight/BAR-Devtools$ sudo flatpak info --show-permissions info.beyondallreason.bar
[sudo: authenticate] Password:
[Context]
shared=network;ipc;
sockets=x11;pulseaudio;
devices=dri;
filesystems=xdg-run/app/com.discordapp.Discord:create;/home/norty/hacknight/BAR-Devtools;
[Environment]
PATH=/usr/bin:/app/bin:/usr/lib/sdk/node20/bin
norty@norty-rog:~/hacknight/BAR-Devtools$ flatpak info --system --show-permissions info.beyondallreason.bar
[Context]
shared=network;ipc;
sockets=x11;pulseaudio;
devices=dri;
filesystems=xdg-run/app/com.discordapp.Discord:create;
[Environment]
PATH=/usr/bin:/app/bin:/usr/lib/sdk/node20/bin
norty@norty-rog:~/hacknight/BAR-Devtools$ flatpak info --user --show-permissions info.beyondallreason.bar
error: info.beyondallreason.bar/*unspecified*/*unspecified* not installed
norty@norty-rog:~/hacknight/BAR-Devtools$ sudo flatpak info --user --show-permissions info.beyondallreason.bar
error: info.beyondallreason.bar/*unspecified*/*unspecified* not installed
norty@norty-rog:~/hacknight/BAR-Devtools$
... and then attempting to naively run it as non-sudo gives me
norty@norty-rog:~/hacknight/BAR-Devtools$ flatpak override info.beyondallreason.bar --filesystem=/home/norty/hacknight/BAR-Devtools
error: Failed to create file “/var/lib/flatpak/overrides/info.beyondallreason.bar.B8W0S3”: Permission denied
So I think having to use sudo here might just be "the nature of being installed at the system level rather than as the user level"
Sure, I could go back and un-install at the system level and re-install at the user level. That's possible. But it would not be something I want to explain to a developer who "just installed the game in the usual fashion in Ubuntu and wants to get up and running"
bb48755 to
af0ff5d
Compare
… are outside DEVTOOLS_DIR
…gesting pr-downloader has run once
…has never been run
…sandbox. Removed redundant comment
29921d3 to
2928bef
Compare
|
Cleaned up, rebased on master. Ready for re-review and merge. |
Background:
Due to the work done in setup to support allowing symlinks in flatpak, I found a case where now
just setup::initcorrectly configures the symlinks for overrides , butjust doctornow improperly reported they were still at risk of having an issue.Changes:
Added check to just doctor to confirm there's a recoil folder in the engine folder, as a proxy check for "the normal game downloader has run once" and the game is in a good state to attach BAR-Devtools to. (This supports CONTRIBUTING.md's
Refactored
just doctorto now use the same get/set permissions functions asjust setupThis had the side effect of possibly requiring sudo if we are diagnosing a system flatpak install, so I added a warning in
just doctorif it is about to prompt for sudo password.Manual testing performed:
BAR_DATA_DIRwhen just setup::init is run, but no folders are linked
when a folder is symlinked but flatpak permissions are not configured correctly
doctor has informational message that it may prompt for sudo access
doctor warns that at least one symlink is outside the flatpak sandbox
Closes #56
LLM Disclosure: diagnosed and partially-edited by Kimi 2.6, partially edited by myself . Guided and reviewed by myself. PR description written by myself. keithharvey offered some good review comments, and I have cleaned things up from there.