goetveld

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

  1. auth-cookie-fix
  2. Merge into trunk
Proposed by Ian Booth
Status: Merged
Merged at revision: 42
Proposed branch: lp:~wallyworld/goetveld/auth-cookie-fix
Merge into: lp:goetveld
Diff against target: 285 lines (+55/-32)
9 files modified
auth.go (+34/-9)
bazaar.go (+2/-3)
doc.go (+0/-1)
form.go (+4/-4)
html/parse.go (+1/-1)
html/parse_test.go (+1/-1)
html/token.go (+1/-1)
log.go (+7/-7)
rietveld.go (+5/-5)
To merge this branch: bzr merge lp:~wallyworld/goetveld/auth-cookie-fix
Reviewer Review Type Date Requested Status
John A Meinel (community) Approve
Dimiter Naydenov (community) Approve
Review via email: mp+147585@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

Please take a look.

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Looks good. I think we should land this.

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
81}81}
8282
83type standardAuth struct {83type standardAuth struct {
84 m sync.RWMutex84 m sync.RWMutex
85 lastLogin time.Time85 lastLogin time.Time
86 user string86 user string
87 ui AuthUI87 ui AuthUI
88 loginURL string88 loginURL string
89 cookies []*http.Cookie89 cookies []*http.Cookie
90}90}
9191
92var redirectBlocked = errors.New("redirect blocked")92var redirectBlocked = errors.New("redirect blocked")
9393
94// A simple cookie jar with just enough functionality for what we need here.
95type SimpleCookieJar struct {
96 m sync.Mutex
97 cookies map[string][]*http.Cookie
98}
99
100func (j *SimpleCookieJar) SetCookies(u *url.URL, cookies []*http.Cookie) {
101 j.m.Lock()
102 defer j.m.Unlock()
103 j.cookies[u.Host] = cookies
104}
105
106func (j *SimpleCookieJar) Cookies(u *url.URL) []*http.Cookie {
107 j.m.Lock()
108 defer j.m.Unlock()
109 return j.cookies[u.Host]
110}
111
94var client = http.Client{112var client = http.Client{
95 CheckRedirect: func(r *http.Request, v []*http.Request) error { return redirectBlocked },113 CheckRedirect: func(r *http.Request, v []*http.Request) error { return redirectBlocked },
114 Jar: &SimpleCookieJar{cookies: make(map[string][]*http.Cookie)},
96}115}
97116
98func (auth *standardAuth) Login(rietveldURL string, after time.Time) (err error) {117func (auth *standardAuth) Login(rietveldURL string, after time.Time) (err error) {
@@ -170,7 +189,11 @@
170 "continue": []string{marker},189 "continue": []string{marker},
171 "auth": []string{args["Auth"]},190 "auth": []string{args["Auth"]},
172 }191 }
173 r, err = client.Get(rietveldURL + "/_ah/login?" + authForm.Encode())192 authUrl, err := url.Parse(rietveldURL + "/_ah/login?" + authForm.Encode())
193 if err != nil {
194 return err
195 }
196 r, err = client.Get(authUrl.String())
174 if err == nil {197 if err == nil {
175 r.Body.Close()198 r.Body.Close()
176 return &LoginError{"AuthError", r.Status}199 return &LoginError{"AuthError", r.Status}
@@ -180,10 +203,12 @@
180203
181 logf("Login on %s successful.", rietveldURL)204 logf("Login on %s successful.", rietveldURL)
182 auth.cookies = nil205 auth.cookies = nil
183 for _, cookie := range r.Cookies() {206 for _, cookie := range client.Jar.Cookies(authUrl) {
184 auth.cookies = append(auth.cookies, cookie)207 auth.cookies = append(auth.cookies, cookie)
185 }208 }
186 auth.lastLogin = time.Now()209 if auth.cookies != nil {
210 auth.lastLogin = time.Now()
211 }
187 return nil212 return nil
188}213}
189214
190215
=== 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 @@
6262
63var bzrRe = regexp.MustCompile(`(?m)^=== (added|removed|renamed|modified) (file|directory) (?:'.*' => )?'(.*)'(?: \(prop.*)?$`)63var bzrRe = regexp.MustCompile(`(?m)^=== (added|removed|renamed|modified) (file|directory) (?:'.*' => )?'(.*)'(?: \(prop.*)?$`)
6464
65
66var bzrRevInfo = `=== added file '[revision details]'65var bzrRevInfo = `=== added file '[revision details]'
67--- [revision details] 2012-01-01 00:00:00 +000066--- [revision details] 2012-01-01 00:00:00 +0000
68+++ [revision details] 2012-01-01 00:00:00 +000067+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -85,7 +84,7 @@
8584
86 diff := &FileDiff{85 diff := &FileDiff{
87 Path: "[revision details]",86 Path: "[revision details]",
88 Op: Added,87 Op: Added,
89 Text: []byte(fmt.Sprintf(bzrRevInfo, b.oldRevision, b.newRevision)),88 Text: []byte(fmt.Sprintf(bzrRevInfo, b.oldRevision, b.newRevision)),
90 }89 }
91 patch = append(patch, diff)90 patch = append(patch, diff)
@@ -114,7 +113,7 @@
114}113}
115114
116func (b *bzrBranches) Base(filename string) (io.ReadCloser, error) {115func (b *bzrBranches) Base(filename string) (io.ReadCloser, error) {
117 output, _, err := run("bzr", "cat", "-r", "revid:" + b.oldRevision, filepath.Join(b.newPath, filename))116 output, _, err := run("bzr", "cat", "-r", "revid:"+b.oldRevision, filepath.Join(b.newPath, filename))
118 if err != nil {117 if err != nil {
119 return nil, err118 return nil, err
120 }119 }
121120
=== 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 @@
22 fmt.Printf("Created new issue: %s\n", rietveld.CodeReview.IssueURL(issue.Id))22 fmt.Printf("Created new issue: %s\n", rietveld.CodeReview.IssueURL(issue.Id))
23*/23*/
24package rietveld24package rietveld
25
2625
=== 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 @@
4 "bytes"4 "bytes"
5 "encoding/json"5 "encoding/json"
6 "fmt"6 "fmt"
7 "launchpad.net/goetveld/rietveld/html"
8 "io"7 "io"
9 "io/ioutil"8 "io/ioutil"
9 "launchpad.net/goetveld/rietveld/html"
10 "mime/multipart"10 "mime/multipart"
11 "net/http"11 "net/http"
12 "reflect"12 "reflect"
@@ -111,7 +111,7 @@
111}111}
112112
113type editLoadHandler struct {113type editLoadHandler struct {
114 op *opInfo114 op *opInfo
115 form chan map[string]string115 form chan map[string]string
116}116}
117117
@@ -137,7 +137,7 @@
137}137}
138138
139type editHandler struct {139type editHandler struct {
140 op *opInfo140 op *opInfo
141 form <-chan map[string]string141 form <-chan map[string]string
142}142}
143143
@@ -155,7 +155,7 @@
155 diffNicks := !reflect.DeepEqual(oldNicks, newNicks)155 diffNicks := !reflect.DeepEqual(oldNicks, newNicks)
156 if diffMails && diffNicks {156 if diffMails && diffNicks {
157 // Send both. The server will deduplicate.157 // Send both. The server will deduplicate.
158 addrs = make([]string, 0, len(newMails) + len(newNicks))158 addrs = make([]string, 0, len(newMails)+len(newNicks))
159 addrs = append(addrs, newMails...)159 addrs = append(addrs, newMails...)
160 addrs = append(addrs, newNicks...)160 addrs = append(addrs, newNicks...)
161 } else if diffMails {161 } else if diffMails {
162162
=== 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 @@
66
7import (7import (
8 "errors"8 "errors"
9 a "launchpad.net/goetveld/rietveld/html/atom"
10 "fmt"9 "fmt"
11 "io"10 "io"
11 a "launchpad.net/goetveld/rietveld/html/atom"
12 "strings"12 "strings"
13)13)
1414
1515
=== 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 @@
8 "bufio"8 "bufio"
9 "bytes"9 "bytes"
10 "errors"10 "errors"
11 "launchpad.net/goetveld/rietveld/html/atom"
12 "flag"11 "flag"
13 "fmt"12 "fmt"
14 "io"13 "io"
15 "io/ioutil"14 "io/ioutil"
15 "launchpad.net/goetveld/rietveld/html/atom"
16 "os"16 "os"
17 "path/filepath"17 "path/filepath"
18 "runtime"18 "runtime"
1919
=== 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 @@
66
7import (7import (
8 "bytes"8 "bytes"
9 "io"
9 "launchpad.net/goetveld/rietveld/html/atom"10 "launchpad.net/goetveld/rietveld/html/atom"
10 "io"
11 "strconv"11 "strconv"
12 "strings"12 "strings"
13)13)
1414
=== 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 @@
15var globalLogger log_Logger15var globalLogger log_Logger
16var globalDebug bool16var globalDebug bool
1717
18const logPrefix = "RIETVELD " 18const logPrefix = "RIETVELD "
1919
20// Specify the *log.Logger object where log messages should be sent to.20// Specify the *log.Logger object where log messages should be sent to.
21func SetLogger(logger log_Logger) {21func SetLogger(logger log_Logger) {
@@ -30,36 +30,36 @@
3030
31func log(v ...interface{}) {31func log(v ...interface{}) {
32 if globalLogger != nil {32 if globalLogger != nil {
33 globalLogger.Output(2, logPrefix + fmt.Sprint(v...))33 globalLogger.Output(2, logPrefix+fmt.Sprint(v...))
34 }34 }
35}35}
3636
37func logln(v ...interface{}) {37func logln(v ...interface{}) {
38 if globalLogger != nil {38 if globalLogger != nil {
39 globalLogger.Output(2, logPrefix + fmt.Sprintln(v...))39 globalLogger.Output(2, logPrefix+fmt.Sprintln(v...))
40 }40 }
41}41}
4242
43func logf(format string, v ...interface{}) {43func logf(format string, v ...interface{}) {
44 if globalLogger != nil {44 if globalLogger != nil {
45 globalLogger.Output(2, logPrefix + fmt.Sprintf(format, v...))45 globalLogger.Output(2, logPrefix+fmt.Sprintf(format, v...))
46 }46 }
47}47}
4848
49func debug(v ...interface{}) {49func debug(v ...interface{}) {
50 if globalDebug && globalLogger != nil {50 if globalDebug && globalLogger != nil {
51 globalLogger.Output(2, logPrefix + fmt.Sprint(v...))51 globalLogger.Output(2, logPrefix+fmt.Sprint(v...))
52 }52 }
53}53}
5454
55func debugln(v ...interface{}) {55func debugln(v ...interface{}) {
56 if globalDebug && globalLogger != nil {56 if globalDebug && globalLogger != nil {
57 globalLogger.Output(2, logPrefix + fmt.Sprintln(v...))57 globalLogger.Output(2, logPrefix+fmt.Sprintln(v...))
58 }58 }
59}59}
6060
61func debugf(format string, v ...interface{}) {61func debugf(format string, v ...interface{}) {
62 if globalDebug && globalLogger != nil {62 if globalDebug && globalLogger != nil {
63 globalLogger.Output(2, logPrefix + fmt.Sprintf(format, v...))63 globalLogger.Output(2, logPrefix+fmt.Sprintf(format, v...))
64 }64 }
65}65}
6666
=== 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 @@
7676
77// Comment holds a message to be added to an issue's thread.77// Comment holds a message to be added to an issue's thread.
78type Comment struct {78type Comment struct {
79 Subject string79 Subject string
80 Message string80 Message string
8181
82 // If Reviewers and/or Cc is not nil, the respective list of people82 // If Reviewers and/or Cc is not nil, the respective list of people
83 // in the issue will be updated to the provided list before the83 // in the issue will be updated to the provided list before the
84 // comment is added.84 // comment is added.
85 Reviewers []string85 Reviewers []string
86 Cc []string86 Cc []string
8787
88 // If NoMail is true, do not mail people when adding comment.88 // If NoMail is true, do not mail people when adding comment.
89 NoMail bool89 NoMail bool
9090
91 // If PublishDrafts is true, inline comments made in the code91 // If PublishDrafts is true, inline comments made in the code
92 // and not yet published will be delivered.92 // and not yet published will be delivered.

Subscribers

People subscribed via source and target branches