feat(cognito-idp): make TOTP MFA work#9785
feat(cognito-idp): make TOTP MFA work#9785bdellegrazie wants to merge 3 commits intogetmoto:masterfrom
Conversation
2acb0b8 to
6e1fb23
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #9785 +/- ##
==========================================
- Coverage 93.11% 93.11% -0.01%
==========================================
Files 1308 1308
Lines 118862 118896 +34
==========================================
+ Hits 110683 110714 +31
- Misses 8179 8182 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
04ee252 to
16859cf
Compare
|
Added a test for the |
bblommers
left a comment
There was a problem hiding this comment.
Hi @bdellegrazie! I imagine that the majority of people will not go through the trouble of setting up TOTP in their tests, and will just use a random UserCode on verification. That is a valid test strategy, as far a I'm concerned.
I do see the benefit of this feature, though - can we hide it behind a feature flag? We typically use environment variables for these usecases, so in this case we could use: MOTO_COGNITO_ENABLE_TOTP=true.
Only if that env variable is set would Moto validate the provided UserCode - if not, Moto accepts everything.
I would also like to see a negative test: what happens when the user provides an invalid user code (with this env var enabled)? How does AWS behave when providing an invalid code?
348e15f to
12f041f
Compare
12f041f to
eff78f7
Compare
|
Hi @bblommers, Thanks for the review!
|
9496ca7 to
5b32839
Compare
Implement TOTP MFA using the existing cryptography API. This allows clients to behave correctly and use proper TOTP validation compared with previously where the UserCode was not handled. Warning: this implementation still uses the fixed constant "secret" and therefore should not be considered for anything other than testing. A solution implementing "real" TOTP was considered but would need storing the secret temporarily in the session during the auth process but the session object is unfortunately a tuple at this time.
* Put working TOTP MFA behind a feature flag - specifically `MOTO_COGNITO_IDP_USER_POOL_ENABLE_TOTP` * Support the FriendlyDeviceName field, although only for error reporting to end-user
5b32839 to
7408e4d
Compare
7408e4d to
62bd97f
Compare
|
@bblommers could you please re-review? I had to delete the test which verifies the old behaviour (TOTP code of anything) because it doesn't work ServerSide. |
Implement TOTP MFA using the existing cryptography API. This allows clients to behave correctly and use proper TOTP validation compared with previously where the UserCode was not handled.
Warning: this implementation still uses the fixed constant "secret" and therefore should not be considered for anything other than testing.
A solution implementing "real" TOTP was considered but would need storing the secret temporarily in the session during the auth process but the session object is unfortunately a tuple at this time.
Closes #9786