Skip to content

Conversation

@mattcreaser
Copy link
Member

  • PR title and description conform to Pull Request guidelines.

Issue #, if available:

Description of changes:

  • Moves the TOTP Setup functions into use case classes.
  • Fixes some bugs in setupTotp where Amplify would hang (never invoke callbacks) in certain scenarios.

How did you test these changes?

  • Unit test

Documentation update required?

  • No
  • Yes (Please include a PR link for the documentation update)

General Checklist

  • Added Unit Tests
  • Added Integration Tests
  • Security oriented best practices and standards are followed (e.g. using input sanitization, principle of least privilege, etc)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mattcreaser mattcreaser requested a review from a team as a code owner February 21, 2025 16:19
@tylerjroach
Copy link
Member

I've noticed this error will never surface anymore:

CognitoAuthExceptionConverter.lookup(
  error,
  "Cannot find a multi-factor authentication (MFA) method."
)

This looks like it was just a generic catch all but want to make sure the change is expected.

@mattcreaser
Copy link
Member Author

The string there is the message set on unmapped exception types (i.e. if we return an UnknownException). It was indeed intentionally omitted since we map exceptions at the top level now, which is much simpler, although it does cause a loss of context. I also thought the message didn't make any sense given what the user is trying to do.

If we wanted to retain the contextual message we can do something like:

internal inline fun <R> mapExceptions(fallbackMessage: String, block: () -> R): R {
    try {
        return block()
    } catch (e: Exception) {
        throw CognitoAuthExceptionConverter.lookup(e, fallbackMessage)
    }
}

and then execute becomes:

suspend fun execute(): TOTPSetupDetails = mapExceptions("Cannot setup TOTP") {
  // ....
}

@tylerjroach
Copy link
Member

Thanks for clarification. I am ok with original change.

tylerjroach
tylerjroach previously approved these changes Feb 24, 2025
Base automatically changed from mattcreaser/usecase-user-attributes to main February 25, 2025 14:43
@mattcreaser mattcreaser dismissed tylerjroach’s stale review February 25, 2025 14:43

The base branch was changed.

@mattcreaser mattcreaser force-pushed the mattcreaser/usecase-totp branch from 0921e41 to 2a06e44 Compare February 25, 2025 14:54
@mattcreaser mattcreaser merged commit e31c581 into main Feb 28, 2025
2 of 3 checks passed
@mattcreaser mattcreaser deleted the mattcreaser/usecase-totp branch February 28, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants