From 247e1e1ba0f027d9867961fdf80e5118c3200f2e Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Fri, 22 Nov 2024 18:24:16 +0100 Subject: [PATCH 1/3] fix: call exec_auth async --- src/exec_auth.ts | 50 +++++++---- src/exec_auth_test.ts | 192 +++++++++++++++++++++++++++++++----------- 2 files changed, 178 insertions(+), 64 deletions(-) diff --git a/src/exec_auth.ts b/src/exec_auth.ts index 34425af5b6..0561bad3fb 100644 --- a/src/exec_auth.ts +++ b/src/exec_auth.ts @@ -20,10 +20,10 @@ export interface Credential { export class ExecAuth implements Authenticator { private readonly tokenCache: { [key: string]: Credential | null } = {}; private execFn: ( - cmd: string, - args: string[], - opts: child_process.SpawnOptions, - ) => child_process.SpawnSyncReturns = child_process.spawnSync; + command: string, + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ) => child_process.ChildProcessWithoutNullStreams = child_process.spawn; public isAuthProvider(user: User): boolean { if (!user) { @@ -41,7 +41,7 @@ export class ExecAuth implements Authenticator { } public async applyAuthentication(user: User, opts: https.RequestOptions): Promise { - const credential = this.getCredential(user); + const credential = await this.getCredential(user); if (!credential) { return; } @@ -70,7 +70,7 @@ export class ExecAuth implements Authenticator { return null; } - private getCredential(user: User): Credential | null { + private async getCredential(user: User): Promise { // TODO: Add a unit test for token caching. const cachedToken = this.tokenCache[user.name]; if (cachedToken) { @@ -99,15 +99,33 @@ export class ExecAuth implements Authenticator { exec.env.forEach((elt) => (env[elt.name] = elt.value)); opts = { ...opts, env }; } - const result = this.execFn(exec.command, exec.args, opts); - if (result.error) { - throw result.error; - } - if (result.status === 0) { - const obj = JSON.parse(result.stdout.toString('utf8')) as Credential; - this.tokenCache[user.name] = obj; - return obj; - } - throw new Error(result.stderr.toString('utf8')); + + return new Promise((accept, reject) => { + let stdoutData: string = ''; + let stderrData: string = ''; + + const subprocess = this.execFn(exec.command, exec.args, opts); + + subprocess.stdout.on('data', (data: Buffer | string) => { + stdoutData += data.toString('utf8'); + }); + + subprocess.stderr.on('data', (data) => { + stderrData += data.toString('utf8'); + }); + + subprocess.on('error', (error) => { + throw error; + }); + + subprocess.on('close', (code) => { + if (code !== 0) { + throw new Error(stderrData); + } + const obj = JSON.parse(stdoutData) as Credential; + this.tokenCache[user.name] = obj; + accept(obj); + }); + }); } } diff --git a/src/exec_auth_test.ts b/src/exec_auth_test.ts index 759c05e91b..7053f695c3 100644 --- a/src/exec_auth_test.ts +++ b/src/exec_auth_test.ts @@ -60,17 +60,28 @@ describe('ExecAuth', () => { const auth = new ExecAuth(); (auth as any).execFn = ( command: string, - args: string[], - opts: child_process.SpawnOptions, - ): child_process.SpawnSyncReturns => { + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ): child_process.ChildProcessWithoutNullStreams => { return { - status: 0, - stdout: Buffer.from(JSON.stringify({ status: { token: 'foo' } })), - } as child_process.SpawnSyncReturns; + stdout: { + on: (_data: string, f: (data: Buffer | string) => void) => { + f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); + }, + }, + stderr: { + on: () => {}, + }, + on: (op: string, f: any) => { + if (op === 'close') { + f(0); + } + }, + } as unknown as child_process.ChildProcessWithoutNullStreams; }; const opts = {} as https.RequestOptions; opts.headers = {} as OutgoingHttpHeaders; - auth.applyAuthentication( + await auth.applyAuthentication( { name: 'user', authProvider: { @@ -94,15 +105,30 @@ describe('ExecAuth', () => { const auth = new ExecAuth(); (auth as any).execFn = ( command: string, - args: string[], - opts: child_process.SpawnOptions, - ): child_process.SpawnSyncReturns => { + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ): child_process.ChildProcessWithoutNullStreams => { return { - status: 0, - stdout: Buffer.from( - JSON.stringify({ status: { clientCertificateData: 'foo', clientKeyData: 'bar' } }), - ), - } as child_process.SpawnSyncReturns; + stdout: { + on: (_data: string, f: (data: Buffer | string) => void) => { + f( + Buffer.from( + JSON.stringify({ + status: { clientCertificateData: 'foo', clientKeyData: 'bar' }, + }), + ), + ); + }, + }, + stderr: { + on: () => {}, + }, + on: (op: string, f: any) => { + if (op === 'close') { + f(0); + } + }, + } as unknown as child_process.ChildProcessWithoutNullStreams; }; const user = { @@ -119,7 +145,7 @@ describe('ExecAuth', () => { opts.headers = {} as OutgoingHttpHeaders; opts.headers = {} as OutgoingHttpHeaders; - auth.applyAuthentication(user, opts); + await auth.applyAuthentication(user, opts); expect(opts.headers.Authorization).to.be.undefined; expect(opts.cert).to.equal('foo'); expect(opts.key).to.equal('bar'); @@ -136,18 +162,31 @@ describe('ExecAuth', () => { var tokenValue = 'foo'; (auth as any).execFn = ( command: string, - args: string[], - opts: child_process.SpawnOptions, - ): child_process.SpawnSyncReturns => { + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ): child_process.ChildProcessWithoutNullStreams => { execCount++; return { - status: 0, - stdout: Buffer.from( - JSON.stringify({ - status: { token: tokenValue, expirationTimestamp: expire }, - }), - ), - } as child_process.SpawnSyncReturns; + stdout: { + on: (_data: string, f: (data: Buffer | string) => void) => { + f( + Buffer.from( + JSON.stringify({ + status: { token: tokenValue, expirationTimestamp: expire }, + }), + ), + ); + }, + }, + stderr: { + on: () => {}, + }, + on: (op: string, f: any) => { + if (op === 'close') { + f(0); + } + }, + } as unknown as child_process.ChildProcessWithoutNullStreams; }; const user = { @@ -207,6 +246,26 @@ describe('ExecAuth', () => { } as child_process.SpawnSyncReturns; }; + (auth as any).execFn = ( + command: string, + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ): child_process.ChildProcessWithoutNullStreams => { + return { + stdout: { + on: (_data: string, f: (data: Buffer | string) => void) => {}, + }, + stderr: { + on: () => {}, + }, + on: (op: string, f: any) => { + if (op === 'error') { + throw new Error('Error: spawnSync /path/to/bin ENOENT'); + } + }, + } as unknown as child_process.ChildProcessWithoutNullStreams; + }; + const user = { name: 'user', authProvider: { @@ -230,16 +289,29 @@ describe('ExecAuth', () => { return; } const auth = new ExecAuth(); + (auth as any).execFn = ( command: string, - args: string[], - opts: child_process.SpawnOptions, - ): child_process.SpawnSyncReturns => { + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ): child_process.ChildProcessWithoutNullStreams => { return { - status: 100, - stdout: Buffer.from(JSON.stringify({ status: { token: 'foo' } })), - stderr: Buffer.from('Some error!'), - } as child_process.SpawnSyncReturns; + stdout: { + on: (_data: string, f: (data: Buffer | string) => void) => { + f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); + }, + }, + stderr: { + on: (_data: string, f: (data: Buffer | string) => void) => { + f(Buffer.from('Some error!')); + }, + }, + on: (op: string, f: any) => { + if (op === 'close') { + f(100); + } + }, + } as unknown as child_process.ChildProcessWithoutNullStreams; }; const user = { @@ -265,18 +337,30 @@ describe('ExecAuth', () => { return; } const auth = new ExecAuth(); - let optsOut: child_process.SpawnOptions = {}; + let optsOut: child_process.SpawnOptions | undefined = {}; (auth as any).execFn = ( command: string, - args: string[], - opts: child_process.SpawnOptions, - ): child_process.SpawnSyncReturns => { - optsOut = opts; + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ): child_process.ChildProcessWithoutNullStreams => { + optsOut = options; return { - status: 0, - stdout: Buffer.from(JSON.stringify({ status: { token: 'foo' } })), - } as child_process.SpawnSyncReturns; + stdout: { + on: (_data: string, f: (data: Buffer | string) => void) => { + f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); + }, + }, + stderr: { + on: () => {}, + }, + on: (op: string, f: any) => { + if (op === 'close') { + f(0); + } + }, + } as unknown as child_process.ChildProcessWithoutNullStreams; }; + process.env.BLABBLE = 'flubble'; const opts = {} as https.RequestOptions; opts.headers = {} as OutgoingHttpHeaders; @@ -313,16 +397,28 @@ describe('ExecAuth', () => { const auth = new ExecAuth(); (auth as any).execFn = ( command: string, - args: string[], - opts: child_process.SpawnOptions, - ): child_process.SpawnSyncReturns => { + args?: readonly string[], + options?: child_process.SpawnOptionsWithoutStdio, + ): child_process.ChildProcessWithoutNullStreams => { return { - status: 0, - stdout: Buffer.from(JSON.stringify({ status: { token: 'foo' } })), - } as child_process.SpawnSyncReturns; + stdout: { + on: (_data: string, f: (data: Buffer | string) => void) => { + f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); + }, + }, + stderr: { + on: () => {}, + }, + on: (op: string, f: any) => { + if (op === 'close') { + f(0); + } + }, + } as unknown as child_process.ChildProcessWithoutNullStreams; }; + const opts = {} as https.RequestOptions; - auth.applyAuthentication( + await auth.applyAuthentication( { name: 'user', authProvider: { From 938f444dddaf36f96140d6bc0eca7adfa15b6271 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Sat, 23 Nov 2024 09:46:48 +0100 Subject: [PATCH 2/3] fix: review --- src/exec_auth.ts | 23 ++++++++++++++++++----- src/exec_auth_test.ts | 14 ++++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/exec_auth.ts b/src/exec_auth.ts index 0561bad3fb..8a03eea689 100644 --- a/src/exec_auth.ts +++ b/src/exec_auth.ts @@ -100,11 +100,14 @@ export class ExecAuth implements Authenticator { opts = { ...opts, env }; } - return new Promise((accept, reject) => { + return new Promise((resolve, reject) => { let stdoutData: string = ''; let stderrData: string = ''; + let savedError: Error | undefined = undefined; const subprocess = this.execFn(exec.command, exec.args, opts); + subprocess.stdout.setEncoding('utf8'); + subprocess.stderr.setEncoding('utf8'); subprocess.stdout.on('data', (data: Buffer | string) => { stdoutData += data.toString('utf8'); @@ -115,16 +118,26 @@ export class ExecAuth implements Authenticator { }); subprocess.on('error', (error) => { + savedError = error; throw error; }); subprocess.on('close', (code) => { + if (savedError) { + reject(savedError); + return; + } if (code !== 0) { - throw new Error(stderrData); + reject(new Error(stderrData)); + return; + } + try { + const obj = JSON.parse(stdoutData) as Credential; + this.tokenCache[user.name] = obj; + resolve(obj); + } catch (error) { + reject(error); } - const obj = JSON.parse(stdoutData) as Credential; - this.tokenCache[user.name] = obj; - accept(obj); }); }); } diff --git a/src/exec_auth_test.ts b/src/exec_auth_test.ts index 7053f695c3..fc512310ce 100644 --- a/src/exec_auth_test.ts +++ b/src/exec_auth_test.ts @@ -65,11 +65,13 @@ describe('ExecAuth', () => { ): child_process.ChildProcessWithoutNullStreams => { return { stdout: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => { f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); }, }, stderr: { + setEncoding: () => {}, on: () => {}, }, on: (op: string, f: any) => { @@ -110,6 +112,7 @@ describe('ExecAuth', () => { ): child_process.ChildProcessWithoutNullStreams => { return { stdout: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => { f( Buffer.from( @@ -121,6 +124,7 @@ describe('ExecAuth', () => { }, }, stderr: { + setEncoding: () => {}, on: () => {}, }, on: (op: string, f: any) => { @@ -168,6 +172,7 @@ describe('ExecAuth', () => { execCount++; return { stdout: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => { f( Buffer.from( @@ -179,6 +184,7 @@ describe('ExecAuth', () => { }, }, stderr: { + setEncoding: () => {}, on: () => {}, }, on: (op: string, f: any) => { @@ -253,9 +259,11 @@ describe('ExecAuth', () => { ): child_process.ChildProcessWithoutNullStreams => { return { stdout: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => {}, }, stderr: { + setEncoding: () => {}, on: () => {}, }, on: (op: string, f: any) => { @@ -297,11 +305,13 @@ describe('ExecAuth', () => { ): child_process.ChildProcessWithoutNullStreams => { return { stdout: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => { f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); }, }, stderr: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => { f(Buffer.from('Some error!')); }, @@ -346,11 +356,13 @@ describe('ExecAuth', () => { optsOut = options; return { stdout: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => { f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); }, }, stderr: { + setEncoding: () => {}, on: () => {}, }, on: (op: string, f: any) => { @@ -402,11 +414,13 @@ describe('ExecAuth', () => { ): child_process.ChildProcessWithoutNullStreams => { return { stdout: { + setEncoding: () => {}, on: (_data: string, f: (data: Buffer | string) => void) => { f(Buffer.from(JSON.stringify({ status: { token: 'foo' } }))); }, }, stderr: { + setEncoding: () => {}, on: () => {}, }, on: (op: string, f: any) => { From d72dd5ebcaec679b200ad58878f823e9852fd4d0 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Sat, 23 Nov 2024 17:46:26 +0100 Subject: [PATCH 3/3] fix: review --- src/exec_auth.ts | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/exec_auth.ts b/src/exec_auth.ts index 8a03eea689..d6eb0a0bf7 100644 --- a/src/exec_auth.ts +++ b/src/exec_auth.ts @@ -109,17 +109,16 @@ export class ExecAuth implements Authenticator { subprocess.stdout.setEncoding('utf8'); subprocess.stderr.setEncoding('utf8'); - subprocess.stdout.on('data', (data: Buffer | string) => { - stdoutData += data.toString('utf8'); + subprocess.stdout.on('data', (data: string) => { + stdoutData += data; }); - subprocess.stderr.on('data', (data) => { - stderrData += data.toString('utf8'); + subprocess.stderr.on('data', (data: string) => { + stderrData += data; }); subprocess.on('error', (error) => { savedError = error; - throw error; }); subprocess.on('close', (code) => {