Dont show system session for user #1917

jelly · 2024-11-22T09:53:42Z

This throws an error when trying to create a system session vm as limited user.

martinpitt · 2024-11-27T08:30:07Z

test/check-machines-create

+        b.click("#create-new-vm")
+        b.wait_visible("#create-vm-dialog")
+        b.wait_not_present("#connection")


Do we have any other test which covers VM creation in unprivileged mode? If so, this would better fit there. If not, that's a major gap indeed, and this test should actually create the VM and check that it starts and boots fine (including the "wait for login prompt").

I believe this is confusingly what TestMachinesLifecycle does:

def _testBasic(self, user=None, superuser=True, expect_empty_list=False, run_pixel_tests=False): b = self.browser m = self.machine # We want no initial watchdog args = self.createVm("subVmTest1", os="linux2016") self.login_and_go("/machines", user=user, superuser=superuser)

So we can drop this test

mvollmer · 2024-12-02T08:40:45Z

src/components/create-vm-dialog/createVmDialog.jsx


        const detailsTab = (
            <>
+                {superuser.allowed &&


Hmm, I think people can create polkit rules to allow non-priv users access to the "system" connection. In fact cockpit-machines always connects to both "system" and "session" on initialization and tolerates AccessDenied errors for non-priv users.

So I think we should make a new global flag that says "can't access system connection", and use that instead of superuser.allowed in this PR. The place to set that flag is app.jsx:94, I guess.

Polkit is a disaster if you ask me, we have no way to detect it. But plumbing the state through should work. But looking at our code this is all a bit confusing I guess we look at systemSocketInactive?

This turns into a yakshave...

} else if ((superuser.allowed && systemSocketInactive) || loadingResources) {

So seems my solution is "correct" :) But ofcourse we can rework this.

Or not, not sure why this even exists.

Right, and with the move virt-xml, being able to connect to the system session vuia D-Bus does not mean that you can successfully run virt-xml.

Let's keep the superuser.allowed check.

Oh, you changed it already....

Or does virt-xml call the D-Bus API?

No, but polkit works inside th actual libvirt daemon as well, and I expect virt-install, virt-xml etc to be subject to the same polkit rules as D-Bus clients.

I'll research this properly.

It's complicated. There are two places to define policy, one for native policy for libvirtd that affects direct clients like virsh and virt-xml, and another one for the D-Bus API, which affects clients using that, like half of cockpit-machines.

This is annoying but it can be made to work, and we should not break people arbitrarily that do that.

mvollmer · 2025-09-03T13:11:13Z

Sorry, I have to look at this again. Luckily I learned a thing or two about c-machines in the mean time.

mvollmer

Thanks, I think this is the right thing to do.

Being able to connecting to the qemu:///system connection does indeed mean that virt-install and virt-xml are also able to work. For example, a limited user in the libvirt group can create and manage VMs on the system connection, using our mix of D-Bus, virt-install, and virt-xml.

Libvirt can use polkit internally (not via D-Bus) for fine grained access control. I have not tested it, but I expect that the D-Bus interface as well as virt-install and virt-xml are subject to it equally.

mvollmer · 2025-09-04T12:13:44Z

src/components/create-vm-dialog/createVmDialog.jsx


        const detailsTab = (
            <>
+                {this.props.systemSocketAvailable &&


Instead of completely hiding the connection selector, it should turn read-only and just say "User session" without any radio buttons.

Sure :) But I did something else woops just disabled both.

Hmm. I have pushed a version without radio buttons.

mvollmer · 2025-09-05T09:07:00Z

but I expect that the D-Bus interface as well as virt-install and virt-xml are subject to it equally.

Hah, being young and naive...

mvollmer

Thanks!

mvollmer · 2025-09-10T07:17:16Z

The tests don't want to go green... I'll have a look.

A limited access user cannot create a system session virtual machine so this would just throw an error after creation.

mvollmer · 2025-09-15T11:10:02Z

Rebased. Now looking at the tests...

cockpituous · 2025-09-15T17:11:41Z

src/components/common/machinesConnectionSelector.tsx

-                   id="connectionName-session"
-                   label={rephraseUI("connections", LIBVIRT_SESSION_CONNECTION)} />
+            { isDisabled
+                ? rephraseUI("connections", connectionName)


This added line is not executed by any test. Details

cockpituous · 2025-09-15T17:11:41Z

src/components/common/machinesConnectionSelector.tsx

+                ? rephraseUI("connections", connectionName)
+                : <>
+                    <Radio isChecked={connectionName === LIBVIRT_SYSTEM_CONNECTION}
+                        onChange={() => onValueChanged('connectionName', LIBVIRT_SYSTEM_CONNECTION)}


This added line is not executed by any test. Details

cockpituous · 2025-09-15T17:11:42Z

src/components/create-vm-dialog/createVmDialog.tsx

            vmName: '',
            suggestedVmName: '',
-            connectionName: LIBVIRT_SYSTEM_CONNECTION,
+            connectionName: this.props.systemSocketAvailable ? LIBVIRT_SYSTEM_CONNECTION : LIBVIRT_SESSION_CONNECTION,


This added line is not executed by any test. Details

jelly requested a review from mvollmer November 22, 2024 09:53

jelly force-pushed the dont-show-system-session-for-user branch from b3dcf68 to 54f83f0 Compare November 26, 2024 16:40

martinpitt reviewed Nov 27, 2024

View reviewed changes

mvollmer requested changes Dec 2, 2024

View reviewed changes

jelly force-pushed the dont-show-system-session-for-user branch from 54f83f0 to 3a96bad Compare December 3, 2024 09:34

jelly force-pushed the dont-show-system-session-for-user branch from 3a96bad to 09ec08f Compare December 16, 2024 12:55

mvollmer self-requested a review September 3, 2025 13:10

mvollmer self-assigned this Sep 3, 2025

mvollmer mentioned this pull request Sep 3, 2025

Can try to create a VM with system session when not administrator #1806

Open

mvollmer approved these changes Sep 4, 2025

View reviewed changes

mvollmer requested changes Sep 4, 2025

View reviewed changes

jelly force-pushed the dont-show-system-session-for-user branch 4 times, most recently from df841ed to 009b375 Compare September 4, 2025 16:23

mvollmer previously approved these changes Sep 5, 2025

View reviewed changes

jelly and others added 2 commits September 15, 2025 14:07

test: drop bogus parentheses

b1107d3

Don't show the system session option on limited access

9c2fbee

A limited access user cannot create a system session virtual machine so this would just throw an error after creation.

create: Omit radio buttons for disabled connection selector

76ac05d

mvollmer dismissed their stale review via 76ac05d September 15, 2025 11:14

mvollmer force-pushed the dont-show-system-session-for-user branch from 342d31e to 76ac05d Compare September 15, 2025 11:14

cockpituous reviewed Sep 15, 2025

View reviewed changes

Dont show system session for user #1917

Are you sure you want to change the base?

Dont show system session for user #1917

Uh oh!

Conversation

jelly commented Nov 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvollmer commented Sep 3, 2025

Uh oh!

mvollmer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvollmer Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mvollmer commented Sep 5, 2025

Uh oh!

mvollmer left a comment

Choose a reason for hiding this comment

Uh oh!

mvollmer commented Sep 10, 2025

Uh oh!

mvollmer commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mvollmer left a comment •

edited

Loading

mvollmer Sep 4, 2025 •

edited

Loading