Skip to content

Conversation

mjiori
Copy link
Contributor

@mjiori mjiori commented Sep 17, 2025

This PR is a draft. There are known issues and some functionality may not work correctly. Tests also may not pass.

This PR adds a new two-factor authentication option to Registry - the “Authenticator App” option. This option allows for users to enroll a device (most often a phone) by installing an authenticator app such as Duo, Google Authenticator, or Microsoft Authenticator, and then scanning a QR code or entering a setup key to complete enrollment. More information on the architecture can be found in the feature specification.

New additions include:
* A new route used to display the QR code/setup key when the user enrolls their device.

  • A new route used to display a prompt to the user to enter their one-time password.
  • A new route used to process the one-time password and validate its authenticity.

All routes are accessible by APTrust sysadmins, Institutional Admins, and Institutional Users - any type of user may enroll themselves in the authenticator app option.

Constants have been added including a formal name for this two-factor option (“Authenticator App”), the name of the issuing party to control how we appear in the authenticator app UI (“APTrust”), and a short name for this option to identify it if selected by the user.

A database migration is necessary because we need to store a secret value in the database for each user who enrolls in this option.

By visiting the “My Account” page and selecting the “Set up Two-Factor Auth” button, users can view the Authenticator App option as selectable alongside the other current options - Authy, which is being phased out, and SMS, alongside the option to turn off 2FA entirely. If the user selects the option to use the Authenticator App and clicks on Submit, the user will be taken to the Authenticator App enrollment page where further instructions are provided, along with the aforementioned QR code and setup key.

Three new external libraries have been introduced:

  • otp, for generating time-based one-time passwords
  • go-qrcode, used to generate the QR codes
  • barcode, used by go-qrcode

The Content-Security-Policy has been updated such that Registry can now display images, as this has been disabled. We need to be able to display images to show the QR codes.

Documentation has been added describing this new option in notes.md.

The pgmodel for users has been updated to add the new secret field, as well as a method to detect if the user has opted in for the authenticator app method. As well, this new method will now be considered the default MFA method going forward.

The one-time password prompt has been added to the multi-factor authentication page.

The one-time password is obfuscated through use of the password type field.

Testing:

  • Ensure existing user may select the Authenticator App option.
  • Ensure QR code and setup key display on the enrollment page.
  • Ensure QR code and setup key both work correctly as options in the following authenticator apps: Google Authenticator, Microsoft Authenticator, Duo. It is assumed that other apps will work due to their compatibility with TOTP-based authentication. Full disclosure, most of the testing was completed with Google Authenticator.
  • Ensure the MFA method actually changes when changed.
  • Ensure Authy registration is not required when authenticator app is enabled.
  • Ensure user can continue from enrollment page to the verification page.
  • Ensure user can verify with a one-time password from their app.
  • Ensure that an invalid one-time password response will be rejected and that login will not be allowed until a valid one-time password is provided.
  • Ensure that MFA can be disabled and that prompts no longer appear if the authenticator app option is disabled by the user.

Documentation has been added to the User Guide in a separate PR.

Tests:
Units ?
Integration ?
E2E ?
Registry ?

@mjiori
Copy link
Contributor Author

mjiori commented Sep 17, 2025

@diamondap

Main things I'm looking at with this now:

  1. Why does /users/2fa_choose redirect back to itself even after entering a correct one-time password? This doesn't occur during the initial enrollment when the user is already logged in to a previous session. Only happens on a new login.
  2. The code to save the secret to the DB is all there, albeit commented out. (I did have the migration also at one point but had to remove it to proceed with testing) I had to use a placeholder value of "nonesuch" for now. Also there's a 1==1 check in there, that will be replaced once the secret in the DB is working. Updating the users.csv fixture with a new column didn't seem to work, as the fixture users were not created.
  3. How to create automated tests? Am looking to investigate how it was done for Authy.

There is some commented out stuff and test code that I need to deal with in this as well, but these are the main points I need to look into. Thank you for your help!

@diamondap
Copy link
Member

  1. 2fa_choose

Check the logic in these locations, which can force a redirect back to /users/2fa_choose:

I think the issue may be in that second link, in the forceCompletionOfTwoFactorAuth function in authenticate.go. You may need to add "/users/validate_totp" as one of the endpoints that is exempt from the security check. That is, add && !strings.HasPrefix(p, "/users/validate_totp").

  1. fixtures

See the messages I sent in Slack: The code that loads fixture data is here: https://github.com/APTrust/registry/blob/master/db/testutil.go#L118. Did you create a migration that includes your new user table column? Just adding the column header in the first line of the CSV file and the comma at the end of each data line should work. The data loader should map header names in the file to column names in its insert statement.

  1. How to create automated tests?

For Authy, I created tests here: https://github.com/APTrust/registry/blob/master/network/authy_test.go. The more meaningful tests in that file are from line 28 down. They talk to a mock Authy API defined from line 98 on in https://github.com/APTrust/registry/blob/master/network/authy_test.go. If you're working with a simple Go API, you can mock it like the Authy API.

If you need to talk to a REST API to test your authenticator, take a look at Go's httptest library. This article will tell you most of all of what you need to know: https://medium.com/@ullauri.byron/using-httptest-server-in-go-to-mock-and-test-external-api-calls-68ce444cf934

You may have to capture and dump out responses that come from the actual authenticator endpoints you want to test. Then create an httptest.NewServer that returns those responses as necessary. For example, if you send it [email protected], it returns an OTP (or whatever). If you send it [email protected], it returns "Unknown user" or whatever. Does that make sense?

Also, if you have access to Claude.ai, I've found it's really good at generating test code. I just started using it on a Python project recently. I've found it's so-so when generating code from scratch, but quite smart when you feed it your own code and ask it to generate tests. It can save a lot of time when testing code that requires mocks, because mocks can be a huge pain in the ass and Claude gets them about 80% right most of the time. Even if its tests aren't immediately usable, they will point you in the right direction.

@mjiori
Copy link
Contributor Author

mjiori commented Sep 17, 2025

Awesome, thank you!

Will check out the code for those redirects.

The fixtures are working fine now. I was trying to add the new field to a different position than the end, which seems to have caused the issue there.

Will look into Claude.ai for generating the test code.

…word but has not validated second factor yet
@mjiori
Copy link
Contributor Author

mjiori commented Sep 18, 2025

Made the fix to middleware. Now I can see my prompt page.

@diamondap
Copy link
Member

That looks good. Let me know if you run into any other issues.

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