perma icon indicating copy to clipboard operation
perma copied to clipboard

Case insensitive email addresses

Open matteocargnelutti opened this issue 3 years ago • 8 comments

First attempt at figuring out a solution for #3152.


I decided to start with the smallest fix I could think of to deal with this issue, and see where the discussion takes us 😄 .

What does it do?

  • It forces str.lower() on the email field of UserForm, so all new users will have a lower case email. This applies to the entire email address.

Why this might be a good idea?

  • It deals with our problem "moving forward", without creating problems for existing users (?).
  • We can then handle existing casing conflicts on a case-by-case basis, as brought to our attention.

Why this might not be a good idea?


PS: @rebeccacremona

I haven't added a test yet, and wanted to ask how you would go about testing this.

First idea: adding a test case in test_views_user_management.py, something that tries to call the /sign-up form twice with the same email, but different casing.

def test_signup_email_case_insensitive(self):
    self.submit_form('sign_up',
                    data = {'e-address': "[email protected]", 'first_name': 'Person', 'last_name': 'Name'},
                    success_url = reverse('register_email_instructions'))

    self.submit_form('sign_up',
                    data = {'e-address': "[email protected]", 'first_name': 'Person', 'last_name': 'Name'},
                    error_keys=['email'])

Ping @kilbergr

matteocargnelutti avatar Aug 31 '22 23:08 matteocargnelutti

Codecov Report

Merging #3153 (2acf2fc) into develop (f681d87) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #3153   +/-   ##
========================================
  Coverage    82.62%   82.62%           
========================================
  Files           53       53           
  Lines         5795     5797    +2     
========================================
+ Hits          4788     4790    +2     
  Misses        1007     1007           
Impacted Files Coverage Δ
perma_web/perma/forms.py 95.09% <100.00%> (+0.04%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

codecov[bot] avatar Aug 31 '22 23:08 codecov[bot]

I am trying to think of anything else we'd need to change if we do this...

So far, I can only think of one other spot: on the https://perma.cc/manage/{admin,registrar,sponsored,organization}-users pages, there is a button for adding a user, e.g. image (click that one to expose the actual button in question) image

It does a thing, where it checks to see if a user already exists, and directs you to a different form depending.

I think downcasing on new UserForm save will get weird, if we don't also add special handling here. How should this work, thinking.....

rebeccacremona avatar Sep 01 '22 15:09 rebeccacremona

Does this sound right?

  • An org user wants to add Joe Yacoboski to their org, and enters [email protected] to the form

  • We check to see if there's already an account associated with "[email protected]" or "[email protected]".lower().

  • If there is one associated with [email protected], we send them to the "he's already got an account; add him to the org" form with that User object.

  • If there is not, but there is exactly one associated with "[email protected]".lower(), we send them to the "he's already got an account; add him to the org" form with that User object. If there is more than one associated with "[email protected]".lower(), we tell them that there is a complication and ask them to contact us.

  • Otherwise, we send them to the "create a new user and add them to the org" form, with the pre-filled email address field downcased, so that they see what we are doing.

That will work, I think?

rebeccacremona avatar Sep 01 '22 15:09 rebeccacremona

@rebeccacremona That makes sense to me and I think it's a valid solution!

I'm trying to think of an alternative, to see if we can "get away" with a solution that doesn't involve adding more complexity to this system.

How about we switched this filter to email__iexact so this mechanism catches existing accounts regardless of casing? If this returns more than 1 account, we should probably show and error message.

Since we know how many accounts would be affect by this, dealing with this problem on a per-case / request basis seems somewhat reasonable?

matteocargnelutti avatar Sep 01 '22 18:09 matteocargnelutti

Re: testing...

Right, the test fixtures in Perma are still the old-fashioned JSON style, rather than the nicer pytest fixture + factoryboy setup we have elsewhere....

We probably want to create another user like this one, maybe [email protected] (which also means adding a folder for them), and maybe [email protected] as well, so we can test explore the above imaginary Joe Yacoboski scenario.

I like your idea of a test hitting the most basic signup form twice. I feel like we want some sort of assertion that it works via the other forms well, even though that sounds tiresome.

rebeccacremona avatar Sep 01 '22 19:09 rebeccacremona

How about we switched this filter to email__iexact so this mechanism catches existing accounts regardless of casing? If this returns more than 1 account, we should probably show and error message.

Sure, showing an error whenever there is > 1 sounds just as good or better.

I'd be interested talking through the flow for people who attempt to sign up with capital letters.

I think without more code, this is what we have: I enter [email protected], possibly let LastPass or my browser remember that, submit, succeed... but then, I have to log in as [email protected], because login is case-sensitive... so I fail. I try to reset my password. I enter [email protected] in the password reset form. I do not get sent a reset email, because that, like login, is case-sensitive.

How can we smooth that out? Nothing great is coming to mind: rejecting capital letters in the signup form with a validation error seems bad too.

rebeccacremona avatar Sep 01 '22 19:09 rebeccacremona

I am going to study the seemingly duplicative accounts we have, and see how many can be easily merged/deleted (for instance, if only one account has been activated, or only one has links, etc.)

rebeccacremona avatar Sep 01 '22 19:09 rebeccacremona

I am making bold to convert this to a draft, pending further discussion about auth-related flows for users.

rebeccacremona avatar Sep 08 '22 19:09 rebeccacremona

Superseded by https://github.com/orgs/harvard-lil/projects/9/views/3

rebeccacremona avatar Apr 27 '23 18:04 rebeccacremona