Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

makes exec_auth spawn the command asynchronously #2046

Merged
merged 3 commits into from
Nov 23, 2024
Merged

makes exec_auth spawn the command asynchronously #2046

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
247e1e1
fix: call exec_auth async
feloy Nov 22, 2024
938f444
fix: review
feloy Nov 23, 2024
d72dd5e
fix: review
feloy Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 34 additions & 16 deletions src/exec_auth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Buffer> = 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) {
Expand All @@ -41,7 +41,7 @@ export class ExecAuth implements Authenticator {
}

public async applyAuthentication(user: User, opts: https.RequestOptions): Promise<void> {
const credential = this.getCredential(user);
const credential = await this.getCredential(user);
if (!credential) {
return;
}
Expand Down Expand Up @@ -70,7 +70,7 @@ export class ExecAuth implements Authenticator {
return null;
}

private getCredential(user: User): Credential | null {
private async getCredential(user: User): Promise<Credential | null> {
// TODO: Add a unit test for token caching.
const cachedToken = this.tokenCache[user.name];
if (cachedToken) {
Expand Down Expand Up @@ -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) => {
feloy marked this conversation as resolved.
Show resolved Hide resolved
let stdoutData: string = '';
let stderrData: string = '';

const subprocess = this.execFn(exec.command, exec.args, opts);

subprocess.stdout.on('data', (data: Buffer | string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Buffer part of the type can be dropped now (not sure if TypeScript will complain about it though).

stdoutData += data.toString('utf8');
feloy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toString() call should be dropped now.

});

subprocess.stderr.on('data', (data) => {
stderrData += data.toString('utf8');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here about dropping the toString().

});

subprocess.on('error', (error) => {
throw error;
feloy marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that it still makes sense to throw here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my mistake, I forgot to remove it

});

subprocess.on('close', (code) => {
if (code !== 0) {
throw new Error(stderrData);
feloy marked this conversation as resolved.
Show resolved Hide resolved
}
const obj = JSON.parse(stdoutData) as Credential;
feloy marked this conversation as resolved.
Show resolved Hide resolved
this.tokenCache[user.name] = obj;
accept(obj);
});
});
}
}
192 changes: 144 additions & 48 deletions src/exec_auth_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Buffer> => {
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<Buffer>;
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: {
Expand All @@ -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<Buffer> => {
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<Buffer>;
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 = {
Expand All @@ -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');
Expand All @@ -136,18 +162,31 @@ describe('ExecAuth', () => {
var tokenValue = 'foo';
(auth as any).execFn = (
command: string,
args: string[],
opts: child_process.SpawnOptions,
): child_process.SpawnSyncReturns<Buffer> => {
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<Buffer>;
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 = {
Expand Down Expand Up @@ -207,6 +246,26 @@ describe('ExecAuth', () => {
} as child_process.SpawnSyncReturns<Buffer>;
};

(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: {
Expand All @@ -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<Buffer> => {
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<Buffer>;
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 = {
Expand All @@ -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<Buffer> => {
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<Buffer>;
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;
Expand Down Expand Up @@ -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<Buffer> => {
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<Buffer>;
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: {
Expand Down