Add verification code protection #110856
Add verification code protection #110856
Conversation
thomheymann
added
Team:Security
|
ACK: Reviewing... |
| message: verificationCode.remainingAttempts | ||
| ? 'Invalid verification code.' | ||
| : 'Maximum number of attempts exceeded.', |
There was a problem hiding this comment.
question: (sorry haven't looked at the client side yet), if we display these errors in UI we'd need to use i18n for them
|
Pinging @elastic/kibana-security (Team:Security) |
| const sanitize = (str: string) => { | ||
| return str | ||
| .split('') | ||
| .filter((char) => char.match(pattern)) | ||
| .join('') | ||
| .substr(0, length); | ||
| }; |
There was a problem hiding this comment.
suggestion: it took me sometime to understand the idea behind being so forgiving to the input (filtering out non-digits and accepting any length) and not just using something like ^[0-9]{6}$ after trim(). Would be great if you can leave a comment here for the future readers.
question: any reason we make pattern, length and separator configurable (via props)? Feels a bit like a premature generalization to me.
There was a problem hiding this comment.
suggestion: it took me sometime to understand the idea behind being so forgiving to the input (filtering out non-digits and accepting any length) and not just using something like
^[0-9]{6}$aftertrim(). Would be great if you can leave a comment here for the future readers.
ok, done!
question: any reason we make
pattern,lengthandseparatorconfigurable (via props)? Feels a bit like a premature generalization to me.
This doesn't add any complexity. I'd need to declare variables for these params anyways unless you want me to hard code them which would make it less readable.
There was a problem hiding this comment.
This doesn't add any complexity.
That's only true when we don't test both default pattern and custom patterns 🙂
I'd need to declare variables for these params anyways unless you want me to hard code them which would make it less readable.
Yeah, I was thinking about constants, like we created for length already. But feel free to keep what you have if you feel strongly about it. I usually try to not parameterize anything if I don't need it to make it easier to test and change (for any breaking change you'd need to go and check all consumers that may theoretically pass the parameter you're going to change).
| verificationCode.verify('invalid'); | ||
| verificationCode.verify('invalid'); | ||
| verificationCode.verify('invalid'); | ||
| expect(verificationCode['failedAttempts']).toBe(3); // eslint-disable-line dot-notation |
There was a problem hiding this comment.
question: why not rely on public remainingAttempts instead (here and in tests below)?
There was a problem hiding this comment.
Because then I need to calculate the difference myself. Here it maps directly to the number of times I called verify which is easier to see in the test
There was a problem hiding this comment.
Yeah, but it's a private field and wouldn't even work at all if you switch to the native JS private fields.
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Page load bundle
History
To update your PR or re-run it, just comment with: cc @thomheymann |
Summary
Add verification code protection to interactive setup.
Verification modal
This modal will only be displayed if the user opens Kibana manually instead of opening the setup link generated during startup.
Kibana server