Merge lp:~vds/usso/validity_check_and_better_auth_failure_handling into lp:usso

Proposed by Vincenzo Di Somma
Status: Merged
Approved by: Vincenzo Di Somma
Approved revision: 43
Merged at revision: 43
Proposed branch: lp:~vds/usso/validity_check_and_better_auth_failure_handling
Merge into: lp:usso
Diff against target: 232 lines (+78/-70)
2 files modified
usso.go (+47/-51)
usso_test.go (+31/-19)
To merge this branch: bzr merge lp:~vds/usso/validity_check_and_better_auth_failure_handling
Reviewer Review Type Date Requested Status
Casey Marshall (community) Approve
Andrew W. Deane (community) Approve
Review via email: mp+208609@code.launchpad.net

Commit message

Improve error handling. Add explicit token validity check.

Description of the change

Improve error handling. Add explicit token validity check.

To post a comment you must log in.
Revision history for this message
Andrew W. Deane (andrew-w-deane) wrote :

Instead of declaring jsonBuffer as an interface at 64+ and then casting to a map at 70+ could jsonBuffer just be declared as a map leaving json.Unmarshal to do the work?

At 75+ if USSO change there source and the json code field is no longer a string then we will panic. If 75+ was replaced with the code below we won't panic should the external source change.

return "", fmt.Errorf("%v",code)

Do you think it is worth changing the code to be on the safe side?

The above questions for 124+, 137+, and 142+ too.

41. By Vincenzo Di Somma

Avoiding useless interface/casting, using fmt to prevent failures due to possible changes in u1 sso.

Revision history for this message
Vincenzo Di Somma (vds) wrote :

> Instead of declaring jsonBuffer as an interface at 64+ and then casting to a
> map at 70+ could jsonBuffer just be declared as a map leaving json.Unmarshal
> to do the work?
>
> At 75+ if USSO change there source and the json code field is no longer a
> string then we will panic. If 75+ was replaced with the code below we won't
> panic should the external source change.
>
> return "", fmt.Errorf("%v",code)
>
> Do you think it is worth changing the code to be on the safe side?
>
> The above questions for 124+, 137+, and 142+ too.

All good suggestions, all fixed, thanks.

42. By Vincenzo Di Somma

Stop using errors module use fmt.Error instead.

43. By Vincenzo Di Somma

Stop using errors module use fmt.Error instead.

Revision history for this message
Andrew W. Deane (andrew-w-deane) :
review: Approve
Revision history for this message
Casey Marshall (cmars) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'usso.go'
2--- usso.go 2014-02-11 11:10:26 +0000
3+++ usso.go 2014-02-27 17:23:23 +0000
4@@ -1,12 +1,9 @@
5 package usso
6
7 import (
8- "bytes"
9 "encoding/json"
10- "errors"
11 "fmt"
12 "io/ioutil"
13- "log"
14 "net/http"
15 "strings"
16 )
17@@ -56,7 +53,6 @@
18 }
19 jsonCredentials, err := json.Marshal(credentials)
20 if err != nil {
21- log.Printf("Error: %s\n", err)
22 return nil, err
23 }
24 response, err := http.Post(
25@@ -67,20 +63,18 @@
26 return nil, err
27 }
28 if response.StatusCode == 404 {
29- return nil, errors.New("Wrong credentials.")
30+ return nil, fmt.Errorf("Wrong credentials.")
31 }
32 if response.StatusCode != 200 && response.StatusCode != 201 {
33 return nil, fmt.Errorf("SSO Error: %s\n", response.Status)
34 }
35 body, err := ioutil.ReadAll(response.Body)
36 if err != nil {
37- log.Println(err)
38 return nil, err
39 }
40 ssodata := SSOData{}
41 err = json.Unmarshal(body, &ssodata)
42 if err != nil {
43- log.Println(err)
44 return nil, err
45 }
46 ssodata.Realm = "API"
47@@ -105,15 +99,28 @@
48 client := &http.Client{}
49 response, err := client.Do(request)
50 if err != nil {
51- fmt.Printf("Error: %s\n", err)
52+ return "", err
53 }
54+
55 body, err := ioutil.ReadAll(response.Body)
56 if err != nil {
57- fmt.Println(err)
58- }
59- var b bytes.Buffer
60- b.Write(body)
61- return fmt.Sprint(b.String()), nil
62+ return "", err
63+ }
64+ if response.StatusCode == 200 {
65+ return string(body), nil
66+ } else {
67+ var jsonMap map[string]interface{}
68+ err = json.Unmarshal(body, &jsonMap)
69+ // In theory, this should never happen.
70+ if err != nil {
71+ return "", fmt.Errorf("NO_JSON_RESPONSE")
72+ }
73+ code, ok := jsonMap["code"]
74+ if !ok {
75+ return "", fmt.Errorf("NO_CODE")
76+ }
77+ return "", fmt.Errorf("%v", code)
78+ }
79 }
80
81 // Given oauth credentials and a request, return it signed.
82@@ -147,46 +154,35 @@
83 client := &http.Client{}
84 response, err := client.Do(request)
85 if err != nil {
86- fmt.Printf("Error: %s\n", err)
87+ return "", err
88 }
89 body, err := ioutil.ReadAll(response.Body)
90 if err != nil {
91- fmt.Println(err)
92- }
93- var b bytes.Buffer
94- b.Write(body)
95- return fmt.Sprint(b.String()), nil
96-}
97-
98-// Register the toke to the U1 File Sync Service.
99-func (server UbuntuSSOServer) RegisterTokenToU1FileSync(ssodata *SSOData) (err error) {
100- rp := RequestParameters{
101- BaseURL: server.tokenRegistrationUrl,
102- HTTPMethod: "GET",
103- SignatureMethod: HMACSHA1{}}
104-
105- request, err := http.NewRequest(rp.HTTPMethod, rp.BaseURL, nil)
106- if err != nil {
107- return err
108- }
109- ssodata.Realm = ""
110- err = SignRequest(ssodata, &rp, request)
111- if err != nil {
112- return err
113- }
114- client := &http.Client{}
115- response, err := client.Do(request)
116- if err != nil {
117- fmt.Printf("Error: %s\n", err)
118- }
119- if response.StatusCode != 200 {
120- body, err := ioutil.ReadAll(response.Body)
121+ return "", err
122+ }
123+ if response.StatusCode == 200 {
124+ return string(body), nil
125+ } else {
126+ var jsonMap map[string]interface{}
127+ err = json.Unmarshal(body, &jsonMap)
128+ // due to bug #1285176, it is possible to get non json code in the response.
129 if err != nil {
130- fmt.Println(err)
131- }
132- var b bytes.Buffer
133- b.Write(body)
134- errors.New(fmt.Sprint(b.String()))
135- }
136- return nil
137+ return "", fmt.Errorf("INVALID_CREDENTIALS")
138+ }
139+ code, ok := jsonMap["code"]
140+ if !ok {
141+ return "", fmt.Errorf("NO_CODE")
142+ }
143+ return "", fmt.Errorf("%v", code)
144+ }
145+}
146+
147+// Verify the validity of the token, abusing the API to get the token details.
148+func (server UbuntuSSOServer) IsTokenValid(ssodata *SSOData) (bool, error) {
149+ details, err := server.GetTokenDetails(ssodata)
150+ if details != "" && err == nil {
151+ return true, nil
152+ } else {
153+ return false, err
154+ }
155 }
156
157=== modified file 'usso_test.go'
158--- usso_test.go 2014-01-22 16:58:23 +0000
159+++ usso_test.go 2014-02-27 17:23:23 +0000
160@@ -64,16 +64,6 @@
161 if r.URL.String() == "/api/v2/tokens/oauth/abcs" {
162 fmt.Fprint(w, tokenDetails)
163 return
164- }
165- if r.URL.String() == "/oauth/sso-finished-so-get-tokens/" {
166- fmt.Fprint(w, "ok")
167-
168- concat := ""
169- for _, v := range r.Header["Authorization"] {
170- concat += v
171- }
172- requestContent = concat
173- return
174 } else {
175 http.Error(w, "404 page not found", http.StatusNotFound)
176 return
177@@ -170,7 +160,7 @@
178 c.Assert(token_details, Equals, string(jsonTokenDetails))
179 }
180
181-func (suite *USSOTestSuite) TestRegisterToken(c *C) {
182+func (suite *USSOTestSuite) TestTokenValidity(c *C) {
183 // Simulate a valid Ubuntu SSO Server response.
184 serverResponseData := map[string]string{
185 "date_updated": "2013-01-16 14:03:36",
186@@ -184,16 +174,38 @@
187 if err != nil {
188 panic(err)
189 }
190- server := newTestServer(string(jsonServerResponseData), "{}", 200)
191- var testSSOServer = &UbuntuSSOServer{server.URL, server.URL + "/oauth/sso-finished-so-get-tokens/"}
192+ tokenDetails := map[string]string{
193+ "token_name": tokenName,
194+ "date_updated": "2014-01-22T13:35:49.867",
195+ "token_key": tokenKey,
196+ "href": "/api/v2/tokens/oauth/JckChNpbXxPRmPkElLglSnqnjsnGseWJmNqTJCWfUtNBSsGtoG",
197+ "date_created": "2014-01-17T20:03:24.993",
198+ "consumer_key": consumerKey,
199+ }
200+ jsonTokenDetails, err := json.Marshal(tokenDetails)
201+ if err != nil {
202+ panic(err)
203+ }
204+ server := newTestServer(string(jsonServerResponseData), string(jsonTokenDetails), 200)
205+ var testSSOServer = &UbuntuSSOServer{server.URL, ""}
206 defer server.Close()
207 ssodata, err := testSSOServer.GetToken(email, password, tokenName)
208- err = testSSOServer.RegisterTokenToU1FileSync(ssodata)
209+ // The returned information is correct.
210+ token_details, err := testSSOServer.GetTokenDetails(ssodata)
211 c.Assert(err, IsNil)
212- c.Assert(true, Equals, strings.Contains(*server.requestContent, "oauth_consumer_key=\"rfyzhdQ\""))
213- c.Assert(true, Equals, strings.Contains(*server.requestContent, "oauth_token=\"abcs\""))
214- c.Assert(true, Equals, strings.Contains(*server.requestContent, "oauth_signature_method=\"HMAC-SHA1\""))
215- c.Assert(true, Equals, strings.Contains(*server.requestContent, "oauth_version=\"1.0\""))
216- c.Assert(true, Equals, strings.Contains(*server.requestContent, ""))
217+ //The request that the fake Ubuntu SSO Server has the token details.
218+ c.Assert(token_details, Equals, string(jsonTokenDetails))
219+ validity, err := testSSOServer.IsTokenValid(ssodata)
220+ c.Assert(validity, Equals, true)
221+}
222
223+// Check invalid token
224+func (suite *USSOTestSuite) TestInvalidToken(c *C) {
225+ server := newTestServer("{}", "{}", 200)
226+ var testSSOServer = &UbuntuSSOServer{server.URL, ""}
227+ defer server.Close()
228+ ssodata := SSOData{"WRONG", "", "", "", "", ""}
229+ validity, err := testSSOServer.IsTokenValid(&ssodata)
230+ c.Assert(err, NotNil)
231+ c.Assert(validity, Equals, false)
232 }

Subscribers

People subscribed via source and target branches

to all changes: