-
Notifications
You must be signed in to change notification settings - Fork 265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: security question verification #1182
fix: security question verification #1182
Conversation
Codecov Report
@@ Coverage Diff @@
## 6.4 #1182 +/- ##
==========================================
+ Coverage 93.32% 93.39% +0.07%
==========================================
Files 153 153
Lines 4418 4422 +4
Branches 994 995 +1
==========================================
+ Hits 4123 4130 +7
+ Misses 276 273 -3
Partials 19 19
Continue to review full report at Codecov.
|
@@ -134,7 +134,14 @@ export const SecurityQuestionAuthenticatorFactory = IdxAuthenticatorFactory.para | |||
type: 'security_question', | |||
methods: [ | |||
{ type: 'security_question' } | |||
] | |||
], | |||
// if already enrolled, contextualData will be 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.
remove this comment?
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 put this in there deliberately, because it seems like the kind of thing we will forget. By default the factory will build an object for enrollment, but if you want to test verification you'll need to add that in yourself.
Fixes an issue with lib/idx/authenticator/SecurityQuestionVerification.ts where it was not accepting
credentials.answer
as provided by the Sign-in Widget.When enrolling in security question there is no "contextualData", but this object is present when verifying. An omission in the mock used by SIW had it testing security question enrollment, but not verification. Fixing the mock revealed the issue with AuthJS.
Adding unit tests to cover these cases.