feat: configure timeout in config #243
Conversation
README.md
Outdated
| @@ -102,6 +102,10 @@ WebAuthn.configure do |config| | |||
| # Relying Party name for display purposes | |||
| config.rp_name = "Example Inc." | |||
|
|
|||
| # Optional numerical hint, in milliseconds to wait for attestation | |||
| # and assertion. This may be overridden by the browser. | |||
| config.timeout = 120000 | |||
There was a problem hiding this comment.
I'm trying to put myself in the shoes of the person configuring the gem for the first time with little context of the standard, and my first impression is that the user will have a hard time understanding what this timeout is for. I specifically mean this for the configuration attribute in the configuration, not the hash of options we give the gem's user.
How about a diff name for the configuration field? default_client_timeout maybe? @grzuy you wrote the issue, maybe you could suggest another name?
Second, I would be just a little more verbose on the comment for it or at least add a reference to doc option
There was a problem hiding this comment.
Thanks for the feedback @brauliomartinezlm. I agree with your comment. It might not be immediately obvious for someone who isn't familiar with the spec.
I've updated timeout to be client_timeout and I've updated the description to be more verbose.
There was a problem hiding this comment.
@brauliomartinezlm You're right. Just timeout is too obscure here.
I think we need to be as explicit and obvious as possible.
I would go with
WebAuthn.configuration.credential_options_timeoutor if we want to start to namespacing properly, eventually we could...
WebAuthn.configuration.credential_options.timeout
| @@ -102,6 +102,13 @@ WebAuthn.configure do |config| | |||
| # Relying Party name for display purposes | |||
| config.rp_name = "Example Inc." | |||
|
|
|||
| # Optionally configure a client timeout hint, in milliseconds. | |||
There was a problem hiding this comment.
This timeout is for waiting user input, with expected values of 15 or 30 seconds and up.
So I think seconds it's enough granularity for this type of timeout. I don't see milliseconds providing any extra value to the user.
There was a problem hiding this comment.
The spec states that the value should be in milliseconds. I agree that it makes more sense to have the value set in seconds. Nonetheless, I think there could be some confusion diverging from the spec.
There was a problem hiding this comment.
Fair enough.
Let's keep milliseconds :-)
| options = { challenge: challenge, allowCredentials: allow_credentials } | ||
| options = { | ||
| challenge: challenge, | ||
| client_timeout: client_timeout, |
There was a problem hiding this comment.
Despite what we call in the gem configuration, the hash key still needs to be timeout, because that's what the browser expects (https://www.w3.org/TR/webauthn/#dom-publickeycredentialcreationoptions-timeout).
README.md
Outdated
| @@ -102,6 +102,10 @@ WebAuthn.configure do |config| | |||
| # Relying Party name for display purposes | |||
| config.rp_name = "Example Inc." | |||
|
|
|||
| # Optional numerical hint, in milliseconds to wait for attestation | |||
| # and assertion. This may be overridden by the browser. | |||
| config.timeout = 120000 | |||
There was a problem hiding this comment.
@brauliomartinezlm You're right. Just timeout is too obscure here.
I think we need to be as explicit and obvious as possible.
I would go with
WebAuthn.configuration.credential_options_timeoutor if we want to start to namespacing properly, eventually we could...
WebAuthn.configuration.credential_options.timeout
MaximeNdutiye
force-pushed
the
timeout_configuration
branch
from
677422d
to
0ed8f49
Compare
| @@ -9,5 +9,9 @@ class CredentialOptions | |||
| def challenge | |||
| @challenge ||= SecureRandom.random_bytes(CHALLENGE_LENGTH) | |||
| end | |||
|
|
|||
| def timeout | |||
| @timeout = WebAuthn.configuration.credential_options_timeout | |||
There was a problem hiding this comment.
I guess the idea was this to be
@timeout ||= WebAuthn.configuration.credential_options_timeout?
Anyway, I think i'll drop using memoization here.
No need to cache the value I think.
|
|
||
| def initialize | ||
| @algorithms = DEFAULT_ALGORITHMS.dup | ||
| @verify_attestation_statement = true | ||
| @credential_options_timeout = 120000 |
There was a problem hiding this comment.
For the record, there's been this addition to the spec https://github.com/w3c/webauthn/pull/1317/files.
Add the ability to configure timeout in config. Defaults to 120s.
Closes #181