Add ability to login with email address#85
Conversation
| return | ||
| LoginThrottle(User.by_name(user_name), user_name).reset() | ||
| user = User.by_name(user_name) | ||
| if not user: |
There was a problem hiding this comment.
This is the same logic as in get_user_throttle, perhaps extract a helper.
There was a problem hiding this comment.
Good idea, thanks.
I've taken care of this by creating a new helper function security_get_user(user_name).
Note that in addition to reset_address_throttle(user_name) and get_user_throttle(user_name) in authenticator.py, login() in utils.py also uses the new helper.
| return { | ||
| 'check_ckan_version': tk.check_ckan_version, | ||
| 'security_enable_totp': security_enable_totp, | ||
| 'security_get_user': security_get_user, |
There was a problem hiding this comment.
Can this have a more descriptive name?
There was a problem hiding this comment.
Yes, what would you suggest?
There was a problem hiding this comment.
Perhaps something more descriptive of its purpose, eg get_user_by_name_or_email
There was a problem hiding this comment.
I have changed the function name from security_get_user to security_get_user_by_name_or_email.
Changed function name security_get_user to security_get_user_by_name_or_email
|
What happens if someone sets their email address to be someone else's username? |
Good question. Based on some investigation I have found the following to be true. CKAN core name_validator does not allow @ symbol. CKAN core email_validator requires a valid email address (i.e. includes an @ symbol) So, unless I'm overlooking something, this means a username cannot be set to an email address, and an email address cannot be set to a username. |
That sounds good. Can you add a unit test to catch any future changes that might alter this? |
3 participants
CKAN 2.10 added the ability to login with email address in addition to username.
In this PR I have added this capability to ckanext-security.
I may need to do something about the way this interacts with throttling. As it is currently implemented, a separate security_throttle_key is tracked for a user's email and user_name. So, for instance, an attacker could theoretically attempt
ckanext.security.login_max_countlogins everyckanext.security.lock_timeoutseconds on an email address and a user_name separately, technically doubling the threat exposure.I know this isn't optimal, but I also don't know how big of an issue I consider it. It would be better if only one security_throttle_key was tracked but that is beyond the scope of what I have time to implement here and now.
Interested to hear if you think this fix is worth merging as it is or if (a) I have introduced non-throttling related security problems with what I have proposed so far or (b) I should first work to also consolidate the security_throttle_keys into one per user as part of this PR.
Thanks, hope this is useful.