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

Two-factor updates. #235

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Two-factor updates. #235

merged 1 commit into from
Dec 17, 2019

Conversation

jwag956
Copy link
Collaborator

@jwag956 jwag956 commented Dec 15, 2019

First - there are many authenticator apps out there - so change all references to google_authenticator to
simply 'authenticator' - in forms, messages, config. Tested with authy and lastpass.

Put in backwards compat code so that existing DB primary_method entries of 'google_authenticator' should still work.

Second - prior to this change - the QRcode uri was @<service_name>. According
to the passlib docs, and consistent with other sites handling of the uri - this has been changed to full email. The
service_name is sent as the issuer (as it already was). Likely this resulted when we converted to passlib.

This should have no backwards compat issues.

@Kishi85 would appreciate a quick look at this to see if it will cause any backwards compat issues or other thoughts.

@jwag956 jwag956 self-assigned this Dec 15, 2019
@jwag956 jwag956 added this to the 3.4 milestone Dec 15, 2019
@Kishi85
Copy link
Contributor

Kishi85 commented Dec 16, 2019

Looking good so far, the only issue I can see (and have after testing in one of my applications) is that when the primary login attribute used is 'id' or something else with further customizing, and E-Mail is set to an empty string or None the qrcode and anything dependent on the E-Mail attribute will fail. It should respect SECURITY_USER_IDENTITY_ATTRIBUTES in some way or another at that point and use the values defined by the attributes there as E-Mail might not always be the preferred and enabled way to identify a user.

@Kishi85
Copy link
Contributor

Kishi85 commented Dec 16, 2019

This is a solution for the issue I mentioned (for QRcode in this case, and the only one I encountered directly during testing). This would change the the whole behavior to be more generic (following configured user identity attributes) while still using email as the username with default settings:

diff --git a/flask_security/views.py b/flask_security/views.py
index 6a810b2..ed62afc 100644
--- a/flask_security/views.py
+++ b/flask_security/views.py
@@ -964,8 +964,15 @@ def two_factor_qrcode():
     totp = user.tf_totp_secret
     try:
         import pyqrcode
-
-        url = pyqrcode.create(get_totp_uri(user.email, totp))
+        identity_attributes = config_value("USER_IDENTITY_ATTRIBUTES")
+        if identity_attributes and len(identity_attributes) > 0:
+            primary_attribute = identity_attributes[0]
+        else:
+            primary_attribute = "id"
+        username = getattr(user, primary_attribute, None)
+        if username is None or len(username) == 0:
+            username = user.id
+        url = pyqrcode.create(get_totp_uri(username, totp))
     except ImportError:
         # For TWO_FACTOR - this should have been checked at app init.
         raise

@jwag956
Copy link
Collaborator Author

jwag956 commented Dec 16, 2019

I like it - the only small thing is that 'id' isn't a user visible field. However - I believe that USER_IDENTITY_ATTRIBUTES must be set to something or else a user can't log in.

flask_security/views.py Outdated Show resolved Hide resolved
First - there are many authenticator apps out there - so change all references to google_authenticator to
simply 'authenticator' - in forms, messages, config. Tested with authy and lastpass.

Put in backwards compat code so that existing DB primary_method entries of 'google_authenticator' should still work.

Second - prior to this change - the QRcode uri was <email-username>@<service_name>. According
to the passlib docs, and consistent with other sites handling of the uri -
this should be the 'username' (e.g. full email, or username).
The service_name is sent as the issuer (as it already was). Likely this resulted when we converted to passlib.

This should have no backwards compat issues.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants