Refactor to allow HostProviders to override store and erase

[mjcheetham]

Refactor the core commands and host provider types to permit providers to customise how they store and erase credentials. This would allow for providers that do not want to use the OS credential store for example.

This change is required to enable non-PAT-based authentication, such as delegating to ADAL/MSAL and using Azure access tokens directly (where we don't lookup, store, or erase from the system credential store).

[mjcheetham]

The providers are now in direct control over how they store (or not) their credentials in the OS credential stores, meaning the common "git:" prefix is being pushed into the providers themselves. The fact they start with "git:" is now a convention, rather than something that is enforced.

[mjcheetham]

The base HostProvider class assumes that inheriting providers do want to use the ICredentialStore for the operating system, but providers can either override the various virtual methods, or directly implement IHostProvider instead.

[jamill]

I think the code changes look good.

I had more of a question around how other credential systems are going to work (e.g. ADAL/MSAL or Azure Access Tokens). How will the flow for these be different than with OS credential storage? How will the credential manager choose the appropriate HostProvider?

Will GitHub be able to use MSAL?

I assume that AzDevOps end points will need to be able to choose between ADAL/MSAL, OS provided storage, and Azure Access Tokens?

Will the ADAL/MSAL flavor only be able to “get” a credential, and not update / erase an entry?

[mjcheetham]

@jamill, perhaps an overview of the current flow, and the flow as proposed by this PR would be useful.

Command invocation

When Git invokes GCM it gives us a 'verb' and a dictionary of information as input. The available verbs are "get", "store", and "erase".
The main Application object loops through all available commands looking for one that understands the given Git verb, the executes it.

Each command ultimately inherits from CommandBase which defines two abstract methods: bool CanExecute(string[] args) and Task ExecuteAsync(ICommandContext ctx, string[] args).

Git Verb	Responding command
get	GetCommand
store	StoreCommand
erase	EraseCommand

The CanExecute method for each of the above commands just matches on the command line arguments supplied by Git, that is the verb name.

Current design

The ExecuteAsync method of each Git 'verb command' (that is to say, those that Git will invoke us with; does not include other commands such as 'help' or 'version') currently share the same initial logic:

1. Determine the correct IHostProvider to service this request from the dictionary from standard input.
2. Compute the credential key used to access stored credentials in the OS ICredentialStore.
3. Invoke the abstract ExecuteInternalAsync method, which is verb specific.

Each of the verb commands inherit from the HostProviderCommandBase class which performs those 3 steps. The derived command implements the lookup/create/delete/store logic.

GetCommand does a check for an existing credential in the OS store, and returns if it found. Otherwise it calls the selected IHostProvider to CreateCredentialAsync.

// GetCommand
ICredential cred = credStore.Get(credKey);
if (cred is null)
{
    cred = await hostProvider.CreateCredentialAsync(...);
}

return cred;

StoreCommand and EraseCommand similarly just parse call the AddOrUpdate and Remove methods on the ICredentialStore using the computed credential key.

// StoreCommand
var cred = new Credential(input.UserName, input.Password);
credStore.AddOrUpdate(credKey, cred);

// EraseCommand
credStore.Remove(credKey);

Motivation for change

The assumption here is that all host providers are called for two things:

1. Generate a new credential
2. Compute a stable identifier for the credential (the key)

The GCM infrastructure is responsible for retrieving, storing and erasing these credentials using the provided keys.

In order to support scenarios such as using the AAD Access Token (AT) directly (rather than an Azure DevOps PAT), we need to delegate storage, lookup, and deletion to the ADAL/MSAL authentication library. It does not use the ICredentialStore but it's own cache. We don't want to store the AAD AT in the OS credential store.

Change overview

The primary goal in the PR is to remove the assumption above, and instead allow the host providers to do what they want want when called with "get", "store", or "erase".

The GetCommand, StoreCommand, and EraseCommand are now simplified to handle only the interactions with the standard input and output streams, and convert between text <--> objects for the selected IHostProvider; we delegate to the IHostProvider for all the logic.

// GetCommand
var cred = await provider.GetCredentialAsync(input);

output["protocol"] = input.Protocol;
output["host"]     = input.Host;
output["username"] = cred.UserName;
output["password"] = cred.Password;

// StoreCommand
provider.StoreCredentialAsync(input);

// EraseCommand
provider.EraseCredentialAsync(input);

The shared input parsing, and the selection of the correct IHostProvider remains in the HostProviderCommandBase abstract class that all the verb commands inherit from. The credential key computation responsibility has been pushed fully into the host providers.

Keeping it easy to implement a host provider

Since most of the current providers do want to use the OS credential store to lookup/store credentials, rather than make each provider have to reimplement the ICredentialStore.Get/Add/Remove logic, this has been pushed into the abstact HostProvider class. The Get|Store|EraseCredentialAsync methods have all been made virtual such that each provider can either:

1. Get the shared OS credential store lookup/store functionality for free
2. Hook any of the actions to perform any additional work (like clearing other provider specific caches), for example:

override Task EraseCredentialAsync(..)
{
    // Erase our special cache
    contosoOrgCache.EraseEntry(...);

    // Call the shared logic to erase the actual credential from the OS store
    base.EraseCredentialAsync();
}

3. Completely override the shared logic by either:
   1. Overriding all the virtual methods, or
   2. Implement the IHostProvider interface directly
4. Choose dynamically between 1, 2, or 3 at runtime by chosing to either call to the base or not.

---

To answer your question @Jamil:

Will GitHub be able to use MSAL?

The design of GCM Core is such that the authentication methods are more orthogonal to the host provider than GCM Windows.

If GitHub wished to provide a "sign in with Microsoft" option in the future, the GitHubHostProvider can simply consume the IMicrosoftAuthentication component and call GetAccessTokenAsync(..) passing the appropriate request options.

Such a feature (depending on how implemented) might look like this:

// GitHubHostProvider
override Task GetCredentialAsync(..)
{
    bool msAuthSupported = gitHubApi.CheckIfMicrosoftAuthIsSupported();
    bool userSelectedMsAuth = ShowSelectAuthTypePrompt();

    if (msAuthSupported && userSelectedMsAuth)
    {
        // Use existing MS auth flows
        IMicrosoftAuthentication msAuth = ..;
        var accessToken = msAuth.GetAccessToken(..);
        // ..
    }
    else
    {
        // Use existing GitHub auth flows
        IGitHubAuthentication ghAuth = ..;
        var cred = ghAuth.PromptForCredentials(..);
        // ..
    }
}

[jamill]

Thank you for the detailed comments!