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

MSAL & Govt/National Clouds #9141

Closed
wants to merge 50 commits into from
Closed

MSAL & Govt/National Clouds #9141

wants to merge 50 commits into from

Conversation

aaomidi
Copy link
Contributor

@aaomidi aaomidi commented Feb 13, 2020

Second take on Azure Authentication.

Implements version 2 of the Microsoft Identity Platform: https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-overview

Two authentication models are supported:

  1. Device Code
  2. Auth Code Grant with PKCE (Proof Key for Code Exchange)

This changes some of the interface as well.

This PR ONLY enables US Govt Cloud - other clouds should just be a configuration change. I'll do those in a future PR.

@aaomidi aaomidi marked this pull request as ready for review February 14, 2020 22:22
@aaomidi
Copy link
Contributor Author

aaomidi commented Feb 14, 2020

Apologies for the large PR. Issues in various areas of the application were popping up as I was enabling national clouds.

@aaomidi
Copy link
Contributor Author

aaomidi commented Feb 14, 2020

Assuming the user has more than one cloud enabled:

image

For now, we're displaying both of these authentication mechanisms:

image

Both of these options work for national and public clouds. The mechanism behind the authentication of these hasn't changed much, except now you won't get the happy blue screen if there was an issue.

We should design a sign-in failure page as well.

@aaomidi
Copy link
Contributor Author

aaomidi commented Feb 14, 2020

@anthonydresser any idea why PR builds are failing? I wasn't able to understand that error.


const azureAccount = account as AzureAccount;

const response: TokenResponse = {};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parts of our code need a very weird mapping of tenants and subscriptions to tokens. So that's essentially what this does.

let _handleFirewallRule = async (errorCode: number, errorMessage: string, connectionTypeId: string): Promise<azdata.HandleFirewallRuleResponse> => {
try {
let params: HandleFirewallRuleParams = { errorCode: errorCode, errorMessage: errorMessage, connectionTypeId: connectionTypeId };
const x = await client.sendRequest(HandleFirewallRuleRequest.type, params);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is debug and needs to be removed

@adsbot adsbot bot added the Stale PR label Mar 4, 2020
@adsbot adsbot bot removed the Stale PR label Mar 12, 2020
@github-actions
Copy link

Coverage Status

Coverage decreased (-0.04%) to 31.043% when pulling 4cdd950 on amir/msal into c9170aa on master.

@aaomidi
Copy link
Contributor Author

aaomidi commented Mar 13, 2020

Closing this in favor of #9618 - but I'm keeping the branch for future.

@adsbot adsbot bot added the Stale PR label Mar 21, 2020
@aaomidi aaomidi closed this Mar 27, 2020
@adsbot adsbot bot removed the Stale PR label Mar 27, 2020
@Charles-Gagnon Charles-Gagnon deleted the amir/msal branch December 16, 2020 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants