From c7ec7ea14b0ad165a0b505ccd92b77c4349cf2a2 Mon Sep 17 00:00:00 2001 From: Karthik Nadig Date: Tue, 29 Sep 2020 12:26:36 -0700 Subject: [PATCH] More tests --- .../common/environmentIdentifier.ts | 2 +- .../pythonEnvironments/common/windowsUtils.ts | 76 ----------------- .../locators/services/windowsStoreLocator.ts | 83 +++++++++++++++++-- .../locators/windowsStoreLocator.unit.test.ts | 17 +++- 4 files changed, 92 insertions(+), 86 deletions(-) diff --git a/src/client/pythonEnvironments/common/environmentIdentifier.ts b/src/client/pythonEnvironments/common/environmentIdentifier.ts index 33af08a24b209..3340cb01bdaf2 100644 --- a/src/client/pythonEnvironments/common/environmentIdentifier.ts +++ b/src/client/pythonEnvironments/common/environmentIdentifier.ts @@ -7,8 +7,8 @@ import { isPyenvEnvironment } from '../discovery/locators/services/pyenvLocator' import { isVenvEnvironment } from '../discovery/locators/services/venvLocator'; import { isVirtualenvEnvironment } from '../discovery/locators/services/virtualenvLocator'; import { isVirtualenvwrapperEnvironment } from '../discovery/locators/services/virtualenvwrapperLocator'; +import { isWindowsStoreEnvironment } from '../discovery/locators/services/windowsStoreLocator'; import { EnvironmentType } from '../info'; -import { isWindowsStoreEnvironment } from './windowsUtils'; /** * Gets a prioritized list of environment types for identification. diff --git a/src/client/pythonEnvironments/common/windowsUtils.ts b/src/client/pythonEnvironments/common/windowsUtils.ts index 48eb418db68e2..d9be7d78fdf91 100644 --- a/src/client/pythonEnvironments/common/windowsUtils.ts +++ b/src/client/pythonEnvironments/common/windowsUtils.ts @@ -2,8 +2,6 @@ // Licensed under the MIT License. import * as path from 'path'; -import { traceWarning } from '../../common/logger'; -import { getEnvironmentVariable } from '../../common/utils/platform'; /** * Checks if a given path ends with python*.exe @@ -22,77 +20,3 @@ export function isWindowsPythonExe(interpreterPath:string): boolean { return windowsPythonExes.test(path.basename(interpreterPath)); } - -/** - * Gets path to the Windows Apps directory. - * @returns {string} : Returns path to the Windows Apps directory under - * `%LOCALAPPDATA%/Microsoft/WindowsApps`. - */ -export function getWindowsStoreAppsRoot(): string { - const localAppData = getEnvironmentVariable('LOCALAPPDATA') || ''; - return path.join(localAppData, 'Microsoft', 'WindowsApps'); -} - -/** - * Checks if a given path is under the forbidden windows store directory. - * @param {string} interpreterPath : Absolute path to the python interpreter. - * @returns {boolean} : Returns true if `interpreterPath` is under - * `%ProgramFiles%/WindowsApps`. - */ -export function isForbiddenStorePath(interpreterPath:string):boolean { - const programFilesStorePath = path - .join(getEnvironmentVariable('ProgramFiles') || 'Program Files', 'WindowsApps') - .normalize() - .toUpperCase(); - return path.normalize(interpreterPath).toUpperCase().includes(programFilesStorePath); -} - -/** - * Checks if the given interpreter belongs to Windows Store Python environment. - * @param interpreterPath: Absolute path to any python interpreter. - * - * Remarks: - * 1. Checking if the path includes `Microsoft\WindowsApps`, `Program Files\WindowsApps`, is - * NOT enough. In WSL, `/mnt/c/users/user/AppData/Local/Microsoft/WindowsApps` is available as a search - * path. It is possible to get a false positive for that path. So the comparison should check if the - * absolute path to 'WindowsApps' directory is present in the given interpreter path. The WSL path to - * 'WindowsApps' is not a valid path to access, Windows Store Python. - * - * 2. 'startsWith' comparison may not be right, user can provide '\\?\C:\users\' style long paths in windows. - * - * 3. A limitation of the checks here is that they don't handle 8.3 style windows paths. - * For example, - * `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE` - * is the shortened form of - * `C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe` - * - * The correct way to compare these would be to always convert given paths to long path (or to short path). - * For either approach to work correctly you need actual file to exist, and accessible from the user's - * account. - * - * To convert to short path without using N-API in node would be to use this command. This is very expensive: - * `> cmd /c for %A in ("C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe") do @echo %~sA` - * The above command will print out this: - * `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE` - * - * If we go down the N-API route, use node-ffi and either call GetShortPathNameW or GetLongPathNameW from, - * Kernel32 to convert between the two path variants. - * - */ -export async function isWindowsStoreEnvironment(interpreterPath: string): Promise { - const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase(); - const localAppDataStorePath = path - .normalize(getWindowsStoreAppsRoot()) - .toUpperCase(); - if (pythonPathToCompare.includes(localAppDataStorePath)) { - return true; - } - - // Program Files store path is a forbidden path. Only admins and system has access this path. - // We should never have to look at this path or even execute python from this path. - if (isForbiddenStorePath(pythonPathToCompare)) { - traceWarning('isWindowsStoreEnvironment called with Program Files store path.'); - return true; - } - return false; -} diff --git a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts index 27b01747ad514..fcf77b6d371ad 100644 --- a/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts +++ b/src/client/pythonEnvironments/discovery/locators/services/windowsStoreLocator.ts @@ -4,7 +4,8 @@ import * as fsapi from 'fs-extra'; import * as path from 'path'; import { Event, EventEmitter } from 'vscode'; -import { Architecture } from '../../../../common/utils/platform'; +import { traceWarning } from '../../../../common/logger'; +import { Architecture, getEnvironmentVariable } from '../../../../common/utils/platform'; import { PythonEnvInfo, PythonEnvKind, PythonReleaseLevel, PythonVersion, } from '../../../base/info'; @@ -12,9 +13,83 @@ import { parseVersion } from '../../../base/info/pythonVersion'; import { ILocator, IPythonEnvsIterator } from '../../../base/locator'; import { PythonEnvsChangedEvent } from '../../../base/watcher'; import { getFileInfo } from '../../../common/externalDependencies'; -import { getWindowsStoreAppsRoot, isWindowsPythonExe, isWindowsStoreEnvironment } from '../../../common/windowsUtils'; +import { isWindowsPythonExe } from '../../../common/windowsUtils'; import { IEnvironmentInfoService } from '../../../info/environmentInfoService'; +/** + * Gets path to the Windows Apps directory. + * @returns {string} : Returns path to the Windows Apps directory under + * `%LOCALAPPDATA%/Microsoft/WindowsApps`. + */ +export function getWindowsStoreAppsRoot(): string { + const localAppData = getEnvironmentVariable('LOCALAPPDATA') || ''; + return path.join(localAppData, 'Microsoft', 'WindowsApps'); +} + +/** + * Checks if a given path is under the forbidden windows store directory. + * @param {string} interpreterPath : Absolute path to the python interpreter. + * @returns {boolean} : Returns true if `interpreterPath` is under + * `%ProgramFiles%/WindowsApps`. + */ +export function isForbiddenStorePath(interpreterPath:string):boolean { + const programFilesStorePath = path + .join(getEnvironmentVariable('ProgramFiles') || 'Program Files', 'WindowsApps') + .normalize() + .toUpperCase(); + return path.normalize(interpreterPath).toUpperCase().includes(programFilesStorePath); +} + +/** + * Checks if the given interpreter belongs to Windows Store Python environment. + * @param interpreterPath: Absolute path to any python interpreter. + * + * Remarks: + * 1. Checking if the path includes `Microsoft\WindowsApps`, `Program Files\WindowsApps`, is + * NOT enough. In WSL, `/mnt/c/users/user/AppData/Local/Microsoft/WindowsApps` is available as a search + * path. It is possible to get a false positive for that path. So the comparison should check if the + * absolute path to 'WindowsApps' directory is present in the given interpreter path. The WSL path to + * 'WindowsApps' is not a valid path to access, Windows Store Python. + * + * 2. 'startsWith' comparison may not be right, user can provide '\\?\C:\users\' style long paths in windows. + * + * 3. A limitation of the checks here is that they don't handle 8.3 style windows paths. + * For example, + * `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE` + * is the shortened form of + * `C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe` + * + * The correct way to compare these would be to always convert given paths to long path (or to short path). + * For either approach to work correctly you need actual file to exist, and accessible from the user's + * account. + * + * To convert to short path without using N-API in node would be to use this command. This is very expensive: + * `> cmd /c for %A in ("C:\Users\USER\AppData\Local\Microsoft\WindowsApps\python3.7.exe") do @echo %~sA` + * The above command will print out this: + * `C:\Users\USER\AppData\Local\MICROS~1\WINDOW~1\PYTHON~2.EXE` + * + * If we go down the N-API route, use node-ffi and either call GetShortPathNameW or GetLongPathNameW from, + * Kernel32 to convert between the two path variants. + * + */ +export async function isWindowsStoreEnvironment(interpreterPath: string): Promise { + const pythonPathToCompare = path.normalize(interpreterPath).toUpperCase(); + const localAppDataStorePath = path + .normalize(getWindowsStoreAppsRoot()) + .toUpperCase(); + if (pythonPathToCompare.includes(localAppDataStorePath)) { + return true; + } + + // Program Files store path is a forbidden path. Only admins and system has access this path. + // We should never have to look at this path or even execute python from this path. + if (isForbiddenStorePath(pythonPathToCompare)) { + traceWarning('isWindowsStoreEnvironment called with Program Files store path.'); + return true; + } + return false; +} + /** * Gets paths to the Python executable under Windows Store apps. * @returns: Returns python*.exe for the windows store app root directory. @@ -59,7 +134,7 @@ export class WindowsStoreLocator implements ILocator { public async resolveEnv(env: string | PythonEnvInfo): Promise { const executablePath = typeof env === 'string' ? env : env.executable.filename; - if (isWindowsStoreEnvironment(executablePath)) { + if (await isWindowsStoreEnvironment(executablePath)) { const interpreterInfo = await this.envService.getEnvironmentInfo(executablePath); if (interpreterInfo) { const data = await getFileInfo(executablePath); @@ -68,7 +143,6 @@ export class WindowsStoreLocator implements ILocator { ...data, }; return Promise.resolve({ - id: '', name: '', location: '', kind: this.kind, @@ -100,7 +174,6 @@ export class WindowsStoreLocator implements ILocator { }; } return { - id: '', name: '', location: '', kind: this.kind, diff --git a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts index a54531238d381..e6fecc4243e48 100644 --- a/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts +++ b/src/test/pythonEnvironments/discovery/locators/windowsStoreLocator.unit.test.ts @@ -156,7 +156,7 @@ suite('Windows Store', () => { const data = pathToData.get(k); if (data) { return { - id: '', + name: '', location: '', kind: PythonEnvKind.WindowsStore, @@ -182,7 +182,7 @@ suite('Windows Store', () => { test('resolveEnv(string)', async () => { const python38path = path.join(testStoreAppRoot, 'python3.8.exe'); const expected = { - id: '', + name: '', location: '', kind: PythonEnvKind.WindowsStore, @@ -200,7 +200,7 @@ suite('Windows Store', () => { test('resolveEnv(PythonEnvInfo)', async () => { const python38path = path.join(testStoreAppRoot, 'python3.8.exe'); const expected = { - id: '', + name: '', location: '', kind: PythonEnvKind.WindowsStore, @@ -210,7 +210,6 @@ suite('Windows Store', () => { // Partially filled in env info object const input:PythonEnvInfo = { - id: '', name: '', location: '', kind: PythonEnvKind.WindowsStore, @@ -236,5 +235,15 @@ suite('Windows Store', () => { assertEnvEqual(actual, expected); }); + test('resolveEnv(string): Non store python', async () => { + // Use a non store root path + const python38path = path.join(testLocalAppData, 'python3.8.exe'); + + const envService = new EnvironmentInfoService(); + const locator = new WindowsStoreLocator(envService); + const actual = await locator.resolveEnv(python38path); + + assert.deepStrictEqual(actual, undefined); + }); }); });