auth: Add AzureSessionProvider and related types

[alexweininger]

Goals: improve structure of our auth package so it can be more easily consumed by partner extensions that don't necessary follow our tree view structure. This is specifically coming from Peter, who works on the AKS extension and basically wrote all of this code as part of their own migration off of Azure Account.

Putting this PR up so I can give feedback and ask questions easier as we iterate over this code.

[alexweininger]

For AuthScenario.GetSessionPrompt the comment states:

// the 'createIfNone' option cannot be used with 'silent', but really we want both
 // flags here (i.e. create a session silently, but do create one if it doesn't exist).
 // To allow this, we first try to get a session silently.

But the documentation for the createIfNone option is:

Whether login should be performed if there is no matching session.

So to me, it sounds like enabling createIfNone will do what you're trying to achieve. If there is not matching session, it will prompt. Why do we have to call getSession with silent enabled before hand?

[peterbom]

I'm pretty sure I tried that. However, I think it pops up a confirmation dialog if you enable createIfNone, which is what I was trying to avoid with the
silent option. I'd be happy to be proved wrong on that though - this is one of my least favourite parts of the code.

(It also might be a feature request on the VS Code side, but I'm not sure I've got a comprehensive idea of what the options/behaviour I'm asking for would be.)

[alexweininger]

(It also might be a feature request on the VS Code side, but I'm not sure I've got a comprehensive idea of what the options/behaviour I'm asking for would be.)

Yeah, it sounds like it could be a bug. Or at least the docstring for createIfNone is misleading.

[alexweininger]

Although these are only used within this class, I'd love to remove this extra layer of abstraction. I want to avoid adding layers of logic/abstraction on top of the VS Code API as much as we can avoid. I think for now it's fine, but I'm exploring refactoring this to avoid using these.

[peterbom]

Introducing this was really a ripple effect of the "silent first" approach you commented on below.

I agree, ideally we'd pass a get-session options object around, but when the receiving code converts that into two calls with different options values, the enum abstraction starts to look a bit better.

[peterbom]

I think it's worth reconsidering the parameters for getAuthSession. For my purposes at least, the only reason for including scopes in there was to specify the special VSCODE_CLIENT_ID value.

The way I've done it here, it's a bit weird that the special TENANT_ID scope is abstracted away, but consumers are expected specify VSCODE_CLIENT_ID explicitly.

Could we consider something like an options object? E.g.

export type GetAuthSessionParams {
    tenantId: string;
    behavior: GetSessionBehavior;
    applicationClientId?: string;
}

If there's a need to specify unconstrained scopes it could be added later. However, if we add it now and it causes confusion ("what scopes do I actually need to specify?"), it'll be hard to remove.

[alexweininger]

1. I'm not familiar with the special VSCODE_CLIENT_ID? How and why are you using this?
2. And I agree, I think an options object would be good here. We can add an optional scopes property in there too.

export type GetAuthSessionParams {
    tenantId: string;
    behavior: GetSessionBehavior;
    applicationClientId?: string;
    scopes?: string[];
}

[alexweininger]

Also, there are scenarios where my team has needed to exclude the ARM scope, and add in something else. I will research into exactly what that was and report back. I want to make sure we can cover that scenario too.

[peterbom]

I'm not familiar with the special VSCODE_CLIENT_ID? How and why are you using this?

I'm not yet, but I'm experimenting with using the session credentials to access graph endpoints for listing and creating applications and service principals. The default "Visual Studio Code" application does not have the required delegated permissions to do that. You can use the VSCODE_CLIENT_ID scope to specify a different application.

Speaking of that, when I said above I wasn't using the scopes option I think I was wrong. When creating an authentication provider needed to initialise the graph client, you need to implement a getAccessToken function. This takes some optional scopes and I'm passing them on to my getAuthSession function. If I didn't do that, I think the aud claim in the resulting access token would be wrong, and requests would be rejected by the graph endpoints.

Perhaps the scenarios you're referring to above are similar (e.g. accessing graph rather than ARM).

[alexweininger]

@peterbom could you describe how the AKS extension would utilize these changes? Would you just be instantiating a VSCodeAzureSessionProvider and ignoring VSCodeAzureSubscriptionProvider? Would you be instantiating both?

[peterbom]

@peterbom could you describe how the AKS extension would utilize these changes? Would you just be instantiating a VSCodeAzureSessionProvider and ignoring VSCodeAzureSubscriptionProvider? Would you be instantiating both?

@alexweininger - yes, that's what I'm thinking: instantiating a VSCodeAzureSessionProvider and ignoring VSCodeAzureSubscriptionProvider

A quick summary of my reasoning:

• Consistency of use: It enables us to use the same pattern for interacting with ARM SDKs (getting credentials and instantiating clients), regardless of whether a command was launched from a TreeView containing subscriptions or from the command palette.
• Separation of concerns: I don't feel that subscriptions are conceptually tied to sessions/credentials, and when moving away from the Azure Account extension I reworked the treeview to that it only exposed persistent properties of resources like IDs and names, and not sessions/credentials (which I worried about getting stale). Since the equivalent of the AzureSessionProvider gets us credentials easily I don't think we'd have a big need for an AzureSubscriptionProvider.

[alexweininger]

yes, that's what I'm thinking: instantiating a VSCodeAzureSessionProvider and ignoring VSCodeAzureSubscriptionProvider

Great, that's what I figured. And I completely agree with your reasoning. I think other extensions like should do the same.

[alexweininger]

Since the session provider holds state (tenants and their signed-in statuses), I'd like to avoid having multiple instantiated within the same extension context since they could become out of sync. Currently if someone does new VSCodeAzureSessionProvider() and new VSCodeAzureSubscriptionProvider(), there will be two instances.

I don't think we need to proactively prevent instantiating multiple VSCodeSessionProviders, but since this might be a common scenario, I'd like to prevent it here. A simple solution would be to make the session provider a constructor argument instead.

[peterbom]

Agree. I was trying to avoid breaking the existing contract.

If we were going to make breaking changes, I wonder if we could go a step further and provide something like an activateVSCodeSessionProviders method (called by consumers from their extension's activate method) that creates the VSCodeAzureXxxxProvider instances, and registers them with the extension context (for proper disposal later, although I'm not sure how important that is). And then getXxxxProvider methods that return the created instances. Sort of like a singleton pattern, VS Code style.

I ended up doing something like that here.

That might be too big of a change, but just throwing it out there for discussion.

[alexweininger]

IMO singleton pattern here makes sense, but I think a note in our documentation and making it an optional constructor argument could work fine for now.

If we think this is a big enough issue then we can consider breaking changes.

[alexweininger]

@peterbom When you have a chance, could you add me as a contributor to https://github.com/peterbom/vscode-azuretools/tree/experiment/session-provider? That you don't have to do all the changes yourself.

[alexweininger]

I originally wanted to try to get this PR in before we officially announce the Azure Account extension deprecation, but now I've decided to wait. Once announced, I'm expecting teams to come to us with specific asks and needs for their migration, and I'd like to make sure we cover those in this refactor.

I've gathered a list of all extensions that depend on Azure Account and I don't think as many will actually need these changes as I thought. Only 2-3 out of about 25. If my research is correct, and that's truly the case, then I'd like to hold off on putting these changes through. The added complexity of the session provider might not be worth it if only the 2-3 partners are depending on it, while 25 others only need a subscription provider.

[bwateratmsft]

I originally wanted to try to get this PR in before we officially announce the Azure Account extension deprecation, but now I've decided to wait. Once announced, I'm expecting teams to come to us with specific asks and needs for their migration, and I'd like to make sure we cover those in this refactor.

I've gathered a list of all extensions that depend on Azure Account and I don't think as many will actually need these changes as I thought. Only 2-3 out of about 25. If my research is correct, and that's truly the case, then I'd like to hold off on putting these changes through. The added complexity of the session provider might not be worth it if only the 2-3 partners are depending on it, while 25 others only need a subscription provider.

@alexweininger agreed, good plan. Those needing a session provider should also consider just directly using the account APIs provided by VSCode, these are more session-shaped.