MFA #14049
MFA #14049
Conversation
…or various mfa scenarios
| } catch (error) { | ||
| this.errors = error.errors; | ||
| // update if specific error can be parsed for incorrect passcode | ||
| // this.newCodeDelay.perform(); |
There was a problem hiding this comment.
Do we want to keep this commented out?
There was a problem hiding this comment.
I commented this out recently until I can confirm whether or not the backend is sending a specific error response for this scenario or if the delay is necessary.
| } catch (e) { | ||
| // check for totp not configured mfa error before throwing | ||
| const errors = this.handleError(e); | ||
| // stubbing error - verify once API is finalized |
There was a problem hiding this comment.
Should we add this as a TODO?
There was a problem hiding this comment.
Good call I updated the comment
| * @param [isInline=false] {Bool} - Whether or not the select should be displayed as inline-block or block. | ||
| * @param [isFullwidth=false] {Bool} - Whether or not the select should take up the full width of the parent element. | ||
| * @param onChange=null {Func} - The action to take once the user has selected an item. This method will be passed the `value` of the select. | ||
| * @param {string} [label=null] - The label for the select element. |
ui/mirage/factories/mfa-method.js
Outdated
| @@ -0,0 +1,13 @@ | |||
| import { Factory } from 'ember-cli-mirage'; | |||
| import faker from 'faker'; | |||
There was a problem hiding this comment.
I thought we pulled out this dep 🤔
There was a problem hiding this comment.
I haven't merged main as of yet but thanks for the reminder 🙂. I'll get this updated before it breaks post merge.
| @@ -0,0 +1,135 @@ | |||
| import { module, test } from 'qunit'; | |||
| > | ||
| Verify | ||
| </button> | ||
| {{#if this.newCodeDelay.isRunning}} |
There was a problem hiding this comment.
and this is the real reason concurrency rocks. the rest of it is difficult, but this is so nice.
| @@ -0,0 +1,146 @@ | |||
| import { Response } from 'miragejs'; | |||
There was a problem hiding this comment.
@zofskeez do we need the mirage handler now that the API is done, or will this be useful later for any testing or debugging?
There was a problem hiding this comment.
I'm not sure we landed on whether or not to keep feature handlers around. In this case I am using it in the tests since push notifications require user input from a secondary device.
My gut says it's probably a good idea to keep mirage handlers around after feature work is complete in case we want to use them for testing in the future.
ui/tests/acceptance/mfa-test.js
Outdated
| assert | ||
| .dom('[data-test-mfa-description]') | ||
| .includesText( | ||
| '2 methods are required for successful authentication.', |
There was a problem hiding this comment.
not sure where this is in the code, but if possible it seems like this should be a written two instead of 2. Any numbers under 10 are generally written out.
There was a problem hiding this comment.
I was wondering if we have a library or helper that converts integers to their English representation? This value comes from an array length so I wasn't sure how to handle converting it, or more accurately when to stop since the array could theoretically be any length. If the pattern should be any number under 10 is written out I could write a util and also a template helper for that.
There was a problem hiding this comment.
I think a helper for this would be useful! Angel is right, usually numbers under 10 are wordy
This feature adds multi-factor authentication support to the UI. If the user is required to take further action to satisfy the an mfa requirement they will be presented a form, otherwise for push notifications the standard login form will wait until the mfa requirements are fulfilled and display a message to the user.