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

[electron] require reauth after scopes change #276

Merged
merged 19 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from 17 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
4 changes: 1 addition & 3 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,10 @@ jobs:
- name: Setup node
uses: actions/setup-node@v2
with:
node-version: "18.12.0"
node-version: "20.18.x"
anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved

- name: Setup pnpm
uses: pnpm/action-setup@v4
with:
version: latest

- name: Install packages
run: pnpm install
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/codeql-analysis.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jobs:

steps:
- name: Checkout branch
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
fetch-depth: 0

Expand Down
8 changes: 3 additions & 5 deletions .github/workflows/create-release.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,17 @@ jobs:
name: Release packages
steps:
- name: Checkout
uses: actions/checkout@v3
uses: actions/checkout@v4
with:
token: ${{ secrets.IMJS_ADMIN_GH_TOKEN }}

- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: latest

- name: Use Node.js 18
- name: Use Node.js 22
uses: actions/setup-node@v3
with:
node-version: 18.16.x
node-version: 20.18.x
anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved
registry-url: https://registry.npmjs.org/

- name: Install dependencies
Expand Down
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ dist

# testing
.nyc_output
.parcel-cache
junit_results.xml
/**/test-results/**/*
generated-docs
Expand Down
4 changes: 2 additions & 2 deletions .pipelines/integration-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ jobs:
persistCredentials: true

- task: NodeTool@0
displayName: Use Node 18
displayName: Use Node 20
inputs:
versionSpec: 18.x
versionSpec: 20.x
anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved

- script: npm install -g pnpm@9
displayName: Install pnpm
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Delete stored token if application scopes change",
"packageName": "@itwin/electron-authorization",
"email": "ben-polinsky@users.noreply.github.com",
"dependentChangeType": "patch"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "none",
"comment": "",
"packageName": "@itwin/oidc-signin-tool",
"email": "ben-polinsky@users.noreply.github.com",
"dependentChangeType": "none"
}
5 changes: 3 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,6 @@
"devDependencies": {
"beachball": "^2.49.1",
"lage": "^2.11.13"
}
}
},
ben-polinsky marked this conversation as resolved.
Show resolved Hide resolved
"packageManager": "pnpm@9.15.0+sha256.09a8fe31a34fda706354680619f4002f4ccef6dadff93240d24ef6c831f0fd28"
}
10 changes: 4 additions & 6 deletions packages/electron/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
"test": "mocha \"./lib/cjs/test/**/*.test.js\"",
"test:integration": "pnpm test:integration:build && pnpm exec playwright test",
"test:integration:start": "pnpm test:integration:build && electron dist/integration-test/test-app/index.js",
"test:integration:build": "pnpm test:integration:parcel && tsc -p ./src/integration-test/",
"test:integration:parcel": "parcel build src/integration-test/test-app/index.html --dist-dir dist/integration-test/test-app --public-url ./",
"test:integration:build": "pnpm test:integration:vite && tsc -p ./src/integration-test/",
"test:integration:vite": "vite build --config vite.config.mts",
anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved
"pack": "npm pack",
"rebuild": "npm run clean && npm run build"
},
Expand Down Expand Up @@ -66,13 +66,11 @@
"eslint": "^8.56.0",
"mocha": "^10.2.0",
"nyc": "^17.0.0",
"parcel": "~2.12.0",
"path-browserify": "~1.0.1",
"process": "~0.11.10",
"rimraf": "^3.0.2",
"sinon": "^15.0.1",
"source-map-support": "^0.5.9",
"typescript": "~5.3.3"
"typescript": "~5.3.3",
"vite": "^6.0.3"
},
"peerDependencies": {
"@itwin/core-bentley": "^3.3.0 || ^4.0.0",
Expand Down
25 changes: 23 additions & 2 deletions packages/electron/src/integration-test/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const userDataPath = getElectronUserDataPath();
let electronApp: ElectronApplication;
let electronPage: Page;
const testHelper = new TestHelper(signInOptions);
const tokenStore = new RefreshTokenStore(getTokenStoreFileName(),getTokenStoreKey(), userDataPath);
const tokenStore = new RefreshTokenStore(getTokenStoreFileName(), getTokenStoreKey(), userDataPath);

function getTokenStoreKey(issuerUrl?: string): string {
const authority = new URL(issuerUrl ?? "https://ims.bentley.com");
Expand Down Expand Up @@ -79,6 +79,12 @@ test.afterEach(async () => {
await electronApp.close();
});

test("browser.newPage smoke test", async ({ browser }) => {
const page = await browser.newPage();
await page.goto("https://playwright.dev");
await expect(page.getByText("Playwright enables reliable end-to-end testing for modern web apps.")).toBeVisible();
});

anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved
test("buttons exist", async () => {
await electronPage.waitForLoadState("domcontentloaded");
const signInButton = electronPage.getByTestId("signIn");
Expand All @@ -93,7 +99,8 @@ test("sign in successful", async ({ browser }) => {
const page = await browser.newPage();
await testHelper.checkStatus(electronPage, false);
await testHelper.clickSignIn(electronPage);
await testHelper.signIn(page, await getUrl(electronApp));
const url = await getUrl(electronApp);
await testHelper.signIn(page, url);
await page.waitForLoadState("networkidle");
await testHelper.checkStatus(electronPage, true);
await page.close();
Expand All @@ -110,3 +117,17 @@ test("sign out successful", async ({ browser }) => {
await testHelper.checkStatus(electronPage, false);
await page.close();
});

test("when scopes change, sign in is required", async ({ browser }) => {
const page = await browser.newPage();
await testHelper.clickSignIn(electronPage);
await testHelper.signIn(page, await getUrl(electronApp));
await page.waitForLoadState("networkidle");
anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved
await testHelper.checkStatus(electronPage, true);

// Admittedly this is cheating: no user would interact
// with the tokenStore directly, but this is a tough
// case to test otherwise.
await tokenStore.load("itwin-platform realitydata:read");
await testHelper.checkStatus(electronPage, false);
anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved
});
34 changes: 17 additions & 17 deletions packages/electron/src/integration-test/test-app/index.html
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8" />
<!-- https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP -->
<meta
http-equiv="Content-Security-Policy"
content="default-src 'self'; script-src 'self'"
/>
<title>Hello World!</title>
<script type="module" src="./renderer.ts"></script>
</head>
<body>
<button data-testid="signIn" id="signIn">Sign In</button>
<button data-testid="signOut" id="signOut">Sign Out</button>
<button data-testid="getStatus" id="getStatus">Get Sign-in Status</button>
<h2 data-testid="status" id="status"></h2>
</body>
</html>

<head>
<meta charset="UTF-8" />
<!-- https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP -->
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'" />
<title>Hello World!</title>
<script type="module" src="./renderer.ts"></script>
</head>

<body>
<button data-testid="signIn" id="signIn">Sign In</button>
<button data-testid="signOut" id="signOut">Sign Out</button>
<button data-testid="getStatus" id="getStatus">Get Sign-in Status</button>
<h2 data-testid="status" id="status"></h2>
</body>

</html>
6 changes: 3 additions & 3 deletions packages/electron/src/integration-test/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"compilerOptions": {
"module": "node16",
"moduleResolution": "node16",
"module": "NodeNext",
"moduleResolution": "nodenext",
"lib": ["DOM"],
"target": "ES5",
"skipLibCheck": true,
Expand All @@ -10,4 +10,4 @@
},
"include": ["./test-app/index.ts", "../renderer/ElectronPreload.ts"],
"exclude": ["node_modules"]
}
}
5 changes: 3 additions & 2 deletions packages/electron/src/main/Client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,8 @@ export class ElectronMainAuthorization implements AuthorizationClient {
* @return AccessToken if it's possible to get a valid access token, and undefined otherwise.
*/
private async loadAccessToken(): Promise<AccessToken> {
const refreshToken = await this._refreshTokenStore.load();
const refreshToken = await this._refreshTokenStore.load(this._scopes);

if (!refreshToken)
return "";

Expand Down Expand Up @@ -526,7 +527,7 @@ export class ElectronMainAuthorization implements AuthorizationClient {
): Promise<AccessToken> {
this._refreshToken = tokenResponse.refreshToken;
if (this._refreshToken)
await this._refreshTokenStore.save(this._refreshToken);
await this._refreshTokenStore.save(this._refreshToken, this._scopes);

const expiresAtMilliseconds =
(tokenResponse.issuedAt + (tokenResponse.expiresIn ?? 0)) * 1000;
Expand Down
77 changes: 51 additions & 26 deletions packages/electron/src/main/TokenStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import { safeStorage } from "electron";
// eslint-disable-next-line @typescript-eslint/naming-convention
const Store = require("electron-store"); // eslint-disable-line @typescript-eslint/no-var-requires

/**
* Utility class used to store and read OAuth refresh tokens.
* @internal
Expand Down Expand Up @@ -34,31 +35,8 @@ export class RefreshTokenStore {
});
}

private async getUserName(): Promise<string | undefined> {
if (!this._userName) {
this._userName = await (await import("username")).username();
}

return this._userName;
}

// Note: this is intentionally made async in case this code doesn't run in electron's main process and safeStorage must be polyfilled with async function
private async encryptRefreshToken(token: string): Promise<Buffer> {
return safeStorage.encryptString(token);
}

// Note: this is intentionally made async in case this code doesn't run in electron's main process and safeStorage must be polyfilled with async function
private async decryptRefreshToken(encryptedToken: Buffer): Promise<string> {
return safeStorage.decryptString(Buffer.from(encryptedToken));
}

private async getKey(): Promise<string> {
const userName = await this.getUserName();
return `${this._appStorageKey}${userName}`;
}

/** Load refresh token if available */
public async load(): Promise<string | undefined> {
/** (Load) refresh token if available */
public async load(scopes?: string): Promise<string | undefined> {
const userName = await this.getUserName();
if (!userName)
return undefined;
Expand All @@ -67,19 +45,26 @@ export class RefreshTokenStore {
if (!this._store.has(key)) {
return undefined;
}

if (scopes && !(await this.scopesMatch(scopes)))
return;

const encryptedToken = this._store.get(key);
const refreshToken = await this.decryptRefreshToken(encryptedToken).catch(() => undefined);

return refreshToken;
}

/** Save refresh token after signin */
public async save(refreshToken: string): Promise<void> {
public async save(refreshToken: string, scopes?: string): Promise<void> {
const userName = await this.getUserName();
if (!userName)
return;
const encryptedToken = await this.encryptRefreshToken(refreshToken);
const key = await this.getKey();
this._store.set(key, encryptedToken);
if (scopes)
this._store.set(`${key}:scopes`, scopes);
}

/** Delete refresh token after signout */
Expand All @@ -90,5 +75,45 @@ export class RefreshTokenStore {

const key = await this.getKey();
await this._store.delete(key);
await this._store.delete(`${key}:scopes`);
}

private async getUserName(): Promise<string | undefined> {
ben-polinsky marked this conversation as resolved.
Show resolved Hide resolved
if (!this._userName) {
this._userName = await (await import("username")).username();
anmolshres98 marked this conversation as resolved.
Show resolved Hide resolved
}

return this._userName;
}

// Note: this is intentionally made async in case this code doesn't run in electron's main process and safeStorage must be polyfilled with async function
private async encryptRefreshToken(token: string): Promise<Buffer> {
return safeStorage.encryptString(token);
}

// Note: this is intentionally made async in case this code doesn't run in electron's main process and safeStorage must be polyfilled with async function
private async decryptRefreshToken(encryptedToken: Buffer): Promise<string> {
return safeStorage.decryptString(Buffer.from(encryptedToken));
}

private async getKey(): Promise<string> {
const userName = await this.getUserName();
return `${this._appStorageKey}${userName}`;
}

private async scopesMatch(scopes: string): Promise<boolean> {
ben-polinsky marked this conversation as resolved.
Show resolved Hide resolved
const key = await this.getKey();
const savedScopes = this._store.get(`${key}:scopes`);
if (savedScopes) {
return this.arrayEquals(savedScopes.split(" "), scopes.split(" "));
}

// no stored scopes, so all good
return true;
}

private arrayEquals(arr1: string[], arr2: string[]): boolean {
ben-polinsky marked this conversation as resolved.
Show resolved Hide resolved
return arr1.sort().join(" ") === arr2.sort().join(" ");
}

}
Loading
Loading