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.

Sign up for GitHub

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

Jump to bottom

Improve login error message #431

Closed
wants to merge 3 commits into from
Closed

Improve login error message #431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1ad027
Improve login error message
Or-Geva Oct 11, 2023
902238b
Fix tests
Or-Geva Oct 11, 2023
735a0a8
Part 1 - Fix CR comments
Or-Geva Nov 7, 2023
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
117 changes: 57 additions & 60 deletions src/main/connect/connectionManager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,14 @@ import { LogManager } from '../log/logManager';
import { ScanUtils } from '../utils/scanUtils';
import { ConnectionUtils } from './connectionUtils';

export enum LoginStatus {
export enum TestConnectionStatus {
Success = 'SUCCESS',
Failed = 'FAILED',
FailedSaveCredentials = 'FAILED_SAVE_CREDENTIALS',
FailedServerNotSupported = 'FAILED_SERVER_NOT_SUPPORTED',
FailedTimeout = 'FAILED_TIMEOUT',
FailedBadCredentials = 'FAILED_BAD_CREDENTIALS'
FailedBadCredentials = 'FAILED_BAD_CREDENTIALS',
FailedServerNotFound = 'FAILED_SERVER_NOT_FOUND'
}

/**
Expand Down Expand Up @@ -128,7 +129,10 @@ export class ConnectionManager implements ExtensionComponent, vscode.Disposable

public async connect(): Promise<boolean> {
this._logManager.logMessage('Trying to read credentials from Secret Storage...', 'DEBUG');
if (!(await this.pingCredential())) {
if (!(await this.loadCredential())) {
Or-Geva marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
if (!(await this.testConnectionHealth())) {
this.deleteCredentialsFromMemory();
return false;
}
Expand All @@ -137,17 +141,6 @@ export class ConnectionManager implements ExtensionComponent, vscode.Disposable
return true;
}

private async pingCredential(): Promise<boolean> {
if (await this.loadCredential()) {
try {
return await ConnectionUtils.validateXrayConnection(this.xrayUrl, this._username, this._password, this._accessToken);
} catch (error) {
return false;
}
}
return false;
}

public async loadCredential(): Promise<boolean> {
return (
(await this.setUrlsFromFilesystem()) &&
Expand Down Expand Up @@ -184,32 +177,34 @@ export class ConnectionManager implements ExtensionComponent, vscode.Disposable
public areCompleteCredentialsSet(): boolean {
return !!(this._xrayUrl && this._rtUrl && ((this._username && this._password) || this._accessToken));
}

public async verifyCredentials(prompt: boolean): Promise<boolean> {
if (!this.areXrayCredentialsSet()) {
return false;
/**
* Test whether the connection is valid and the user has permissions to access Xray and Artifactory.
*/
public async testConnection(prompt: boolean): Promise<TestConnectionStatus> {
if (!(await this.testConnectionHealth())) {
this.deleteCredentialsFromMemory();
return TestConnectionStatus.FailedServerNotFound;
Comment on lines +184 to +186
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!(await this.testConnectionHealth())) {
this.deleteCredentialsFromMemory();
return TestConnectionStatus.FailedServerNotFound;

And what if we get 401 in the ping? This request seems redundant, because testComponentPermission already handle the response parsing.

}
if (
!(await ConnectionUtils.checkXrayConnectionAndPermissions(
this._xrayUrl,
this._username,
this._password,
this._accessToken,
prompt,
this._logManager
))
!(await ConnectionUtils.checkXrayPermissions(this._xrayUrl, this._username, this._password, this._accessToken, prompt, this._logManager))
) {
return false;
this.deleteCredentialsFromMemory();
return TestConnectionStatus.FailedBadCredentials;
Copy link
Member

Choose a reason for hiding this comment

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

How can you determine that this is a Bad Credential (401) error rather than:

  • A permissions problem (403)
  • An inactive server - I'm not sure if it is relevant anymore but worth checking
  • Any other error, like 5xx

Take a look at testComponentPermission. I recommend returning the precise status from this function.

}
if (this.areCompleteCredentialsSet()) {
return ConnectionUtils.checkArtifactoryConnection(
this._rtUrl,
this._username,
this._password,
this._accessToken,
prompt,
this._logManager
);
return TestConnectionStatus.Success;
}

public async testConnectionHealth(): Promise<boolean> {
try {
if (!(await ConnectionUtils.pingXray(this.xrayUrl, this._username, this._password, this._accessToken))) {
return false;
}
if (this.areCompleteCredentialsSet()) {
return await ConnectionUtils.pingArtifactory(this.rtUrl, this._username, this._password, this._accessToken);
}
} catch (error) {
this._logManager.logMessage('testConnectionHealth failed. Error:' + error, 'WARN');
Or-Geva marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
return true;
}
Expand Down Expand Up @@ -366,7 +361,7 @@ export class ConnectionManager implements ExtensionComponent, vscode.Disposable
// Assuming platform URL was extracted. Checking against Artifactory.
this._url = this._url.substring(0, this._url.lastIndexOf('/xray'));
this._rtUrl = this.getRtUrlFromPlatform();
if (!(await ConnectionUtils.validateArtifactoryConnection(this._rtUrl, this._username, this._password, this._accessToken))) {
if (!(await ConnectionUtils.pingArtifactory(this._rtUrl, this._username, this._password, this._accessToken))) {
this._url = '';
this._rtUrl = '';
}
Expand All @@ -393,33 +388,35 @@ export class ConnectionManager implements ExtensionComponent, vscode.Disposable
return this.getServiceUrlFromPlatform(this._url, 'xray');
}

public async tryCredentialsFromEnv(): Promise<LoginStatus> {
public async tryCredentialsFromEnv(): Promise<TestConnectionStatus> {
if (!(await this.getCredentialsFromEnv())) {
this.deleteCredentialsFromMemory();
return LoginStatus.Failed;
return TestConnectionStatus.Failed;
}
if (!(await this.verifyCredentials(false))) {
const status: TestConnectionStatus = await this.testConnection(false);
if (status !== TestConnectionStatus.Success) {
this.deleteCredentialsFromMemory();
return LoginStatus.FailedBadCredentials;
return status;
}
if (process.env[ConnectionManager.STORE_CONNECTION_ENV]?.toUpperCase() === 'TRUE') {
// Store credentials in file system if JFROG_IDE_STORE_CONNECTION environment variable is true
return (await this.storeConnection()) ? LoginStatus.Success : LoginStatus.FailedSaveCredentials;
const shouldStoreConnection: boolean = process.env[ConnectionManager.STORE_CONNECTION_ENV]?.toUpperCase() === 'TRUE';
if (shouldStoreConnection) {
return (await this.storeConnection()) ? status : TestConnectionStatus.FailedSaveCredentials;
}
return LoginStatus.Success;
return status;
}

public async tryCredentialsFromJfrogCli(): Promise<LoginStatus> {
public async tryCredentialsFromJfrogCli(): Promise<TestConnectionStatus> {
this._logManager.logMessage('Trying to read credentials from JFrog CLI...', 'DEBUG');
if (!(await this.readCredentialsFromJfrogCli())) {
this.deleteCredentialsFromMemory();
return LoginStatus.Failed;
return TestConnectionStatus.Failed;
}
if (!(await this.verifyCredentials(true))) {
const status: TestConnectionStatus = await this.testConnection(false);
if (status !== TestConnectionStatus.Success) {
this.deleteCredentialsFromMemory();
return LoginStatus.FailedBadCredentials;
return status;
}
return (await this.storeConnection()) ? LoginStatus.Success : LoginStatus.FailedSaveCredentials;
return (await this.storeConnection()) ? status : TestConnectionStatus.FailedSaveCredentials;
}
/**
* Initiates a web-based login process and obtains an access token.
Expand All @@ -428,21 +425,21 @@ export class ConnectionManager implements ExtensionComponent, vscode.Disposable
* @param xrayUrl - The Xray URL.
* @returns A promise that resolves to the login status.
*/
public async startWebLogin(url: string, artifactoryUrl: string, xrayUrl: string): Promise<LoginStatus> {
public async startWebLogin(url: string, artifactoryUrl: string, xrayUrl: string): Promise<TestConnectionStatus> {
if (url === '' || artifactoryUrl === '' || xrayUrl === '') {
return LoginStatus.Failed;
return TestConnectionStatus.Failed;
}
this._logManager.logMessage('Start Web-Login with "' + url + '"', 'DEBUG');
const sessionId: string = crypto.randomUUID();
if (!(await this.registerWebLoginId(url, sessionId))) {
return LoginStatus.FailedServerNotSupported;
return TestConnectionStatus.FailedServerNotSupported;
}
if (!(await this.openBrowser(this.createWebLoginEndpoint(url, sessionId)))) {
return LoginStatus.Failed;
return TestConnectionStatus.Failed;
}
const accessToken: string = await this.getWebLoginAccessToken(url, sessionId);
if (accessToken === '') {
return LoginStatus.FailedTimeout;
return TestConnectionStatus.FailedTimeout;
}
return this.tryStoreCredentials(url, artifactoryUrl, xrayUrl, '', '', accessToken);
}
Expand Down Expand Up @@ -513,19 +510,19 @@ export class ConnectionManager implements ExtensionComponent, vscode.Disposable
username?: string,
password?: string,
accessToken?: string
): Promise<LoginStatus> {
): Promise<TestConnectionStatus> {
this._url = url;
this._xrayUrl = xrayUrl;
this._rtUrl = rtUrl;
this._username = username || '';
this._password = password || '';
this._accessToken = accessToken || '';
const valid: boolean = await this.verifyCredentials(false);
if (!valid) {
const status: TestConnectionStatus = await this.testConnection(false);
if (status !== TestConnectionStatus.Success) {
this.deleteCredentialsFromMemory();
return LoginStatus.FailedBadCredentials;
return status;
}
return (await this.storeConnection()) ? LoginStatus.Success : LoginStatus.FailedSaveCredentials;
return (await this.storeConnection()) ? status : TestConnectionStatus.FailedSaveCredentials;
}

public async getCredentialsFromEnv(): Promise<boolean> {
Expand Down
36 changes: 3 additions & 33 deletions src/main/connect/connectionUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export class ConnectionUtils {
* @param password - Password
* @param accessToken - Access Token
*/
public static async validateArtifactoryConnection(rtUrl: string, username: string, password: string, accessToken: string): Promise<boolean> {
public static async pingArtifactory(rtUrl: string, username: string, password: string, accessToken: string): Promise<boolean> {
let jfrogClient: JfrogClient = this.createJfrogClient('', rtUrl, '', username, password, accessToken);
return await jfrogClient
.artifactory()
Expand All @@ -90,7 +90,7 @@ export class ConnectionUtils {
* @param password - Password
* @param accessToken - Access Token
*/
public static async validateXrayConnection(xray: string, username: string, password: string, accessToken: string): Promise<boolean> {
public static async pingXray(xray: string, username: string, password: string, accessToken: string): Promise<boolean> {
let jfrogClient: JfrogClient = this.createJfrogClient('', '', xray, username, password, accessToken);
return await jfrogClient
.xray()
Expand Down Expand Up @@ -120,7 +120,7 @@ export class ConnectionUtils {
* @param password
* @param accessToken - Access Token
*/
public static async checkXrayConnectionAndPermissions(
public static async checkXrayPermissions(
xrayUrl: string,
username: string,
password: string,
Expand Down Expand Up @@ -153,36 +153,6 @@ export class ConnectionUtils {
return Promise.resolve(true);
}

/**
* Check Artifactory connection.
* @returns true iff success.
* @param rtUrl
* @param username
* @param password
* @param accessToken - Access Token
*/
public static async checkArtifactoryConnection(
rtUrl: string,
username: string,
password: string,
accessToken: string,
prompt: boolean,
logger: LogManager
): Promise<boolean> {
const status: boolean = await ConnectionUtils.validateArtifactoryConnection(rtUrl, username, password, accessToken);
let statusStr: string = 'failed.';
if (status) {
statusStr = 'success.';
}
const msg: string = 'Artifactory connection ' + statusStr;
if (prompt) {
vscode.window.showInformationMessage(msg);
} else {
logger.logMessage(msg, 'DEBUG');
}
return Promise.resolve(status);
}

public static async testXrayVersion(jfrogClient: JfrogClient): Promise<string> {
let xrayVersion: string = await this.getXrayVersion(jfrogClient);
if (!(await this.isXrayVersionCompatible(xrayVersion, ConnectionUtils.MINIMAL_XRAY_VERSION_SUPPORTED))) {
Expand Down
20 changes: 11 additions & 9 deletions src/main/webview/event/tasks/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { ILoginPage, ISendLoginEventData, LoginConnectionType, LoginProgressStat
import { EventSender } from '../eventSender';
import * as vscode from 'vscode';
import { LogManager } from '../../../log/logManager';
import { ConnectionManager, LoginStatus } from '../../../connect/connectionManager';
import { ConnectionManager, TestConnectionStatus } from '../../../connect/connectionManager';
import { ClientUtils } from 'jfrog-client-js';

/**
Expand Down Expand Up @@ -66,7 +66,7 @@ export class LoginTask {
*/
private async doLogin(): Promise<LoginProgressStatus> {
try {
let status: LoginStatus;
let status: TestConnectionStatus;
switch (this.updatedPageStatus.connectionType) {
case LoginConnectionType.Sso:
status = await this.connectionManager.startWebLogin(this.platformUrl, this.artifactoryUrl, this.xrayUrl);
Expand Down Expand Up @@ -94,18 +94,20 @@ export class LoginTask {
}
}

public toWebviewLoginStatus(ideStatus: LoginStatus) {
public toWebviewLoginStatus(ideStatus: TestConnectionStatus) {
switch (ideStatus) {
case LoginStatus.Success:
case TestConnectionStatus.Success:
return LoginProgressStatus.Success;
case LoginStatus.FailedBadCredentials:
case TestConnectionStatus.FailedBadCredentials:
return LoginProgressStatus.FailedBadCredentials;
case LoginStatus.FailedTimeout:
case TestConnectionStatus.FailedTimeout:
return LoginProgressStatus.FailedTimeout;
case LoginStatus.FailedServerNotSupported:
return LoginProgressStatus.FailedServerNotFound;
case LoginStatus.FailedSaveCredentials:
case TestConnectionStatus.FailedServerNotSupported:
return LoginProgressStatus.FailedServerNotSupported;
case TestConnectionStatus.FailedSaveCredentials:
return LoginProgressStatus.FailedSaveCredentials;
case TestConnectionStatus.FailedServerNotFound:
return LoginProgressStatus.FailedServerNotFound;
Copy link
Member

Choose a reason for hiding this comment

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

Let's add another status for permission denied

default:
return LoginProgressStatus.Failed;
}
Expand Down
12 changes: 6 additions & 6 deletions src/test/tests/connectionManager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { execSync } from 'child_process';
import { IJfrogClientConfig, IProxyConfig } from 'jfrog-client-js';
import * as path from 'path';
import * as vscode from 'vscode';
import { ConnectionManager, LoginStatus } from '../../main/connect/connectionManager';
import { ConnectionManager, TestConnectionStatus } from '../../main/connect/connectionManager';
import { ConnectionUtils } from '../../main/connect/connectionUtils';
import { LogManager } from '../../main/log/logManager';
import { createTestConnectionManager, getCliHomeDir, setCliHomeDir } from './utils/utils.test';
Expand Down Expand Up @@ -254,7 +254,7 @@ describe('Connection Manager Tests', () => {
sinon.assert.notCalled(deleteCredentialsFromMemoryStub);
});

it('Empty KeyStore', async () => {
it.only('Empty KeyStore', async () => {
setUrlsFromFilesystemStub.resolves(false);
setUsernameFromFilesystemStub.resolves(false);
getPasswordFromSecretStorageStub.resolves(false);
Expand All @@ -268,7 +268,7 @@ describe('Connection Manager Tests', () => {
sinon.assert.notCalled(setUsernameFromFilesystemStub);
sinon.assert.notCalled(getPasswordFromSecretStorageStub);
sinon.assert.notCalled(getAccessTokenFromSecretStorageStub);
sinon.assert.calledOnce(deleteCredentialsFromMemoryStub);
sinon.assert.notCalled(deleteCredentialsFromMemoryStub);
sinon.assert.notCalled(resolveUrlsStub);
sinon.assert.notCalled(onSuccessConnectStub);
});
Expand Down Expand Up @@ -342,13 +342,13 @@ describe('Connection Manager Tests', () => {
const registerWebLoginIdStub: any = sinon.stub(mockConnectionManager as any, 'registerWebLoginId').resolves(true);
const openBrowserStub: any = sinon.stub(mockConnectionManager as any, 'openBrowser').resolves(true);
const getWebLoginAccessTokenStub: any = sinon.stub(mockConnectionManager as any, 'getWebLoginAccessToken').resolves('mock-access-token');
const tryStoreCredentialsStub: any = sinon.stub(mockConnectionManager, 'tryStoreCredentials').resolves(LoginStatus.Success);
const tryStoreCredentialsStub: any = sinon.stub(mockConnectionManager, 'tryStoreCredentials').resolves(TestConnectionStatus.Success);

// Call the function
const result: LoginStatus = await mockConnectionManager.startWebLogin('mock-url', 'mock-artifactoryUrl', 'mock-xrayUrl');
const result: TestConnectionStatus = await mockConnectionManager.startWebLogin('mock-url', 'mock-artifactoryUrl', 'mock-xrayUrl');

// Check the return value and ensure that necessary methods are called
assert.strictEqual(result, LoginStatus.Success);
assert.strictEqual(result, TestConnectionStatus.Success);
sinon.assert.calledWith(logMessageStub, 'Start Web-Login with "mock-url"', 'DEBUG');
sinon.assert.calledOnce(registerWebLoginIdStub);
sinon.assert.calledOnce(openBrowserStub);
Expand Down
Loading