Merge pull request #238 from gjsjohnmurray/fix-233

gjsjohnmurray · web-flow · commit 6c18cafe247e · 2024-10-08T19:20:54.000+01:00
If connect with stored password fails, delete and re-prompt
diff --git a/src/api/getServerSpec.ts b/src/api/getServerSpec.ts
@@ -1,31 +1,17 @@
 import * as vscode from "vscode";
 import { IServerSpec } from "@intersystems-community/intersystems-servermanager";
 
-interface ICredentialSet {
-	username: string;
-	password: string;
-}
-
-export let credentialCache = new Map<string, ICredentialSet>();
-
 /**
  * Get a server specification.
  *
  * @param name The name.
  * @param scope The settings scope to use for the lookup.
- * @param flushCredentialCache Flush the session's cache of credentials obtained from keystore and/or user prompting.
- * @param noCredentials Set username and password as undefined; do not fetch credentials from anywhere.
  * @returns Server specification or undefined.
  */
 export async function getServerSpec(
 	name: string,
 	scope?: vscode.ConfigurationScope,
-	flushCredentialCache: boolean = false,
-	noCredentials: boolean = false,
 ): Promise<IServerSpec | undefined> {
-	if (flushCredentialCache) {
-		credentialCache[name] = undefined;
-	}
 	// To avoid breaking existing users, continue to return a default server definition even after we dropped that feature
 	let server: IServerSpec | undefined = vscode.workspace.getConfiguration("intersystems.servers", scope).get(name) || legacyEmbeddedServer(name);
 
diff --git a/src/authenticationProvider.ts b/src/authenticationProvider.ts
@@ -14,6 +14,8 @@ import {
 } from "vscode";
 import { ServerManagerAuthenticationSession } from "./authenticationSession";
 import { globalState } from "./extension";
+import { getServerSpec } from "./api/getServerSpec";
+import { makeRESTRequest } from "./makeRESTRequest";
 
 export const AUTHENTICATION_PROVIDER = "intersystems-server-credentials";
 const AUTHENTICATION_PROVIDER_LABEL = "InterSystems Server Credentials";
@@ -35,6 +37,7 @@ export class ServerManagerAuthenticationProvider implements AuthenticationProvid
 	private readonly _secretStorage;
 
 	private _sessions: ServerManagerAuthenticationSession[] = [];
+	private _checkedSessions: ServerManagerAuthenticationSession[] = [];
 
 	private _serverManagerExtension = extensions.getExtension("intersystems-community.servermanager");
 
@@ -52,7 +55,7 @@ export class ServerManagerAuthenticationProvider implements AuthenticationProvid
 		this._initializedDisposable?.dispose();
 	}
 
-	// This function is called first when `vscode.authentication.getSessions` is called.
+	// This function is called first when `vscode.authentication.getSession` is called.
 	public async getSessions(scopes: string[] = []): Promise<readonly AuthenticationSession[]> {
 		await this._ensureInitialized();
 		let sessions = this._sessions;
@@ -61,7 +64,13 @@ export class ServerManagerAuthenticationProvider implements AuthenticationProvid
 		for (let index = 0; index < scopes.length; index++) {
 			sessions = sessions.filter((session) => session.scopes[index] === scopes[index].toLowerCase());
 		}
-		return sessions;
+
+		if (sessions.length === 1) {
+			if (!(await this._isStillValid(sessions[0]))) {
+				sessions = [];
+			}
+		}
+		return sessions || [];
 	}
 
 	// This function is called after `this.getSessions` is called, and only when:
@@ -104,9 +113,17 @@ export class ServerManagerAuthenticationProvider implements AuthenticationProvid
 
 		// Return existing session if found
 		const sessionId = ServerManagerAuthenticationProvider.sessionId(serverName, userName);
-		const existingSession = this._sessions.find((s) => s.id === sessionId);
+		let existingSession = this._sessions.find((s) => s.id === sessionId);
 		if (existingSession) {
-			return existingSession;
+			if (this._checkedSessions.find((s) => s.id === sessionId)) {
+				return existingSession;
+			}
+
+			// Check if the session is still valid
+			if (await this._isStillValid(existingSession)) {
+				this._checkedSessions.push(existingSession);
+				return existingSession;
+			}
 		}
 
 		let password: string | undefined = "";
@@ -190,25 +207,52 @@ export class ServerManagerAuthenticationProvider implements AuthenticationProvid
 		return session;
 	}
 
+	private async _isStillValid(session: ServerManagerAuthenticationSession): Promise<boolean> {
+		if (this._checkedSessions.find((s) => s.id === session.id)) {
+			return true;
+		}
+		const serverSpec = await getServerSpec(session.serverName);
+		if (serverSpec) {
+			serverSpec.username = session.userName;
+			serverSpec.password = session.accessToken;
+			const response = await makeRESTRequest("HEAD", serverSpec);
+			if (response?.status === 401) {
+				await this._removeSession(session.id, true);
+				return false;
+			}
+		}
+		this._checkedSessions.push(session);
+		return true;
+	}
+
 	// This function is called when the end user signs out of the account.
 	public async removeSession(sessionId: string): Promise<void> {
+		this._removeSession(sessionId);
+	}
+
+	private async _removeSession(sessionId: string, alwaysDeletePassword = false): Promise<void> {
 		const index = this._sessions.findIndex((item) => item.id === sessionId);
 		const session = this._sessions[index];
 
-		let deletePassword = false;
 		const credentialKey = ServerManagerAuthenticationProvider.credentialKey(sessionId);
-		if (await this.secretStorage.get(credentialKey)) {
-			const passwordOption = workspace.getConfiguration("intersystemsServerManager.credentialsProvider")
-				.get<string>("deletePasswordOnSignout", "ask");
-			deletePassword = (passwordOption === "always");
-			if (passwordOption === "ask") {
-				const choice = await window.showWarningMessage(
-					`Do you want to keep the password or delete it?`,
-					{ detail: `The ${AUTHENTICATION_PROVIDER_LABEL} account you signed out (${session.account.label}) is currently storing its password securely on your workstation.`, modal: true },
-					{ title: "Keep", isCloseAffordance: true },
-					{ title: "Delete", isCloseAffordance: false },
-				);
-				deletePassword = (choice?.title === "Delete");
+		let deletePassword = false;
+		const hasStoredPassword = await this.secretStorage.get(credentialKey) !== undefined;
+		if (alwaysDeletePassword) {
+			deletePassword = hasStoredPassword;
+		} else {
+			if (hasStoredPassword) {
+				const passwordOption = workspace.getConfiguration("intersystemsServerManager.credentialsProvider")
+					.get<string>("deletePasswordOnSignout", "ask");
+				deletePassword = (passwordOption === "always");
+				if (passwordOption === "ask") {
+					const choice = await window.showWarningMessage(
+						`Do you want to keep the password or delete it?`,
+						{ detail: `The ${AUTHENTICATION_PROVIDER_LABEL} account you signed out (${session.account.label}) is currently storing its password securely on your workstation.`, modal: true },
+						{ title: "Keep", isCloseAffordance: true },
+						{ title: "Delete", isCloseAffordance: false },
+					);
+					deletePassword = (choice?.title === "Delete");
+				}
 			}
 		}
 		if (deletePassword) {
diff --git a/src/extension.ts b/src/extension.ts
@@ -254,7 +254,7 @@ export function activate(context: vscode.ExtensionContext) {
 			if (pathParts && pathParts.length === 4) {
 				const serverName = pathParts[1];
 				const namespace = pathParts[3];
-				const serverSpec = await getServerSpec(serverName, undefined, undefined, true);
+				const serverSpec = await getServerSpec(serverName);
 				if (serverSpec) {
 					const ISFS_ID = "intersystems-community.vscode-objectscript";
 					const isfsExtension = vscode.extensions.getExtension(ISFS_ID);
@@ -379,28 +379,28 @@ export function activate(context: vscode.ExtensionContext) {
 		/**
 		 * Get specification for the named server.
 		 *
-		 * If the `"intersystemsServerManager.authentication.provider"` setting is "intersystems-server-credentials":
-		 *  - the returned object will not contain `password`. To get this:
+		 *  The returned object will not contain `password`. To get that:
 		 * ```
-		 *      const session = await vscode.authentication.getSession('intersystems-server-credentials', [serverSpec.name, serverSpec.username]);
+		 *      const session: vscode.AuthenticationSession = await vscode.authentication.getSession('intersystems-server-credentials', [serverSpec.name, serverSpec.username]);
 		 * ```
 		 *    The `accessToken` property of the returned [`AuthenticationSession`](https://code.visualstudio.com/api/references/vscode-api#AuthenticationSession) is the password.
-		 *  - `flushCredentialsCache` param will be ignored;
-		 *  - `noCredentials` property of `options` param has no effect;
+		 *
+		 *  The `flushCredentialsCache` param is obsolete and has no effect;
+		 *  The `noCredentials` property of `options` param is obsolete and has no effect;
 		 *
 		 * @param name Name of the server, used as the key into the 'intersystems.servers' settings object
 		 * @param scope Settings scope to look in.
-		 * @param flushCredentialCache If passed as true, flush extension's credential cache.
+		 * @param flushCredentialCache Obsolete, has no effect.
 		 * @param options
 		 * @returns { IServerSpec } Server specification object.
 		 */
 		async getServerSpec(
 			name: string,
 			scope?: vscode.ConfigurationScope,
 			flushCredentialCache: boolean = false,
-			options?: { hideFromRecents?: boolean, noCredentials?: boolean },
+			options?: { hideFromRecents?: boolean, /* Obsolete */ noCredentials?: boolean },
 		): Promise<IServerSpec | undefined> {
-			const spec = await getServerSpec(name, scope, flushCredentialCache, options?.noCredentials);
+			const spec = await getServerSpec(name, scope);
 			if (spec && !options?.hideFromRecents) {
 				await view.addToRecents(name);
 			}
diff --git a/src/makeRESTRequest.ts b/src/makeRESTRequest.ts
@@ -165,7 +165,7 @@ export async function makeRESTRequest(
 		return respdata;
 	} catch (error) {
 		console.log(error);
-		return undefined;
+		return error.response;
 	}
 }
 
@@ -176,7 +176,7 @@ export async function makeRESTRequest(
  */
 export async function logout(serverName: string) {
 
-	const server = await getServerSpec(serverName, undefined, false, true);
+	const server = await getServerSpec(serverName, undefined);
 
 	if (!server) {
 		return;
diff --git a/src/ui/serverManagerView.ts b/src/ui/serverManagerView.ts
@@ -1,6 +1,6 @@
 import * as vscode from "vscode";
 import { getServerNames } from "../api/getServerNames";
-import { credentialCache, getServerSpec } from "../api/getServerSpec";
+import { getServerSpec } from "../api/getServerSpec";
 import { getServerSummary } from "../api/getServerSummary";
 import { IServerName } from "@intersystems-community/intersystems-servermanager";
 import { makeRESTRequest } from "../makeRESTRequest";
@@ -383,10 +383,14 @@ async function serverFeatures(element: ServerTreeItem, params?: any): Promise<Fe
 			return undefined;
 		}
 
-		const response = await makeRESTRequest("HEAD", serverSpec);
-		if (!response || response.status !== 200) {
+		let response = await makeRESTRequest("HEAD", serverSpec);
+		if (response?.status === 401) {
+			// Authentication error, so retry in case first attempt cleared a no-longer-valid stored password
+			serverSpec.password = undefined;
+			response = await makeRESTRequest("HEAD", serverSpec);
+		}
+		if (response?.status !== 200) {
 			children.push(new OfflineTreeItem({ parent: element, label: name, id: name }, serverSpec.username || 'UnknownUser'));
-			credentialCache[name] = undefined;
 		} else {
 			children.push(new NamespacesTreeItem({ parent: element, label: name, id: name }, element.name, serverSpec.username || 'UnknownUser'));
 		}
@@ -459,12 +463,11 @@ async function serverNamespaces(element: ServerTreeItem, params?: any): Promise<
 		}
 
 		const response = await makeRESTRequest("GET", serverSpec);
-		if (!response || response.status !== 200) {
+		if (response?.status !== 200) {
 			children.push(new OfflineTreeItem({ parent: element, label: name, id: name }, serverSpec.username || 'UnknownUser'));
-			credentialCache[params.serverName] = undefined;
 		} else {
 			const serverApiVersion = response.data.result.content.api;
-			response.data.result.content.namespaces.map((namespace) => {
+			response.data.result.content.namespaces.map((namespace: string) => {
 				children.push(new NamespaceTreeItem({ parent: element, label: name, id: name }, namespace, name, serverApiVersion));
 			});
 		}
@@ -557,7 +560,7 @@ async function namespaceProjects(element: ProjectsTreeItem, params?: any): Promi
 			{ apiVersion: 1, namespace: params.ns, path: "/action/query" },
 			{ query: "SELECT Name, Description FROM %Studio.Project", parameters: [] }
 		);
-		if (response !== undefined) {
+		if (response?.status === 200) {
 			if (response.data.result.content === undefined) {
 				let message;
 				if (response.data.status?.errors[0]?.code === 5540) {
@@ -643,7 +646,7 @@ async function namespaceWebApps(element: ProjectsTreeItem, params?: any): Promis
 			serverSpec,
 			{ apiVersion: 1, namespace: "%SYS", path: `/cspapps/${params.ns}` }
 		);
-		if (response !== undefined) {
+		if (response?.status === 200) {
 			if (response.data.result.content === undefined) {
 				vscode.window.showErrorMessage(response.data.status.summary);
 				return undefined;

Original file line number	Diff line number	Diff line change
`@@ -165,7 +165,7 @@ export async function makeRESTRequest(`
`165`	`165`	`return respdata;`
`166`	`166`	`} catch (error) {`
`167`	`167`	`console.log(error);`
`168`		`- return undefined;`
	`168`	`+ return error.response;`
`169`	`169`	`}`
`170`	`170`	`}`
`171`	`171`
`@@ -176,7 +176,7 @@ export async function makeRESTRequest(`
`176`	`176`	`*/`
`177`	`177`	`export async function logout(serverName: string) {`
`178`	`178`
`179`		`- const server = await getServerSpec(serverName, undefined, false, true);`
	`179`	`+ const server = await getServerSpec(serverName, undefined);`
`180`	`180`
`181`	`181`	`if (!server) {`
`182`	`182`	`return;`