fix: count MFA addresses in CountActiveMultiFactorCredentials for code method #4318
Conversation
|
|
wewelll
force-pushed
the
fix-count-active-multi-factor-credentials
branch
from
4108816 to
953042d
Compare
wewelll
changed the title
wewelll
force-pushed
the
fix-count-active-multi-factor-credentials
branch
2 times, most recently
from
be41841 to
82572bd
Compare
There was a problem hiding this comment.
Looks really good! I guess the only question is if there are regression cases where this would break an existing set up. So for example:
- Dev has set up code MFA
- An identity exists where config does not have the address
- MFA does no longer work for them
The best way to figure this out is here and here. A regression case for code is here. More regression cases are here.
Also it makes sense to add an end to end test in test/e2e to test the case you have: An identity schema with a field for code MFA that is not required, and an identity that is trying to sign in without filling in the data (should not show 2fa), and one that is singing in with filling out the data (should show 2fa). With such a test we can ensure that this won't break in the future when someone changes something.
Thank you!
wewelll
force-pushed
the
fix-count-active-multi-factor-credentials
branch
from
dcfd798 to
82572bd
Compare
|
Thank you for the advice @aeneasr |
wewelll
force-pushed
the
fix-count-active-multi-factor-credentials
branch
from
106a477 to
66ed39e
Compare
|
It looks like a test is broken in |
|
Yes, that's on master - I'll rebase this PR then it should pass |
aeneasr
force-pushed
the
fix-count-active-multi-factor-credentials
branch
from
66ed39e to
d02a07a
Compare
wewelll
force-pushed
the
fix-count-active-multi-factor-credentials
branch
from
d02a07a to
2e379d6
Compare
wewelll
force-pushed
the
fix-count-active-multi-factor-credentials
branch
from
1b39978 to
653bdf3
Compare
wewelll
force-pushed
the
fix-count-active-multi-factor-credentials
branch
from
7012e34 to
e95ecb9
Compare
…ut highest_available aal is required for the settings flow
|
Thank you, all tests are passing now. Here is a summary of the changes on the tests:
Let me know if there is something more to do ! |
Attempt to fix ory/network#409
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments