implemented git credential caching #743

rossmeier · 2018-04-09T19:31:32Z

See #740
TODO:

Write some kind of tests?
Do not overwrite existing values with the same value
Properly resolve autosync hangup
Use a better and more configurable way to create storage paths
Add the possibility to use multiple accounts on one host

codecov · 2018-04-09T20:17:40Z

Codecov Report

Merging #743 into master will increase coverage by 0.13%.
The diff coverage is 73.53%.

@@            Coverage Diff             @@
##           master     #743      +/-   ##
==========================================
+ Coverage   64.81%   64.94%   +0.13%     
==========================================
  Files         169      170       +1     
  Lines        9134     9270     +136     
==========================================
+ Hits         5920     6020     +100     
- Misses       2489     2511      +22     
- Partials      725      739      +14

Impacted Files	Coverage Δ
commands.go	`91.79% <100%> (+0.38%)`	⬆️
pkg/action/git-credential.go	`70.97% <70.97%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4734dd4...0b45a29. Read the comment docs.

dominikschulz

Very nice PR overall. Thanks a lot!
I left some comments, please review them.
Also I'd like to see some kind of tests for this.
We have a lot of tests dealing with stdin to get you started.

dominikschulz · 2018-04-10T07:15:49Z

pkg/action/git-credential.go

+	"os"
+	"strings"
+
+	"github.com/justwatchcom/gopass/pkg/store/sub"


Missing gofmt?

I used goimports, which handles it like that...

dominikschulz · 2018-04-10T07:16:13Z

pkg/action/git-credential.go

+	Password string
+}
+
+func (c *gitCredentials) WriteTo(w io.Writer) (int64, error) {


Please add a godoc comment on this method

dominikschulz · 2018-04-10T07:16:51Z

pkg/action/git-credential.go

+	c := &gitCredentials{}
+	for {
+		key, err := rd.ReadString('=')
+		if err == io.EOF {


I'd prefer to write it as if err != nil { if err == io.EOF { ... } }

dominikschulz · 2018-04-10T07:20:36Z

pkg/action/git-credential.go

+	rd := bufio.NewReader(r)
+	c := &gitCredentials{}
+	for {
+		key, err := rd.ReadString('=')


Why not just do

line, err := rd.ReadString('\n') kv := strings.SplitN(line, "=", 2) key = strings.TrimSpace(kv[0]) value = strings.TrimSpace(kv[1])

I admit that it's not much shorter, but that's the way I usually do this.

It just seemed more straightforward to read the format exactly as it is described in the man page

Ok, if you're doing this on purpose (i.e. because it follows the man page closely), I'm good with that.

dominikschulz · 2018-04-10T07:23:46Z

pkg/action/git-credential.go

+		return ExitError(ctx, ExitUnsupported, err, "Error: %v while parsing git-credential", err)
+	}
+	path := "git/" + fsutil.CleanFilename(cred.Host) + "/" + fsutil.CleanFilename(cred.Username)
+	if !s.Store.Exists(ctx, path) {


Some comments on this would be nice. I assume you're first checking the root store and then doing a search which should find any matching entry in a mounted substore?

dominikschulz · 2018-04-10T07:24:34Z

pkg/action/git-credential.go

+	if err != nil {
+		return ExitError(ctx, ExitUnsupported, err, "Error: %v while parsing git-credential", err)
+	}
+	path := "git/" + fsutil.CleanFilename(cred.Host) + "/" + fsutil.CleanFilename(cred.Username)


This will always use the root store. For me that's ok, but I bet some users will complain ;)

Can't you just mount a substore to git?

Yes, you can.

rossmeier · 2018-04-10T15:44:41Z

pkg/action/git-credential.go

+	if cred.Username != "" {
+		_ = secret.SetValue("login", cred.Username)
+	}
+	// TODO: Skip this step if the same secret is already in the store. How to implement this?


How can comparison to existing keys be done?

Are you looking for root.Exists?

No, I'm looking for a way to check if the secret that would be stored equals the secret already stored by encrypting the current secret and comparing it to the stored secret (no need to decrypt the stored secret here)

This is not supported. I don't think this edge case would be enough to extend the (already pretty big) store interface. If you want to compare it the old value will need to be decrypted.

metalmatze · 2018-05-07T23:04:47Z

pkg/action/git-credential.go

+	path := "git/" + fsutil.CleanFilename(cred.Host) + "/" + fsutil.CleanFilename(cred.Username)
+	if !s.Store.Exists(ctx, path) {
+		// if the looked up path is a directory with only one entry (e.g. one user per host), take the subentry instead
+		if s.Store.IsDir(ctx, path) {


Negate and return early:

if !s.Store.IsDir(ctx, path) { return nil } ...

Remove the else case below.

metalmatze · 2018-05-07T23:07:11Z

pkg/action/git-credential.go

+				return ExitError(ctx, ExitDecrypt, err, "Error: %v while listing the storage", err)
+			}
+			entries := sub.List(0)
+			if len(entries) == 1 {


Negate and return early:

if len(entries) != 1 { fmt.Fprintln(os.Stderr, "gopass error: too many entries") } path = "git/" + entries[0]

Not a major issue in this case, but would prefer it that way.

dominikschulz · 2018-05-14T14:08:45Z

@veecue I'd love to see this merged. Do you plan to continue working on this PR?

rossmeier · 2018-05-15T07:29:47Z

Yes, I do. However, I'm not shure if the last two items on my TODO-List are necessary at all

dominikschulz · 2018-05-15T07:30:53Z

I think addressing our review comments and the first three bullet points should be enough.

dominikschulz · 2018-05-23T09:33:03Z

@veecue That looks pretty good already. Could you rebase (or merge master) again, so we can merge this as an alpha feature?
Thanks a lot!

updated codebase

* implemented git credential caching * code style and documentation * tests and handle existing entires * added tests

rossmeier force-pushed the feature/git-credential branch 4 times, most recently from 596d546 to bdb7db1 Compare April 9, 2018 20:11

dominikschulz self-requested a review April 10, 2018 07:13

dominikschulz requested changes Apr 10, 2018

View reviewed changes

dominikschulz added the feature Enhancements and new features label Apr 10, 2018

dominikschulz added this to the 1.7.0 - Experimental Crypto milestone Apr 10, 2018

rossmeier commented Apr 10, 2018

View reviewed changes

metalmatze previously requested changes May 7, 2018

View reviewed changes

rossmeier force-pushed the feature/git-credential branch 2 times, most recently from 5bfaab5 to 2b70ca4 Compare May 8, 2018 09:16

dominikschulz modified the milestones: 1.7.0 - Experimental Crypto, 1.x.x May 16, 2018

dominikschulz modified the milestones: 1.x.x, 1.7.1 - Bugfix release, 1.8.0 - Store Integrity May 23, 2018

rossmeier added 4 commits May 23, 2018 13:57

implemented git credential caching

3c4590b

code style and documentation

ec9a5fb

tests and handle existing entires

7fd796e

added tests

0b45a29

rossmeier force-pushed the feature/git-credential branch from f6104b3 to 0b45a29 Compare May 23, 2018 11:58

dominikschulz approved these changes May 23, 2018

View reviewed changes

dominikschulz merged commit a3d6081 into gopasspw:master May 23, 2018

kpitt pushed a commit to kpitt/gopass that referenced this pull request Jul 21, 2022

implemented git credential caching (gopasspw#743)

1daf7d7

* implemented git credential caching * code style and documentation * tests and handle existing entires * added tests

Uh oh!

implemented git credential caching #743

implemented git credential caching #743

Uh oh!

Conversation

rossmeier commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dominikschulz 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!

dominikschulz Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

dominikschulz commented May 14, 2018

Uh oh!

rossmeier commented May 15, 2018

Uh oh!

dominikschulz commented May 15, 2018

Uh oh!

dominikschulz commented May 23, 2018

Uh oh!

Uh oh!

rossmeier commented Apr 9, 2018 •

edited

Loading

codecov bot commented Apr 9, 2018 •

edited

Loading

dominikschulz Apr 10, 2018 •

edited

Loading