Merge lp:~wallyworld/goetveld/auth-cookie-fix into lp:goetveld

[Ian Booth]

Fix auth cookie handling

I came across this issue when my cached credentials expired and I couldn't login again to propose a code review.

It turns out that the authentication with my username/password was succeeding, and the server returned the correct
cookie information, but the cookies were being ignored by the client and not saved to the .goetveld_codereview.appspot.com
file. Playing with curl and reviewing the Go http client source showed that unless a cookie jar was set up when the client
is created, any cookies generated by the server are ignored. And the relevant tests failed also it turns out.

I've made the necessary changes and everything now works. Perhaps this library was developed using an earlier version of Go
where the API was different? Do we need to test if this change works with eariler Go versions? I'm using 1.0.3.

My changes only affected the auth.go file, but when I ran go fmt over the code base it made small changes to a bunch of other files, and these are included also.

[Ian Booth (wallyworld)]

Please take a look.

[Dimiter Naydenov (dimitern)]

Looks good. I think we should land this.

review: Approve

[John A Meinel (jameinel)]

It would probably be easier to review if you posted the 'go fmt' fix before the cookie change, but I can confirm that with go-1.0.3 I'm able to use lbox now, so LGTM.

review: Approve

=== modified file 'auth.go'
--- auth.go	2012-03-12 20:12:51 +0000
+++ auth.go	2013-02-11 01:15:27 +0000
@@ -81,18 +81,37 @@
 }
 
 type standardAuth struct {
-	m           sync.RWMutex
-	lastLogin   time.Time
-	user        string
-	ui          AuthUI
-	loginURL    string
-	cookies     []*http.Cookie
+	m         sync.RWMutex
+	lastLogin time.Time
+	user      string
+	ui        AuthUI
+	loginURL  string
+	cookies   []*http.Cookie
 }
 
 var redirectBlocked = errors.New("redirect blocked")
 
+// A simple cookie jar with just enough functionality for what we need here.
+type SimpleCookieJar struct {
+	m       sync.Mutex
+	cookies map[string][]*http.Cookie
+}
+
+func (j *SimpleCookieJar) SetCookies(u *url.URL, cookies []*http.Cookie) {
+	j.m.Lock()
+	defer j.m.Unlock()
+	j.cookies[u.Host] = cookies
+}
+
+func (j *SimpleCookieJar) Cookies(u *url.URL) []*http.Cookie {
+	j.m.Lock()
+	defer j.m.Unlock()
+	return j.cookies[u.Host]
+}
+
 var client = http.Client{
 	CheckRedirect: func(r *http.Request, v []*http.Request) error { return redirectBlocked },
+	Jar:           &SimpleCookieJar{cookies: make(map[string][]*http.Cookie)},
 }
 
 func (auth *standardAuth) Login(rietveldURL string, after time.Time) (err error) {
@@ -170,7 +189,11 @@
 		"continue": []string{marker},
 		"auth":     []string{args["Auth"]},
 	}
-	r, err = client.Get(rietveldURL + "/_ah/login?" + authForm.Encode())
+	authUrl, err := url.Parse(rietveldURL + "/_ah/login?" + authForm.Encode())
+	if err != nil {
+		return err
+	}
+	r, err = client.Get(authUrl.String())
 	if err == nil {
 		r.Body.Close()
 		return &LoginError{"AuthError", r.Status}
@@ -180,10 +203,12 @@
 
 	logf("Login on %s successful.", rietveldURL)
 	auth.cookies = nil
-	for _, cookie := range r.Cookies() {
+	for _, cookie := range client.Jar.Cookies(authUrl) {
 		auth.cookies = append(auth.cookies, cookie)
 	}
-	auth.lastLogin = time.Now()
+	if auth.cookies != nil {
+		auth.lastLogin = time.Now()
+	}
 	return nil
 }
 

=== modified file 'bazaar.go'
--- bazaar.go	2012-11-22 05:45:31 +0000
+++ bazaar.go	2013-02-11 01:15:27 +0000
@@ -62,7 +62,6 @@
 
 var bzrRe = regexp.MustCompile(`(?m)^=== (added|removed|renamed|modified) (file|directory) (?:'.*' => )?'(.*)'(?: \(prop.*)?$`)
 
-
 var bzrRevInfo = `=== added file '[revision details]'
 --- [revision details]	2012-01-01 00:00:00 +0000
 +++ [revision details]	2012-01-01 00:00:00 +0000
@@ -85,7 +84,7 @@
 
 	diff := &FileDiff{
 		Path: "[revision details]",
-		Op: Added,
+		Op:   Added,
 		Text: []byte(fmt.Sprintf(bzrRevInfo, b.oldRevision, b.newRevision)),
 	}
 	patch = append(patch, diff)
@@ -114,7 +113,7 @@
 }
 
 func (b *bzrBranches) Base(filename string) (io.ReadCloser, error) {
-	output, _, err := run("bzr", "cat", "-r", "revid:" + b.oldRevision, filepath.Join(b.newPath, filename))
+	output, _, err := run("bzr", "cat", "-r", "revid:"+b.oldRevision, filepath.Join(b.newPath, filename))
 	if err != nil {
 		return nil, err
 	}

=== modified file 'doc.go'
--- doc.go	2011-11-17 05:54:48 +0000
+++ doc.go	2013-02-11 01:15:27 +0000
@@ -22,4 +22,3 @@
 	fmt.Printf("Created new issue: %s\n", rietveld.CodeReview.IssueURL(issue.Id))
 */
 package rietveld
-

=== modified file 'form.go'
--- form.go	2012-07-03 18:03:56 +0000
+++ form.go	2013-02-11 01:15:27 +0000
@@ -4,9 +4,9 @@
 	"bytes"
 	"encoding/json"
 	"fmt"
-	"launchpad.net/goetveld/rietveld/html"
 	"io"
 	"io/ioutil"
+	"launchpad.net/goetveld/rietveld/html"
 	"mime/multipart"
 	"net/http"
 	"reflect"
@@ -111,7 +111,7 @@
 }
 
 type editLoadHandler struct {
-	op *opInfo
+	op   *opInfo
 	form chan map[string]string
 }
 
@@ -137,7 +137,7 @@
 }
 
 type editHandler struct {
-	op *opInfo
+	op   *opInfo
 	form <-chan map[string]string
 }
 
@@ -155,7 +155,7 @@
 	diffNicks := !reflect.DeepEqual(oldNicks, newNicks)
 	if diffMails && diffNicks {
 		// Send both. The server will deduplicate.
-		addrs = make([]string, 0, len(newMails) + len(newNicks))
+		addrs = make([]string, 0, len(newMails)+len(newNicks))
 		addrs = append(addrs, newMails...)
 		addrs = append(addrs, newNicks...)
 	} else if diffMails {

=== modified file 'html/parse.go'
--- html/parse.go	2012-07-03 18:03:56 +0000
+++ html/parse.go	2013-02-11 01:15:27 +0000
@@ -6,9 +6,9 @@
 
 import (
 	"errors"
-	a "launchpad.net/goetveld/rietveld/html/atom"
 	"fmt"
 	"io"
+	a "launchpad.net/goetveld/rietveld/html/atom"
 	"strings"
 )
 

=== modified file 'html/parse_test.go'
--- html/parse_test.go	2012-07-03 18:03:56 +0000
+++ html/parse_test.go	2013-02-11 01:15:27 +0000
@@ -8,11 +8,11 @@
 	"bufio"
 	"bytes"
 	"errors"
-	"launchpad.net/goetveld/rietveld/html/atom"
 	"flag"
 	"fmt"
 	"io"
 	"io/ioutil"
+	"launchpad.net/goetveld/rietveld/html/atom"
 	"os"
 	"path/filepath"
 	"runtime"

=== modified file 'html/token.go'
--- html/token.go	2012-07-03 18:03:56 +0000
+++ html/token.go	2013-02-11 01:15:27 +0000
@@ -6,8 +6,8 @@
 
 import (
 	"bytes"
+	"io"
 	"launchpad.net/goetveld/rietveld/html/atom"
-	"io"
 	"strconv"
 	"strings"
 )

=== modified file 'log.go'
--- log.go	2011-11-09 16:03:12 +0000
+++ log.go	2013-02-11 01:15:27 +0000
@@ -15,7 +15,7 @@
 var globalLogger log_Logger
 var globalDebug bool
 
-const logPrefix = "RIETVELD " 
+const logPrefix = "RIETVELD "
 
 // Specify the *log.Logger object where log messages should be sent to.
 func SetLogger(logger log_Logger) {
@@ -30,36 +30,36 @@
 
 func log(v ...interface{}) {
 	if globalLogger != nil {
-		globalLogger.Output(2, logPrefix + fmt.Sprint(v...))
+		globalLogger.Output(2, logPrefix+fmt.Sprint(v...))
 	}
 }
 
 func logln(v ...interface{}) {
 	if globalLogger != nil {
-		globalLogger.Output(2, logPrefix + fmt.Sprintln(v...))
+		globalLogger.Output(2, logPrefix+fmt.Sprintln(v...))
 	}
 }
 
 func logf(format string, v ...interface{}) {
 	if globalLogger != nil {
-		globalLogger.Output(2, logPrefix + fmt.Sprintf(format, v...))
+		globalLogger.Output(2, logPrefix+fmt.Sprintf(format, v...))
 	}
 }
 
 func debug(v ...interface{}) {
 	if globalDebug && globalLogger != nil {
-		globalLogger.Output(2, logPrefix + fmt.Sprint(v...))
+		globalLogger.Output(2, logPrefix+fmt.Sprint(v...))
 	}
 }
 
 func debugln(v ...interface{}) {
 	if globalDebug && globalLogger != nil {
-		globalLogger.Output(2, logPrefix + fmt.Sprintln(v...))
+		globalLogger.Output(2, logPrefix+fmt.Sprintln(v...))
 	}
 }
 
 func debugf(format string, v ...interface{}) {
 	if globalDebug && globalLogger != nil {
-		globalLogger.Output(2, logPrefix + fmt.Sprintf(format, v...))
+		globalLogger.Output(2, logPrefix+fmt.Sprintf(format, v...))
 	}
 }

=== modified file 'rietveld.go'
--- rietveld.go	2012-03-15 02:02:17 +0000
+++ rietveld.go	2013-02-11 01:15:27 +0000
@@ -76,17 +76,17 @@
 
 // Comment holds a message to be added to an issue's thread.
 type Comment struct {
-	Subject       string
-	Message       string
+	Subject string
+	Message string
 
 	// If Reviewers and/or Cc is not nil, the respective list of people
 	// in the issue will be updated to the provided list before the
 	// comment is added.
-	Reviewers     []string
-	Cc            []string
+	Reviewers []string
+	Cc        []string
 
 	// If NoMail is true, do not mail people when adding comment.
-	NoMail        bool
+	NoMail bool
 
 	// If PublishDrafts is true, inline comments made in the code
 	// and not yet published will be delivered.