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.

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

Config field username check #817

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ and this project adheres to

## Unreleased

- Added config field `username` check and exception. Windows reserves
environment variable `USERNAME`.
- Added new config field value `allowUsername` to support backwards
compatibility of `username` See
`packages/integration-sdk-runtime/src/execution/__tests__/config.test.ts` for
more details

## 8.28.0 - 2022-10-18

- Publish the following new custom metrics when entities, relationships, and
Expand Down
7 changes: 7 additions & 0 deletions docs/integrations/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,13 @@ The `type` will ensure the values are cast when
[read from `.env`](#command-j1-integration-collect). It is important to mark
secrets with `mask: true` to facilitate safe logging.

###### Cross-platform Support

Using `username` as a config field name is not supported. Microsoft Windows
reserves `USERNAME` environment variable and can not be overridden. Avoid using
`username` for config field names except for backwards compatability. Use
`allowUsername: true` to disable the associated exception.

Example:

```typescript
Expand Down
3 changes: 2 additions & 1 deletion docs/integrations/development_guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ The `instanceConfigFields` object lets us control how the integration will
execute. A common use is to provide credentials to authenticate requests. For
example, DigitalOcean requires a `Personal Access Token` (see below). Other
common config values include a `Client ID`, `API Key`, or `API URL`. Any outside
information the integration needs at runtime can be defined here.
information the integration needs at runtime can be defined here. Avoid using
`username` as a config field name due to cross-platform issues.

DigitalOcean requires a `Person Access Token`, so I'll edit the fields to show
that.
Expand Down
1 change: 1 addition & 0 deletions packages/integration-sdk-core/src/types/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export interface IntegrationInstanceConfigField {
type?: 'string' | 'boolean';
mask?: boolean;
optional?: boolean;
allowUsername?: boolean;
}

export type IntegrationInstanceConfigFieldMap<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ jest.mock('fs');
beforeEach(() => {
process.env.STRING_VARIABLE = 'string';
process.env.BOOLEAN_VARIABLE = 'true';
process.env.USERNAME = 'string';
});

afterEach(() => {
delete process.env.STRING_VARIABLE;
delete process.env.BOOLEAN_VARIABLE;
delete process.env.USERNAME;

vol.reset();
});
Expand All @@ -42,6 +44,40 @@ test('loads config fields from environment variables', () => {
});
});

test('throws error if username is used as a config field name - windows OS limitation', () => {
const instanceConfigFields: IntegrationInstanceConfigFieldMap<
Record<'username', IntegrationInstanceConfigField>
> = {
username: {
type: 'string',
allowUsername: undefined,
},
};

expect(() =>
loadConfigFromEnvironmentVariables(instanceConfigFields),
).toThrowError(
'Config field `username` is not supported on all platforms. Please rename or add `allowUsername: true` to the config field.',
);
});

test('allowUsername provides backwards compatability', () => {
const instanceConfigFields: IntegrationInstanceConfigFieldMap<
Record<'username', IntegrationInstanceConfigField>
> = {
username: {
type: 'string',
allowUsername: true,
},
};

const config = loadConfigFromEnvironmentVariables(instanceConfigFields);

expect(config).toEqual({
username: 'string',
});
});

test('throws error if expected environment is not set for config field', () => {
const instanceConfigFields: IntegrationInstanceConfigFieldMap<
Record<'mySuperAwesomeEnvironmentVariable', IntegrationInstanceConfigField>
Expand Down
6 changes: 6 additions & 0 deletions packages/integration-sdk-runtime/src/execution/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,12 @@ export function loadConfigFromEnvironmentVariables<
// pull in environment variables from .env file if available
dotenvExpand(dotenv.config());

if (configMap.username && !configMap.username.allowUsername) {
throw new Error(
'Config field `username` is not supported on all platforms. Please rename or add `allowUsername: true` to the config field.',
);
}

return Object.entries(configMap)
.map(([field, config]): [string, string | boolean | undefined] => {
const environmentVariableName = snakeCase(field).toUpperCase();
Expand Down