Implement SASL OAuth2 implementation #193
Conversation
|
|
Ok, fine. I'll push the review button on this PR.
XOAUTH2 has two possible flows for the error cases, the second one involving the sasl-challenge frame. Having unit test supporting both was important. |
sasl.go
Outdated
| } | ||
|
|
||
| func saslXOAUTH2InitialResponse(username string, bearer string) ([]byte, error) { | ||
| re := regexp.MustCompile("^[\x20-\x7E]+$") |
There was a problem hiding this comment.
Please use a loop instead of regex for this.
| response: response, | ||
| } | ||
| return handler.init() | ||
| } |
There was a problem hiding this comment.
The anonymous function can be omitted by constructing handler before the map assignment and setting c.saslHandlers[saslMechanismXOAUTH2] = handler.init.
There was a problem hiding this comment.
Good point, much neater. Done.
sasl.go
Outdated
| return s.saslXOAUTH2Step | ||
| } | ||
|
|
||
| func (s saslXOAUTH2Handler) saslXOAUTH2Step() stateFunc { |
There was a problem hiding this comment.
Since the receiver type includes "saslXOAUTH2" I think it'd be less repetitive to name the method step.
sasl.go
Outdated
| Response: []byte{}, | ||
| }, | ||
| }) | ||
| return s.saslXOAUTH2Step |
There was a problem hiding this comment.
What do you think about adding a check that multiple challenges aren't received? Hypothetically this could loop for as long as the peer sends them.
There was a problem hiding this comment.
I've added an additional guard. I now fail the negotiation on receipt of the second challenge from the server, including the contents of both error responses in the connection's error.
Saving the error response as a field in the receiver type had the additional benefit I could include it in the "SASL XOAUTH2 auth failed" error produced when the authentication fails.
sasl_test.go
Outdated
| client, err := New(c, | ||
| ConnSASLXOAUTH2("someuser@example.com", "ya29.vF9dft4qmTc2Nvb3RlckBhdHRhdmlzdGEuY29tCg", 512), | ||
| ConnIdleTimeout(10*time.Minute)) | ||
| if client != nil { |
There was a problem hiding this comment.
Go tends to assume that if err != nil the other return values are invalid regardless of whether they're nil or not (unless documented otherwise). Can you change this to if err != nil?
sasl_test.go
Outdated
| client, err := New(c, | ||
| ConnSASLXOAUTH2("someuser@example.com", "ya29.vF9dft4qmTc2Nvb3RlckBhdHRhdmlzdGEuY29tCg", 512), | ||
| ConnIdleTimeout(10*time.Minute)) | ||
| if client != nil { |
|
@vcabbage thanks for the feedback, hopefully I've addressed your comments above. |
|
Thank you. Unfortunately this is going to be one of the last PRs to be merged. Please see #205 for details. Thank you again for your contributions. |
This patch add supports for a SASL OAuth2 mechanism.
I had some problems implementing tests for the new mechanism. The patch currently uses the
testconnbut I'm uncertain that using this package from the unit tests correct. I'll appreciate feedback on the test approach/implementation.