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

Move node-cli to be pure ESM, use Conf for persistent storage #203

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
67e1402
Migrate from node-keytar to crypto
hl662 Aug 18, 2023
3a739b4
add option to configure refresh token storage path
hl662 Aug 21, 2023
c3577ce
move @types/node-persist to devDep
hl662 Aug 21, 2023
d3f9ea9
update pnpm-lock
hl662 Aug 21, 2023
f29e56b
rush change
hl662 Aug 21, 2023
81edc40
Apply suggestions from code review
hl662 Aug 21, 2023
3690ccb
delete cached entry when scopes are different
hl662 Aug 21, 2023
4d3339a
Merge branch 'nam/node-cli-keytar' of github.com:iTwin/auth-clients i…
hl662 Aug 21, 2023
13d7cde
resolve merge conflicts
hl662 Aug 22, 2023
1f1f120
Merge branch 'main' into nam/node-cli-keytar
hl662 Aug 22, 2023
a98f439
Change files
hl662 Aug 22, 2023
b3114a9
remove previous rush change file
hl662 Aug 22, 2023
2e462ee
move from cjs to esm
hl662 Aug 22, 2023
f9f9f6d
Merge branch 'nam/node-cli-keytar' into nam/esm-conf
hl662 Aug 22, 2023
7dce78b
use Conf in node-cli esm
hl662 Aug 22, 2023
17f9e6c
add main field in package.json, fix config path for token store
hl662 Aug 23, 2023
da4ba81
move from cjs to esm
hl662 Aug 22, 2023
77aba02
Migrate from node-keytar to crypto
hl662 Aug 18, 2023
35dd5ff
add option to configure refresh token storage path
hl662 Aug 21, 2023
8a7aae6
move @types/node-persist to devDep
hl662 Aug 21, 2023
2519feb
rush change
hl662 Aug 21, 2023
fdefb4d
delete cached entry when scopes are different
hl662 Aug 21, 2023
3d2c41e
Apply suggestions from code review
hl662 Aug 21, 2023
0cba13f
resolve merge conflicts
hl662 Aug 22, 2023
45a1ac1
remove previous rush change file
hl662 Aug 22, 2023
04640d5
use Conf in node-cli esm
hl662 Aug 22, 2023
aff3dae
add main field in package.json, fix config path for token store
hl662 Aug 23, 2023
5289bb3
Merge branch 'nam/esm-conf' of github.com:iTwin/auth-clients into nam…
hl662 Aug 23, 2023
e531e3e
add interface definition for object stored
hl662 Aug 23, 2023
e6e1c4f
Change files
hl662 Aug 23, 2023
f6b9dc4
revert mocha config spec change
hl662 Aug 23, 2023
aadfada
remove mix of import and require in statements
hl662 Aug 23, 2023
5bcda63
use nodenext in tsconfig, use 4.x in core deps
hl662 Aug 23, 2023
c33fc77
bump itwin build tools off of dev
hl662 Aug 23, 2023
0d2c008
fix indent
hl662 Aug 23, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "major",
"comment": "Move from cjs to esm and use Conf",
"packageName": "@itwin/node-cli-authorization",
"email": "50554904+hl662@users.noreply.github.com",
"dependentChangeType": "patch"
}
27 changes: 15 additions & 12 deletions packages/node-cli/package.json
Original file line number Diff line number Diff line change
@@ -1,22 +1,26 @@
{
"name": "@itwin/node-cli-authorization",
"version": "1.0.1",
"main": "lib/cjs/index.js",
"types": "lib/cjs/index.d.ts",
"type": "module",
"description": "Node.js command-line authorization client for iTwin platform",
"engines": {
"node": ">=18"
},
"scripts": {
"build": "npm run -s build:cjs",
"build:cjs": "tsc 1>&2 --outDir lib/cjs && npm run copy:assets",
"copy:assets": "cpx src/static/* lib/cjs/static",
"build": "npm run -s build:esm",
"build:esm": "tsc 1>&2 --outDir lib/esm && npm run copy:assets",
"copy:assets": "cpx src/static/* lib/esm/static",
"clean": "rimraf lib",
"docs": "RUSHSTACK_FILE_ERROR_BASE_FOLDER='../..' betools docs --includes=../../generated-docs/extract --json=../../generated-docs/auth-clients/node-cli-authorization/file.json --tsIndexFile=./index.ts --onlyJson",
"lint": "eslint -f visualstudio \"./src/**/*.ts\" 1>&2",
"lint:fix": "npm run lint -- --fix",
"test": "mocha \"./lib/cjs/test/**/*.test.js\"",
"test": "mocha \"./lib/esm/test/**/*.test.js\"",
"cover": "nyc npm test",
"pack": "npm pack",
"rebuild": "npm run clean && npm run build"
},
"exports": "./lib/esm/index.js",
"typings": "./lib/esm/index.d.ts",
aruniverse marked this conversation as resolved.
Show resolved Hide resolved
"author": {
"name": "Bentley Systems, Inc.",
"url": "http://www.bentley.com"
Expand All @@ -28,20 +32,19 @@
"directory": "packages/node-cli"
},
"dependencies": {
"@itwin/core-common": "^3.3.0 || ^4.0.0",
"@itwin/core-common": "^4.0.0",
"@openid/appauth": "^1.3.1",
"node-persist": "^3.1.3",
"conf": "^11.0.2",
"open": "^8.3.0",
"username": "^5.1.0"
},
"devDependencies": {
"@itwin/build-tools": "^4.0.0-dev.93",
"@itwin/core-bentley": "^3.7.0",
"@itwin/build-tools": "^4.0.0",
"@itwin/core-bentley": "^4.0.0",
"@itwin/eslint-plugin": "^3.7.0",
"@types/chai-as-promised": "^7.1.1",
"@types/chai": "^4.2.22",
"@types/mocha": "^8.2.3",
"@types/node-persist": "^3.1.3",
"@types/node": "^18.11.5",
"@types/sinon": "^10.0.16",
"chai-as-promised": "^7.1.1",
Expand All @@ -56,7 +59,7 @@
"typescript": "~5.0.2"
},
"peerDependencies": {
"@itwin/core-bentley": "^3.3.0 || ^4.0.0"
"@itwin/core-bentley": "^4.0.0"
},
"eslintConfig": {
"plugins": [
Expand Down
8 changes: 3 additions & 5 deletions packages/node-cli/src/Client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@
import { readFileSync } from "fs";
import * as path from "path";
import * as Http from "http";
import * as open from "open";
import open from "open";
import { assert, BeEvent, BentleyError, Logger } from "@itwin/core-bentley";
import {
AuthorizationError, AuthorizationNotifier, AuthorizationRequest, AuthorizationRequestHandler, AuthorizationResponse,
AuthorizationServiceConfiguration, BaseTokenRequestHandler, BasicQueryStringUtils, GRANT_TYPE_AUTHORIZATION_CODE, GRANT_TYPE_REFRESH_TOKEN,
TokenRequest,
} from "@openid/appauth";
import { NodeCrypto, NodeRequestor } from "@openid/appauth/built/node_support";
import { TokenStore } from "./TokenStore";
import { NodeCrypto, NodeRequestor } from "@openid/appauth/built/node_support/index.js";
Copy link
Member

Choose a reason for hiding this comment

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

do we really need to modify imports of our deps to point to the js file?

Copy link
Contributor Author

@hl662 hl662 Aug 23, 2023

Choose a reason for hiding this comment

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

unfortunately, yes...
Relative imports need to explicitly have the file extensions .js attached to them when we set module and moduleresolution to Node16 or NodeNext. That was a rule seemingly defined by the TS team - what the TS compiler does is try to resolve the import to a specific file. If it can't find a specific file, it adds a .js extension (this is what the node resolver does). Apparently, this is a side-effect, rather than the compiler's intention.

Specifying --experimental-specifier-resolution=node would have worked, bypassing the TS linter rules, but it's experimental, and starting from Node 19 this flag was dropped :(

See the comments on this post on an explanation on what goes on underneath the compiler, and see this github discussion on the TS devs being tired of explaining their reasoning 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like you can replicate the effects from this flag --experimental-specifier-resolution=node by creating a custom loader for node but yeah that sounds much more troublesome than just adding .js extension 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading all the comments about this, it's starting to make sense... including the explicit file extension improves module resolution performance (the loader doesn't need to go through every permutation of .js or .d.js or .ejs), and it reduces ambiguity. The thing is, not having the file extension was how the majority of devs have done it in the past, so while including it is the recommended, it's doesn't help with uniformity

import { TokenStore } from "./TokenStore.js";

import type { AccessToken } from "@itwin/core-bentley";
import type { AuthorizationClient } from "@itwin/core-common";
Expand Down Expand Up @@ -152,8 +152,6 @@ export class NodeCliAuthorizationClient implements AuthorizationClient {
// Would ideally set up in constructor, but async...
if (!this._configuration)
this._configuration = await AuthorizationServiceConfiguration.fetchFromIssuer(this._bakedConfig.issuerUrl, new NodeRequestor());

await this._tokenStore.initialize();
}

private async loadAndRefreshAccessToken() {
Expand Down
45 changes: 29 additions & 16 deletions packages/node-cli/src/TokenStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,25 @@
* See LICENSE.md in the project root for license terms and full copyright notice.
*--------------------------------------------------------------------------------------------*/

import * as OperatingSystemUserName from "username";
import type { TokenResponseJson } from "@openid/appauth";
import OperatingSystemUserName from "username";
import { TokenResponse } from "@openid/appauth";
import { createCipheriv, createDecipheriv, randomBytes, scryptSync } from "node:crypto";
import * as path from "node:path";
import * as NodePersist from "node-persist";
import Conf from "conf";

type CacheEntry = TokenResponseJson & {scopesForCacheValidation?: string};
interface ConfEntry {
encryptedCache: string;
iv: string;
}
/**
* Utility to store OIDC AppAuth in secure storage
* @internal
*/
export class TokenStore {
private readonly _appStorageKey: string;
private readonly _scopes: string;
private readonly _store: NodePersist.LocalStorage;
private readonly _store: Conf;
public constructor(namedArgs: {clientId: string, issuerUrl: string, scopes: string}, dir?: string) {
// A stored credential is only valid for a combination of the clientId, the issuing authority and the requested scopes.
// We make the storage key a combination of clientId and issuing authority so that keys can stay cached when switching
Expand All @@ -29,14 +32,25 @@ export class TokenStore {
.replace(/[.]/g, "%2E") // Replace all '.' with URL Percent-encoding representation
.replace(/[\/]/g, "%2F"); // Replace all '/' with URL Percent-encoding representation;
this._scopes = namedArgs.scopes;
this._store = NodePersist.create({
dir: dir ?? path.join(process.cwd(), ".configStore"), // specifies storage file path.
});
}

public async initialize(): Promise<void> {
await this._store.init();
let confConfig: object = {
configName: configFileName, // specifies storage file name.
encryptionKey: "iTwin", // obfuscates the storage file's content, in case a user finds the file and wants to modify it.
};
if (dir) {
confConfig = {
...confConfig,
cwd: dir, // specifies the directory that contains the storage file.
};
} else {
confConfig = {
...confConfig,
projectName: configFileName, // specifies the directory that contains the storage file.
};
}
this._store = new Conf(confConfig);
}

private _userName?: string;
private async getUserName(): Promise<string | undefined> {
if (!this._userName)
Expand All @@ -60,7 +74,7 @@ export class TokenStore {
* Uses node's native `crypto` module to encrypt the given cache entry.
* @returns an object containing a hexadecimal encoded token, returned as a string, as well as the initialization vector.
*/
private encryptCache(cacheEntry: CacheEntry): {encryptedCache: string, iv: string} {
private encryptCache(cacheEntry: CacheEntry): ConfEntry {
const iv = randomBytes(16);
const cipher = createCipheriv("aes-256-cbc", this.generateCipherKey(), iv);

Expand All @@ -86,18 +100,17 @@ export class TokenStore {
return undefined;

const key = await this.getKey();
const storeKeys = await this._store.keys();
if (!storeKeys.includes(key)) {
if (!this._store.has(key)) {
return undefined;
}
const storedObj = await this._store.getItem(key);
const storedObj = this._store.get(key) as ConfEntry;
const encryptedCache = storedObj.encryptedCache;
const iv = storedObj.iv;
const cacheEntry = this.decryptCache(encryptedCache, Buffer.from(iv, "hex"));
// Only reuse token if matching scopes. Don't include cache data for TokenResponse object.
const tokenResponseObj = JSON.parse(cacheEntry) as CacheEntry;
if (tokenResponseObj?.scopesForCacheValidation !== this._scopes) {
await this._store.removeItem(key);
this._store.delete(key);
return undefined;
}
delete tokenResponseObj.scopesForCacheValidation;
Expand All @@ -124,6 +137,6 @@ export class TokenStore {
};
const objToStore = this.encryptCache(cacheEntry);
const key = await this.getKey();
await this._store.setItem(key, objToStore);
this._store.set(key, objToStore);
}
}
2 changes: 1 addition & 1 deletion packages/node-cli/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* Copyright (c) Bentley Systems, Incorporated. All rights reserved.
* See LICENSE.md in the project root for license terms and full copyright notice.
*--------------------------------------------------------------------------------------------*/
export * from "./Client";
export * from "./Client.js";

/** @docs-package-description
* Provides auth functionality for node command-line applications.
Expand Down
28 changes: 14 additions & 14 deletions packages/node-cli/src/test/Client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,21 @@
* See LICENSE.md in the project root for license terms and full copyright notice.
*--------------------------------------------------------------------------------------------*/

import * as chai from "chai";
import * as chaiAsPromised from "chai-as-promised";
import { expect, use } from "chai";
import chaiAsPromised = require("chai-as-promised");

import { BakedAuthorizationConfiguration } from "../Client";
import type { NodeCliAuthorizationConfiguration } from "../Client";
import { BakedAuthorizationConfiguration } from "../Client.js";
import type { NodeCliAuthorizationConfiguration } from "../Client.js";

chai.use(chaiAsPromised);
use(chaiAsPromised);

describe("NodeCliAuthorizationConfiguration defaults", () => {
it("should throw if clientId is an empty string", () => {
chai.expect(() => new BakedAuthorizationConfiguration({ clientId: "", scope: "testScope" })).to.throw();
expect(() => new BakedAuthorizationConfiguration({ clientId: "", scope: "testScope" })).to.throw();
});

it("should throw if scope is an empty string", () => {
chai.expect(() => new BakedAuthorizationConfiguration({ clientId: "testClientId", scope: "" })).to.throw();
expect(() => new BakedAuthorizationConfiguration({ clientId: "testClientId", scope: "" })).to.throw();
});
});

Expand All @@ -31,31 +31,31 @@ describe("NodeCliAuthorizationConfiguration Authority URL Logic", () => {
it("should use config authority without prefix", async () => {
process.env.IMJS_URL_PREFIX = "";
const bakedConfig = new BakedAuthorizationConfiguration({ ...config, issuerUrl: testAuthority });
chai.expect(bakedConfig.issuerUrl).equals(testAuthority);
expect(bakedConfig.issuerUrl).equals(testAuthority);
});

it("should use config authority and ignore prefix", async () => {
process.env.IMJS_URL_PREFIX = "prefix-";
const bakedConfig = new BakedAuthorizationConfiguration({ ...config, issuerUrl: testAuthority });
chai.expect(bakedConfig.issuerUrl).equals("https://test.authority.com");
expect(bakedConfig.issuerUrl).equals("https://test.authority.com");
});

it("should use default authority without prefix ", async () => {
process.env.IMJS_URL_PREFIX = "";
const bakedConfig = new BakedAuthorizationConfiguration(config);
chai.expect(bakedConfig.issuerUrl).equals("https://ims.bentley.com");
expect(bakedConfig.issuerUrl).equals("https://ims.bentley.com");
});

it("should use default authority with prefix ", async () => {
process.env.IMJS_URL_PREFIX = "prefix-";
const bakedConfig = new BakedAuthorizationConfiguration(config);
chai.expect(bakedConfig.issuerUrl).equals("https://prefix-ims.bentley.com");
expect(bakedConfig.issuerUrl).equals("https://prefix-ims.bentley.com");
});

it("should reroute dev prefix to qa if on default ", async () => {
process.env.IMJS_URL_PREFIX = "dev-";
const bakedConfig = new BakedAuthorizationConfiguration(config);
chai.expect(bakedConfig.issuerUrl).equals("https://qa-ims.bentley.com");
expect(bakedConfig.issuerUrl).equals("https://qa-ims.bentley.com");
});
});

Expand All @@ -67,15 +67,15 @@ describe("NodeCliAuthorizationConfiguration Config Scope Logic", () => {

it("Should add offline_access scope", async () => {
const bakedConfig = new BakedAuthorizationConfiguration(config);
chai.expect(bakedConfig.scopes).equals(`${config.scope} offline_access`);
expect(bakedConfig.scopes).equals(`${config.scope} offline_access`);
});

it("Should not add offline_access scope", async () => {
const bakedConfig = new BakedAuthorizationConfiguration({
clientId: "testClientId",
scope: "testScope offline_access",
});
chai.expect(bakedConfig.scopes).equals("testScope offline_access");
expect(bakedConfig.scopes).equals("testScope offline_access");
});
});

20 changes: 9 additions & 11 deletions packages/node-cli/src/test/TokenStore.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { TokenResponse } from "@openid/appauth";
import * as chai from "chai";
import * as chaiAsPromised from "chai-as-promised";
import { TokenStore } from "../TokenStore";
import { assert, expect, use } from "chai";
import chaiAsPromised = require("chai-as-promised");
import { TokenStore } from "../TokenStore.js";
import { rmSync } from "fs";
import * as sinon from "sinon";
chai.use(chaiAsPromised);
import { spy } from "sinon";
use(chaiAsPromised);

describe("TokenStore", () => {
let tokenStore: TokenStore;
Expand All @@ -20,7 +20,6 @@ describe("TokenStore", () => {
issuerUrl: "https://testUrl.com",
scopes: "testScope1 testScope2",
}, `${process.cwd()}/testConfig`);
await tokenStore.initialize();
});

afterEach(() => {
Expand All @@ -31,9 +30,9 @@ describe("TokenStore", () => {
if (process.platform === "linux")
return;

const saveSpy = sinon.spy(tokenStore as any, "encryptCache");
const saveSpy = spy(tokenStore as any, "encryptCache");
await tokenStore.save(testTokenResponse);
chai.assert.isTrue(saveSpy.calledOnce);
assert.isTrue(saveSpy.calledOnce);
});

it("should decrypt cache on load", async () => {
Expand All @@ -43,7 +42,7 @@ describe("TokenStore", () => {
await tokenStore.save(testTokenResponse);

const retrievedToken = await tokenStore.load();
chai.expect(retrievedToken!.refreshToken).equals(testTokenResponse.refreshToken);
expect(retrievedToken!.refreshToken).equals(testTokenResponse.refreshToken);
});

it("load() should return undefined when scopes are mismatched", async () => {
Expand All @@ -57,9 +56,8 @@ describe("TokenStore", () => {
issuerUrl: "https://testUrl.com",
scopes: "testScope1 testScope2 testScope3",
}, `${process.cwd()}/testConfig`);
await tokenStore2.initialize();

const retrievedToken = await tokenStore2.load();
chai.expect(retrievedToken).to.be.undefined;
expect(retrievedToken).to.be.undefined;
});
});
5 changes: 3 additions & 2 deletions packages/node-cli/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
{
"extends": "./node_modules/@itwin/build-tools/tsconfig-base.json",
"compilerOptions": {
"target": "es2021",
"module": "commonjs",
"target": "ES2022",
"module": "NodeNext",
"moduleResolution": "NodeNext",
"outDir": "./lib"
},
"include": [
Expand Down