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

Additional Login Options #1786

Merged
merged 13 commits into from
Dec 7, 2017
Merged

Additional Login Options #1786

merged 13 commits into from
Dec 7, 2017

Conversation

valaparthvi
Copy link
Contributor

@valaparthvi valaparthvi commented Sep 11, 2017

This PR includes:

  1. VerificationDevice model and Removal of 48hr email verification period
  2. Registration with Phone number and Account Verification Page
  3. Add phone to User Profile
  4. Allow user to login with phone and add Resend Token Page
  5. Reset Password with Phone
  6. Twilio Integration and More update notification

Proposed changes in this pull request

  • Add phone and phone_verified properties to the User model, and remove verify_email_by property. A user will not be given the grace period of 48 hours to verify their email address.

  • Add a new VerificationDevice model that will be used to generate and verify a token for phone verification. This new model uses django-otp module. Every token is valid for 60 minutes. This VerificationDevice is connected to the User via a ForeignKey.
    VerificationDevice has 2 main purposes: i) Phone Verification and ii) Reset password with phone
    Every User instance will have at most 2 VerificationDevice model linked to it. These VerificationDevice instances will be deleted once their purpose has been served, i.e. once a phone has been verified, the device used to verify the phone will be deleted.

  • Migration accounts/0010_phone_and_verification_device.py includes above mentioned changes to the models.

  • As soon as user registers, a verification email and/or verification token will be sent to user's email address and/or phone number. The user will then be redirected to a Account Verification page immediately after successful registration. On this page, the user will be asked to verify their phone/email in order to access their account. If the user had registered with a phone, the user will see a Token Verification form on the same page. The user enters the received token in the form, and if the token matches, user's phone will be verified, else the user will see an error message.

  • Every time a user changes their existing phone number or adds a phone number to their profile, they will be redirected to the Account Verification page to verify their phone.

  • If the user does not receive any verification email/token, the user can click on here button placed at the bottom of Account Verification page to try again. This here button will redirect the user to a Resend Token page. Here user will see 2 forms, one form will take email and another phone. The user can enter their registered email/phone to receive a verification email/token.

  • Reset Password with Phone: On the password reset page, the user can see a new form which takes a phone. A user can enter their registered phone to reset the password with the phone. If the entered phone is linked to any user account, a verification token will be sent to the user, else not.

  • If user logs in with unverified email/phone or if user account status is inactive, instead of automatically sending an email verification link or a token, the user will be redirected to the Resend Token page. Here user will enter their registered email/phone to receive a verification link/token. If both phone and email linked to the user account are unverified, user account status will be set to inactive.

  • Add a PhoneAuthenticationBackend to allow user authentication with phone.

  • Add two new exceptions to accounts/exceptions.py: i) PhoneNotVerifiedError and ii) AccountInactiveError
    As the name suggests, errors will be raised when phone and/or email is/are unverified. This errors will be raised by AccountLoginSerializer.
    Also, modify existing EmailNotVerifiedError by initiating it with an error message.

  • Integrate Twilio services on the platform to send SMS to user's phone. To implement this, a accounts/gateways.py file has been added. 2 Gateways have been defined here: i) TwilioGateway and ii) FakeGateway
    TwilioGateway will be used for staging/demo/production to send actual SMS via Twilio. FakeGateway will be used for development and Travis environment. The FakeGateway uses logger to print messages in the console.
    Twilio Credentials have been added to Ansible config, thanks to @amplifi.

  • Many new update notifications have been added. Notification will be sent to phone/email linked to the user account when any new activity has been recorded. These notifications will be sent regardless of phone/email('s) verification status. Notifications will be sent in following cases:

  1. User changes phone and/or email
  2. User deletes phone and/or email
  3. User adds phone and/or email to their account
  4. User resets/changes password

Here's a link to the doc file. This document contains detail about every page, their functionality, required changes to the codebase and a list of required test cases for every page.

When should this PR be merged

  • reviews, QA, bug fixes and design improvements

Risks

  • Normal

Follow-up actions

  • Install django-otp, phonenumbers and twilio.
  • Migration accounts/0010_phone_and_verification_device.py

Checklist (for reviewing)

General

  • Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
    • Review 1
    • Review 2
  • Is the PR labeled correctly? It should have the migration label if a new migration is added.
    • Review 1
    • Review 2
  • Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
    • Review 1
    • Review 2

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
    • Review 1
    • Review 2
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.
    • Review 1
    • Review 2

Code

  • Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
    • Review 1
    • Review 2
  • Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
    • Review 1
    • Review 2
  • Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.
    • Review 1
    • Review 2
  • Is the code documented sufficiently? Large and complex classes, functions or methods must be annotated with comments following our code-style guidelines.
    • Review 1
    • Review 2
  • Has the scalability of this change been evaluated?
    • Review 1
    • Review 2
  • Is there a maintenance plan in place?
    • Review 1
    • Review 2

Tests

  • Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
    • Review 1
    • Review 2
  • If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
    • Review 1
    • Review 2
  • If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?
    • Review 1
    • Review 2

Security

  • Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
    • Review 1
    • Review 2
  • Are all UI and API inputs run through forms or serializers?
    • Review 1
    • Review 2
  • Are all external inputs validated and sanitized appropriately?
    • Review 1
    • Review 2
  • Does all branching logic have a default case?
    • Review 1
    • Review 2
  • Does this solution handle outliers and edge cases gracefully?
    • Review 1
    • Review 2
  • Are all external communications secured and restricted to SSL?
    • Review 1
    • Review 2

Documentation

  • Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
    • Review 1
    • Review 2
  • Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
    • Review 1
    • Review 2
  • Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.
    • Review 1
    • Review 2

@clash99
Copy link
Contributor

clash99 commented Sep 11, 2017

Hi @valaparthvi - Kudos to you for all this hard work.

I have some UI concerns regarding this new registration page. I think removing the required asterisk from the email field and adding the phone number field is very confusing. We shouldn't be waiting for the user to fail before informing them that they need either an email address or phone number to register.

I would recommend keeping the email field with the required asterisk since this is the main path for our users and then adding the ability to toggle to a phone number field if necessary. Here is what we discussed implementing earlier:

registration with email or phone

@valaparthvi valaparthvi force-pushed the additional-login-options branch 4 times, most recently from 7f67781 to dc8af08 Compare September 26, 2017 18:42
@oliverroick oliverroick removed the request for review from amplifi October 2, 2017 15:54
@valaparthvi valaparthvi force-pushed the additional-login-options branch 3 times, most recently from 2970441 to 3d3307c Compare October 5, 2017 17:10
@valaparthvi valaparthvi force-pushed the additional-login-options branch 2 times, most recently from 70be15a to 0f9f196 Compare October 20, 2017 06:22
@oliverroick oliverroick added this to the 1.14.0 milestone Oct 24, 2017
current_phone_set = VerificationDevice.objects.filter(
user=self.instance)
if current_phone_set.exists():
current_phone_set.delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

The tiniest of optimizations:

We don't actually need to care if the current_phone_set exists or not when we issue the delete command, we could simply run VerificationDevice.objects.filter(user=self.instance).delete() or self.instance.verificationdevice_set.all().delete().

if current_email != new_email:
email_set = instance.emailaddress_set.all()
if email_set.exists():
email_set.delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, could simplify by just running instance.emailaddress_set.all().delete()

if current_phone != new_phone:
phone_set = VerificationDevice.objects.filter(user=instance)
if phone_set.exists():
phone_set.delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

user.verificationdevice_set.all().delete()

@valaparthvi
Copy link
Contributor Author

@oliverroick has currently opened a PR to this branch, maybe he can do these changes in his PR.

valaparthvi and others added 11 commits December 6, 2017 09:03
* VerificationDevice model and Removal of 48hr email verification period (#1606)

* Registration with Phone number (#1662)

* Add phone to User Profile (#1698)

* Add Ansible provisioning for Twilio

* Allow user to login with phone and add Resend Token Page (#1708)

* Reset Password with Phone (#1717)

* Twilio Integration and More update notification (#1719)

* Add API endpoint for phone verification(2) (#1748)
* upgraded django-skivvy to 0.1.9
* Remove unnecessary lines of code
* Design improvements to Registration Form and Resend Token Form

minor changes

* Design improvements in password reset page

* UI and Parsley changes

* fix bug

minor changes

* Toggle phone js, UI fixes

* minor change to user dashboard

* change phone regex, add tests to check minimum phone length and add some other missed test cases

add some other missed test cases in serializers

* changes addressed to the PR

* Error messaging updates

* Additional form pages and js

* add tests and code to handle invalid NumberParseException thrown by phonenumbers when invalid country code is entered
…ers (#1906)

* Remove option to switch between phone and email from form
* Remove option to switch between phone and email from serializer
* Add conditions to show/hide phone/email form fields
* Make either phone or email required
* Validate either phone or email is provided to serializer
* Catch invalid phone number in AccountRegister
* Catch invalid phone number in AccountProfile
* Catch invalid phone number in api.AccountUser
* Catch invalid phone number in api.AccountRegister
* Catch Twilio 500 errors
* Catch errors in ResendTokenView
* Catch errors in PasswordResetView
* Log Twilio 500-level errors with Opbeat
* Write opbeat tokens to expected env variables
@oliverroick oliverroick merged commit 82871ef into master Dec 7, 2017
@oliverroick oliverroick deleted the additional-login-options branch December 7, 2017 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants