-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
github: add multi-account prompt and login/logout commands #1267
Conversation
Add ability to suppress the GUI from the command-line argument `--no-ui`. We achieve this by setting the process enviroment variable that already exists and is used to control GUI prompts.
c23e5e9
to
3be9a3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! 🚀 I did notice something in my testing that may or may not be by design. Running the below forced a prompt, even though I had GitHub creds for ldennington in my KeyChain.
Users/ldennington/repos/git-credential-manager/out/shared/Git-Credential-Manager/bin/Debug/net6.0/git-credential-manager github login --username ldennington
I also had to consent:
I then tried Device code and the same thing happened:
Users/ldennington/repos/git-credential-manager/out/shared/Git-Credential-Manager/bin/Debug/net6.0/git-credential-manager github login --username ldennington
--device
If this is by design, carry on! But wanted to call it out just in case.
/// <param name="command">Command to add options to.</param> | ||
/// <param name="arity">Specify the required arity of the options.</param> | ||
/// <param name="options">Set of options to add.</param> | ||
public static void AddOptionSet(this Command command, OptionArity arity, params Option[] options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very interesting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, mutually exclusive options or option 'set' isn't natively in System.CommandLine, so here's a custom validator to add that functionality!
@@ -35,7 +37,12 @@ public SecretServiceCollection(string @namespace) | |||
|
|||
#region ICredentialStore | |||
|
|||
public unsafe ICredential Get(string service, string account) | |||
public ICredential Get(string service, string account) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good separation.
// | ||
var loginCmd = new Command("login", "Add a GitHub account."); | ||
var userNameOpt = new Option<string>("--username", "User name to authenticate with"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit
var userNameOpt = new Option<string>("--username", "User name to authenticate with"); | |
var userNameOpt = new Option<string>("--username", "Username to authenticate with"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere in GCM public facing strings, we've used "user name" as two words rather than "username".
if (token is not null) | ||
{ | ||
// Resolve the GitHub user handle if the user didn't supply one | ||
if (string.IsNullOrEmpty(userName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this ever be empty since we have the above userName
null check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We're doing userName = userName ?? url.GetUserName()
and so userName
could still be null or empty if there was no --username
option passed and no user in the remote URL.
git-credential-manager github login --token $TOKEN
is a complete valid command, and --username
is not required.
@@ -56,9 +56,15 @@ protected AuthenticationBase(ICommandContext context) | |||
// Kill the process upon a cancellation request | |||
ct.Register(() => process.Kill()); | |||
|
|||
if (!(standardInput is null)) | |||
// Write the standard input to the process if we have any to write | |||
if (standardInput is not null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice cleanup.
@@ -86,4 +95,28 @@ You can use the `github [list | login | logout]` commands to manage your GitHub | |||
accounts. These commands are documented in the [command-line usage][cli-usage] | |||
or by running `git-credential-manager github --help`. | |||
|
|||
## TL;DR: Tell GCM to remember which account to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great explanation!
This is expected, I guess. We don't check for existing credentials during The GitHub CLI takes the opposite approach, and say "you're already logged in, login again?": % gh auth login
? What account do you want to log into? github.com
? You're already logged into github.com. Do you want to re-authenticate? (y/N) I guess we could modify our
Thoughts? |
I like this approach - it aligns better with what I expected to happen when I ran the command. |
Add an extension method for adding multiple options to a `Command`, optionally setting a validator to enforce the arity of their usage. This allows us to create mutually exclusive option sets.
Add the ability to pull all the accounts from a credential store.
Change GitHubHostProvider to implement IHostProvider directly rather than derive from the abstract HostProvider class. Inline the previously super-class implementations to preserve behaviour.
Refactor the `GenerateCredentialAsync` method to take a remote URI and optional user name hint, rather than requiring an instance of `InputArguments` from Git.
Add a set of basic provider commands to add, remove and list GitHub accounts. Users can explicitly login to github.com or any GHES instance, or remove an account.
Add a few options to the `login` command to customise how auth happens, without needing to respond to an interactive prompt. --username <username> Specify the user name to use; useful for login hints in the web --browser/--web Select the web browser OAuth flow --device Select the device code flow --pat/--token <token> Use the specified personal access token. The user name is resolved if not provided with the --username option.
3be9a3b
to
323fef9
Compare
Sure! I can work on this. |
If we find a credential already exists for the same user prior to authenticating, require the user explicitly pass --force to make us re-authenticate the same account.
Add a new prompt to allow the user to select between GitHub accounts, or using a new account.
Check for multiple existing accounts, and if there is more than one account with credentials in the store AND no user name hint provided in the remote URL, then show the new account selection prompt.
323fef9
to
28b2091
Compare
Added! cc @ldennington Description:
Add a GitHub account.
Usage:
git-credential-manager github login [options]
Options:
--url <url> URL of the GitHub instance to target, otherwise use github.com
--username <username> User name to authenticate with
--device Use device flow to authenticate
--browser, --web Use a web browser to authenticate
--pat, --token <token> Use personal access token to authenticate
+ --force Force re-authentication even if a credential already exists for the account
--no-ui Do not use graphical user interface prompts
-?, -h, --help Show help and usage information
% git-credential-manager github login --username mjcheetham
Account 'mjcheetham' already has credentials for https://github.com; use --force to re-authenticate |
Awesome! Took a look at the corresponding changes and they all look good to me. |
Add the ability to double-click to select an account and 'continue' for the account selection Avalonia window.
@ldennington just pushed one more commit (b94ade0) on top to allow double-clicking the account from the list, to accept and continue... so you don't have to select the account from the list and then press the 'Continue' button. |
**Changes:** - Use in-proc methods for getting OS version number (#1240, #1264) - Update System.CommandLine (#1265) - Suppress GUI from command-line argument (#1267) - Add github (login|logout|list) commands (#1267) - cURL Cookie file support (#1251) - Update target framework on Mac/Linux to .NET 7 (#1274, #1282) - Replace JSON.NET with System.Text.Json (#1274) - Preserve exact redirect URI formatting in OAuth requests (#1281) - Use IP localhost redirect for GitHub (#1286) - Use WWW-Authenticate headers from Git for Azure Repos authority (#1288) - Better GitHub Enterprise Managed User (EMU) account support (#1190)
Add multi-account support for the GitHub host provider.
Previously it was possible to use multiple accounts with GitHub, but this required prior knowledge about including the username in the remote URL (e.g.
https://mona_alt@github.com/mona/test
), and without this the incorrect account may be selected, and subsequently erased due to insufficient permissions.This PR adds a few different features and prompts to improve discoverability and experience using multiple GitHub accounts with GCM:
login
,logout
andlist
commands for the GitHub provider--no-ui
One thing that is absent compared to the Azure Repos provider's multi-account support is the lack of account 'binding'. This is due to lack of knowledge, by default, of the full remote URL from Git to be able to discern between different repositories or organisations on the same host.
Command-line usage:
Select account UI prompts: