From fb38309e98a6455ffe1d2cc501d5c1f9e0a694ba Mon Sep 17 00:00:00 2001 From: dc Date: Wed, 4 Nov 2026 18:30:00 +0000 Subject: [PATCH] fix(deploy): make the folder picker work on both estates Three defects behind staging's "Invalid parent" and the picker that could not list a subfolder. Found by running the real mpe_dirlist on sas.4gl.io, not by reading it. 1. The browser host answers SYSSCPL "browser". The client looked for the first letter of a unix token (L/H/A/S) and fell through to the Windows default for anything else, so the deploy screen pre-filled C:\DataController on a POSIX runtime, which the server then refused as an invalid parent. Windows is now recognised by its own tokens (WIN, DNTHOST); anything else, including a value the list has never seen, gets the POSIX path. 2. mpe_dirlist emitted a non-blank parent's children at LEVEL 0, because mp_dirlist numbers a folder's contents with the folder's own level. The picker keeps LEVEL>0 for a non-root listing, so every row was discarded and a subfolder could never be browsed. The children are shifted to start at 1; a blank parent still lists the platform roots at 0. Verified on the estate: /tmp at DEPTH=1 returns levels [1], a blank parent returns [0, 1] (the root, then its children). 3. The mock's write probe generated one name and wrote it, truncating a file that was already called that; mpe_writecheck tries up to ten names with fileexist()=0 first. The mock now does the same and reports 'No free name for the write probe' when all ten are taken. The mock's DIRLIST now matches the estate's shape too: children at LEVEL 1, roots at 0 followed by their children, and the cap stops the walk at MAXOBS rather than slicing afterwards. Tests: 35 pass across the five mock suites (makedata-actions 15, two of them new: the probe collision and a folder that cannot be written to), sasjs lint clean, client spec 8/8 with the new platform cases and 2 failing without the fix. --- .../sasjs-configurator.component.spec.ts | 54 +++++++++++ .../sasjs-configurator.component.ts | 16 +-- sas/mocks/sasjs/services/admin/makedata.js | 97 ++++++++++++------- sas/mocks/test/makedata-actions.test.js | 75 ++++++++++++-- sas/sasjs/macros/mpe_dirlist.sas | 15 +++ sas/sasjs/macros/mpe_dirlist.test.sas | 9 ++ 6 files changed, 219 insertions(+), 47 deletions(-) diff --git a/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.spec.ts b/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.spec.ts index 6038352..50cfcd5 100644 --- a/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.spec.ts +++ b/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.spec.ts @@ -87,3 +87,57 @@ describe('SasjsConfiguratorComponent (SAS9 / SASjs setup)', () => { expect(spy).toHaveBeenCalled() }) }) + +/** + * The deploy screen pre-fills the directory from SYSSCPL. A token it does not + * recognise used to fall through to the Windows default, which offers the + * server a drive path it refuses - so an unknown value has to mean POSIX. + */ +describe('SasjsConfiguratorComponent directory default', () => { + const directoryFor = async (sysscpl: any) => { + const sasService: any = { + request: () => + Promise.resolve({ + adapterResponse: { + MF_GETUSER: 'demo', + SYSUSERID: '0', + SYSHOSTNAME: 'host', + SYSVLONG: 'browser', + SYSSCPL: sysscpl, + groups: [{ GROUPNAME: 'AllUsers' }] + } + }) + } + const sasjsService: any = { + getServerInfo: () => ({ subscribe: () => undefined }) + } + const component = new SasjsConfiguratorComponent(sasService, sasjsService, { + replaceState: () => undefined + } as any) + + component.getUserGroups() + // the default is set in the promise callback this call schedules + await Promise.resolve() + await Promise.resolve() + + return component.dcDirectory + } + + it('gives the POSIX path for the browser host, whose token is not a platform', async () => { + expect(await directoryFor('browser')).toBe('/tmp/DataController') + }) + + it('gives the POSIX path for the unix tokens', async () => { + expect(await directoryFor('LINUX')).toBe('/tmp/DataController') + expect(await directoryFor('AIX')).toBe('/tmp/DataController') + }) + + it('gives the drive path for the Windows tokens', async () => { + expect(await directoryFor('WIN')).toBe('C:\\DataController') + expect(await directoryFor('DNTHOST')).toBe('C:\\DataController') + }) + + it('gives the POSIX path when the token is absent', async () => { + expect(await directoryFor(undefined)).toBe('/tmp/DataController') + }) +}) diff --git a/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts b/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts index 7bca0ee..f940808 100644 --- a/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts +++ b/client/src/app/deploy/sections/sasjs-configurator/sasjs-configurator.component.ts @@ -89,20 +89,24 @@ export class SasjsConfiguratorComponent implements OnInit { user who is installing Data Controller. This path should be appropriate to the server type, ie windows or linux. We can use the SYSSCPL variable from the service response above, and - derive the OS based on the first letter. + derive the OS from it. The list of values is available here: https://documentation.sas.com/doc/en/pgmsascdc/9.4_3.5/mcrolref/n0e7s8lf147kazn17u45trpwb6sw.htm + Windows is recognised by its own tokens (WIN, DNTHOST). Anything + else gets the POSIX path: the previous test looked for the first + letter of a unix token, so a value it did not know - the + browser-hosted build answers "browser" - fell through to a drive + path, and the server then refused the folder it had been offered. */ + const sysscpl = String(res.adapterResponse.SYSSCPL || '').toUpperCase() this.dcDirectory = this.tmpDirectories[ - ['L', 'H', 'A', 'S'].includes( - res.adapterResponse.SYSSCPL.substring(0, 1) - ) - ? 'linux' - : 'windows' + sysscpl.startsWith('W') || sysscpl.startsWith('D') + ? 'windows' + : 'linux' ] this.dcAdminGroupList = res.adapterResponse.groups diff --git a/sas/mocks/sasjs/services/admin/makedata.js b/sas/mocks/sasjs/services/admin/makedata.js index 6f8c884..2336bbc 100644 --- a/sas/mocks/sasjs/services/admin/makedata.js +++ b/sas/mocks/sasjs/services/admin/makedata.js @@ -160,54 +160,67 @@ function brDirList(parent, depth, maxobs) { const rows = [] /* the cap bounds the walk, not only the response: browsing /usr or /proc at depth 3 must not stat everything below it and then throw most of it away */ - const full = () => rows.length > maxobs + let capped = false + const full = () => { + if (rows.length < maxobs) return false + capped = true + return true + } + /* + LEVEL is the level the entries sit at, and the picker keeps LEVEL>0 for a + non-root listing - so children start at 1. mp_dirlist is the reference: on + the estate a DEPTH=1 browse of /tmp comes back with its folders at level 1, + and a blank parent returns the platform roots at level 0 followed by their + children at level 1. + */ + let frontier = [] if (!parent) { for (const root of brRoots()) { if (full()) break rows.push(brFolderRow(root, 0, 'folder', '')) + frontier.push(root) } } else { - rows.push(brFolderRow(parent, 0, 'folder', '')) - let frontier = [parent] - walk: for (let level = 1; level <= depth; level++) { - const next = [] - for (const folder of frontier) { - if (full()) break walk - let names + frontier = [parent] + } + + for (let level = 1; level <= depth; level++) { + const next = [] + for (const folder of frontier) { + if (full()) break + let names + try { + names = fs.readdirSync(folder).slice().sort() + } catch (err) { + continue + } + for (const name of names) { + if (full()) break + const child = nodePath.join(folder, name) + let isFolder = false try { - names = fs.readdirSync(folder).slice().sort() + isFolder = fs.statSync(child).isDirectory() } catch (err) { + rows.push(brFolderRow(child, level, 'locked', err.message)) continue } - for (const name of names) { - if (full()) break walk - const child = nodePath.join(folder, name) - let isFolder = false - try { - isFolder = fs.statSync(child).isDirectory() - } catch (err) { - rows.push(brFolderRow(child, level, 'locked', err.message)) - continue - } - if (!isFolder) continue - rows.push(brFolderRow(child, level, 'folder', '')) - next.push(child) - } + if (!isFolder) continue + rows.push(brFolderRow(child, level, 'folder', '')) + next.push(child) } - frontier = next - if (!frontier.length) break } + frontier = next + if (!frontier.length) break } rows.sort(function (a, b) { return a.FILEPATH < b.FILEPATH ? -1 : a.FILEPATH > b.FILEPATH ? 1 : 0 }) - const truncated = rows.length > maxobs return { - rows: truncated ? rows.slice(0, maxobs) : rows, - truncated: truncated + rows: rows.slice(0, maxobs), + truncated: capped } } @@ -248,13 +261,27 @@ function brWriteCheck(path) { } row.IS_FOLDER = 'Y' - const probe = nodePath.join( - target, - '.dcprobe_' + - (typeof process !== 'undefined' && process.pid ? process.pid : 0) + - '_' + - Math.floor(Math.random() * 1e9) - ) + /* mpe_writecheck tries up to ten names with fileexist()=0 before fopen('O'), + so a probe never truncates a file that is already there */ + let probe = '' + for (let attempt = 1; attempt <= 10; attempt++) { + const candidate = nodePath.join( + target, + '.dcprobe_' + + (typeof process !== 'undefined' && process.pid ? process.pid : 0) + + '_' + + Math.floor(Math.random() * 1e9) + ) + if (!fs.existsSync(candidate)) { + probe = candidate + break + } + } + if (!probe) { + row.MSG = 'No free name for the write probe' + return row + } + try { fs.writeFileSync(probe, '') fs.unlinkSync(probe) diff --git a/sas/mocks/test/makedata-actions.test.js b/sas/mocks/test/makedata-actions.test.js index 8391558..7c97c0d 100644 --- a/sas/mocks/test/makedata-actions.test.js +++ b/sas/mocks/test/makedata-actions.test.js @@ -44,10 +44,16 @@ test('a browse does not seed the control library', () => { const body = parse(webout) assert.equal(body.dirinfo[0].PARENT, '') assert.equal(body.dirinfo[0].TRUNCATED, 'N') + // a blank parent is the platform roots at LEVEL 0 (mp_dirlist also expands + // them one level, which is what the SAS service returns for DEPTH=1) assert.deepEqual( - body.dirlist.map((row) => row.FILEPATH), + body.dirlist.filter((row) => row.LEVEL === 0).map((row) => row.FILEPATH), ['/'], - 'a blank parent lists the platform roots' + 'the platform root is listed at LEVEL 0' + ) + assert.ok( + body.dirlist.every((row) => row.LEVEL === 0 || row.LEVEL === 1), + 'children of the root come back at LEVEL 1' ) // the failure this test exists for: nothing written, program still there @@ -89,11 +95,10 @@ test('DIRLIST lists the folders below the parent, one level deep', () => { assert.deepEqual( body.dirlist.map((row) => [row.FILENAME, row.LEVEL, row.FILE_OR_FOLDER]), [ - [path.basename(estate.pickme), 0, 'folder'], ['child', 1, 'folder'], ['second', 1, 'folder'] ], - 'the parent at level 0, then its folders - a file is not offered' + 'the folders at LEVEL 1, the level the picker keeps - a file is not offered' ) }) @@ -107,13 +112,14 @@ test('DIRLIST caps the listing and says it was truncated', () => { ACTION: 'DIRLIST', PARENT: estate.pickme, DEPTH: 1, - MAXOBS: 2 + MAXOBS: 1 } ] }) ) - assert.equal(body.dirlist.length, 2) + // one folder fits, the second is refused rather than walked and discarded + assert.equal(body.dirlist.length, 1) assert.equal(body.dirinfo[0].TRUNCATED, 'Y') }) @@ -207,6 +213,63 @@ test('WRITECHECK reports a file as not a folder', () => { assert.equal(row.MSG, 'Path exists but is not a folder') }) +test('WRITECHECK reports a folder it cannot write to', (t) => { + const estate = makeEstate() + const dir = estate.readonly + fs.chmodSync(dir, 0o555) + + if (typeof process.getuid === 'function' && process.getuid() === 0) { + fs.chmodSync(dir, 0o755) + t.skip('running as root, where a read-only folder is still writable') + return + } + + try { + const row = parse( + runMakedata(estate, { rows: [{ ACTION: 'WRITECHECK', PATH: dir }] }) + ).checkwrite[0] + + assert.equal(row.PATH_EXISTS, 'Y') + assert.equal(row.IS_FOLDER, 'Y') + assert.equal(row.WRITEABLE, 'N') + assert.ok(row.MSG, 'the runtime message is passed on') + } finally { + fs.chmodSync(dir, 0o755) + } +}) + +test('the write probe never overwrites a file that is already there', () => { + const estate = makeEstate() + + // pin the random part so the probe name is predictable, then occupy it + const realRandom = Math.random + Math.random = () => 0.5 + const probe = path.join( + estate.pickme, + '.dcprobe_' + process.pid + '_' + Math.floor(0.5 * 1e9) + ) + fs.writeFileSync(probe, 'someone else\'s file') + + try { + const row = parse( + runMakedata(estate, { + rows: [{ ACTION: 'WRITECHECK', PATH: estate.pickme }] + }) + ).checkwrite[0] + + // mpe_writecheck gives up after ten names rather than truncating one + assert.equal(row.WRITEABLE, 'N') + assert.equal(row.MSG, 'No free name for the write probe') + assert.equal( + fs.readFileSync(probe, 'utf8'), + "someone else's file", + 'the existing file is untouched' + ) + } finally { + Math.random = realRandom + } +}) + test('WRITECHECK refuses a missing or relative path', () => { const estate = makeEstate() diff --git a/sas/sasjs/macros/mpe_dirlist.sas b/sas/sasjs/macros/mpe_dirlist.sas index 47f82f1..647aede 100644 --- a/sas/sasjs/macros/mpe_dirlist.sas +++ b/sas/sasjs/macros/mpe_dirlist.sas @@ -134,6 +134,21 @@ data work.dirlist; end; run; +/* + mp_dirlist numbers a folder's children with the folder's own level, so the + contents of a non-blank PARENT arrive at level 0 - the level the picker + reserves for the parent row it does not draw (it keeps LEVEL>0 for a non-root + listing). Shift the children to start at 1, or the picker is handed rows it + discards and a subfolder can never be browsed. A blank parent lists the + platform roots, which are the top level and stay at 0. +*/ +%if %length(%superq(br_parent)) > 0 %then %do; + data work.dirlist; + set work.dirlist; + level=level+1; + run; +%end; + /* a predictable order makes the picker stable between calls */ proc sort data=work.dirlist; by filepath; diff --git a/sas/sasjs/macros/mpe_dirlist.test.sas b/sas/sasjs/macros/mpe_dirlist.test.sas index 2293379..b9d1014 100644 --- a/sas/sasjs/macros/mpe_dirlist.test.sas +++ b/sas/sasjs/macros/mpe_dirlist.test.sas @@ -85,6 +85,15 @@ run; desc=The file beside it is not offered, outds=work.test_results ) +data _null_; + set work.dirlist; + call symputx('level2',level,'l'); +run; +%mp_assert( + iftrue=(&level2=1), + desc=Children of the parent are level 1, the level the picker keeps, + outds=work.test_results +) /** * Test 3 - depth and its cap