feat(auth): Allow users to attach GH profile via social auth#53767
feat(auth): Allow users to attach GH profile via social auth#53767
Conversation
|
🎭 Playwright report · View test results →
These issues are not necessarily caused by your changes. |
Query snapshots: Backend query snapshots updatedChanges: 1 snapshots (1 modified, 0 added, 0 deleted) What this means:
Next steps:
|
4905a00 to
f070d6d
Compare
|
Size Change: +1.53 kB (0%) Total Size: 129 MB ℹ️ View Unchanged
|
a9e240c to
bc1f374
Compare
|
| def sso_login(request: HttpRequest, backend: str) -> HttpResponse: | ||
| request.session.flush() |
There was a problem hiding this comment.
If I understand correctly, redirecting to /login/<backend>/ now silently links the social provider to the existing user. This is a pretty big behavior change with side effects from just visiting/being redirected to a GET url. We should show the user an explicit confirmation page, like we do during OAuth connection for ex, so that they can confirm this action.
There was a problem hiding this comment.
You're right, we should still flush by default.
For the PH Code linking case, I don't think we should have an extra confirmation screen though. But this is simple - we can only do the "link social auth provider to the existing, currently-logged-in PostHog user" thing when the connect_from arg is set.
Implemented this change, so there is no blast radius here.
There was a problem hiding this comment.
You're right, we should still flush by default.
For the PH Code linking case, I don't think we should have an extra confirmation screen though.
Can you tell me more about this case? IMO we should have an explicit confirmation dialog whenever linking a third party to an existing account.
There was a problem hiding this comment.
Pinged you in Slack which already has a bit of that context :)
There was a problem hiding this comment.
Discussed offline - the account linking will have already occurred by this point. We should still show an explicit confirmation dialog, but that's out of scope of this change.
posthog/api/signup.py
Outdated
| logger.info(f"social_create_user_is_not_new") | ||
|
|
||
| if not user.is_email_verified and user.password is not None: | ||
| if not user.is_email_verified: |
There was a problem hiding this comment.
It feels kinda fishy, as the social email of the user could differ from their PostHog email, but I assume if they are already authentificated it's alright.
Is it technically possible for me to create an account with bill.gates@posthog.com, and then add my GitHub, so my email will be marked confirmed? :)
There was a problem hiding this comment.
Indeed, this part is not needed or good here. I think this was left over from a slightly different approach earlier. Reverted
posthog/api/user.py
Outdated
| def get_github_login(self, instance: User) -> Optional[str]: | ||
| # Use all() to hit the prefetch cache from get_queryset; filter in Python | ||
| for sa in instance.social_auth.all(): | ||
| if sa.provider == "github" and isinstance(sa.extra_data, dict): |
There was a problem hiding this comment.
We can't have more than one GitHub sync, right? Like, technically we can, in the DB, but not practically?
There was a problem hiding this comment.
Yeah, there's no user flow to do that. I think if you do through the GH login route with the PH Code-only connect_from param twice with two different GitHub accounts, this can technically result in two GH accounts being connected. But there isn't really a way to get into that situation, and it's not a problem, that works because it's natively supported by django-social-auth
posthog/api/user.py
Outdated
|
|
||
| def get_github_login(self, instance: User) -> Optional[str]: | ||
| # Use all() to hit the prefetch cache from get_queryset; filter in Python | ||
| for sa in instance.social_auth.all(): |
There was a problem hiding this comment.
Nit: UserSocialAuth.objects.filter(user_id="bla-bla", provider='github').values_list('extra_data__login', flat=True).first() to avoid getting blobs if we need only login?
There was a problem hiding this comment.
Yup, great point, this should be at the prefetch level. Narrowed that down
There was a problem hiding this comment.
Now with the change I described in PostHog/code#1561, we're now also fetching Integration to determine github_login - so moved this altogether to its own GET action on the user, as we only need this in PH Code
b3c72c1 to
c281b2f
Compare
| def sso_login(request: HttpRequest, backend: str) -> HttpResponse: | ||
| request.session.flush() |
There was a problem hiding this comment.
Discussed offline - the account linking will have already occurred by this point. We should still show an explicit confirmation dialog, but that's out of scope of this change.
MCP UI Apps size report
|
…nking - Add `github_login` field to UserSerializer, returning the GitHub username from UserSocialAuth (or null). - Allow authenticated users to link social auth accounts via /login/<backend>/ by skipping session flush when already logged in. - Preserve password when linking a social account to an existing authenticated user (previously wiped for unverified emails). Generated-By: PostHog Code Task-Id: 3089dc72-e26f-4715-8999-1a194fc7030c
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
bb0d453 to
6c4738c
Compare

Problem
PostHog needs to know a mapping of PostHog user to GH user, which we can do via social auth.
Changes
Addsig
github_loginread-only field toUserSerializer, returning the GitHub username fromUserSocialAuth(ornull). Then, allowing authenticated users to link social auth accounts via/login/<backend>/by skipping session flush when already logged in. This preserves password when linking a social account to an existing authenticated user (previously wiped for unverified emails). Also, adding a new scene to neatly tell the user to go back to PH Code.Created with PostHog Code