Multi-factor authentication #1817

2025-02-07T01:56:06+08:00

Shpigford commented

2025-02-07 01:56:06 +08:00

(Migrated from github.com)

Implements #1372

https://github.com/user-attachments/assets/b4047e5d-bb24-4c1a-8a2c-e376c8d5eec1

Implements #1372 https://github.com/user-attachments/assets/b4047e5d-bb24-4c1a-8a2c-e376c8d5eec1

zachgoll (Migrated from github.com) reviewed 2025-02-07 04:03:34 +08:00

zachgoll (Migrated from github.com) left a comment

Everything works great!

From a security standpoint, we should probably protect the password_resets_controller with 2FA if the user has it enabled. Other than that, I think the flow looks good.

Everything works great! From a security standpoint, we should probably protect the `password_resets_controller` with 2FA if the user has it enabled. Other than that, I think the flow looks good.

app/controllers/mfa_controller.rb

						
				@@ -0,0 +1,53 @@

				class MfaController < ApplicationController

zachgoll (Migrated from github.com) commented

2025-02-07 04:00:14 +08:00

We can remove the Current.session check since we're skipping this route for authentication.

We can remove the `Current.session` check since we're skipping this route for authentication.

app/models/user.rb

						
				@@ -133,4 +168,26 @@ class User < ApplicationRecord

				        errors.add(:profile_image, :invalid_file_size, max_megabytes: 10)

zachgoll (Migrated from github.com) commented

2025-02-07 02:50:42 +08:00

      ROTP::TOTP.new(otp_secret, issuer: "Maybe Finance")

Just to stay consistent with our domain URL to increase trust

```suggestion ROTP::TOTP.new(otp_secret, issuer: "Maybe Finance") ``` Just to stay consistent with our domain URL to increase trust

test/controllers/sessions_controller_test.rb

zachgoll (Migrated from github.com) commented

2025-02-07 03:12:35 +08:00

We can delete "destroys session" and "rejects invalid credentials" as they are both dups of existing tests

zachgoll (Migrated from github.com) commented

2025-02-07 03:14:44 +08:00

Can delete this and throw the assert Session.exists?(user_id: @user.id) up in the "can sign in" test

Can delete this and throw the `assert Session.exists?(user_id: @user.id)` up in the "can sign in" test

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#1817

				`@@ -0,0 +1,53 @@`
				`class MfaController < ApplicationController`

				`@@ -133,4 +168,26 @@ class User < ApplicationRecord`
				`errors.add(:profile_image, :invalid_file_size, max_megabytes: 10)`