-
Notifications
You must be signed in to change notification settings - Fork 212
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
task(auth): Update /totp/destroy to take an sms code #18154
Conversation
.string() | ||
.max(32) | ||
.regex(validators.DIGITS) | ||
.optional() |
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.
To keep this backwards compatible, I made the code optional. However that means that for now there's no enforcement of providing a code in order to remove TOTP. Perhaps there should also be check to see if the account has a back up phone, and if it does an SMS must be provided. This was not in the ACs for the ticket though...
payload: isA.object({ | ||
code: isA | ||
.string() | ||
.max(32) |
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.
sorry I have not been following the SMS work that closely but is this a code sent to the user?
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.
Yes it would be a code sent to the users phone. There's some things about this ticket that are a little fuzzy to me as well. I'm thinking when we do the front end implementation the usage will be more clear.
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.
32 characters is a looooong code. 😅
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.
👍 Make sense to me but I do have a couple questions/concerns (see inline comments).
Because
This pull request
Issue that this pull request solves
Closes: # (issue number)
Checklist
Put an
x
in the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)