-
Notifications
You must be signed in to change notification settings - Fork 224
Conversation
lib/okta.go
Outdated
PassCode: mfaCode, | ||
}) | ||
|
||
// only add the mfaCode to the payload if it's non-empty |
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.
Isn't this equivalent to the old code? PassCode
is a string so its zero value should be ""
, same as mfaCode
.
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.
In terms of functionality, it is the same. Just wanted to make it explicit how PassCode
was being set.
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 appreciate the sentiment, but I don't think it makes things appreciably clearer. In fact, now the err
-check below is actually checking something else in the case that mfaCode == ""
. So I'd say change it back
Also, what's with https://github.com/segmentio/aws-okta/pull/201/files#diff-5057c2ca588b57c0ada94b258bba5624R301? Seems superfluous.
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.
On second thought, I agree. Will return this block to its original implementation.
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.
Awesome. Had one small question, but happy to merge as is.
1f802f9
to
ce4e66e
Compare
Made the changes, feel free to merge whenever. |
adds support for U2F FIDO devices such as yubikey. The U2F device must already be registered with Okta and be plugged in and available for use. retry if open fails, there are issues with the low level `hid_open` call that we rely on in `hidapi`. These are the libraries we depend on: https://github.com/marshallbrekka/go-u2fhost/blob/master/hid/wrapper.go https://github.com/marshallbrekka/go.hid https://github.com/signal11/hidapi
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.
Awesome, let's merge!
adds support for U2F FIDO devices such as yubikey. The U2F device must
already be registered with Okta and be plugged in and available for use.
retry if open fails, there are issues with the low level
hid_open
callthat we rely on in
hidapi
.These are the libraries we depend on:
https://github.com/marshallbrekka/go-u2fhost/blob/master/hid/wrapper.go
https://github.com/marshallbrekka/go.hid
https://github.com/signal11/hidapi