lfsapi: don't query askpass for given creds #3126

PastelMobileSuit · 2018-07-13T22:16:32Z

When creds have already been provided to git (i.e. through a
git URL), use those instead of querying the askpass program

Closes #2894

ttaylorr

Looking great so far, thanks for tackling this one!

ttaylorr · 2018-07-13T22:21:47Z

lfsapi/creds.go

 func (a *AskPassCredentialHelper) Fill(what Creds) (Creds, error) {
 	var user bytes.Buffer
 	var pass bytes.Buffer
 	var err bytes.Buffer

+	var username string


OK, my reading of this code indicates that username and password are used to hold the final values of user and pass when we defer to the GIT_ASKPASS or core.askpass program.

It took me a couple of reads to determine the difference between user and username. I think there are a couple of ways that it could be clarified:

Document the difference between the two above in a comment.

Move the user, pass, and err variables to the else block in L109.

Replace all three with:

var ( user strings.Builder pass strings.Builder )

and then write appropriately into both.

I like option 3, since we'll only have one variable for each string. I'll go ahead and update this

ttaylorr · 2018-07-13T22:24:47Z

lfsapi/creds.go

+	} else {
+		// 'ucmd' will run the GIT_ASKPASS (or core.askpass) command prompting
+		// for a username.
+		ucmd := exec.Command(a.Program, a.args(fmt.Sprintf("Username for %q", u))...)


Another option is that we could extract this entire block into another function and call that. What do you think of the signature:

func fillFromProgram(what Creds) error { var ( user bytes.Buffer err bytes.Buffer ) cmd := exec.Command(a.Program, a.args(fmt.Sprintf("Username for %q", u))...) cmd.Stderr = &err cmd.Stdout = &user if err := cmd.Run(); err != nil { return err } if err.Len() > 0 { return errors.New(err.String()) } what["username"] = user.String() // etc ... return nil }

I think that's a great idea! I think though I'd prefer if the function we extract returns a string, and the Fill() function can handle adding that to the credential as necessary. I think it'll be a little cleaner, and we don't need to worry about the fact that Username has different capitalization with the command and the credentials map.

ttaylorr · 2018-07-13T22:25:38Z

test/test-askpass.sh

+  git commit -m "initial commit"
+
+  # $password is defined from test/cmd/lfstest-gitserver.go (see: skipIfBadAuth)
+  export LFS_ASKPASS_USERNAME="fakeuser"


Sanity checking for myself, this export should be OK since it's run in a sub-shell (and therefore won't leak into the environment).

ttaylorr · 2018-07-13T22:25:51Z

test/test-askpass.sh

+  # $password is defined from test/cmd/lfstest-gitserver.go (see: skipIfBadAuth)
+  export LFS_ASKPASS_USERNAME="fakeuser"
+  export LFS_ASKPASS_PASSWORD="fakepass"
+  git config "credential.helper" ""


Let's make sure that this is repository-local with the --local flag.

ttaylorr · 2018-07-13T22:26:47Z

test/test-askpass.sh

+
+  GIT_ASKPASS="lfs-askpass" SSH_ASKPASS="dont-call-me" GIT_TRACE=1 GIT_CURL_VERBOSE=1 git push origin master 2>&1 | tee push.log
+
+  GITSERVER_USER="$(printf $GITSERVER | sed -e 's/http:\/\//http:\/\/user@/')"


Hmm... is there a way that we could move some of this into a function and not duplicate it in the above? I'm not sure if it makes sense to do that, but perhaps we should give it a shot so that this sed invocation isn't duplicated.

Some of this (including GITSERVER_USER) isn't actually being used in the test, so it may just be best to remove it

PastelMobileSuit · 2018-07-13T23:53:09Z

@ttaylorr Thanks for the review! I've gone ahead and addressed your feedback

ttaylorr · 2018-07-16T18:49:57Z

lfsapi/creds.go

+
+	// 'ucmd' will run the GIT_ASKPASS (or core.askpass) command prompting
+	// for the desired valueType (`Username` or `Password`)
+	ucmd := exec.Command(a.Program, a.args(fmt.Sprintf("%s for %q", valueType, u))...)


Nit: do we want to call this cmd instead of ucmd? My impression is that ucmd is a relic of when we had separate exec.Command() invocations to capture usernames and passwords.

ttaylorr · 2018-07-16T18:51:24Z

lfsapi/creds.go

+
+	// 'ucmd' will run the GIT_ASKPASS (or core.askpass) command prompting
+	// for the desired valueType (`Username` or `Password`)
+	ucmd := exec.Command(a.Program, a.args(fmt.Sprintf("%s for %q", valueType, u))...)


This line looks good otherwise. A gentle suggestion would be that we switch valueType to make sure that it's one of the valid things we allow (i.e., Username or Password). Alternatively, we could even extract a type, like:

type credValueType int const ( credValueTypeUnknown credValueType = iota credValueTypeUsername credValueTypePassword )

ttaylorr · 2018-07-16T18:52:40Z

lfsapi/creds.go

-	creds := make(Creds)
-	creds["username"] = username
-	creds["password"] = strings.TrimSpace(password)
+	creds["username"] = strings.TrimSpace(creds["username"])


I think these final strings.TrimSpace calls are unnecessary, since the body of getFromProgram calls strings.TrimFromSpace(and its an invariant thatwhat` will have space-less values, too).

ttaylorr · 2018-07-16T18:53:45Z

lfsapi/creds.go

 	u := &url.URL{
 		Scheme: what["protocol"],
 		Host:   what["host"],
 		Path:   what["path"],
 	}

+	creds := make(Creds)
+
 	if givenUser, ok := what["username"]; ok {


This pattern roughly breaks down for me as:

See if what has a given key, ex. "username".

If so, copy it to creds.

If not, invoke getFromProgram with some mapping of username -> Username.

If the above is OK, copy that into creds.

If not, report an error.

If this is the same for username and password, could we extract another function that captures the above flow?

When creds have already been provided to git (i.e. through a git URL), use those instead of querying the askpass program

Extract askpass querying into its own separate function

Cleanup the `provided credentials` askpass test by removing unused/unneeded environment variables. Also, ensure config changes are local to the repository.

Add a new `getValue` askpass function that returns the value of a particular credential type by returning the existing value if that credential has already been provided, or by querying the askpass program if that credential has not yet been provided.

PastelMobileSuit · 2018-07-16T22:10:35Z

@ttaylorr Thanks for the review! I've just pushed up a new commit to address your feedback, and have also rebased this PR onto master

ttaylorr · 2018-07-17T16:32:06Z

@ttaylorr Thanks for the review! I've just pushed up a new commit to address your feedback, and have also rebased this PR onto master

Thanks for taking my feedback :-). This looks to be in good shape to me, so I've merged it into master while you're on vacation.

PastelMobileSuit mentioned this pull request Jul 13, 2018

git clone prompts for username despite being given it in the clone URL #2894

Closed

ttaylorr reviewed Jul 13, 2018

View reviewed changes

ttaylorr reviewed Jul 16, 2018

View reviewed changes

PastelMobileSuit added 4 commits July 16, 2018 14:37

lfsapi: don't query askpass for given creds

6564e99

When creds have already been provided to git (i.e. through a git URL), use those instead of querying the askpass program

lfsapi: extract askpass querying

6ee4a84

Extract askpass querying into its own separate function

test: cleanup provided credentials test

54fd43a

Cleanup the `provided credentials` askpass test by removing unused/unneeded environment variables. Also, ensure config changes are local to the repository.

Add getValue askpass function

76ccd68

Add a new `getValue` askpass function that returns the value of a particular credential type by returning the existing value if that credential has already been provided, or by querying the askpass program if that credential has not yet been provided.

PastelMobileSuit force-pushed the pastelmobilesuit-askpass-username branch from ce23689 to 76ccd68 Compare July 16, 2018 22:10

ttaylorr approved these changes Jul 17, 2018

View reviewed changes

ttaylorr merged commit 1a3327f into master Jul 17, 2018

ttaylorr deleted the pastelmobilesuit-askpass-username branch July 17, 2018 16:30


		GIT_ASKPASS="lfs-askpass" SSH_ASKPASS="dont-call-me" GIT_TRACE=1 GIT_CURL_VERBOSE=1 git push origin master 2>&1 \| tee push.log

		GITSERVER_USER="$(printf $GITSERVER \| sed -e 's/http:\/\//http:\/\/user@/')"

lfsapi: don't query askpass for given creds #3126

lfsapi: don't query askpass for given creds #3126

Uh oh!

Conversation

PastelMobileSuit commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ttaylorr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastelMobileSuit commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PastelMobileSuit commented Jul 16, 2018

Uh oh!

ttaylorr commented Jul 17, 2018 via email

Uh oh!

Uh oh!

PastelMobileSuit commented Jul 13, 2018 •

edited

Loading