-
Notifications
You must be signed in to change notification settings - Fork 97
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a loop instead of regex for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making those changes. Left a few more minor ones. Should be good to merge after they're resolved.
response: response, | ||
} | ||
return handler.init() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, much neater. Done.
sasl.go
Outdated
return s.saslXOAUTH2Step | ||
} | ||
|
||
func (s saslXOAUTH2Handler) saslXOAUTH2Step() stateFunc { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the receiver type includes "saslXOAUTH2" I think it'd be less repetitive to name the method step
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
sasl.go
Outdated
Response: []byte{}, | ||
}, | ||
}) | ||
return s.saslXOAUTH2Step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@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
testconn
but I'm uncertain that using this package from the unit tests correct. I'll appreciate feedback on the test approach/implementation.