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

Github/Gitlab social auth #416

Merged
merged 18 commits into from Apr 6, 2020
Merged

Github/Gitlab social auth #416

merged 18 commits into from Apr 6, 2020

Conversation

EDsCODE
Copy link
Member

@EDsCODE EDsCODE commented Mar 29, 2020

Addresses #404

Implemented: When the SOCIAL_AUTH_GITLAB_KEY (or SOCIAL_AUTH_GITHUB_KEY)and SOCIAL_AUTH_GITLAB_SECRET (or SOCIAL_AUTH_GITHUB_SECRET) are provided as env variables wherever your instance is deployed, social auth with that platform will be available. The auth is implemented using python-social-auth as suggested. Currently, github and gitlab auth have been implemented.

Note: The redirect_uri that github or gitlab asks for should be in the form http://{domain_name}/complete/{gitlab or github}/

What's left:

  • style the buttons as they're quite barebones right now
  • tests?
  • possible edge case where the social auth doesn't return email at all will break login

Screen Shot 2020-03-29 at 5 43 44 PM

@timgl
Copy link
Collaborator

timgl commented Mar 29, 2020

This is awesome! Thank you so much for doing this. It looks good, will do a proper QA tomorrow.

I like the bare-bones styling of the buttons, it would be good if they aligned with the button above on the left and right

@timgl
Copy link
Collaborator

timgl commented Mar 30, 2020

There's a couple of things missing:

@EDsCODE
Copy link
Member Author

EDsCODE commented Mar 31, 2020

There's a couple of things missing:

Agreed! In fact, I was reviewing the implementation and realized I wasn't being thorough with the access points because I'm guessing these sign in options should be available on the initial admin sign up where a new team is created right? Going to add a custom pipeline to distinguish these signup/login paths.

@timgl
Copy link
Collaborator

timgl commented Mar 31, 2020

I'm actually less concerned about the admin setup page, as I don't think anyone will have set up GitHub keys before setting up the master account, plus we still need to get the other fields (company name, full name etc) that we can't get easily from github/gitlab.

@EDsCODE
Copy link
Member Author

EDsCODE commented Mar 31, 2020

hmm this will be a blocker for the standard login.html page too because if a new user trying to join a team ends up trying to auth from the default login page, social auth will complete but land them on a blank page as they're not associated with a team. There's two routes I can take with this PR:

  1. (slightly lengthier) I can setup a unique flow where the admin setup and regular login page both have social auth capabilities. If a new user is joining (no team association), we can use a partial pipeline to redirect the user to a form where the user can enter a team token or create a new team.
  2. (condensed) I can leave the social auth as it is now and just finish including the team addition in the pipeline as noted above. If a user who isn't associated with a team attempts to login from the normal page, they can be redirected to an error page noting that they're unable to login due to not belonging to a team.

I'd suggest 1 but 2 keeps this PR contained a bit better!

@timgl
Copy link
Collaborator

timgl commented Mar 31, 2020

Good find! I much prefer 2 anyway as that feels right (who would know a team code of the top of their head?). We should make sure no user gets created, which I think you should be able to do by putting the function at the right point in the pipeline.

👍

'social_core.pipeline.social_auth.social_user',
'social_core.pipeline.user.get_username',
'social_core.pipeline.social_auth.associate_by_email',
'posthog.urls.social_create_user',
Copy link
Member Author

Choose a reason for hiding this comment

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

uprooted and used our own custom create user

posthog/urls.py Show resolved Hide resolved
posthog/urls.py Show resolved Hide resolved
posthog/urls.py Show resolved Hide resolved
posthog/urls.py Show resolved Hide resolved
posthog/urls.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@timgl timgl left a comment

Choose a reason for hiding this comment

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

QA'd, functionally works beautifully. Just two small things and then we're good to go 👍

posthog/urls.py Outdated Show resolved Hide resolved
posthog/templates/signup_to_team.html Show resolved Hide resolved
@timgl timgl merged commit 827b575 into PostHog:master Apr 6, 2020
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.

None yet

2 participants