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 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
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: 18.x

- 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
6 changes: 2 additions & 4 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
uses: actions/setup-node@v3
with:
node-version: 18.16.x
node-version: 18.x
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
2 changes: 1 addition & 1 deletion .pipelines/integration-test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ jobs:
- task: NodeTool@0
displayName: Use Node 18
inputs:
versionSpec: 18.x
versionSpec: 18.16.x

- 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",
"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
19 changes: 17 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 @@ -93,7 +93,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 +111,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
72 changes: 46 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,40 @@ 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 savedScopes.split(" ").sort().join(" ") === scopes.split(" ").sort().join(" ");
}

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