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

[Vincenzo Di Somma]

"Got rid of useless error, using a struct to handle the oauth_signature_method, separated ssodata from temporary request parameters, fixed tests and example accordingly.

"Got rid of useless error, using a struct to handle the oauth_signature_method, separated ssodata from temporary request parameters, fixed tests and example accordingly.

[Domas Monkus (tasdomas)]

Looks ok.

review: Approve

[Graham Binns (gmb)]

Hi Vincenzo,

Thank you for this branch! There are a couple of things you need to add before you land this, but there's nothing that needs me to re-review it after you've made the changes, so feel free to make them and then mark the MP as approved.

[1]

149 +func (PLAINTEXT) Name() string { return "PLAINTEXT" }
150 +func (PLAINTEXT) Signature(

160 +func (HMACSHA1) Name() string { return "HMAC-SHA1" }
161 +func (HMACSHA1) Signature(

These all need some documentation comments.

[2]

275 +// Test the request signing with oauth_signature_method = PLAINTEXT
301 // Test the request signing with oauth_signature_method = SHA1

We should avoid the "Test the..." style when writing documentation for tests. Instead, we should write things like:

  // It is possible to sign a request with a PLAINTEXT oauth_signature_method.
  // It is possible to sign a request with a SHA1 oauth_signature_method.

Put another way, the documentation on a test should be something that can be declared true if the test passes, false otherwise.

review: Approve (code)

[lp:~vds/usso/handle_order_of_parameters]

25. By Vincenzo Di Somma on 2013-02-05

Fixing/adding comments and cleaning up.

=== modified file 'example/usso_example.go'
--- example/usso_example.go	2013-01-25 16:39:11 +0000
+++ example/usso_example.go	2013-02-05 12:38:20 +0000
@@ -9,7 +9,7 @@
 	"net/http"
 )
 
-var email, password, tokenName, signature_method string
+var email, password string
 
 func inputParams() {
 	fmt.Println("This application will query the staging Ubuntu SSO Server" +
@@ -19,9 +19,6 @@
 	fmt.Print("Enter password: ")
 	fmt.Scanf("%s", &password)
 	fmt.Print("Enter token name: ")
-	fmt.Scanf("%s", &tokenName)
-	fmt.Print("Enter signature method (PLAINTEXT or HMAC-SHA1): ")
-	fmt.Scanf("%s", &signature_method)
 }
 
 func main() {
@@ -32,7 +29,7 @@
 	server := usso.StagingUbuntuSSOServer
 	// One would use server := usso.ProductionUbuntuSSOServer 
 	// to use the production Ubuntu SSO Server.
-	ssodata, err := server.GetToken(email, password, tokenName)
+	ssodata, err := server.GetToken(email, password, "usso")
 	if err != nil {
 		panic(err)
 	}
@@ -48,14 +45,12 @@
 	//fmt.Printf("Got accounts info: %s\n", accounts)
 
 	// But this shows how to sign a generic request.
-	ssodata.BaseURL = fmt.Sprintf(
+	rp := usso.RequestParameters{BaseURL: fmt.Sprintf(
 		"https://login.staging.ubuntu.com/api/v2/accounts/%s",
-		ssodata.ConsumerKey)
-	ssodata.HTTPMethod = "GET"
-	ssodata.SignatureMethod = signature_method
-	request, _ := http.NewRequest(ssodata.HTTPMethod, ssodata.BaseURL, nil)
-	usso.SignRequest(ssodata, request)
-
+		ssodata.ConsumerKey), HTTPMethod: "GET",
+		SignatureMethod: usso.HMACSHA1{}}
+	request, _ := http.NewRequest(rp.HTTPMethod, rp.BaseURL, nil)
+	usso.SignRequest(ssodata, &rp, request)
 	if err != nil {
 		fmt.Printf("Error: %s\n", err)
 	}
 
=== modified file 'oauth.go'
--- oauth.go	2013-01-29 11:23:37 +0000
+++ oauth.go	2013-02-05 12:38:20 +0000
@@ -4,7 +4,6 @@
 	"crypto/hmac"
 	"crypto/sha1"
 	"encoding/base64"
-	"errors"
 	"fmt"
 	"math/rand"
 	"net/http"
@@ -30,72 +29,91 @@
 
 // Contains the oauth data to perform a request.
 type SSOData struct {
-	HTTPMethod      string     `json:"-"`
-	BaseURL         string     `json:"-"`
-	Params          url.Values `json:"-"`
-	Nonce           string     `json:"-"`
-	Timestamp       string     `json:"-"`
-	SignatureMethod string     `json:"-"`
-	ConsumerKey     string     `json:"consumer_key"`
-	ConsumerSecret  string     `json:"consumer_secret"`
-	TokenKey        string     `json:"token_key"`
-	TokenName       string     `json:"token_name"`
-	TokenSecret     string     `json:"token_secret"`
-}
-
-// Depending on the signature method, create the signature from the 
-// consumer secret, the token secret and, if required, the URL.
-// Supported signature methods are PLAINTEXT and HMAC-SHA1.
-func (oauth *SSOData) signature() (string, error) {
-	switch oauth.SignatureMethod {
-	case "PLAINTEXT":
-		return fmt.Sprintf(
-			`%s%%26%s`,
-			oauth.ConsumerSecret,
-			oauth.TokenSecret), nil
-	case "HMAC-SHA1":
-		base_url, err := NormalizeURL(oauth.BaseURL)
-		if err != nil {
-			return "", err
-		}
-		params, err := NormalizeParameters(oauth.Params)
-		if err != nil {
-			return "", err
-		}
-		base_string := fmt.Sprintf(`%s&%s&%s%s%s%s%s%s%s`,
-			oauth.HTTPMethod,
-			url.QueryEscape(base_url),
-			url.QueryEscape(params),
-			url.QueryEscape("oauth_consumer_key="+oauth.ConsumerKey),
-			url.QueryEscape("&oauth_nonce="+oauth.Nonce),
-			url.QueryEscape("&oauth_signature_method="+oauth.SignatureMethod),
-			url.QueryEscape("&oauth_timestamp="+oauth.Timestamp),
-			url.QueryEscape("&oauth_token="+oauth.TokenKey),
-			url.QueryEscape("&oauth_version=1.0"))
-		hashfun := hmac.New(sha1.New, []byte(
-			oauth.ConsumerSecret+"&"+oauth.TokenSecret))
-		hashfun.Write([]byte(base_string))
-		rawsignature := hashfun.Sum(nil)
-		base64signature := make(
-			[]byte, base64.StdEncoding.EncodedLen(len(rawsignature)))
-		base64.StdEncoding.Encode(base64signature, rawsignature)
-		return string(base64signature), nil
-	default:
-		return "", errors.New(
-			"usso/oauth: Oauth Signature Method not supported.")
-	}
-	return "", nil
+	ConsumerKey    string `json:"consumer_key"`
+	ConsumerSecret string `json:"consumer_secret"`
+	TokenKey       string `json:"token_key"`
+	TokenName      string `json:"token_name"`
+	TokenSecret    string `json:"token_secret"`
+}
+
+type RequestParameters struct {
+	HTTPMethod      string
+	BaseURL         string
+	Params          url.Values
+	Nonce           string
+	Timestamp       string
+	SignatureMethod SignatureMethod
+}
+
+type SignatureMethod interface {
+	Name() string
+	Signature(
+		ssodata *SSOData, rp *RequestParameters) (string, error)
+}
+
+type PLAINTEXT struct{}
+
+// Return the name of the signature method, used to compose the 
+// Authentication Header.
+func (PLAINTEXT) Name() string { return "PLAINTEXT" }
+
+// Calculate the oaut_signature part of the Authentication Header.
+func (PLAINTEXT) Signature(
+	ssodata *SSOData, rp *RequestParameters) (string, error) {
+	return fmt.Sprintf(
+		`%s&%s`,
+		ssodata.ConsumerSecret,
+		ssodata.TokenSecret), nil
+}
+
+type HMACSHA1 struct{}
+
+// Return the name of the signature method, used to compose the 
+// Authentication Header.
+func (HMACSHA1) Name() string { return "HMAC-SHA1" }
+
+// Calculate the oaut_signature part of the Authentication Header.
+func (HMACSHA1) Signature(
+	ssodata *SSOData, rp *RequestParameters) (string, error) {
+	baseUrl, err := NormalizeURL(rp.BaseURL)
+	if err != nil {
+		return "", err
+	}
+	params, err := NormalizeParameters(rp.Params)
+	if err != nil {
+		return "", err
+	}
+	baseString := fmt.Sprintf(`%s&%s&%s%s%s%s%s%s%s`,
+		rp.HTTPMethod,
+		url.QueryEscape(baseUrl),
+		url.QueryEscape(params),
+		url.QueryEscape("oauth_consumer_key="+ssodata.ConsumerKey),
+		url.QueryEscape("&oauth_nonce="+rp.Nonce),
+		url.QueryEscape(
+			"&oauth_signature_method="+string(rp.SignatureMethod.Name())),
+		url.QueryEscape("&oauth_timestamp="+rp.Timestamp),
+		url.QueryEscape("&oauth_token="+ssodata.TokenKey),
+		url.QueryEscape("&oauth_version=1.0"))
+	hashfun := hmac.New(sha1.New, []byte(
+		ssodata.ConsumerSecret+"&"+ssodata.TokenSecret))
+	hashfun.Write([]byte(baseString))
+	rawsignature := hashfun.Sum(nil)
+	base64signature := make(
+		[]byte, base64.StdEncoding.EncodedLen(len(rawsignature)))
+	base64.StdEncoding.Encode(base64signature, rawsignature)
+	return string(base64signature), nil
 }
 
 // Sign the provided request.
-func (oauth *SSOData) GetAuthorizationHeader() (string, error) {
-	if oauth.Nonce == "" {
-		oauth.Nonce = nonce()
-	}
-	if oauth.Timestamp == "" {
-		oauth.Timestamp = timestamp()
-	}
-	signature, err := oauth.signature()
+func (ssodata *SSOData) GetAuthorizationHeader(
+	rp *RequestParameters) (string, error) {
+	if rp.Nonce == "" {
+		rp.Nonce = nonce()
+	}
+	if rp.Timestamp == "" {
+		rp.Timestamp = timestamp()
+	}
+	signature, err := rp.SignatureMethod.Signature(ssodata, rp)
 	if err != nil {
 		return "", err
 	}
@@ -108,19 +126,20 @@
 			`oauth_timestamp="%s", `+
 			`oauth_nonce="%s", `+
 			`oauth_version="1.0"`,
-		url.QueryEscape(oauth.ConsumerKey),
-		url.QueryEscape(oauth.TokenKey),
-		oauth.SignatureMethod,
+		url.QueryEscape(ssodata.ConsumerKey),
+		url.QueryEscape(ssodata.TokenKey),
+		rp.SignatureMethod.Name(),
 		signature,
-		url.QueryEscape(oauth.Timestamp),
-		url.QueryEscape(oauth.Nonce))
+		url.QueryEscape(rp.Timestamp),
+		url.QueryEscape(rp.Nonce))
 
 	return auth, nil
 }
 
 // Sign the provided request.
-func (oauth *SSOData) SignRequest(req *http.Request) error {
-	auth, error := oauth.GetAuthorizationHeader()
+func (ssodata *SSOData) SignRequest(
+	rp *RequestParameters, req *http.Request) error {
+	auth, error := ssodata.GetAuthorizationHeader(rp)
 	req.Header.Add("Authorization", auth)
 	return error
 }
 
=== modified file 'oauth_test.go'
--- oauth_test.go	2013-01-29 11:23:37 +0000
+++ oauth_test.go	2013-02-05 12:38:20 +0000
@@ -6,47 +6,62 @@
 	"net/url"
 )
 
-func (suite *USSOTestSuite) TestSignRequestPlainText(c *C) {
+type OAuthTestSuite struct {
+	ssodata SSOData
+	rp      RequestParameters
+	request *http.Request
+}
+
+var _ = Suite(&OAuthTestSuite{})
+
+func (suite *OAuthTestSuite) SetUpTest(c *C) {
 	baseUrl := "https://localhost"
-	ssodata := SSOData{BaseURL: baseUrl, ConsumerKey: consumerKey,
+	suite.ssodata = SSOData{ConsumerKey: consumerKey,
 		ConsumerSecret: consumerSecret, TokenKey: tokenKey,
 		TokenName: tokenName, TokenSecret: tokenSecret}
-	request, _ := http.NewRequest("GET", baseUrl, nil)
-	ssodata.HTTPMethod = "GET"
-	ssodata.SignatureMethod = "PLAINTEXT"
-	err := ssodata.SignRequest(request)
-	c.Assert(err, IsNil)
-	authHeader := request.Header["Authorization"][0]
+	suite.rp = RequestParameters{BaseURL: baseUrl, HTTPMethod: "GET",
+		Nonce: "10888885", Timestamp: "1358853126"}
+	suite.request, _ = http.NewRequest("GET", baseUrl, nil)
+}
+
+// It is possible to sign a request with oauth_signature_method = PLAINTEXT
+func (suite *OAuthTestSuite) TestSignRequestPlainText(c *C) {
+	suite.rp.SignatureMethod = PLAINTEXT{}
+	err := suite.ssodata.SignRequest(&suite.rp, suite.request)
+	if err != nil {
+		c.Log(err)
+		c.FailNow()
+	}
+	authHeader := suite.request.Header["Authorization"][0]
 	c.Assert(authHeader, Matches, `^OAuth.*`)
 	c.Assert(authHeader, Matches, `.*realm="API".*`)
 	c.Assert(authHeader, Matches,
-		`.*oauth_consumer_key="`+url.QueryEscape(ssodata.ConsumerKey)+`".*`)
+		`.*oauth_consumer_key="`+url.QueryEscape(
+			suite.ssodata.ConsumerKey)+`".*`)
 	c.Assert(authHeader, Matches,
-		`.*oauth_token="`+url.QueryEscape(ssodata.TokenKey)+`".*`)
+		`.*oauth_token="`+url.QueryEscape(suite.ssodata.TokenKey)+`".*`)
 	c.Assert(authHeader, Matches,
 		`.*oauth_signature="`+url.QueryEscape(
-			ssodata.ConsumerSecret+`&`+ssodata.TokenSecret)+`.*`)
+			suite.ssodata.ConsumerSecret)+`&`+url.QueryEscape(
+			suite.ssodata.TokenSecret)+`.*`)
 }
 
-// Test the request signing with oauth_signature_method = SHA1
-func (suite *USSOTestSuite) TestSignRequestSHA1(c *C) {
-	baseUrl := "https://localhost"
-	ssodata := SSOData{BaseURL: baseUrl, ConsumerKey: consumerKey,
-		ConsumerSecret: consumerSecret, TokenKey: tokenKey,
-		TokenName: tokenName, TokenSecret: tokenSecret,
-		Nonce: "10888885", Timestamp: "1358853126"}
-	request, _ := http.NewRequest("GET", baseUrl, nil)
-	ssodata.HTTPMethod = "GET"
-	ssodata.SignatureMethod = "HMAC-SHA1"
-	err := ssodata.SignRequest(request)
-	c.Assert(err, IsNil)
-	authHeader := request.Header["Authorization"][0]
+// It is possible to sign a request  with oauth_signature_method = SHA1
+func (suite *OAuthTestSuite) TestSignRequestSHA1(c *C) {
+	suite.rp.SignatureMethod = HMACSHA1{}
+	err := suite.ssodata.SignRequest(&suite.rp, suite.request)
+	if err != nil {
+		c.Log(err)
+		c.FailNow()
+	}
+	authHeader := suite.request.Header["Authorization"][0]
 	c.Assert(authHeader, Matches, `^OAuth.*`)
 	c.Assert(authHeader, Matches, `.*realm="API".*`)
 	c.Assert(authHeader, Matches,
-		`.*oauth_consumer_key="`+url.QueryEscape(ssodata.ConsumerKey)+`".*`)
+		`.*oauth_consumer_key="`+url.QueryEscape(
+			suite.ssodata.ConsumerKey)+`".*`)
 	c.Assert(authHeader, Matches,
-		`.*oauth_token="`+url.QueryEscape(ssodata.TokenKey)+`".*`)
+		`.*oauth_token="`+url.QueryEscape(suite.ssodata.TokenKey)+`".*`)
 	c.Assert(authHeader, Matches,
 		`.*oauth_signature="`+"amJnYeek4G9ObTgTiE2y6cwTyPg="+`.*`)
 }
 
=== modified file 'url.go'
--- url.go	2013-01-29 11:23:37 +0000
+++ url.go	2013-02-05 12:38:20 +0000
@@ -3,43 +3,60 @@
 import (
 	"fmt"
 	"net/url"
+	"sort"
 	"strings"
 )
 
 // Remove the standard ports from the URL.
-func normalizeHost(scheme, host_spec string) string {
-	standard_ports := map[string]string{
+func normalizeHost(scheme, hostSpec string) string {
+	standardPorts := map[string]string{
 		"http":  "80",
 		"https": "443",
 	}
-	host_parts := strings.Split(host_spec, ":")
-	if len(host_parts) == 2 && host_parts[1] == standard_ports[scheme] {
+	hostParts := strings.Split(hostSpec, ":")
+	if len(hostParts) == 2 && hostParts[1] == standardPorts[scheme] {
 		// There's a port, but it's the default one.  Leave it out.
-		return host_parts[0]
+		return hostParts[0]
 	}
-	return host_spec
+	return hostSpec
 }
 
 // Normalize the URL according to OAuth specs.
-func NormalizeURL(input_url string) (string, error) {
-	parsed_url, err := url.Parse(input_url)
+func NormalizeURL(inputUrl string) (string, error) {
+	parsedUrl, err := url.Parse(inputUrl)
 	if err != nil {
 		return "", err
 	}
 
-	host := normalizeHost(parsed_url.Scheme, parsed_url.Host)
-	normalized_url := fmt.Sprintf(
-		"%v://%v%v", parsed_url.Scheme, host, parsed_url.Path)
-	return normalized_url, nil
+	host := normalizeHost(parsedUrl.Scheme, parsedUrl.Host)
+	normalizedUrl := fmt.Sprintf(
+		"%v://%v%v", parsedUrl.Scheme, host, parsedUrl.Path)
+	return normalizedUrl, nil
 }
 
 // Normalize the parameters in the query string according to OAuth specs.
+// url.Values.Encode encoded the GET parameters in a consistent order
+// we do the encoding ourselves.
 func NormalizeParameters(parameters url.Values) (string, error) {
-	filtered_map := make(url.Values, len(parameters))
-	for param, value := range parameters {
-		if param != "oauth_signature" {
-			filtered_map[param] = value
-		}
-	}
-	return filtered_map.Encode(), nil
+	filteredMap := make(url.Values, len(parameters))
+	keys := make([]string, len(parameters))
+	i := 0
+	for key, _ := range parameters {
+		keys[i] = key
+		i++
+	}
+	sort.Strings(keys)
+	for _, key := range keys {
+		if key != "oauth_signature" {
+			filteredMap[key] = parameters[key]
+		}
+	}
+	parts := make([]string, 0, len(filteredMap))
+	for _, key := range keys {
+		prefix := url.QueryEscape(key) + "="
+		for _, v := range filteredMap[key] {
+			parts = append(parts, prefix+url.QueryEscape(v))
+		}
+	}
+	return strings.Join(parts, "&"), nil
 }
 
=== modified file 'url_test.go'
--- url_test.go	2013-01-29 14:27:25 +0000
+++ url_test.go	2013-02-05 12:38:20 +0000
@@ -60,7 +60,7 @@
 	output, err := NormalizeParameters(
 		url.Values{"a": []string{"1"}, "b": []string{"2"}})
 	c.Check(err, gocheck.Equals, nil)
-	c.Check(output, gocheck.Matches, "(a=1&b=2|b=2&a=1)")
+	c.Check(output, gocheck.Matches, "(a=1&b=2)")
 }
 
 // NormalizeParameters() escapes the parameters correctly when encoding
@@ -84,5 +84,5 @@
 	}
 	output, err := NormalizeParameters(params)
 	c.Check(err, gocheck.Equals, nil)
-	c.Check(output, gocheck.Matches, "(a=1&z=26|z=26&a=1)")
+	c.Check(output, gocheck.Matches, "(a=1&z=26)")
 }
 
=== modified file 'usso.go'
--- usso.go	2013-01-29 11:23:37 +0000
+++ usso.go	2013-02-05 12:38:20 +0000
@@ -49,7 +49,7 @@
 		"password":   password,
 		"token_name": tokenName,
 	}
-	json_credentials, err := json.Marshal(credentials)
+	jsonCredentials, err := json.Marshal(credentials)
 	if err != nil {
 		log.Printf("Error: %s\n", err)
 		return nil, err
@@ -57,7 +57,7 @@
 	response, err := http.Post(
 		server.tokenURL(),
 		"application/json",
-		strings.NewReader(string(json_credentials)))
+		strings.NewReader(string(jsonCredentials)))
 	if err != nil {
 		return nil, err
 	}
@@ -77,14 +77,16 @@
 
 // Returns all the Ubuntu SSO information related to this account.
 func (server UbuntuSSOServer) GetAccounts(ssodata *SSOData) (string, error) {
-	ssodata.BaseURL = server.AccountsURL() + ssodata.ConsumerKey
-	ssodata.HTTPMethod = "GET"
-	ssodata.SignatureMethod = "HMAC-SHA1"
-	request, err := http.NewRequest(ssodata.HTTPMethod, ssodata.BaseURL, nil)
+	rp := RequestParameters{
+		BaseURL:         server.AccountsURL() + ssodata.ConsumerKey,
+		HTTPMethod:      "GET",
+		SignatureMethod: HMACSHA1{}}
+
+	request, err := http.NewRequest(rp.HTTPMethod, rp.BaseURL, nil)
 	if err != nil {
 		return "", err
 	}
-	err = SignRequest(ssodata, request)
+	err = SignRequest(ssodata, &rp, request)
 	if err != nil {
 		return "", err
 	}
@@ -103,12 +105,14 @@
 }
 
 // Given oauth credentials and a request, return it signed.
-func SignRequest(ssodata *SSOData, request *http.Request) error {
-	return ssodata.SignRequest(request)
+func SignRequest(
+	ssodata *SSOData, rp *RequestParameters, request *http.Request) error {
+	return ssodata.SignRequest(rp, request)
 }
 
 // Given oauth credentials return a valid http authorization header.
-func GetAuthorizationHeader(ssodata *SSOData) (string, error) {
-	header, err := ssodata.GetAuthorizationHeader()
+func GetAuthorizationHeader(
+	ssodata *SSOData, rp *RequestParameters) (string, error) {
+	header, err := ssodata.GetAuthorizationHeader(rp)
 	return header, err
 }