Broken state of git-credential implementation (w.r.t. non-native helpers)

[Orochimarufan]

Credentials are currently handled internally for various credential.helpers, effectively re-implementing the git functionality.

When encountering an unsupported value in credential.helper, the generic GitCredential class is supposed to call through to the actual helper binary to do the work. There are a number of issues with the current implementation:

1. Only the Gittyup install dir is searched, which is unlikely to produce a result
2. Implementations that come with their own UI (e.g. Git Credential Manager) may not actually save the credentials
3. It is perfectly valid to have multiple credential.helpers configured. As far as I can tell, this is not taken into account at all.
4. Longer-running credential.helper implementations (such as those with built-in UI) will block the Gittyup UI

Details

1. Search Path

This is a rather simple fix, just search PATH and falling back to some hard-coded default locations. I have a working implementation for this, but it isn't particularly useful without addressing the other issues (Since the most prominent use would be GCM)

2. External UI support

The way the external UI is implemented is by blocking the helper's get subcommand on user input (login dialog or similar) and returning the login information. This is especially useful for implementing non-basic authentication schemes (like OAuth). Note that this may NOT actually save the credentials to the helper's store yet (this is the case at least with GCM).

I believe this is as intended by the git credential.helper protocol, and it is expected that the helper be called again with the store subcommand to confirm that the given credentials were acceptable and should be persisted. This is exactly what git itself does when running e.g. a git fetch on a https remote.

Gittyup currently doesn't do this and assumes that, if the helper returns credentials, that they are always taken from a persistent store and need not be sent back to be persisted. Ideally, this should only be done after verifying the credentials work.

3. Multiple credential helpers

Git supports specifying credential.helper multiple times to query multiple helpers for a stored password (See here). They should be tried in turn, with the first one to give a full username+password combination winning out. I have not tested whether or not the store subcommand goes to all or only some of the configured helpers, but I would expect the former.

While I'm not sure whether or not this is a commonly used feature, at the very least non-support should be documented and a runtime warning emitted when encountering a config with multiple helpers. Correctly implementing it would probably not be particularly hard, but needs to be tested against the actual git implementation.

4. Blocking UI

There is no guarantee that the helper returns (with or without info) right away, so blocking the UI on it is not great UX. While this is mostly a cosmetic issue, having non-responding UI doesn't look good.

The credential implementation should therefore be asynchronous.

Proposal

While it is certainly possible to fix these issues and have a functional re-implementation of the git credentials mechanism in Gittyup, I'm not sure this is a good allocation of time.

Instead, I'd like to propose completely replacing the current credential system with a new (asynchronous) implementation written from scratch. It wouldn't try to re-implement git internals and instead always delegate to the public interface git provides for this purpose, git-credential. Additionally, it is probably desirable to provide a GIT_ASKPASS implementation calling back into Gittyup to keep UX with non-UI helpers consistent. git credential fill does not have an option to return without asking the user when no stored password is found AFAICT.

It may make sense to add an additional (non-persistent) credentials cache inside Gittyup on top of this to reduce the need for repeatedly calling external processes, but this can be added at a later point.

[exactly-one-kas]

Very good ideas!

Using git-credential directly would require us to build the executable for that builtin subcommand by ourselves (or the full GIT CLI), which would rely even more on GIT internals, so I don't think this would be the best solution

Removing the wincred CredentialHelper should be easily doable, since we already bundle it; cache and store need to be implemented by us, but store could become its own executable to make credential handling more generic

Also, we should look into bundling git-credential-manager

[Orochimarufan]

I see that #513 fixed 1. and got rid of built-in WinCred. Unfortunately, this will probably only make it more confusing to users of GCM since it should now be found and appear to work but will not actually persist credentials (because of 2.)

Very good ideas!

Using git-credential directly would require us to build the executable for that builtin subcommand by ourselves (or the full GIT CLI), which would rely even more on GIT internals, so I don't think this would be the best solution

I hadn't realized before but I see now that Gittyup is supposed to work without any installed git distribution. I'm honestly unsure how to best handle both running independent of system git and, when available, being as seemless an integration as possible.

Removing the wincred CredentialHelper should be easily doable, since we already bundle it; cache and store need to be implemented by us, but store could become its own executable to make credential handling more generic

As was done in #513. Honestly, I'm not at all sure it makes sense to bundle helpers instead of building them into the app. At that point, we're relying on a specific bundled binary instead of the system installation (as would be used by CLI or other tools), might as well not deal with the subprocess shenanigans and build it right in.

Also, we should look into bundling git-credential-manager

I think bundling external projects is usually not the best of ideas as it creates ambiguities w.r.t. support and means updates to the bundled software are bound to bundler releases. It's also going to create needless confusion because people may have multiple different versions of GCM (e.g. Git4win and Gittyup) installed without realizing and wonder why some new feature only works in certain conditions. To me, that sounds like a lot of additional (busy) work for little benefit.

Perhaps it would make sense to split credential handling across 2 paths, selected on app startup depending on whether a system Git installation could be found:

• Restore built-in WinCred and get rid of GitCredential in it's current form, such that Git-less environments continue to work as before. Only built-in helpers (mainly store and wincred) would be valid in this mode and an error message would be shown to the user when unknown credential.helper values are encountered in config (probably noting that users can install system git for additional helpers). I think it would be a valid choice to limit this mode to a single credential.helper as well, since it's fairly easy to move to system git when more complex setups are required.
• On systems with Git installed, completely delegate credential handling to system git-credential (as was proposed in OP), to ensure an uniform experience across CLI and GUI

An additional cache-layer inside Gittyup can still be added on top of both

[exactly-one-kas]

Using git-credential when available would make sense, we just need to remember to use -c credential.guiPrompt=true

@Murmele what do you think?

[Murmele]

@exactly-one-kas sorry I have overseen this. Where do we need this credential.guiPrompt?