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

Support sign in and sign up with GH/GL/BB #4022

Merged

Conversation

davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Apr 25, 2018

While this was technically supported if you knew the URLs, this adds formal support for logging in and signing up with our providers (currently GitHub, BitBucket, and GitLab).

screen shot 2018-04-25 at 12 41 55 pm

How this works

Let's say your user account already is connected to GitHub because you actually use Read the Docs with your GitHub account. The "Sign in with GitHub" button will just work for you. You can still use your old username and password if you like.

If your GitHub account is not connected to anything and you click the "Sign in with GitHub", you will authenticate with GitHub, and then you will get a confirmation screen where you tie your GitHub account and email to a new Read the Docs username (see screenshot below). After confirming, you'll be logged in. The first and last name may be populated from the provider if available.

screen shot 2018-04-25 at 12 45 31 pm

BitBucket works exactly the same as GitHub. I didn't actually test GitLab but I assume it does.

Note: There is no technical difference between the "Sign in with.." and "Sign up with.." buttons. If your social account is already connected to a user account, you will be logged in. If it isn't, you will be asked to confirm a new sign up.

Weird edge cases

  • Say your social account isn't connected, you click "Sign in with..." and go to the confirm new user screen. The email address already exists and you try to submit it again. You will see an error An account already exists with this e-mail address. Please sign in to that account first, then connect your Bitbucket account. This is strange a little bit at first but I believe it is correct functionality. It also won't let you create a duplicate username but that seems standard.
  • Say a brand new user signs up with their GitHub account and then disconnects GitHub from their account (on https://readthedocs.org/accounts/social/connections/). The user won't be able to login with GitHub any longer (the accounts aren't connected) but they also can't login with a username and password (their account has no password). The only way to retrieve the account in this case is with a password reset to the email on file.

@davidfischer davidfischer added the Improvement Minor improvement to code label Apr 25, 2018
Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

Looks good to me. I do worry about the edge cases that you mention, which is folks who disconnect their social account and then get locked out.

I guess that isn't any different than losing your password though, so being able to recover it is a good enough solution in that case.


{% load i18n %}

{% block head_title %}{% trans "Signup" %}{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

We are using "Signup" and "Sign up", I think we need to use only one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, it looks like this is present in allauth itself:
https://github.com/pennersr/django-allauth/blob/2894261/allauth/templates/account/signup.html#L5-L8

Nonetheless, I'll change it for us. I lean toward "sign up" being two words

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

The only way to retrieve the account in this case is with a password reset to the email on file.

Did you test this flow? I'm not sure yet, but I think that our password reset flow may not work if there is no password in the User object.


<div class="clearfix">
<ul class="socialaccount_providers">
{% include "socialaccount/snippets/provider_list.html" with process="login" next="" verbiage="Sign up with" %}
Copy link
Member

Choose a reason for hiding this comment

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

process="login" and verbiage="Sign up with"

Just in case, process shouldn't be signup or similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

login is correct, I believe. I tested the full workflow.

@ericholscher
Copy link
Member

I just tested the email reset and it worked. 👍

@ericholscher
Copy link
Member

I don't believe I got an verification email though, which might make things not work in the future. I wonder if there's a setting or something about email verification that we need to turn on?

@davidfischer
Copy link
Contributor Author

I don't believe I got an verification email though, which might make things not work in the future. I wonder if there's a setting or something about email verification that we need to turn on?

I'll see if there's an option.

@davidfischer
Copy link
Contributor Author

davidfischer commented Apr 26, 2018

The setting is SOCIALACCOUNT_EMAIL_VERIFICATION and it takes the same values as ACCOUNT_EMAIL_VERIFICATION. For some reason it is set to 'none' in development.

I'll make the settings match in dev and open a PR for the production settings.

Edit: it looks like no changes should be necessary in prod. The default value is the same as ACCOUNT_EMAIL_VERIFICATION. We merely need to remove setting it to none in settings/base.py

@ericholscher
Copy link
Member

👍 looks good to me. I imagine we might get some support requests around this for edge cases, but we can take those as they come.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Minor improvement to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants