Remove Authy

[mjiori]

Removes references to Authy as an MFA method in Registry, including the ability to enroll in Authy as an MFA option, documentation changes to remove references to Authy, and code and tests that call Authy's APIs.

[diamondap]

This looks good so far, but when I try to run tests locally, the build fails with these errors:

common/context.go:29:22: undefined: network.AuthyClientInterface
common/context.go:88:25: undefined: network.NewAuthyClient

I think you need to remove or rename those.

[mjiori]

All build errors should be resolved.

All Registry tests are passing. I validated the changes in interactive mode, and Authy no longer appears as an option.

Removed go-authy package, and ran go mod tidy.

DB migration and 2 parameter removals are required to deploy.

[diamondap]

This looks good, except for two issues:

1. You need to resolve a merge conflict in go.mod. Take a look at https://github.com/APTrust/registry/blob/remove-authy/go.mod. It contains merge annotations like <<<<<<< HEAD on lines 45-52.
2. It looks like the document at https://github.com/APTrust/registry/blob/remove-authy/two_factor_notes.md says user can SMS, backup codes or no 2FA. Will there also be a 2FA method for an authenticator app?

[mjiori]

This looks good, except for two issues:

1. You need to resolve a merge conflict in go.mod. Take a look at https://github.com/APTrust/registry/blob/remove-authy/go.mod. It contains merge annotations like <<<<<<< HEAD on lines 45-52.
2. It looks like the document at https://github.com/APTrust/registry/blob/remove-authy/two_factor_notes.md says user can SMS, backup codes or no 2FA. Will there also be a 2FA method for an authenticator app?

Thanks. I fixed the merge conflict there.
There will be a 2FA method that uses authenticator apps and I have that code in a separate PR #35
But it's independent from this one. All this is doing is removing Authy.

[diamondap]

Tests fail for this due to SQL loading errors. I think the issue is due to CSV columns not matching up to table column names after migration 14. This issue affected PR #131. The fix for that is in branch https://github.com/APTrust/registry/tree/db-opts-agent-obj-fix. If you merge that branch into PR #131, you should be able to merge the changes to testutil.go from df84ad5#diff-c2eca278ea2094a443ba598f1be2928d09af7e9a1c65f01289e414280f0e2213 into this branch to fix the tests. Once that's done, I can test again.

[mjiori]

Tests fail for this due to SQL loading errors. I think the issue is due to CSV columns not matching up to table column names after migration 14. This issue affected PR #131. The fix for that is in branch https://github.com/APTrust/registry/tree/db-opts-agent-obj-fix. If you merge that branch into PR #131, you should be able to merge the changes to testutil.go from df84ad5#diff-c2eca278ea2094a443ba598f1be2928d09af7e9a1c65f01289e414280f0e2213 into this branch to fix the tests. Once that's done, I can test again.

Thanks @diamondap , Registry tests are now passing.

I fixed the SQL errors that were due to some mistakes I made in the migration syntax for renaming the authy_status column and also fixed some issues in the migration for updating existing Authy users.

This uncovered a couple issues with other tests that I have also fixed, particularly one regarding the default MFA method for an enabled and confirmed MFA user with no specified MFAStatus (formerly AuthyStatus). The default is SMS for past users which I was able to deduce thanks to your test comment. Which required one more downstream change in IsSMSUser as well.

I will check the other fix to column names under the db-opts-agent-obj-fix separately

[diamondap]

These changes look good. If these are the final changes, you can go ahead and merge.

[mjiori]

These changes look good. If these are the final changes, you can go ahead and merge.

Yup, these are the last for this PR removing Authy. The DB optimizations branch is independent from this, and I will look at the changes you requested there next thing. Thanks for the review!