Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method for when 2nd factor auth is pending #452

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

janko
Copy link
Contributor

@janko janko commented Nov 17, 2024

This is useful for when applications want to modify configuration depending on whether 2FA is pending. For example, the Rails demo app currently redirects directly to 2FA after login:

login_redirect { uses_two_factor_authentication? && !two_factor_authenticated? ? two_factor_auth_required_redirect : super() }
login_notice_flash { uses_two_factor_authentication? && !two_factor_authenticated? ? "Please authenticate with 2nd factor" : super() }

(It's necessary to check !two_factor_authenticated? here as well, because passkey login can authenticate both factors at once.) With this method available, the code can be simplified:

login_redirect { two_factor_authentication_pending? ? two_factor_auth_required_redirect : super() }
login_notice_flash { two_factor_authentication_pending? ? "Please authenticate with 2nd factor" : super() }

Copy link
Owner

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of this idea, but I would like a better method name. The issue I have with pending? is that Rodauth is designed to handle cases where certain actions do not require 2FA, and other cases do. Just because 2FA is available does not mean that it is required by the application.

@janko
Copy link
Contributor Author

janko commented Nov 17, 2024

Yeah, I'm totally open to the naming change, as I was struggling with that precisely for the reason you mentioned. My previous ideas were two_factor_authentication_required? and needs_two_factor_authentication?, but those reinforced more the idea that 2FA is required when it's not.

If I take your words verbatim, maybe two_factor_authentication_available? would be an improvement? Still looking for something a bit more precise, as this sounds similar to uses_two_factor_authentication?.

@janko
Copy link
Contributor Author

janko commented Nov 17, 2024

Or I could flip it around and call it two_factor_authenticaton_satisfied?. I like this option best so far.

@janko janko force-pushed the two-factor-auth-pending branch from 48060f4 to 0ea3c89 Compare November 17, 2024 19:42
@jeremyevans
Copy link
Owner

two_factor_authentication_satisfied? seems weird to return true if two factor is not setup.

The best name I can think of two_factor_partially_authenticated?, with semantics similar to the proposal of two_factor_authentication_pending?. I think this makes sense, because if two factor is not setup, it would return false, and if it is two factor authenticated, it would also return false. I think it would also have to check that you are partially authenticated (so that at least one authentication factor is present), which means it would also need a require_login? check. What are your thoughts on that?

@janko janko force-pushed the two-factor-auth-pending branch from 0ea3c89 to a9adcd6 Compare November 18, 2024 09:36
@janko
Copy link
Contributor Author

janko commented Nov 18, 2024

I agree that it sounded strange when 2FA is not setup. I like the suggestion, updated 👍🏻

@jeremyevans jeremyevans merged commit a9adcd6 into jeremyevans:master Nov 18, 2024
16 checks passed
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