Code review comment for lp:~vds/usso/support_sha1

Revision history for this message
Raphaƫl Badin (rvb) wrote :

[2]

These two blocks seem very redundant to me, isn't it possible refactor that a bit to unify/simplify it a bit?

289 + `OAuth realm="API", `+
290 + `oauth_consumer_key="%s", `+
291 + `oauth_token="%s", `+
292 + `oauth_signature_method="%s", `+
293 + `oauth_signature="%s", `+
294 + `oauth_timestamp="%s", `+
295 + `oauth_nonce="%s", `+
296 + `oauth_version="1.0"`,
297 + url.QueryEscape(oauth.ConsumerKey),
298 + url.QueryEscape(oauth.TokenKey),
299 + oauth.SignatureMethod,
300 + signature,
301 + url.QueryEscape(oauth.Timestamp),
302 + url.QueryEscape(oauth.Nonce))

337 + base_string := fmt.Sprintf(`%s&%s&%s%s%s%s%s%s%s`,
338 + oauth.HTTPMethod,
339 + url.QueryEscape(base_url),
340 + url.QueryEscape(params),
341 + url.QueryEscape("oauth_consumer_key="+oauth.ConsumerKey),
342 + url.QueryEscape("&oauth_nonce="+oauth.Nonce),
343 + url.QueryEscape("&oauth_signature_method="+oauth.SignatureMethod),
344 + url.QueryEscape("&oauth_timestamp="+oauth.Timestamp),
345 + url.QueryEscape("&oauth_token="+oauth.TokenKey),
346 + url.QueryEscape("&oauth_version=1.0"))

[3]

328 + switch oauth.SignatureMethod {
329 + case "PLAINTEXT":
330 + return fmt.Sprintf(
331 + `%s%%26%s`,
332 + oauth.ConsumerSecret,
333 + oauth.TokenSecret)
334 + case "HMAC-SHA1":

This is probably something that we can fix later but implementing the signature like this looks like a design flaw to me. I think we should be using the strategy pattern here: define a "signing" interface and have two different types implement it (one with a plain text signature, the other with a SHA1 signature).
This would provide a much better encapsulation of the signing code and would make it much more easy to add new signature types if we ever need to.

« Back to merge proposal