feat(cognito-idp): make TOTP MFA work - #9785
Conversation
2acb0b8 to
6e1fb23
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (89.74%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #9785 +/- ##
==========================================
- Coverage 93.12% 93.12% -0.01%
==========================================
Files 1314 1314
Lines 119183 119217 +34
==========================================
+ Hits 110991 111022 +31
- Misses 8192 8195 +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
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
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. |
|
Hi @bdellegrazie! There is no marker, but we typically just add an if-statement to the top of the test: from unittest import SkipTest
from moto import mock_aws, settings
def test():
if not settings.TEST_DECORATOR_MODE:
raise SkipTest("Can only change setting in DecoratorMode")Everything else LGTM, so if we can keep the test to verify the old behaviour, I'd be happy to merge. |
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
62bd97f to
a37d6d7
Compare
|
@bblommers I've added the additional negative test. Anything else? |
bblommers
left a comment
bblommers
left a comment
There was a problem hiding this comment.
LGTM - thank you so much for adding this feature to Moto @bdellegrazie!
2 participants
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