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

Login with email code #270

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

LeeDalchow
Copy link

Resolves #172
Type: feature

Issue

Adds the ability to login to the system using an e-mail code instead of the password

Solution

Adds a link on the Login page that allows the user to send themself an e-mail that contains a link to log the user on directly to the system.

Breaking changes

I have edited both InstallDataMessageTemplates.cs & DefaultLanguage.xml, so upgrade scripts will need to be provided to update this. I checked the migrations folder and they seemed to be specific to version upgrades. Was unsure where the code for this upgrade should go.

Testing

  1. Visit frontpage of shop without being logged into an account.
  2. Head to the Login page
  3. Click "Login with E-mail Code"
  4. Enter your e-mail address
  5. Head to your e-mail and click the link provided
  6. You should now be logged into the system.

@KrzysztofPajak
Copy link
Member

@LeeDalchow Thanks for contributing.
I would like to suggest:

  1. Adding new customer settings - Allow to login by email
  2. Add migration process (IMigration) to insert a new message template (to version 2.1.0)
  3. Do not extend the Customer entity by adding new field LoginCode
  4. Move const int MINUTES_TO_EXPIRE - add new customer settings
  5. Login code can be used only once - after use it should be dropped
  6. Using the only link to login - is it a secure way, maybe it's worth to use extra Cookies or session ?, Maybe adding settings "Allow to login by email" and add extra info (is not a secure way to use this)

@LeeDalchow
Copy link
Author

LeeDalchow commented Jun 6, 2022

@LeeDalchow Thanks for contributing. I would like to suggest:

  1. Adding new customer settings - Allow to login by email
  2. Add migration process (IMigration) to insert a new message template (to version 2.1.0)
  3. Do not extend the Customer entity by adding new field LoginCode
  4. Move const int MINUTES_TO_EXPIRE - add new customer settings
  5. Login code can be used only once - after use it should be dropped
  6. Using the only link to login - is it a secure way, maybe it's worth to use extra Cookies or session ?, Maybe adding settings "Allow to login by email" and add extra info (is not a secure way to use this)

@KrzysztofPajak Thanks for your review! Will post my replies below alongside each suggestion number:

  1. I will make this change.
  2. Thanks for highlighting the version, I wasn't sure which migration to update as they were named after version numbers - will add in these message templates
  3. If the Customer entity should not be extended, how do you propose getting the loginCode passed to LiquidCustomer?

Would it be a good solution to modify:

public LiquidObjectBuilder AddCustomerTokens(Customer customer, Store store, DomainHost host, Language language, CustomerNote customerNote = null)

to be something like:

public LiquidObjectBuilder AddCustomerTokens(Customer customer, Store store, DomainHost host, Language language, CustomerNote customerNote = null, string? loginCode = null)

and then assign loginCode (if not null) to AdditionalTokens, which can then be passed into the message?

  1. I will make this change.
  2. This is already the case - but it does it by setting the code to expire after use, rather than removing it from the database. I will change this to remove it completely from the database.
  3. I imagined somebody using the login with e-mail code to potentially login from another device. (ie. a mobile phone, after generating the link from their laptop). Is this not a valid use case in your view? I imagined if a malicious actor had access to their e-mail account, they can also just do "I forgot my password" regardless of additional security on the "Login Code"

If we add extra security (such as a session cookie ensuring the login is from the same browser) to the login by email page, it would make sense to also add that change to the "I forgot my password" area.

@LeeDalchow
Copy link
Author

Just a quick note to say I've been busy lately but still intend to finish the updates!

@LeeDalchow
Copy link
Author

LeeDalchow commented Jun 26, 2022

@KrzysztofPajak

@LeeDalchow Thanks for contributing. I would like to suggest:

  1. Adding new customer settings - Allow to login by email Done
  2. Add migration process (IMigration) to insert a new message template (to version 2.1.0) Done, although please check over this as I'm not sure if I've done everything I need to to have this work correctly.
  3. Do not extend the Customer entity by adding new field LoginCode Removed from Customer entity and instead now pass down as a parameter stored within AdditionalTokens in LiquidCustomer.
  4. Move const int MINUTES_TO_EXPIRE - add new customer settings Done
  5. Login code can be used only once - after use it should be dropped This is already the case, but I added an extra line to remove the old hash from the database to be safe
  6. Using the only link to login - is it a secure way, maybe it's worth to use extra Cookies or session ?, Maybe adding settings "Allow to login by email" and add extra info (is not a secure way to use this) I imagined if a malicious actor had access to their e-mail account, they can also just do "I forgot my password" so any extra security added also needs to be added to that area. I'm also not sure if a cookie is appropriate since I imagined a user potentially logging in from another device (ie. a mobile phone, after generating a link from their laptop). Is this not a valid use case in your view? How would you propose we implement this? Happy to take your lead on this and I'll implement however you think is appropriate.

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.

Add possibility to log in with the email code instead of a password
2 participants