New Azure Auth Provider #9664

aaomidi · 2020-03-18T20:30:29Z

This is the largest chunk of the PRs we're looking for this feature.

github-actions · 2020-03-18T20:51:11Z

Coverage increased (+0.2%) to 30.287% when pulling 43d9ed0 on amir/azureAuth/newProvider into 166f0d1 on master.

extensions/azurecore/src/account-provider/auths/azureAuthCodeGrant.ts

extensions/azurecore/extension.webpack.config.js

abist · 2020-03-20T18:20:10Z

extensions/azurecore/src/account-provider/auths/azureAuth.ts

+import { MemoryDatabase } from '../utils/memoryDatabase';
+const localize = nls.loadMessageBundle();
+
+export interface AccountKey {


Not sure if those comments are useful unless they provide some information about the fields

Similar cases in the interfaces below :)

The other ones seem pretty self explanatory :D

extensions/azurecore/package.json

extensions/azurecore/src/account-provider/auths/azureAuth.ts

abist · 2020-03-20T18:34:02Z

extensions/azurecore/src/account-provider/auths/azureAuth.ts

+		} catch (err) {
+			console.dir(err);
+			const msg = localize('azure.noToken', "Retrieving the token failed.");
+			throw new Error(msg);


I think you missed showing the localized error to the user here instead of throwing the localized error.

alanrenmsft · 2020-03-20T18:35:47Z

could you please provide more description for this PR?

aaomidi · 2020-03-20T18:54:36Z

The PR here should provide the information @alanrenmsft #9618

This PR is essentially that one just split into multiple chunks. This provides a new Azure Account Provider that is far less error prone and is ready to upgrade to MSAL when we're unblocked from doing so.

extensions/azurecore/src/account-provider/auths/azureAuth.ts

abist · 2020-03-20T20:52:58Z

extensions/azurecore/src/account-provider/auths/azureAuth.ts

+
+		if (!accessToken || !refreshToken) {
+			console.log('Access or refresh token were undefined');
+			const msg = localize('azure.refreshTokenError', "Error when refreshing your account.");


Hmm not this one. It's called from two places and one of them I don't want to show it to the user.

Charles-Gagnon · 2020-03-20T22:08:21Z

extensions/azurecore/src/account-provider/azureAccountProvider.ts

-		// 2) Remove them
-		let results = await this._tokenCache.findThenable(query);
-		this._tokenCache.removeThenable(results);
+	getSecurityToken(account: azdata.Account, resource: azdata.AzureResource): Thenable<{}> {


Shouldn't the type here be TokenResponse (and really should be TokenResponse | undefined given what _getSecurityToken can actually return)

Yeah technically this is only called as an interface method defined

azuredatastudio/src/sql/azdata.d.ts

Line 2277 in 43d9ed0

getSecurityToken(account: Account, resource: AzureResource): Thenable<{}>;

But yeah since that accepts any object I'll do that and actually update the azdata definition as well.

Charles-Gagnon · 2020-03-20T22:09:15Z

extensions/azurecore/src/account-provider/azureAccountProvider.ts

-		let self = this;
+		if (this.authMappings.size === 0) {
+			const msg = localize('azure.NoAuthMethod', "No Azure auth method selected");
+			console.log('noAuthMethodSelected');


Do we really need to log this?

It's just so when we get bug reports in the future, more logs isn't a problem potentially?

I guess - should probably consider logging to a separate stream though if we care about that so we don't have to pick through console logging. Anyways fine with it either way - although I'd prefer you make the output more descriptive since someone seeing this in the logs would have absolutely no idea what this is referring to unless they knew the code.

In reply to: 395911374 [](ancestors = 395911374)

That makes sense, i'll update the message.

If it helps this is a serious edgecase if you have multiple auth methods enabled. (Default will just be one)

Charles-Gagnon · 2020-03-20T22:13:15Z

extensions/azurecore/src/account-provider/azureAccountProvider.ts

-							};
-						});
-				});
+		const pick = await vscode.window.showQuickPick(options, { canPickMany: false });


Handle user cancelling out of this correctly

Charles-Gagnon

Some smaller stuff but didn't see anything hugely stand out. I didn't really delve deep into the logic though since it gets pretty complex - I think we're just going to have to make sure to do thorough testing on it manually.

Charles-Gagnon · 2020-03-20T22:14:45Z

extensions/azurecore/src/account-provider/auths/azureAuth.ts

+	protected readonly memdb = new MemoryDatabase();
+
+	protected readonly WorkSchoolAccountType: string = 'work_school';
+	protected readonly MicrosoftAccountType: string = 'microsoft';


These feel like they should be defined in a more central location (or at least as const values somewhere)

Yeah I'm not really sure where though

extensions/azurecore/src/account-provider/auths/azureAuth.ts

extensions/azurecore/src/account-provider/auths/azureDeviceCode.ts

extensions/azurecore/src/account-provider/azureAccountProvider.ts

extensions/azurecore/src/account-provider/azureAccountProviderService.ts

src/sql/azdata.d.ts

kburtram · 2020-03-23T19:01:10Z

extensions/azurecore/package.json

@@ -199,12 +230,16 @@
    "@azure/arm-subscriptions": "1.0.0",
    "adal-node": "^0.2.1",
    "axios": "^0.19.2",
+    "keytar": "^5.4.0",
+    "qs": "^6.9.1",


we should update the 3rd-party notices when adding new runtime NPM modules.

kburtram · 2020-03-23T19:02:54Z

extensions/azurecore/src/account-provider/simpleTokenCache.ts

+
+function getSystemKeytar(): Keytar | undefined {
+	try {
+		return require('keytar');


what do we do on Linux systems that don't have an installed key manager?

It'll fallback to encrypted file based storage.

Amir Omidi added 4 commits March 17, 2020 17:22

Fixes how azure auth is handled on the azure pane

345ab98

Merge branch 'master' into amir/azureAuth/providerFixes

d4163e2

Merge branch 'master' into amir/azureAuth/newProvider

67bf54f

New auth provider

43d9ed0

aaomidi requested a review from Charles-Gagnon March 18, 2020 20:30

aaomidi commented Mar 18, 2020

View reviewed changes

extensions/azurecore/src/account-provider/auths/azureAuthCodeGrant.ts Show resolved Hide resolved

Add externals

3331b31

aaomidi requested review from kburtram and abist March 19, 2020 01:09

Merge branch 'master' into amir/azureAuth/newProvider

f61cbce

aaomidi requested a review from alanrenmsft March 20, 2020 00:32

alanrenmsft reviewed Mar 20, 2020

View reviewed changes

extensions/azurecore/extension.webpack.config.js Show resolved Hide resolved

abist reviewed Mar 20, 2020

View reviewed changes

alanrenmsft reviewed Mar 20, 2020

View reviewed changes

extensions/azurecore/package.json Show resolved Hide resolved

abist reviewed Mar 20, 2020

View reviewed changes

extensions/azurecore/src/account-provider/auths/azureAuth.ts Show resolved Hide resolved

abist reviewed Mar 20, 2020

View reviewed changes

Feedback

6efc34f

aaomidi requested review from abist and alanrenmsft March 20, 2020 19:49

abist reviewed Mar 20, 2020

View reviewed changes

extensions/azurecore/src/account-provider/auths/azureAuth.ts Show resolved Hide resolved

abist reviewed Mar 20, 2020

View reviewed changes

Charles-Gagnon reviewed Mar 20, 2020

View reviewed changes

Change azdata

3c9d3d0

Charles-Gagnon reviewed Mar 20, 2020

View reviewed changes

Fix other issues

1ff1e93

Charles-Gagnon approved these changes Mar 20, 2020

View reviewed changes

Amir Omidi added 6 commits March 20, 2020 16:35

Review feedback

402f304

Feedback

ba4e355

Feedback updates

db00fe1

Move AKV to azdata.d.ts

357d690

Merge branch 'master' into amir/azureAuth/newProvider

44ae633

Fix yarn.lock

7a691bd

kburtram approved these changes Mar 23, 2020

View reviewed changes

Update third party notices

c780df8

alanrenmsft approved these changes Mar 23, 2020

View reviewed changes

aaomidi merged commit c15ac47 into master Mar 23, 2020

aaomidi deleted the amir/azureAuth/newProvider branch March 23, 2020 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Azure Auth Provider #9664

New Azure Auth Provider #9664

aaomidi commented Mar 18, 2020 •

edited

Loading

github-actions bot commented Mar 18, 2020

abist Mar 20, 2020

aaomidi Mar 20, 2020

abist Mar 20, 2020

aaomidi Mar 20, 2020

abist Mar 20, 2020

alanrenmsft commented Mar 20, 2020

aaomidi commented Mar 20, 2020

abist Mar 20, 2020

aaomidi Mar 20, 2020

aaomidi Mar 20, 2020

Charles-Gagnon Mar 20, 2020

aaomidi Mar 20, 2020

Charles-Gagnon Mar 20, 2020

aaomidi Mar 20, 2020

Charles-Gagnon Mar 20, 2020

aaomidi Mar 20, 2020

Charles-Gagnon Mar 20, 2020

Charles-Gagnon left a comment

Charles-Gagnon Mar 20, 2020

aaomidi Mar 20, 2020

kburtram Mar 23, 2020

kburtram Mar 23, 2020

aaomidi Mar 23, 2020

New Azure Auth Provider #9664

New Azure Auth Provider #9664

Conversation

aaomidi commented Mar 18, 2020 • edited Loading

github-actions bot commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanrenmsft commented Mar 20, 2020

aaomidi commented Mar 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charles-Gagnon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaomidi commented Mar 18, 2020 •

edited

Loading