Pass XDG_ACTIVATION_TOKEN to apps launched via `launch_app_env` #3703

tarek-y-ismail · 2024-12-18T17:54:05Z

No description provided.

src/server/frontend_wayland/xdg_activation_v1.cpp

AlanGriffiths

Drilling into the frontend implementation from the Server abstraction is placing logic in the wrong place, fragile when that implementation changes, and probably won't work.

We first need to break out the token issue & validation logic from xdg-activation so that it can meet the new requirements. (With this scenario there is no "requester" to check still has focus)

src/server/symbols.map

src/server/server.cpp

src/miral/launch_app.h

src/server/server.cpp

src/include/server/mir/shell/token_authority.h

src/server/frontend_wayland/xdg_activation_v1.cpp

tarek-y-ismail · 2024-12-19T18:04:38Z

src/server/server.cpp

+{
+    if (auto const config = self->server_config)
+    {
+        return config->the_token_authority()->issue_token({});


Not sure if this is better or I should add an overload with no parameters

src/include/server/mir/shell/token_authority.h

src/server/frontend_wayland/xdg_activation_v1.cpp

src/server/frontend_wayland/xdg_activation_v1.h

src/server/symbols.map

AlanGriffiths

Looks like a better architecture. But I've not yet looked closely enough to decide how tokens supplied via launch_app_env() will be handled by xdg-activation

src/include/server/mir/shell/token_authority.h

src/server/symbols.map

RAOF

Broadly, I think the TokenAuthority is kinda half way there. It's not actually an authority - code can't use it to check if a Token is valid, and that's tracked separately by XdgActivationV1. This is also why there's spooky action-at-a-distance in XdgActivationV1::invalidate_*() - you're maintaining a second list of valid tokens, mutating XdgActivationV1::pending_tokens by calling token_authority->revoke_token(...).

I think this would be simpler if:
a) The TokenAuthority was the... authority for whether a token was valid or not, and
b) Token was treated more as an opaque... token, rather than immediately decaying into a std::string and all the interfaces taking that.

src/miral/launch_app.cpp

src/server/frontend_wayland/xdg_activation_v1.cpp

src/include/server/mir/shell/token_authority.h

RAOF

Yeah, that's cleaner. On further thought, I'm not sure we sensibly can decouple XdgActivation and the TokenAuthority further.

src/miral/launch_app.cpp

Closes #2586. Adds a new constructor to `FloatingWindowManagerPolicy` to control focus stealing prevention. This in a nutshell stops new windows from being focused and raised. When used with xdg-activation-v1, this improves security as external actors can't just steal focus by opening a new window (in addition to the niceties xdg-activation-v1 adds to usability). TODO: - [ ] ~Fix window offsetting. This is built off the position of the focused window, so all windows after the second one open in the same (x,y) position.~ Split off to #3695 - [ ] ~Fix windows closing in the order of opening instead of front to back. see #3693 (comment) Moved to #3694 - [ ] ~Make sure Xwayland applications work properly. My focus has been on Wayland applications so far.~ Xwayland doesn't seem to support xdg-activation at the moment? - [x] Alt + tab predictably broken :/ Have to focus other applications before they work with alt + tab - [x] Decorations are not pushed behind the focused window - [ ] ~Need some way to focus applications launched via Mir (Ctrl-Alt+t/T for example)~ Will be in its own PR (#3703)

tarek-y-ismail · 2025-01-22T08:02:31Z

TIL that github's conflict editor merges (there's probably a rebase option that I missed)

AlanGriffiths

/usr/bin/ld: unable to find version dependency `MIR_SERVER_INTERNAL_2.19'
/usr/bin/ld: duplicate version tag `MIR_SERVER_INTERNAL_2.20'
collect2: error: ld returned 1 exit status

src/server/symbols.map is ill formed

AlanGriffiths · 2025-01-22T10:19:43Z

(With the symbols.map fixed)

From Gnome:

miral-app --focus-stealing-prevention true
Ctrl-Alt-X to launch xterm
Ctrl-Alt-Shift-T to launch gnome-terminal

Expect: gnome-terminal gets focus and raised
Actual: xterm remains on top with focus

tarek-y-ismail · 2025-01-22T11:42:41Z

Hopefully both issues should be fixed now (facepalm). I'm not very sure about the symbol stuff. I should really spend some time to understand it

I recommend testing with firefox and kgx. gnome-terminal doesn't seem to request activation.

Edit: To save you some frustration, here's a list of apps that request activation and others that don't:

Applications that request activation:

kgx
firefox
nautilus
baobab

Applications that DO NOT request activation:

gnome-terminal
qterminal
weston-terminal
bomber
qtcreator (seems to request activation with random tokens for dropdowns?)
glmark2-wayland
kate
pluma
audacity

AlanGriffiths

I think this is the usecase the PR is intended to address:

From Gnome:
1. `miral-app --focus-stealing-prevention true`

2. `Ctrl-Alt-X` to launch `xterm`

3. `Ctrl-Alt-Shift-T` to launch `gnome-terminal`
Expect: gnome-terminal gets focus and raised
Actual: xterm remains on top with focus

Same with:

miral-app --focus-stealing-prevention true --shell-terminal-emulator=xfce4-terminal
miral-app --focus-stealing-prevention true --shell-terminal-emulator=qterminal

So it isn't just gnome-terminal being weird

AlanGriffiths · 2025-01-22T11:52:36Z

Applications that DO NOT request activation:
* gnome-terminal

gnome terminal does request activation:

[2025-01-22 11:46:17.010620] < - ERROR - > mirserver: XdgActivationV1::activate invalid token: gnome-shell/CLion/962086-6-Octopull-Framework16_TIME194122238

But that's not the token in XDG_ACTIVATION_TOKEN

mattkae

A question about return types, but it is working well for me AFAICT!

src/server/frontend_wayland/xdg_activation_v1.cpp

type

mattkae

I am happy now, but I want to leave it up to @AlanGriffiths for any final thoughts and words!

AlanGriffiths

More nits. But I think this does what it claims

src/server/shell/token_authority.cpp

src/server/shell/CMakeLists.txt

src/server/shell/token_authority.cpp

AlanGriffiths · 2025-01-28T15:11:15Z

src/server/shell/token_authority.cpp

+}
+
+msh::TokenAuthority::TokenAuthority(std::shared_ptr<MainLoop>&& main_loop) :
+    main_loop{main_loop}


Suggested change

main_loop{main_loop}

main_loop{std::move(main_loop)}

Borks the tests because the main loop then points to the void. Not sure why miral-app worked fine though...

Then the tests are wrong:

struct TestTokenAuthority : testing::Test { TestTokenAuthority() : clock{std::make_shared<mir::time::SteadyClock>()}, main_loop{std::make_shared<mir::GLibMainLoop>(clock)}, token_authority{std::forward<std::shared_ptr<mir::MainLoop>>(main_loop)}, main_loop_thread{ [this]() { main_loop->stop(); }, [this]() { main_loop->run(); }} { } std::shared_ptr<mir::time::SteadyClock> clock; std::shared_ptr<mir::MainLoop> main_loop; mir::shell::TokenAuthority token_authority; mir::test::AutoUnblockThread main_loop_thread; };

You use "perfect forwarding" to move from main_loop and transfer ownership. (If you made members const the compiler would know you don't want this)

D'oh! Const correctness bites me again...

AlanGriffiths

Enough with the nits!

obsolete

- Remove comment in `get_token_for_string` in favor of explaining things in `generate_token` - Reduce scope of variables in `revoke_token` to the relevant if statement - Include `token_authority.h` in `SHELL_SOURCES` - Use seconds instead of milliseconds for token timeout since milliseconds weren't offering anything extra (other than the extra zeroes) Co-authored-by: Alan Griffiths <[email protected]>

…en-launching-apps

tarek-y-ismail · 2025-01-28T15:27:17Z

Had a small accident where I merged the testing PR and followed that by a force push without pulling first. Should alright now

AlanGriffiths

A new last nit

AlanGriffiths · 2025-01-28T15:57:25Z

src/server/shell/token_authority.cpp

+}
+
+msh::TokenAuthority::TokenAuthority(std::shared_ptr<MainLoop>&& main_loop) :
+    main_loop{main_loop}


Then the tests are wrong:

struct TestTokenAuthority : testing::Test { TestTokenAuthority() : clock{std::make_shared<mir::time::SteadyClock>()}, main_loop{std::make_shared<mir::GLibMainLoop>(clock)}, token_authority{std::forward<std::shared_ptr<mir::MainLoop>>(main_loop)}, main_loop_thread{ [this]() { main_loop->stop(); }, [this]() { main_loop->run(); }} { } std::shared_ptr<mir::time::SteadyClock> clock; std::shared_ptr<mir::MainLoop> main_loop; mir::shell::TokenAuthority token_authority; mir::test::AutoUnblockThread main_loop_thread; };

You use "perfect forwarding" to move from main_loop and transfer ownership. (If you made members const the compiler would know you don't want this)

src/server/shell/token_authority.cpp

tests/unit-tests/shell/test_token_authority.cpp

tarek-y-ismail self-assigned this Dec 18, 2024

tarek-y-ismail commented Dec 18, 2024

View reviewed changes

src/server/frontend_wayland/xdg_activation_v1.cpp Outdated Show resolved Hide resolved

tarek-y-ismail mentioned this pull request Dec 18, 2024

Add focus stealing prevention to MinimalWindowManager #3693

Merged

6 tasks

tarek-y-ismail requested a review from mattkae December 18, 2024 17:57

AlanGriffiths requested changes Dec 19, 2024

View reviewed changes

src/server/symbols.map Outdated Show resolved Hide resolved

src/server/server.cpp Outdated Show resolved Hide resolved

src/miral/launch_app.h Outdated Show resolved Hide resolved

src/server/server.cpp Outdated Show resolved Hide resolved

tarek-y-ismail commented Dec 19, 2024

View reviewed changes

src/include/server/mir/shell/token_authority.h Show resolved Hide resolved

tarek-y-ismail commented Dec 19, 2024

View reviewed changes

src/include/server/mir/shell/token_authority.h Outdated Show resolved Hide resolved

tarek-y-ismail commented Dec 19, 2024

View reviewed changes

src/server/frontend_wayland/xdg_activation_v1.cpp Outdated Show resolved Hide resolved

tarek-y-ismail commented Dec 19, 2024

View reviewed changes

src/include/server/mir/shell/token_authority.h Outdated Show resolved Hide resolved

tarek-y-ismail commented Dec 19, 2024

View reviewed changes

src/server/frontend_wayland/xdg_activation_v1.cpp Outdated Show resolved Hide resolved

tarek-y-ismail force-pushed the pass-xdg-activation-token-when-launching-apps branch from 852ccf5 to 6994bc1 Compare December 20, 2024 13:28

tarek-y-ismail commented Dec 20, 2024

View reviewed changes

src/server/frontend_wayland/xdg_activation_v1.h Outdated Show resolved Hide resolved

tarek-y-ismail commented Dec 20, 2024

View reviewed changes

src/server/symbols.map Outdated Show resolved Hide resolved

tarek-y-ismail marked this pull request as ready for review December 20, 2024 14:02

tarek-y-ismail requested a review from a team as a code owner December 20, 2024 14:02

tarek-y-ismail requested a review from AlanGriffiths December 20, 2024 14:54

AlanGriffiths requested changes Dec 20, 2024

View reviewed changes

src/include/server/mir/shell/token_authority.h Outdated Show resolved Hide resolved

src/server/symbols.map Show resolved Hide resolved

tarek-y-ismail requested a review from AlanGriffiths January 6, 2025 10:58

RAOF previously requested changes Jan 7, 2025

View reviewed changes

RAOF reviewed Jan 9, 2025

View reviewed changes

src/miral/launch_app.cpp Outdated Show resolved Hide resolved

tarek-y-ismail requested a review from RAOF January 9, 2025 08:22

AlanGriffiths requested changes Jan 22, 2025

View reviewed changes

tarek-y-ismail force-pushed the pass-xdg-activation-token-when-launching-apps branch from a96594e to 1d6a14f Compare January 22, 2025 10:50

AlanGriffiths requested changes Jan 22, 2025

View reviewed changes

tarek-y-ismail force-pushed the pass-xdg-activation-token-when-launching-apps branch from 4399ca1 to e4d92be Compare January 27, 2025 09:07

tarek-y-ismail requested a review from mattkae January 27, 2025 09:07

tarek-y-ismail added 2 commits January 27, 2025 14:10

Copy the main loop shared ptr instead of moving out of it

167f78a

Add a basic test suite for TokenAuthority

11b3fe0

mattkae requested changes Jan 27, 2025

View reviewed changes

src/server/frontend_wayland/xdg_activation_v1.cpp Outdated Show resolved Hide resolved

Remove redundant const from XdgActivationV1::create_token's return

d273459

type

tarek-y-ismail requested a review from mattkae January 27, 2025 15:17

mattkae approved these changes Jan 27, 2025

View reviewed changes

AlanGriffiths requested changes Jan 28, 2025

View reviewed changes

src/server/shell/token_authority.cpp Outdated Show resolved Hide resolved

src/server/shell/token_authority.cpp Outdated Show resolved Hide resolved

src/server/shell/token_authority.cpp Outdated Show resolved Hide resolved

src/server/shell/CMakeLists.txt Outdated Show resolved Hide resolved

tarek-y-ismail requested a review from AlanGriffiths January 28, 2025 14:55

AlanGriffiths reviewed Jan 28, 2025

View reviewed changes

src/server/shell/token_authority.cpp Outdated Show resolved Hide resolved

AlanGriffiths reviewed Jan 28, 2025

View reviewed changes

AlanGriffiths approved these changes Jan 28, 2025

View reviewed changes

AlanGriffiths enabled auto-merge January 28, 2025 15:14

tarek-y-ismail force-pushed the pass-xdg-activation-token-when-launching-apps branch from 4d9ba32 to 94556f0 Compare January 28, 2025 15:19

Merge branch 'test-token-authority' into pass-xdg-activation-token-wh…

6b06163

…en-launching-apps

AlanGriffiths disabled auto-merge January 28, 2025 15:58

AlanGriffiths requested changes Jan 28, 2025

View reviewed changes

Fix faulty main loop moving due to missing const correctness

45c2f00

tarek-y-ismail requested a review from AlanGriffiths January 28, 2025 17:01

AlanGriffiths reviewed Jan 28, 2025

View reviewed changes

tests/unit-tests/shell/test_token_authority.cpp Show resolved Hide resolved

tarek-y-ismail requested a review from AlanGriffiths January 28, 2025 17:35

AlanGriffiths approved these changes Jan 28, 2025

View reviewed changes

tarek-y-ismail added this pull request to the merge queue Jan 28, 2025

Merged via the queue into main with commit 3a8c5a9 Jan 28, 2025
36 of 39 checks passed

tarek-y-ismail deleted the pass-xdg-activation-token-when-launching-apps branch January 28, 2025 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass XDG_ACTIVATION_TOKEN to apps launched via `launch_app_env` #3703

Pass XDG_ACTIVATION_TOKEN to apps launched via `launch_app_env` #3703

tarek-y-ismail commented Dec 18, 2024

AlanGriffiths left a comment

tarek-y-ismail Dec 19, 2024

AlanGriffiths left a comment

RAOF left a comment

RAOF left a comment

tarek-y-ismail commented Jan 22, 2025

AlanGriffiths left a comment

AlanGriffiths commented Jan 22, 2025

tarek-y-ismail commented Jan 22, 2025 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths commented Jan 22, 2025

mattkae left a comment

mattkae left a comment

AlanGriffiths left a comment

AlanGriffiths Jan 28, 2025

tarek-y-ismail Jan 28, 2025

AlanGriffiths Jan 28, 2025

tarek-y-ismail Jan 28, 2025

AlanGriffiths left a comment

tarek-y-ismail commented Jan 28, 2025

AlanGriffiths left a comment

AlanGriffiths Jan 28, 2025

Pass XDG_ACTIVATION_TOKEN to apps launched via launch_app_env #3703

Pass XDG_ACTIVATION_TOKEN to apps launched via launch_app_env #3703

Conversation

tarek-y-ismail commented Dec 18, 2024

AlanGriffiths left a comment

Choose a reason for hiding this comment

tarek-y-ismail Dec 19, 2024

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

RAOF left a comment

Choose a reason for hiding this comment

RAOF left a comment

Choose a reason for hiding this comment

tarek-y-ismail commented Jan 22, 2025

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths commented Jan 22, 2025

tarek-y-ismail commented Jan 22, 2025 • edited Loading

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths commented Jan 22, 2025

mattkae left a comment

Choose a reason for hiding this comment

mattkae left a comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Jan 28, 2025

Choose a reason for hiding this comment

tarek-y-ismail Jan 28, 2025

Choose a reason for hiding this comment

AlanGriffiths Jan 28, 2025

Choose a reason for hiding this comment

tarek-y-ismail Jan 28, 2025

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

tarek-y-ismail commented Jan 28, 2025

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths Jan 28, 2025

Choose a reason for hiding this comment

Pass XDG_ACTIVATION_TOKEN to apps launched via `launch_app_env` #3703

Pass XDG_ACTIVATION_TOKEN to apps launched via `launch_app_env` #3703

tarek-y-ismail commented Jan 22, 2025 •

edited

Loading