Skip to content

Commit 4ef1964

Browse files
simonrey1simonrey1
authored
fix(extension/podman): extract + memoize user admin check (podman-desktop#14722)
* refactor(extension/podman): extract user admin check to Base Check Signed-off-by: Simon Rey <51708585+simonrey1@users.noreply.github.com> * fix(extension/podman): memoize user admin check Signed-off-by: Simon Rey <51708585+simonrey1@users.noreply.github.com> * chore(extensions/podman): update error message as user admin check can be called outside hyper-v Signed-off-by: Simon Rey <51708585+simonrey1@users.noreply.github.com> --------- Signed-off-by: Simon Rey <51708585+simonrey1@users.noreply.github.com>
1 parent e482c73 commit 4ef1964

File tree

7 files changed

+160
-49
lines changed

7 files changed

+160
-49
lines changed

‎extensions/podman/packages/extension/src/checks/windows/hyper-v-check.spec.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import { getPowerShellClient } from '../../utils/powershell';
2525
import { HyperVCheck } from './hyper-v-check';
2626
import type { HyperVInstalledCheck } from './hyper-v-installed-check';
2727
import type { HyperVRunningCheck } from './hyper-v-running-check';
28+
import type { UserAdminCheck } from './user-admin-check';
2829

2930
vi.mock(import('@podman-desktop/api'));
3031
vi.mock(import('../../utils/powershell'), () => ({
@@ -35,6 +36,7 @@ const mockTelemetryLogger = {} as TelemetryLogger;
3536
const isHyperVRunningCheck = { execute: vi.fn() } as unknown as HyperVRunningCheck;
3637
const isHyperVInstalledCheck = { execute: vi.fn() } as unknown as HyperVInstalledCheck;
3738
const isPodmanDesktopElevatedCheck = { execute: vi.fn() } as unknown as PodmanDesktopElevatedCheck;
39+
const userAdminCheck = { execute: vi.fn() } as unknown as UserAdminCheck;
3840

3941
const SUCCESSFUL_CHECK_RESULT: CheckResult = { successful: true };
4042
const FAILED_CHECK_RESULT: CheckResult = { successful: false };
@@ -57,21 +59,22 @@ beforeEach(() => {
5759
isHyperVRunningCheck,
5860
isHyperVInstalledCheck,
5961
isPodmanDesktopElevatedCheck,
62+
userAdminCheck,
6063
);
6164
});
6265

6366
test('expect HyperV preflight check return failure result if non admin user', async () => {
67+
vi.mocked(userAdminCheck.execute).mockResolvedValue({
68+
...FAILED_CHECK_RESULT,
69+
description: 'userAdminCheck',
70+
});
6471
const result = await hyperVCheck.execute();
6572
expect(result.successful).toBeFalsy();
66-
expect(result.description).equal('You must have administrative rights to run Hyper-V Podman machines');
67-
expect(result.docLinks?.[0].url).equal(
68-
'https://learn.microsoft.com/en-us/virtualization/hyper-v-on-windows/quick-start/enable-hyper-v',
69-
);
70-
expect(result.docLinks?.[0].title).equal('Hyper-V Manual Installation Steps');
73+
expect(result.description).equal('userAdminCheck');
7174
});
7275

7376
test('expect HyperV preflight check return failure result if Podman Desktop is not run with elevated privileges', async () => {
74-
vi.mocked(POWERSHELL_CLIENT.isUserAdmin).mockResolvedValue(true);
77+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
7578
vi.mocked(isPodmanDesktopElevatedCheck.execute).mockResolvedValue({
7679
...FAILED_CHECK_RESULT,
7780
description: 'isPodmanDesktopElevatedCheck',
@@ -84,7 +87,7 @@ test('expect HyperV preflight check return failure result if Podman Desktop is n
8487
});
8588

8689
test('expect HyperV preflight check return failure result if HyperV not installed', async () => {
87-
vi.mocked(POWERSHELL_CLIENT.isUserAdmin).mockResolvedValue(true);
90+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
8891
vi.mocked(isPodmanDesktopElevatedCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
8992
vi.mocked(isHyperVInstalledCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
9093
vi.mocked(isHyperVInstalledCheck.execute).mockResolvedValue({
@@ -98,7 +101,7 @@ test('expect HyperV preflight check return failure result if HyperV not installe
98101
});
99102

100103
test('expect HyperV preflight check return failure result if HyperV not running', async () => {
101-
vi.mocked(POWERSHELL_CLIENT.isUserAdmin).mockResolvedValue(true);
104+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
102105
vi.mocked(isPodmanDesktopElevatedCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
103106
vi.mocked(isHyperVInstalledCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
104107
vi.mocked(isHyperVRunningCheck.execute).mockResolvedValue({
@@ -112,7 +115,7 @@ test('expect HyperV preflight check return failure result if HyperV not running'
112115
});
113116

114117
test('expect HyperV preflight check return OK', async () => {
115-
vi.mocked(POWERSHELL_CLIENT.isUserAdmin).mockResolvedValue(true);
118+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
116119
vi.mocked(isPodmanDesktopElevatedCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
117120
vi.mocked(isHyperVInstalledCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
118121
vi.mocked(isHyperVRunningCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);

‎extensions/podman/packages/extension/src/checks/windows/hyper-v-check.ts‎

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,14 @@
1818
import type { CheckResult, TelemetryLogger } from '@podman-desktop/api';
1919
import { inject, injectable } from 'inversify';
2020

21-
import { HYPER_V_DOC_LINKS } from '/@/checks/windows/constants';
2221
import { TelemetryLoggerSymbol } from '/@/inject/symbols';
2322

2423
import { getPowerShellClient } from '../../utils/powershell';
2524
import { BaseCheck } from '../base-check';
2625
import { HyperVInstalledCheck } from './hyper-v-installed-check';
2726
import { HyperVRunningCheck } from './hyper-v-running-check';
2827
import { PodmanDesktopElevatedCheck } from './podman-desktop-elevated-check';
28+
import { UserAdminCheck } from './user-admin-check';
2929

3030
@injectable()
3131
export class HyperVCheck extends BaseCheck {
@@ -37,27 +37,20 @@ export class HyperVCheck extends BaseCheck {
3737
@inject(HyperVRunningCheck) private isHyperVRunningCheck: HyperVRunningCheck,
3838
@inject(HyperVInstalledCheck) private isHyperVInstalledCheck: HyperVInstalledCheck,
3939
@inject(PodmanDesktopElevatedCheck) private isPodmanDesktopElevatedCheck: PodmanDesktopElevatedCheck,
40+
@inject(UserAdminCheck) private userAdminCheck: UserAdminCheck,
4041
) {
4142
super();
4243
}
4344

44-
async isUserAdmin(): Promise<boolean> {
45-
const client = await getPowerShellClient(this.telemetryLogger);
46-
return client.isUserAdmin();
47-
}
48-
4945
async isHyperVInstalled(): Promise<boolean> {
5046
const client = await getPowerShellClient(this.telemetryLogger);
5147
return client.isHyperVInstalled();
5248
}
5349

5450
async execute(): Promise<CheckResult> {
55-
if (!(await this.isUserAdmin())) {
56-
return this.createFailureResult({
57-
description: 'You must have administrative rights to run Hyper-V Podman machines',
58-
docLinksDescription: 'Contact your Administrator to setup Hyper-V.',
59-
docLinks: HYPER_V_DOC_LINKS,
60-
});
51+
const userAdminResult = await this.userAdminCheck.execute();
52+
if (!userAdminResult.successful) {
53+
return userAdminResult;
6154
}
6255

6356
const podmanDesktopElevatedResult = await this.isPodmanDesktopElevatedCheck.execute();

‎extensions/podman/packages/extension/src/checks/windows/user-admin-check.spec.ts‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/**********************************************************************
2+
* Copyright (C) 2025 Red Hat, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*
16+
* SPDX-License-Identifier: Apache-2.0
17+
***********************************************************************/
18+
import type { TelemetryLogger } from '@podman-desktop/api';
19+
import { beforeEach, expect, test, vi } from 'vitest';
20+
21+
import type { PowerShellClient } from '/@/utils/powershell';
22+
import { getPowerShellClient } from '/@/utils/powershell';
23+
24+
import { UserAdminCheck } from './user-admin-check';
25+
26+
vi.mock(import('@podman-desktop/api'));
27+
vi.mock(import('/@/utils/powershell'), () => ({
28+
getPowerShellClient: vi.fn(),
29+
}));
30+
31+
const mockTelemetryLogger = {} as TelemetryLogger;
32+
33+
let userAdminCheck: UserAdminCheck;
34+
35+
const POWERSHELL_CLIENT: PowerShellClient = {
36+
isUserAdmin: vi.fn(),
37+
isHyperVInstalled: vi.fn(),
38+
isVirtualMachineAvailable: vi.fn(),
39+
isRunningElevated: vi.fn(),
40+
isHyperVRunning: vi.fn(),
41+
};
42+
43+
beforeEach(() => {
44+
vi.resetAllMocks();
45+
vi.mocked(getPowerShellClient).mockResolvedValue(POWERSHELL_CLIENT);
46+
userAdminCheck = new UserAdminCheck(mockTelemetryLogger);
47+
});
48+
49+
test('expect UserAdminCheck returns successful result if user is admin', async () => {
50+
vi.mocked(POWERSHELL_CLIENT.isUserAdmin).mockResolvedValue(true);
51+
const result = await userAdminCheck.execute();
52+
expect(result.successful).toBeTruthy();
53+
expect(result.description).toBeUndefined();
54+
});
55+
56+
test('expect UserAdminCheck returns failure result if user is not admin', async () => {
57+
vi.mocked(POWERSHELL_CLIENT.isUserAdmin).mockResolvedValue(false);
58+
const result = await userAdminCheck.execute();
59+
expect(result.successful).toBeFalsy();
60+
expect(result.description).equal('You must have administrative rights');
61+
expect(result.docLinks?.[0]).toBeUndefined();
62+
});

‎extensions/podman/packages/extension/src/checks/windows/user-admin-check.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/**********************************************************************
2+
* Copyright (C) 2025 Red Hat, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*
16+
* SPDX-License-Identifier: Apache-2.0
17+
***********************************************************************/
18+
import type { CheckResult, TelemetryLogger } from '@podman-desktop/api';
19+
import { inject, injectable } from 'inversify';
20+
21+
import { MemoizedBaseCheck } from '/@/checks/memoized-base-check';
22+
import { TelemetryLoggerSymbol } from '/@/inject/symbols';
23+
import { getPowerShellClient } from '/@/utils/powershell';
24+
25+
@injectable()
26+
export class UserAdminCheck extends MemoizedBaseCheck {
27+
title = 'User is Administrator';
28+
29+
constructor(
30+
@inject(TelemetryLoggerSymbol)
31+
private telemetryLogger: TelemetryLogger,
32+
) {
33+
super();
34+
}
35+
36+
async isUserAdmin(): Promise<boolean> {
37+
const client = await getPowerShellClient(this.telemetryLogger);
38+
return client.isUserAdmin();
39+
}
40+
41+
async executeImpl(): Promise<CheckResult> {
42+
if (await this.isUserAdmin()) {
43+
return this.createSuccessfulResult();
44+
} else {
45+
return this.createFailureResult({
46+
description: 'You must have administrative rights',
47+
});
48+
}
49+
}
50+
}

‎extensions/podman/packages/extension/src/checks/windows/wsl2-check.spec.ts‎

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,17 @@
1616
* SPDX-License-Identifier: Apache-2.0
1717
***********************************************************************/
1818

19-
import type { ExtensionContext, RunResult, TelemetryLogger } from '@podman-desktop/api';
19+
import type { CheckResult, ExtensionContext, RunResult } from '@podman-desktop/api';
2020
import { commands, process } from '@podman-desktop/api';
2121
import { beforeEach, expect, test, vi } from 'vitest';
2222

23+
import type { UserAdminCheck } from '/@/checks/windows/user-admin-check';
24+
2325
import { normalizeWSLOutput } from '../../utils/util';
2426
import { WSL2Check } from './wsl2-check';
2527

28+
const userAdminCheck = { execute: vi.fn() } as unknown as UserAdminCheck;
29+
2630
vi.mock('@podman-desktop/api', () => ({
2731
process: {
2832
exec: vi.fn(),
@@ -56,14 +60,18 @@ const extensionContext = {
5660
subscriptions: [],
5761
} as unknown as ExtensionContext;
5862

59-
const mockTelemetryLogger = {} as TelemetryLogger;
63+
const SUCCESSFUL_CHECK_RESULT: CheckResult = { successful: true };
64+
65+
let winWSLCheck: WSL2Check;
6066

6167
beforeEach(() => {
6268
vi.resetAllMocks();
6369
// reset array of subscriptions
6470
extensionContext.subscriptions.length = 0;
6571

6672
vi.mocked(normalizeWSLOutput).mockImplementation((s: string) => s);
73+
74+
winWSLCheck = new WSL2Check(userAdminCheck, extensionContext);
6775
});
6876

6977
test('expect winWSL2 preflight check return successful result if the machine has WSL2 installed and do not need to reboot', async () => {
@@ -82,8 +90,8 @@ test('expect winWSL2 preflight check return successful result if the machine has
8290
});
8391
}
8492
});
93+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
8594

86-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
8795
const result = await winWSLCheck.execute();
8896
expect(result.successful).toBeTruthy();
8997
});
@@ -117,8 +125,7 @@ test('expect winWSL2 preflight check return failure result if the machine has WS
117125
});
118126
}
119127
});
120-
121-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
128+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
122129
const result = await winWSLCheck.execute();
123130
expect(result.description).equal(
124131
'WSL2 seems to be installed but the system needs to be restarted so the changes can take effect.',
@@ -158,8 +165,8 @@ test('expect winWSL2 preflight check return successful result if the machine has
158165
});
159166
}
160167
});
168+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
161169

162-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
163170
const result = await winWSLCheck.execute();
164171
expect(result.successful).toBeTruthy();
165172
});
@@ -193,8 +200,7 @@ test('expect winWSL2 preflight check return successful result if the machine has
193200
});
194201
}
195202
});
196-
197-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
203+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
198204
const result = await winWSLCheck.execute();
199205
expect(result.successful).toBeTruthy();
200206
});
@@ -216,7 +222,7 @@ test('expect winWSL2 preflight check return failure result if user do not have w
216222
}
217223
});
218224

219-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
225+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
220226
const result = await winWSLCheck.execute();
221227
expect(result.description).equal('WSL2 is not installed.');
222228
expect(result.docLinksDescription).equal(`Call 'wsl --install --no-distribution' in a terminal.`);
@@ -240,8 +246,8 @@ test('expect winWSL2 preflight check return failure result if user do not have w
240246
});
241247
}
242248
});
249+
vi.mocked(userAdminCheck.execute).mockResolvedValue({ successful: false });
243250

244-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
245251
const result = await winWSLCheck.execute();
246252
expect(result.description).equal('WSL2 is not installed or you do not have permissions to run WSL2.');
247253
expect(result.docLinksDescription).equal('Contact your Administrator to setup WSL2.');
@@ -261,8 +267,8 @@ test('expect winWSL2 preflight check return failure result if it fails when chec
261267
throw new Error();
262268
}
263269
});
270+
vi.mocked(userAdminCheck.execute).mockRejectedValue({ successful: false });
264271

265-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
266272
const result = await winWSLCheck.execute();
267273
expect(result.description).equal('Could not detect WSL2');
268274
expect(result.docLinks?.[0].url).equal('https://learn.microsoft.com/en-us/windows/wsl/install');
@@ -271,7 +277,6 @@ test('expect winWSL2 preflight check return failure result if it fails when chec
271277

272278
test('expect winWSL2 init to register WSLInstall command', async () => {
273279
const registerCommandMock = vi.mocked(commands.registerCommand);
274-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
275280
await winWSLCheck.init?.();
276281
expect(registerCommandMock).toBeCalledWith('podman.onboarding.installWSL', expect.any(Function));
277282
});
@@ -282,7 +287,6 @@ test('expect winWSL2 command to be registered as disposable', async () => {
282287
dispose: vi.fn(),
283288
};
284289
registerCommandMock.mockReturnValue(disposableMock);
285-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
286290
await winWSLCheck.init?.();
287291
expect(registerCommandMock).toBeCalledWith('podman.onboarding.installWSL', expect.any(Function));
288292

@@ -307,12 +311,13 @@ test('expect winWSL2 check to be memoized', async () => {
307311
});
308312
}
309313
});
310-
311-
const winWSLCheck = new WSL2Check(mockTelemetryLogger, extensionContext);
314+
vi.mocked(userAdminCheck.execute).mockResolvedValue(SUCCESSFUL_CHECK_RESULT);
312315

313316
await winWSLCheck.execute();
314-
expect(process.exec).toHaveBeenCalledTimes(3);
317+
expect(userAdminCheck.execute).toHaveBeenCalledTimes(1);
318+
expect(process.exec).toHaveBeenCalledTimes(2);
315319

316320
await winWSLCheck.execute();
317-
expect(process.exec).toHaveBeenCalledTimes(3);
321+
expect(userAdminCheck.execute).toHaveBeenCalledTimes(1);
322+
expect(process.exec).toHaveBeenCalledTimes(2);
318323
});

0 commit comments

Comments
 (0)